Deposit charged again on renewal of deposit item

Confirming the same behavior in 2.9.

Confirming the behavior exists in EG version 2.11.

Here's a branch in working/user/dyrcona/lp1170794-skip-deposit-fee-on-renewal to not charge the deposit fee on renewal:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dyrcona/lp1170794-skip-deposit-fee-on-renewal

It includes a release note, but no tests. If someone wants tests, that could be added.

You are going to fix it?

J. Elaine Hardy
PINES & Collaborative Projects Manager
Georgia Public Library Service/PINES
1800 Century Place, Ste. 150
Atlanta, GA 30045

404.235.7128 Office
404.548.4241 Cell
404.235.7201 FAX

On Mon, Feb 20, 2017 at 11:41 AM, Terran McCanna <
<email address hidden>> wrote:

> ** Changed in: evergreen
> Assignee: (unassigned) => Terran McCanna (tmccanna)
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1170794
>
> Title:
> Deposit charged again on renewal of deposit item
>
> Status in Evergreen:
> Confirmed
> Status in Evergreen 2.10 series:
> Confirmed
> Status in Evergreen 2.11 series:
> Confirmed
>
> Bug description:
> In EG 2.3.4
>
> For items with deposit flag set to yes and deposit amount indicated in
> item attributes, we expect the deposit amount to be charged on
> checkout and not charged again on subsequent renewal.
>
> Currently, there is a bug that charges deposit amounts on initial
> checkout and on renewal. This means that PINES patrons can be charged
> up to three times rather than once and library staff must void those
> amounts charged on renewal.
>
> Expected behavior for deposit items is for the deposit to be charged
> only on checkout but not again on renewal.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/evergreen/+bug/1170794/+subscriptions
>

Jason Stephenson created a patch for it - I am just going to test it during
bug squashing week.

Terran McCanna
PINES Program Manager
Georgia Public Library Service
1800 Century Place, Suite 150
Atlanta, GA 30345
404-235-7138
<email address hidden>

On Mon, Feb 20, 2017 at 1:18 PM, Elaine Hardy <email address hidden>
wrote:

> You are going to fix it?
>
>
> J. Elaine Hardy
> PINES & Collaborative Projects Manager
> Georgia Public Library Service/PINES
> 1800 Century Place, Ste. 150
> Atlanta, GA 30045
>
> 404.235.7128 Office
> 404.548.4241 Cell
> 404.235.7201 FAX
>
> On Mon, Feb 20, 2017 at 11:41 AM, Terran McCanna <
> <email address hidden>> wrote:
>
> > ** Changed in: evergreen
> > Assignee: (unassigned) => Terran McCanna (tmccanna)
> >
> > --
> > You received this bug notification because you are subscribed to the bug
> > report.
> > https://bugs.launchpad.net/bugs/1170794
> >
> > Title:
> > Deposit charged again on renewal of deposit item
> >
> > Status in Evergreen:
> > Confirmed
> > Status in Evergreen 2.10 series:
> > Confirmed
> > Status in Evergreen 2.11 series:
> > Confirmed
> >
> > Bug description:
> > In EG 2.3.4
> >
> > For items with deposit flag set to yes and deposit amount indicated in
> > item attributes, we expect the deposit amount to be charged on
> > checkout and not charged again on subsequent renewal.
> >
> > Currently, there is a bug that charges deposit amounts on initial
> > checkout and on renewal. This means that PINES patrons can be charged
> > up to three times rather than once and library staff must void those
> > amounts charged on renewal.
> >
> > Expected behavior for deposit items is for the deposit to be charged
> > only on checkout but not again on renewal.
> >
> > To manage notifications about this bug go to:
> > https://bugs.launchpad.net/evergreen/+bug/1170794/+subscriptions
> >
>
> --
> You received this bug notification because you are a member of Evergreen
> Bug Wranglers, which is subscribed to Evergreen.
> Matching subscriptions: ALL BUGS
> https://bugs.launchpad.net/bugs/1170794
>
> Title:
> Deposit charged again on renewal of deposit item
>
> Status in Evergreen:
> Confirmed
> Status in Evergreen 2.10 series:
> Confirmed
> Status in Evergreen 2.11 series:
> Confirmed
>
> Bug description:
> In EG 2.3.4
>
> For items with deposit flag set to yes and deposit amount indicated in
> item attributes, we expect the deposit amount to be charged on
> checkout and not charged again on subsequent renewal.
>
> Currently, there is a bug that charges deposit amounts on initial
> checkout and on renewal. This means that PINES patrons can be charged
> up to three times rather than once and library staff must void those
> amounts charged on renewal.
>
> Expected behavior for deposit items is for the deposit to be charged
> only on checkout but not again on renewal.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/evergreen/+bug/1170794/+subscriptions
>

Wish we could get the rest of deposit fixed. It all got broken several
upgrades back when they forgot about the deposit flag on items

J. Elaine Hardy
PINES & Collaborative Projects Manager
Georgia Public Library Service/PINES
1800 Century Place, Ste. 150
Atlanta, GA 30045

404.235.7128 Office
404.548.4241 Cell
404.235.7201 FAX

On Mon, Feb 20, 2017 at 1:38 PM, Terran McCanna <
<email address hidden>> wrote:

> Jason Stephenson created a patch for it - I am just going to test it during
> bug squashing week.
>
>
> Terran McCanna
> PINES Program Manager
> Georgia Public Library Service
> 1800 Century Place, Suite 150
> Atlanta, GA 30345
> 404-235-7138
> <email address hidden>
>
>
> On Mon, Feb 20, 2017 at 1:18 PM, Elaine Hardy <<email address hidden>
> >
> wrote:
>
> > You are going to fix it?
> >
> >
> > J. Elaine Hardy
> > PINES & Collaborative Projects Manager
> > Georgia Public Library Service/PINES
> > 1800 Century Place, Ste. 150
> > Atlanta, GA 30045
> >
> > 404.235.7128 Office
> > 404.548.4241 Cell
> > 404.235.7201 FAX
> >
> > On Mon, Feb 20, 2017 at 11:41 AM, Terran McCanna <
> > <email address hidden>> wrote:
> >
> > > ** Changed in: evergreen
> > > Assignee: (unassigned) => Terran McCanna (tmccanna)
> > >
> > > --
> > > You received this bug notification because you are subscribed to the
> bug
> > > report.
> > > https://bugs.launchpad.net/bugs/1170794
> > >
> > > Title:
> > > Deposit charged again on renewal of deposit item
> > >
> > > Status in Evergreen:
> > > Confirmed
> > > Status in Evergreen 2.10 series:
> > > Confirmed
> > > Status in Evergreen 2.11 series:
> > > Confirmed
> > >
> > > Bug description:
> > > In EG 2.3.4
> > >
> > > For items with deposit flag set to yes and deposit amount indicated
> in
> > > item attributes, we expect the deposit amount to be charged on
> > > checkout and not charged again on subsequent renewal.
> > >
> > > Currently, there is a bug that charges deposit amounts on initial
> > > checkout and on renewal. This means that PINES patrons can be charged
> > > up to three times rather than once and library staff must void those
> > > amounts charged on renewal.
> > >
> > > Expected behavior for deposit items is for the deposit to be charged
> > > only on checkout but not again on renewal.
> > >
> > > To manage notifications about this bug go to:
> > > https://bugs.launchpad.net/evergreen/+bug/1170794/+subscriptions
> > >
> >
> > --
> > You received this bug notification because you are a member of Evergreen
> > Bug Wranglers, which is subscribed to Evergreen.
> > Matching subscriptions: ALL BUGS
> > https://bugs.launchpad.net/bugs/1170794
> >
> > Title:
> > Deposit charged again on renewal of deposit item
> >
> > Status in Evergreen:
> > Confirmed
> > Status in Evergreen 2.10 series:
> > Confirmed
> > Status in Evergreen 2.11 series:
> > Confirmed
> >
> > Bug description:
> > In EG 2.3.4
> >
> > For items with deposit flag set to yes and deposit amount indicated in
> > item attributes, we expect the deposit amount to be charged on
> > checkout and not charged again on subsequent renewal.
> >
> > Currently, there is a bug t...

Are there open bugs on the other things that broke?

Terran McCanna
PINES Program Manager
Georgia Public Library Service
1800 Century Place, Suite 150
Atlanta, GA 30345
404-235-7138
<email address hidden>

On Mon, Feb 20, 2017 at 1:55 PM, Elaine Hardy <email address hidden>
wrote:

> Wish we could get the rest of deposit fixed. It all got broken several
> upgrades back when they forgot about the deposit flag on items
>
>
> J. Elaine Hardy
> PINES & Collaborative Projects Manager
> Georgia Public Library Service/PINES
> 1800 Century Place, Ste. 150
> Atlanta, GA 30045
>
> 404.235.7128 Office
> 404.548.4241 Cell
> 404.235.7201 FAX
>
> On Mon, Feb 20, 2017 at 1:38 PM, Terran McCanna <
> <email address hidden>> wrote:
>
> > Jason Stephenson created a patch for it - I am just going to test it
> during
> > bug squashing week.
> >
> >
> > Terran McCanna
> > PINES Program Manager
> > Georgia Public Library Service
> > 1800 Century Place, Suite 150
> > Atlanta, GA 30345
> > 404-235-7138
> > <email address hidden>
> >
> >
> > On Mon, Feb 20, 2017 at 1:18 PM, Elaine Hardy <
> <email address hidden>
> > >
> > wrote:
> >
> > > You are going to fix it?
> > >
> > >
> > > J. Elaine Hardy
> > > PINES & Collaborative Projects Manager
> > > Georgia Public Library Service/PINES
> > > 1800 Century Place, Ste. 150
> > > Atlanta, GA 30045
> > >
> > > 404.235.7128 Office
> > > 404.548.4241 Cell
> > > 404.235.7201 FAX
> > >
> > > On Mon, Feb 20, 2017 at 11:41 AM, Terran McCanna <
> > > <email address hidden>> wrote:
> > >
> > > > ** Changed in: evergreen
> > > > Assignee: (unassigned) => Terran McCanna (tmccanna)
> > > >
> > > > --
> > > > You received this bug notification because you are subscribed to the
> > bug
> > > > report.
> > > > https://bugs.launchpad.net/bugs/1170794
> > > >
> > > > Title:
> > > > Deposit charged again on renewal of deposit item
> > > >
> > > > Status in Evergreen:
> > > > Confirmed
> > > > Status in Evergreen 2.10 series:
> > > > Confirmed
> > > > Status in Evergreen 2.11 series:
> > > > Confirmed
> > > >
> > > > Bug description:
> > > > In EG 2.3.4
> > > >
> > > > For items with deposit flag set to yes and deposit amount indicated
> > in
> > > > item attributes, we expect the deposit amount to be charged on
> > > > checkout and not charged again on subsequent renewal.
> > > >
> > > > Currently, there is a bug that charges deposit amounts on initial
> > > > checkout and on renewal. This means that PINES patrons can be
> charged
> > > > up to three times rather than once and library staff must void
> those
> > > > amounts charged on renewal.
> > > >
> > > > Expected behavior for deposit items is for the deposit to be
> charged
> > > > only on checkout but not again on renewal.
> > > >
> > > > To manage notifications about this bug go to:
> > > > https://bugs.launchpad.net/evergreen/+bug/1170794/+subscriptions
> > > >
> > >
> > > --
> > > You received this bug notification because you are a member of
> Evergreen
> > > Bug Wranglers, which is subscribed to Evergreen.
> > > Matching subscriptions: ALL BUGS
> > > https://bugs.launchpad.net/bugs/1170794
> > >
> > > Title...

I've found a very minor problem with the patch.

If a patron checks out the deposit item, an alert is given on the first renewal that a deposit is owed to the patron. On the second renewal, that alert does not appear, nor on check-in after a renewal. While deposit items should be marked as such, the alert makes a nice reminder to staff to actually return the deposit to the patron.

I can confirm the behavior Sam describes in his comment. I've removed the pullrequest for the moment.

Re-assigning myself on the assumptions that I'll make some time to look at this again, soonish.

I'm concerned about the consequences of keeping the original circ in play and open across multiple renewals. Would it be reasonable to simply move the deposit billing line item along with the renewal by changing the xact field directly?

Something to confirm: how do opac renewals currently interact with deposits?

To answer Mike's question if there is a deposit amount on an item currently an opac renewal will fail with the message 'Payment of an item rental fee is required.'

patch here: user/rogan/lp1170794_skip_renewal_deposit_charge

This is similar to Jason's patch but maybe some things changed between then and now. Testing on master as of today showed this behavior with an item that had a deposit:

Staff Client Checkout: showed ITEM_DEPOSIT_REQUIRED message, charged amount
Staff Client Renewal: showed ITEM_DEPOSIT_REQUIRED and charged amount
OPAC Login: attempted to renew and got "Failed to renew 1 item(s)"

after patch testing gave:

Staff Client Checkout: showed ITEM_DEPOSIT_REQUIRED message, charged amount
Staff Client Renewal: did not show ITEM_DEPOSIT_REQUIRED and did not charge deposit again
OPAC Login: attempted to renew and it successfully renewed

Note, that I did find that if you are renewing from an aborted transit that message supersedes the deposit one. That may be an existing bug somewhere but I suspect is broader in scope than this testing. The behavior of the ITEM_DEPOSIT_PAID message remained consistent before and after. I'm not a fond of it's terminology but that's not in the scope of this either of course.

We are trying to get this loaded on one of our stock test servers. It may be next week before we can test it but we are going to test

Blake was able to put it on a sandbox for us.

I created an item on Record ID 94 with the deposit amount of $10.

1. Checked out to Patron Leon Anderson. Charged deposit correctly with warning dialog box. Paid deposit.

2. Renewed item from patron account in staff client.Dialog box for deposit already charged and for message for deposit again. Forced action and deposit was charged.

Repeated with patron Norm Barnes with same result -- deposit charged on renewal.

We had the same results in our stock test server.

That's not the behavior I saw so I'm removing the pull request for now to take another look at it.

New patch here: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=commit;h=2a10da6967503b4b0c82fd69cafc31febf0e1acd

user/rogan/lp1170794_deposit_bad_on_renew

Tested this on one of our test servers running stock. Still seeing a deposit charged with each renewal.

Interesting. I'm definitely not seeing that on my test VM. I'm going to see if I can pull in other eyes and test this. Either there is a difference in our testing environments or some workflow difference that needs accounting for. I'm leaving the pull request on for now simply because I'd like to see more eyes on it and testing.

My workflow, for reference

I created an item, checked the deposit flag for yes and added 10 to the deposit amount.

Test 1:

Checked book out to stock patron at same branch as owning library.

Deposit was charged.

Renewed item. 3 part warning message: that patron exceeded fines, was charged a deposit and that deposit would be charged.

Clicked to override. Patron charged another deposit fee.

Checked book in

Test 2:

Checked book out to patron at same branch as owning library.

Patron charged deposit.
Paid deposit.
Renewed item. Two part warning message: patron was charged a deposit and patron would be charged a deposit.
Overrode for renewal.
Patron charged deposit again.

This is a server with sotck Evergreen and the concerto database(no PINES customizations).

I tested with the concerto data and the patch worked. I edited an item and made it have a deposit amount of $1 and checked it out. The patron was charged a $1. Before I applied the patch the patron was charged upon renewal after I applied the patch the patron was not charged upon renewal.

After I applied the patch I did restart the opensrf services and apache2 before testing the affects of the patch.

I'll try to test again next week. We did not have Chris's expertise today to load the patch so we might have missed something.

Apparently I for got to sign off on this in git so ...

I consent to signing off on this patch with my name and email: Rogan Hamby, <email address hidden>

I confirm that the last patch prevents the ITEM_DEPOSIT_PAID event from being raised on item check-in. Arguably, that event is already broken because it's only true if the patron actually paid the deposit, though I imagine that would be true with most workflows?

I'm inclined to push this as is as it fixes a more serious issue (charging upon renewal), and open a new bug for a new PATRON_OWED_DEPOSIT_RETURN event.

Thoughts?

To clarify what I meant about ITEM_DEPOSIT_PAID being true or false, I was considering what it means in English. In my test scenario, no payments were made, yet the system was claiming otherwise with the event description.

Coming back to this with fresh eyes, "I confirm that the last patch prevents the ITEM_DEPOSIT_PAID event from being raised on item check-in." This is true only if the item has been renewed. I'm a little more hesitant now to create a smaller bug (potentially having no ITEM_DEPOSIT_PAID alert on check-in) to fix a larger one (extra deposits being charged upon renewal). What do folks think? I think in practice, most patrons will remember if they're owed a deposit or not (particularly if it matters to them), but we might not want to be intentionally introducing bugs as a matter of policy.

Re-assigning to me. I was looking at this purely in context of renewals. I'm going to look a the checkin scenario and will repatch.

I tend to agree about not knowingly creating bugs.

With the ITEM_DEPOSIT_PAID bug while a patron might remember they have a deposit refund due, if they put the item in a book drop it would be checked in without the notice for the refund causing delays in getting that refund to the patron.

This could become a big customer service issue for libraries if patrons are consistently having to ask for the refund rather than EG prompting for the refund on return.

cleaned up and minimalistic branch that only deals with the charging on renewals: user/rogan/lp1170794_deposits_and_renewals_oh_my

So, what is happening has to do with the function that on checkin checks to see if a deposit was required for the circulation. It only checks the _existing_ circ. I think it could be argued this is a separate bug and not introduced by the bug though I doubt it matters to end users. This is relevant for explaining it because of this is what historically has happened:

Checkout without renewal -
I check a book out with a deposit. The system has to make a deposit billing for that transaction. I check the book in, that deposit billing exists so I get the ITEM_DEPOSIT_PAID event.

Checkout with renewal -
I check a book out with a deposit, charged deposit, I renew it and when I check it in I get the ITEM_DEPOSIT_PAID message not because the original circ had a deposit but because the renewal did.

Post fix:

Since ITEM_DEPOSIT_PAID is generated on checkin and we're checking the renewal not the original circ .... well, that's why we get this. This is easy to test because if you take the patch and check an item out and check it directly in without a renewal you will still get the ITEM_DEPOSIT_PAID message.

I have some concerns about pulling in circ chains during the check in process and potential speed impact but will investigate it.

I haven't had time to investigate the check-in speed issue but I'm still of the opinion that this patch fixes the issue in this bug. I think the check-in only checking existing circulation and not parent circs should be considered a separate bug and may affect issues beyond the scope of this bug report.

Unsubscribe.

On Thu, Jul 23, 2020 at 10:25 AM Evergreen Bug Maintenance <
<email address hidden>> wrote:

> ** Changed in: evergreen/3.3
> Status: Confirmed => Won't Fix
>
> ** Changed in: evergreen/3.3
> Milestone: 3.3.8 => None
>
> --
> You received this bug notification because you are subscribed to
> Evergreen.
> https://bugs.launchpad.net/bugs/1170794
>
> Title:
> Deposit charged again on renewal of deposit item
>
> Status in Evergreen:
> Confirmed
> Status in Evergreen 3.3 series:
> Won't Fix
> Status in Evergreen 3.4 series:
> Confirmed
>
> Bug description:
> In EG 2.3.4
>
> For items with deposit flag set to yes and deposit amount indicated in
> item attributes, we expect the deposit amount to be charged on
> checkout and not charged again on subsequent renewal.
>
> Currently, there is a bug that charges deposit amounts on initial
> checkout and on renewal. This means that PINES patrons can be charged
> up to three times rather than once and library staff must void those
> amounts charged on renewal.
>
> Expected behavior for deposit items is for the deposit to be charged
> only on checkout but not again on renewal.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/evergreen/+bug/1170794/+subscriptions
>

--

______________________________________________________________
Be careful when you follow the Masses....Sometimes the 'M' is silent.

Donna Rowland (<email address hidden>)

Finance Specialist

Fontana Regional Library

33 Fryemont Rd.

Bryson City, NC 28713

Voice: 828-488-2382 ext.124

Fax: 828-488-2638

“Opinions expressed in this message may not represent the policy of my
library. All e-mail sent to or from the Fontana Regional Library e-mail
systems is subject to monitoring and disclosure to third parties, including
law enforcement personnel.”

This is still charging a deposit fee upon renewal.

This is still charging a deposit on renewal.

Assuming that the test case is the same and it's charging now then something has changed and it needs review and perhaps a different solution now. Removing the pull request.

We've tested this patch:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/rogan/lp1170794_deposits_and_renewals_oh_my

Just to be clear (because there are a couple of patches in this thread) - the patch that we have tested introduces a single line:

--- a/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Circulate.pm
+++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Circulate.pm
@@ -1086,6 +1086,7 @@ sub is_deposit_exempt {
     for my $grp (@$groups) {
         return 1 if $self->is_group_descendant($grp, $pid);
     }
+ return 1 if $self->is_renewal;
     return 0;
 }

With this patch, we can confirm that a circulation (with deposit) - does throw the exception "ITEM_DEPOSIT_PAID" staff about deposits upon renewal. Evergreen does not charge a second deposit on renewal. Furthermore, if the item is not renewed, Evergreen will inform the staff about the deposit (on checkin). Exactly what we want.

There is still an issue in the case where a deposited circulation goes through a renewal. And then finally returned. The checkin does not inform the staff of the deposit. I realize that this issue has been mentioned already, but I thought I would be clear about what code change we've tested and the outcome.

Just spitballing here:

Idea 1)

Can we move the billing stuff to the new circulation? (update money.billing set xact=new.xact where xact=?) same for money.payment?

Making the billing stuff follow the open circulation? Not a good idea? Some issues could come if there were other billing lines other than the deposit-related ones. But could those be snipered? Essentially moving the money matters with the circulation.

Idea 2)

On renewal, can Evergreen "pretend" that the library refunded the deposit (completing the original circulation deposit billing cycle) - and then make a new deposit billing (and payment if existed before) on the (re)enewed circulation?

Since we're using this in production, I've updated the patch to merge onto master (3.9ish).

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/blake/lp1170794_deposit_charged_again_on_renewal_of_deposit_item