Deposit refunds not applying as expected on item check in

[Elaine Hardy]

For EG 2.3.4

For items with deposit flag set to yes and deposit amount indicated in item attributes, the system indicates that a refund is due the patron. The deposit refund is not automatically applied to the patron account as expected and staff must manually void the deposit on the patron account in order to reconcile the account.

We expect the system to automatically apply the refund on check in as it automatically applies the deposit on checkout. We also expect that once library staff refunds the amount to the patron, the account be reconciled.

[Erica Rohlfs (erohlfs)]

Confirmed in 2.9.

[Jennifer Pringle (jpringle-u)]

Confirmed in 2.10

[Blake GH (bmagic)]

Confirmed in 2.11

[Chris Sharp (chrissharp123)]

From my reading of the history of Circulate.pm, this has never worked as the original bug report expects. During item checkin, it checks to see if there is a matching deposit bill for the circ, and if so it pushes the ITEM_DEPOSIT_PAID event, which causes the alert in the client during checkin, but nothing is done to refund the bill. The "check_circ_deposit" subroutine has remained unchanged since September 2008 when the original rental/deposit feature was added.

[Blake GH (bmagic)]

Here is what I got:

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

[Erica Rohlfs (erohlfs)]

Tested this on Master (sb1) for bug week. Checking out an item that requires a deposit to the patron applies the bill to patron account, pay the deposit, check item in, Evergreen does not automatically apply the refund to the patron account. I cannot signoff.

[Blake GH (bmagic)]

Erica - did you set the library setting?

[Erica Rohlfs (erohlfs)]

Hi Blake! Good to know the setting doesn't default to true, and I'm sorry I overlooked that new feature. The setting is misspelled. It reads: "Void item depsoit fee upon checkin" where depsoit should read deposit. Once the setting is set to TRUE, the system does refund the paid deposit. If the deposit has not been paid, then Evergreen removes the billing from the patron account. Should I sign-off on this bug and create a new one for the misspelling? Or wait until the spelling is corrected?

[Kathy Lussier (klussier)]

I'm late to the party on commenting on this one, but, before this is merged, I'm questioning whether we need a library setting for this one. If we go by the definition of the words, deposit means that the money should be returned. Rental means that the money should not be returned. I would expect that setting deposit to true is that act that tells the system that the money should be returned.

Or are we worried that, after the system has not worked that way for years, people are setting Deposit = true for items that really should be rental items?

FWIW, to support what Chris said earlier about previous behavior, I dug up this old release notes page from 1.2.4 https://wiki.evergreen-ils.org/doku.php?id=feature_list_1_2_4

"At present, the system does not automatically remove/void deposit billings at item checkin. It must be managed by staff."

Although that functionality was not built into the system, the release notes entry implies that there was an expectation that deposit means billings should be voided.

Putting that question aside, I think it would also be worth adding a test to this code to verify that returned deposit items void the fee while returned rental items do not void the fee.

[Erica Rohlfs (erohlfs)]

Good point about testing rental items. With the Deposit Library Setting set to TRUE, it does not affect the behavior of rental items. If unpaid and item is checked in, the rental billing stays on the patron account. If rental fee is paid, Evergreen does not generate a refund for paid rental billing.

[Kathy Lussier (klussier)]

Sorry for the confusion, Erica! When I referred to adding tests, I was talking about adding live tests to our automated test suite to prevent a future regression on this functionality. But, now that you've done it, that was probably a good test to do too! Thanks for verifying the behavior there! :)

[Blake GH (bmagic)]

Why can't words spell themselves? I took care of the spelling issue and committed it, but it sounds like I have some additional work to do. Regression tests and remove the library setting? Can we get consensus on the library setting issue? I decided to create the setting because I didn't want to assume that every library wanted the system to refund automatically. I could imagine a library who prefers to do it by hand in order to handle the particulars of every patron situation.

[Terran McCanna (tmccanna)]

+1 to a library setting for this. YAOUS FTW.

[Michele Morgan (mmorgan)]

I don't see mention of a partial payment of the deposit as part of the testing. If the deposit is actually being voided, it may lead to negative balance issues.

As far as the library setting goes, I'd be reluctant to just change the long standing behavior of the requirement for manually refunding, so I'm leaning toward keeping the library setting.

[Erica Rohlfs (erohlfs)]

That was also a good point to test, Michele. I just tested partial payment, and it did not cause a negative balance issue. For my test, there was a $5.00 deposit. My patron paid $2.00. When the item was checked back in, Evergreen removed the unpaid $3.00 portion and only generated a refund for the $2.00 that was paid.

[Elaine Hardy (ehardy)]

+1 for library setting.

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 Fri, Mar 3, 2017 at 11:15 AM, Michele Morgan <email address hidden>
wrote:

> I don't see mention of a partial payment of the deposit as part of the
> testing. If the deposit is actually being voided, it may lead to
> negative balance issues.
>
> As far as the library setting goes, I'd be reluctant to just change the
> long standing behavior of the requirement for manually refunding, so I'm
> leaning toward keeping the library setting.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1170795
>
> Title:
> Deposit refunds not applying as expected on item check in
>
> Status in Evergreen:
> Confirmed
>
> Bug description:
> For EG 2.3.4
>
> For items with deposit flag set to yes and deposit amount indicated in
> item attributes, the system indicates that a refund is due the patron.
> The deposit refund is not automatically applied to the patron account
> as expected and staff must manually void the deposit on the patron
> account in order to reconcile the account.
>
> We expect the system to automatically apply the refund on check in as
> it automatically applies the deposit on checkout. We also expect that
> once library staff refunds the amount to the patron, the account be
> reconciled.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/evergreen/+bug/1170795/+subscriptions
>

[Kathy Lussier (klussier)]

I haven't had a chance to look at this yet, but I withdraw my question regarding YAOUS for this feature. On further reflection, I agree that a setting is required. Thanks all!

That means we just need the test, and, since I would characterize this as a new feature, a release notes entry. Thank you Blake for your work on this, Erica for your testing and everyone else for your feedback!

[Bill Erickson (berick)]

Removing pullrequest -- still has needstest and needsreleasenotes

[Scott Thomas (scott-thomas-9)]

Is there anything new to report with this other than what's here? We are at 3.0.3 and I cannot locate this feature in Library Settings nor are System: Deposit bills automatically removed.

Scott

[Kathy Lussier (klussier)]

Hi Scott,

No, there is nothing new to report on this. It looks like the code is still missing a regression test and a release notes entry, which is a requirement for code to be accepted by the developer community. I can volunteer to write a release notes entry, but we still need the test.

For a little more background information, a feature or bug fix in a LP bug will only be available in an Evergreen release if you see the Fix Released status at the top of the page. At that time, a milestone will also be set, and that will tell you which Evergreen release where you can expect to see the bug fix or feature.

[Jason Boyer (jboyer)]

I've refactored this patch and added a release note. I can put together a live test if we think that's necessary, but the only database change is an insert for a setting, so that's unlikely to be a useful thing to test.

Here's the branch: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jboyer/lp1170795_void_deposit_on_checkin / working/user/jboyer/lp1170795_void_deposit_on_checkin

[Blake GH (bmagic)]

Thanks Jason! Will give it whril.

[Terran McCanna (tmccanna)]

Adding pullrequest back to this one, though it probably needs a rebase first due to its age.

[Michele Morgan (mmorgan)]

The branch in comment 21 picks clean to current master, removing needsrepatch.

[Terran McCanna (tmccanna)]

This is great! I tested it with full payments, partial payments, no payments, and renewals, and it all worked as expected.

My sign off is at:
https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/mccanna/lp1170795_void_deposit_on_checkin_signoff

[Jason Boyer (jboyer)]

Pushed to master for 3.8. Thanks everyone!