Bug 31427 - Automatic renewal errors should come before many other renewal errors
Summary: Automatic renewal errors should come before many other renewal errors
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Martin Renvoize (ashimema)
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-08-19 19:54 UTC by Lucas Gass (lukeg)
Modified: 2024-12-23 08:05 UTC (History)
17 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.04
Circulation function:


Attachments
Bug 31427: Get auto renewal errors before other renewal errors (3.85 KB, patch)
2023-07-20 03:28 UTC, David Cook
Details | Diff | Splinter Review
Bug 31427: Get auto renewal errors before other renewal errors (3.89 KB, patch)
2023-07-20 15:39 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 31427: Get auto renewal errors before other renewal errors (3.94 KB, patch)
2023-09-26 14:15 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 31427: (follow-up) Unit tests (2.93 KB, patch)
2023-10-26 08:09 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 31427: Get auto renewal errors before other renewal errors (3.93 KB, patch)
2024-02-16 15:32 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 31427: (follow-up) Unit tests (3.00 KB, patch)
2024-02-16 15:32 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 31427: Get auto renewal errors before other renewal errors (4.03 KB, patch)
2024-03-01 10:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31427: (follow-up) Unit tests (3.09 KB, patch)
2024-03-01 10:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31427: Remove a few refs to CanBookBeRenewed (1.30 KB, patch)
2024-03-01 10:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass (lukeg) 2022-08-19 19:54:18 UTC
I am not sure if this error, too_many, should be removed or just handled differently. It is confusing for patrons, here is why:


Day 1, a patron gets an email telling them an item has been successfully renewed. It is auto renewal 5/5, the item will no longer renew. 

Day 2: The next a patron receives a new auto renewal message telling them the same item, from yesterday, cannot be renewed.

Patrons are confused.
Comment 1 Jason Robb 2022-08-22 16:48:12 UTC
Agree that this is confusing and misleading to the patron.

We're seeing a consistent duplication of messages whenever something auto renews, both with digests and one-off messaging thanks to the odd too_many behavior.

The too_many error is also getting sent while a title has renewals available.

For example, we had a scenario where Book A and Book B both had one renew available. Checkout period was 14 days. No renewal before was set to 0 so that the auto-renew happened on the day the item came due. Book A was due 7/1. Book B was due 7/7.

The digested messaging went like this:
7/1 - Book A was renewed, Book B was not renewed (too_many error)
7/2 - Both titles were not renewed (too_many error)
7/7 - Book B was renewed, Book A was not renewed (too_many error)
7/8 - Both titles were not renewed (too_many error)

The messages on 7/1 and 7/2 gave Book B the too_many error so it appears like no renewals were available, proven untrue when the title auto-renewed on its due date of 7/7.

Ideally the 'too_many' error would only pop when all renewals have been exhausted. Titles that have renewals available but aren't getting renewed yet because they aren't due yet should be handled separately so they can be explained/ignored in the templates. 

The goal for digested messaging in the above scenario would be something like: 
7/1 - Book A was renewed, Book B is not yet eligible for auto renewal
7/7 - Book B was renewed, Book A is no longer eligible for auto renewal (too_many)
7/15 - Book A and Book B are no longer eligible for auto renewal (too_many)
7/21 - Book A and Book B are no longer eligible for auto renewal (too_many)
Comment 2 Azucena Aguayo 2022-08-22 17:24:36 UTC
Our library has the same problem. Our patrons get confused with the back to back emails for the same item telling them the item was renewed / not renewed.
Comment 3 Spyros Pierros 2022-08-24 06:24:05 UTC
In our case the loan period can't be shorten for any reason. 

The renewal routine seems that it examines every checkout of a patron and then the notice Auto renewals (Digest) reports that some items didn't renew, which is true, but this occurs because there is not any reason to be renewed or returned (they have been renewed yesterday for example), and this is so confusing. 

It should skip these items and  to report only 
1) the items that they have been renewed, and 
2) the items that they haven't been renewed for a specific reason provided by the library rules. 

Furthermore, the selection of digest should stop the RENEWAL notice
Comment 4 David Cook 2023-04-18 00:22:40 UTC
I'm starting to think that automatic renewal errors shouldn't be sent out at all. That only automatic successes should be sent out.

Although in this case I actually have a local customization where I skip renewal handling if it's too soon to renew. The current precedence of errors in automatic renewals is completely illogical.
Comment 5 Ray Delahunty 2023-06-08 14:06:55 UTC
We are testing auto-renewals in 22.11 (preparing to ditch our auto-renewal workaround which avoided various bugs like 19014 in place since 17.11). We have the ‘No renewal before’ set to zero and the renewal script running at 00:15 after the item was late. So far we have found two interesting issues. One is the renewal handling too soon problem that should be solved if we could have David Cook’s local customisation. Is the customisation TT code added to the AUTO_RENEWAL_DGST? I don’t want ‘Visual merchandising’ tacked on as an orphan on this digest:

There were 1 items that were not renewed.
There were 2 items that were renewed.
Mastering wildlife photography (barcode: 54250053) was not renewed as it is reserved by another reader.
Cool infographics (barcode 54197511) was renewed until 12/06/2023
The information design handbook (barcode 54240693) was renewed until 12/06/2023
Visual merchandising / : 40620336

The other issue was the digest not dealing well with leavers. We have added one renewal failure reason missing from the default digest shipped with 22.11: 

[% ELSIF checkout.auto_renew_error == 'auto_account_expired' %] could not be renewed as your account has expired. 

This new reason should work well with our renewals assessment taking place the day after the account expiry. But unexpectedly, if I have 2 items due for renewal assessment tonight and 2 more not due for some days, for leavers, Koha lumps all 4 items together and tells me all 4 items have failed to renew ([% error %]=4 not 2), and the two ones that aren’t eligible yet for renewal assessment are not left tacked onto the end of the renewed / failed renew list (so, no ‘Visual merchandising’ orphans for leavers). OK, it might seem odd that items could be due after expiry, but our data feed manipulates the borrower.dateexpiry and the scenario is (unfortunately) possible. Perhaps the customisation might fix this and see [% error %] set to two and the digest only listing the 2 items due now. OK, there would be a second digest for the other items due later, but that is OK.
Comment 6 David Cook 2023-06-09 00:33:14 UTC
(In reply to Ray Delahunty from comment #5)
> We are testing auto-renewals in 22.11 (preparing to ditch our auto-renewal
> workaround which avoided various bugs like 19014 in place since 17.11). We
> have the ‘No renewal before’ set to zero and the renewal script running at
> 00:15 after the item was late. So far we have found two interesting issues.
> One is the renewal handling too soon problem that should be solved if we
> could have David Cook’s local customisation. Is the customisation TT code
> added to the AUTO_RENEWAL_DGST? 

The local customisation I'd done was in the code in the automatic_renewals.pl script. However, it hasn't been working correctly in 21.11 and 22.11.

I haven't had any complaints about 22.11, so I'm not planning on looking at it again unless I get people complaining about confusion/unexpected behaviours locally. 

(In reply to Ray Delahunty from comment #5)
> Is the customisation TT code
> added to the AUTO_RENEWAL_DGST? I don’t want ‘Visual merchandising’ tacked
> on as an orphan on this digest:
> 
> There were 1 items that were not renewed.
> There were 2 items that were renewed.
> Mastering wildlife photography (barcode: 54250053) was not renewed as it is
> reserved by another reader.
> Cool infographics (barcode 54197511) was renewed until 12/06/2023
> The information design handbook (barcode 54240693) was renewed until
> 12/06/2023
> Visual merchandising / : 40620336

It might be that you need to tweak your template as this does look a bit odd.
Comment 7 David Cook 2023-07-17 03:58:12 UTC
I'm looking at master and it looks like this would still be a problem.

It seems to me that _CanBookBeAutoRenewed() and friends should be moved up in C4::Circulation::CanBookBeRenewed() to before the return for "too_many".
Comment 8 David Cook 2023-07-20 03:27:47 UTC
I'm going to change the title from "Automatic renewal too_many error is confusing in notices" to "Automatic renewal errors should come before other renewal errors"
Comment 9 David Cook 2023-07-20 03:28:52 UTC
Created attachment 153696 [details] [review]
Bug 31427: Get auto renewal errors before other renewal errors

This patch changes CanBookBeRenewed so that automatic renewal
errors pop up before other renewal errors. This means that a book
will be considered "auto_too_soon" before things like "too_many" or
"restricted". (Otherwise, you'll get an email saying you can't renew
a book the day after using your last auto renewal, even though the
earliest renewal isn't available until later.)

Test plan:
0. Apply patch
1. prove t/db_dependent/Circulation.t
2. prove t/db_dependent/Holds.t
3. prove t/db_dependent/Koha/Account/Line.t
4. prove t/db_dependent/Koha/Account.t

Additional tests:
5. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=RestrictionBlockRenewing
6. Change to "block"
7. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=AutoRenewalNotices
8. Change to "according to patron messaging preferences"
9. Go to http://localhost:8081/cgi-bin/koha/admin/smart-rules.pl
10. Set "Automatic renewal" to "Yes" and "No renewal before" to 4
11. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51
12. Checkout 39999000001310 with a due date 4 days in the future
13. Add a manual restriction
14. Run `perl ./misc/cronjobs/automatic_renewals.pl`
15. Note that it says something like the following:
Issue id: 1237 for borrower: 51 and item: 73 would not be renewed. (auto_too_soon)

Instead of something like the following:
Issue id: 1237 for borrower: 51 and item: 73 would not be renewed. (restriction)
Comment 10 David Cook 2023-07-20 03:31:43 UTC
Note: You can test "too_many" error by changing the circ rule "Renewals allowed (count)" to 0.

With the patch, it will say "auto_too_soon" until the the earliest renewal date is available. And then it will say "too_many". 

This should be in line with what Lucas would want in the original description. It's also what all my libraries have wanted over the years.
Comment 11 Sam Lau 2023-07-20 15:39:45 UTC
Created attachment 153732 [details] [review]
Bug 31427: Get auto renewal errors before other renewal errors

This patch changes CanBookBeRenewed so that automatic renewal
errors pop up before other renewal errors. This means that a book
will be considered "auto_too_soon" before things like "too_many" or
"restricted". (Otherwise, you'll get an email saying you can't renew
a book the day after using your last auto renewal, even though the
earliest renewal isn't available until later.)

Test plan:
0. Apply patch
1. prove t/db_dependent/Circulation.t
2. prove t/db_dependent/Holds.t
3. prove t/db_dependent/Koha/Account/Line.t
4. prove t/db_dependent/Koha/Account.t

Additional tests:
5. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=RestrictionBlockRenewing
6. Change to "block"
7. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=AutoRenewalNotices
8. Change to "according to patron messaging preferences"
9. Go to http://localhost:8081/cgi-bin/koha/admin/smart-rules.pl
10. Set "Automatic renewal" to "Yes" and "No renewal before" to 4
11. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51
12. Checkout 39999000001310 with a due date 4 days in the future
13. Add a manual restriction
14. Run `perl ./misc/cronjobs/automatic_renewals.pl`
15. Note that it says something like the following:
Issue id: 1237 for borrower: 51 and item: 73 would not be renewed. (auto_too_soon)

Instead of something like the following:
Issue id: 1237 for borrower: 51 and item: 73 would not be renewed. (restriction)

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 12 Martin Renvoize (ashimema) 2023-09-22 09:23:58 UTC
I would actually say this isn't an enhancement at all.. it's a regression fix.

Bug 19014 mentions exactly this sort of issue as part of what it was attempting to resolve.

However, we need to be very careful we don't cause an alternative regression.. I think the auto checks need to be conditional to on top of them being moved.
Comment 13 Martin Renvoize (ashimema) 2023-09-26 14:15:19 UTC
Created attachment 156232 [details] [review]
Bug 31427: Get auto renewal errors before other renewal errors

This patch changes CanBookBeRenewed so that automatic renewal
errors pop up before other renewal errors. This means that a book
will be considered "auto_too_soon" before things like "too_many" or
"restricted". (Otherwise, you'll get an email saying you can't renew
a book the day after using your last auto renewal, even though the
earliest renewal isn't available until later.)

Test plan:
0. Apply patch
1. prove t/db_dependent/Circulation.t
2. prove t/db_dependent/Holds.t
3. prove t/db_dependent/Koha/Account/Line.t
4. prove t/db_dependent/Koha/Account.t

Additional tests:
5. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=RestrictionBlockRenewing
6. Change to "block"
7. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=AutoRenewalNotices
8. Change to "according to patron messaging preferences"
9. Go to http://localhost:8081/cgi-bin/koha/admin/smart-rules.pl
10. Set "Automatic renewal" to "Yes" and "No renewal before" to 4
11. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51
12. Checkout 39999000001310 with a due date 4 days in the future
13. Add a manual restriction
14. Run `perl ./misc/cronjobs/automatic_renewals.pl`
15. Note that it says something like the following:
Issue id: 1237 for borrower: 51 and item: 73 would not be renewed. (auto_too_soon)

Instead of something like the following:
Issue id: 1237 for borrower: 51 and item: 73 would not be renewed. (restriction)

Signed-off-by: Sam Lau <samalau@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize (ashimema) 2023-09-26 14:16:15 UTC
I think this all works as I would expect.. however I think we need a unit test to try and prevent future regression again in this complex and frequently flip-flopping piece of code.
Comment 15 Martin Renvoize (ashimema) 2023-09-26 14:16:59 UTC
Sorry David.. if you find yourself struggling on unit tests I can try to find a moment.. I must admit, I've not looked at what already exists for these methods.
Comment 16 David Cook 2023-09-26 23:36:15 UTC
(In reply to Martin Renvoize from comment #14)
> I think this all works as I would expect.. however I think we need a unit
> test to try and prevent future regression again in this complex and
> frequently flip-flopping piece of code.

(In reply to Martin Renvoize from comment #15)
> Sorry David.. if you find yourself struggling on unit tests I can try to
> find a moment.. I must admit, I've not looked at what already exists for
> these methods.

I was thinking the 4 existing unit tests that cover CanBookBeRenewed would be enough, but I suppose there wasn't a unit test that revealed the problem, so clearly there is at least 1 unit test missing...

This one is pretty low on my priority list, as I have a different (less optimal) workaround for this problem that I've been using locally for many years. 

I'll get to it eventually but if someone else wants to write that test first I wouldn't complain hehe.
Comment 17 Martin Renvoize (ashimema) 2023-10-26 08:09:26 UTC
Created attachment 157894 [details] [review]
Bug 31427: (follow-up) Unit tests

This patch adds a unit test for error precidence where autorenewals is
involved.

It is not comprehensive however, and I'm a little confused by the logic
around cron vs non-cron handling...
Comment 18 Martin Renvoize (ashimema) 2023-10-26 08:12:17 UTC
I think I need some additional QA help here.. I've started writing a unit test to prevent future regressions, but it's raised more questions for me.

When exactly should an auto-renewal error get passed up.. for 'too_soon' we only pass when called via the cron.. but for other autorenewal errors we pass regardless.. which seems a little odd... if it's a non-cron check of renewability.. should we not skip the auto-renew failure code for those other failures too?
Comment 19 Nick Clemens (kidclamp) 2023-10-26 11:07:57 UTC
(In reply to Martin Renvoize from comment #18)
> I think I need some additional QA help here.. I've started writing a unit
> test to prevent future regressions, but it's raised more questions for me.
> 
> When exactly should an auto-renewal error get passed up.. for 'too_soon' we
> only pass when called via the cron.. but for other autorenewal errors we
> pass regardless.. which seems a little odd... if it's a non-cron check of
> renewability.. should we not skip the auto-renew failure code for those
> other failures too?

I think part of the issue here is that before bug 25393 an issue being marked as 'auto_renew' in the issues table meant that it was not allowed to be renewed via staff/opac at all - so passing the auto_renew_error codes back was realy for information purposes.

After that bug, I think you are right, the auto renew specific errors should not be returned when the check is not coming from the cron.

Looking at the logic, I think we can actually move all of the auto_renew checks to the auto_renew checks to _CanBookBeAutoRenewed and have that routine call CanBookBeRenewed - rather than having to check if we are coming from the cron.

So I think the order of blocks are:

For all:
no_item
no_checkout
onsite_checkout
item_issued_to_other_patron
item_denied_renewal

For auto_renew cron:
auto_account_expired
auto_too_late
auto_too_much_oweing
auto_too_soon
too_many
too_unseen
restriction
overdue
recalled
on_reserve
too_soon

For staff:
too_many
too_unseen
restriction
overdue
recalled
on_reserve
too_soon

For opac:
OPACFineNoRenewals - checked in script opac-user.pl
BlockExpiredPatronActions - checked in script opac-renew.pl
too_many
too_unseen
restriction
overdue
recalled
on_reserve
too_soon

'too_unseen' for staff actually feels wrong as well, staff side renewals will succeed, but passing this is useful in that it reminds staff they are supposed to 'see' the item?

Bug 34924 will have an effect here too, and I think there are rabbit holes all around this because sometimes the 'error' is trying to give information, and sometimes actually blocking (like the too_unseen above)
Comment 20 Katrin Fischer 2023-11-09 20:40:24 UTC
It sounds from the last comment like this needs a little more work?
Comment 21 Martin Renvoize (ashimema) 2023-11-09 21:25:29 UTC
Yes and no.. the patch here, and accompanying unit test, does enough to fix the issue as it is.. but Nick's suggestion is a really valid one and I think it's the way forward. I just have no idea when I'll find a moment to spend on it .
Comment 22 Marcel de Rooy 2023-11-10 09:37:00 UTC
No Assignee. Please always use this field, David :)
Comment 23 Marcel de Rooy 2023-11-10 09:46:58 UTC
 WARN   t/db_dependent/Circulation.t
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 1438, now: 1449)
Comment 24 Marcel de Rooy 2023-11-10 09:47:27 UTC
        * Proving /usr/share/koha/t/db_dependent/Circulation.t KO!
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Incorrect datetime value: 'SCALAR(0x5627926767b8)' for column `koha_myclone`.`borrowers`.`updated_on` at row 1 at /usr/share/koha/Koha/Object.pm line 170
     # No tests run!

#   Failed test 'No tests run for subtest "CanBookBeRenewed tests"'
#   at /usr/share/koha/t/db_dependent/Circulation.t line 1729.
Invalid value passed, borrowers.updated_on=SCALAR(0x5627926767b8) expected type is datetime # Looks like your test exited with 11 just after 17.
 /usr/share/koha/t/db_dependent/Circulation.t ..
Comment 25 David Cook 2023-11-13 23:12:22 UTC
(In reply to Marcel de Rooy from comment #22)
> No Assignee. Please always use this field, David :)

If you look at the history, you'll see that I did back in July, Marcel :)

Last week Martin changed it to him and then to the default. 

At the moment, I don't intend to work on this further. I've been trying to get this issue fixed for years, but autorenewals is a moving target. That is, it's constantly changing. Perhaps Nick's ideas in Comment 19 are a better way to go. 

Resetting assignee to default in any case.
Comment 26 Martin Renvoize (ashimema) 2023-11-14 08:24:42 UTC
The reset to default on my part I think was a page reload or bz conflict.  I grabbed it from David knowing he'd said he didn't have time to work on it.

I intend to come back here soon and work through Nick's suggestion.. we discussed it out if band and both agreed it needed a little more than we currently have here to ensure we don't end up regressing again.
Comment 27 Martin Renvoize (ashimema) 2024-02-16 15:15:35 UTC
Bug  34924 is now in Koha.. is it time to revisit this
Comment 28 Martin Renvoize (ashimema) 2024-02-16 15:32:19 UTC
Created attachment 162239 [details] [review]
Bug 31427: Get auto renewal errors before other renewal errors

This patch changes CanBookBeRenewed so that automatic renewal
errors pop up before other renewal errors. This means that a book
will be considered "auto_too_soon" before things like "too_many" or
"restricted". (Otherwise, you'll get an email saying you can't renew
a book the day after using your last auto renewal, even though the
earliest renewal isn't available until later.)

Test plan:
0. Apply patch
1. prove t/db_dependent/Circulation.t
2. prove t/db_dependent/Holds.t
3. prove t/db_dependent/Koha/Account/Line.t
4. prove t/db_dependent/Koha/Account.t

Additional tests:
5. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=RestrictionBlockRenewing
6. Change to "block"
7. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=AutoRenewalNotices
8. Change to "according to patron messaging preferences"
9. Go to http://localhost:8081/cgi-bin/koha/admin/smart-rules.pl
10. Set "Automatic renewal" to "Yes" and "No renewal before" to 4
11. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51
12. Checkout 39999000001310 with a due date 4 days in the future
13. Add a manual restriction
14. Run `perl ./misc/cronjobs/automatic_renewals.pl`
15. Note that it says something like the following:
Issue id: 1237 for borrower: 51 and item: 73 would not be renewed. (auto_too_soon)

Instead of something like the following:
Issue id: 1237 for borrower: 51 and item: 73 would not be renewed. (restriction)

Signed-off-by: Sam Lau <samalau@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize (ashimema) 2024-02-16 15:32:22 UTC
Created attachment 162240 [details] [review]
Bug 31427: (follow-up) Unit tests

This patch adds a unit test for error precidence where autorenewals is
involved.

It is not comprehensive however, and I'm a little confused by the logic
around cron vs non-cron handling...
Comment 30 Martin Renvoize (ashimema) 2024-02-16 15:33:01 UTC
Rebased.. I still think this is important
Comment 31 Marcel de Rooy 2024-03-01 09:00:43 UTC
Still having trouble with tests:

Running tests (1)
        * Proving /usr/share/koha/t/db_dependent/Circulation.t KO!
DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.  SQL that returns multiple rows is DEPRECATED for ->find and ->single at /usr/share/koha/Koha/Objects.pm line 96
 DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.  SQL that returns multiple rows is DEPRECATED for ->find and ->single at /usr/share/koha/Koha/Objects.pm line 96
 DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.  SQL that returns multiple rows is DEPRECATED for ->find and ->single at /usr/share/koha/Koha/Objects.pm line 96
 DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.  SQL that returns multiple rows is DEPRECATED for ->find and ->single at /usr/share/koha/Koha/Objects.pm line 96
     # Looks like you planned 9 tests but ran 3.

#   Failed test 'transferbook tests'
#   at /usr/share/koha/t/db_dependent/Circulation.t line 5754.
 Exception 'Koha::Exceptions::MissingParameter' thrown 'The to parameter is mandatory'
 # Looks like your test exited with 11 just after 57.
 /usr/share/koha/t/db_dependent/Circulation.t ..
Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 12/68 subtests

Test Summary Report
-------------------
 /usr/share/koha/t/db_dependent/Circulation.t (Wstat: 2816 Tests: 57 Failed: 1)
  Failed test:   57
   Non-zero exit status: 11
  Parse errors: Bad plan.  You planned 68 tests but ran 57.
 Files=1, Tests=57, 41 wallclock secs ( 0.11 usr  0.01 sys + 32.32 cusr  4.83 csys = 37.27 CPU)
 Result: FAIL
Comment 32 Marcel de Rooy 2024-03-01 09:43:59 UTC
Needs a little digging. Test fails without this patch too.
And culprit is transferbook:

    my $item = Koha::Items->find( { barcode => $barcode } );

If barcode is empty and you have items without barcode, things go wrong.

Solved it on bug 36212. Chose to not record as dependency.
Comment 33 Marcel de Rooy 2024-03-01 10:08:32 UTC
Another related observation in CanBookBeRenewed:

    if ( $auto_renew eq 'auto_too_soon' ) {

        # If its cron, tell it it's too soon for a an auto renewal
        return ( 0, $auto_renew, { soonest_renew_date => $soonest } ) if $cron;

        # Check if it's too soon for a manual renewal
        my $soonestManual = GetSoonestRenewDate( $patron, $issue );
        if ( $soonestManual > dt_from_string() ) {
            return ( 0, "too_soon", { soonest_renew_date => $soonestManual } ) unless $override_limit;
        }
    }

    $soonest = GetSoonestRenewDate($patron, $issue);
    if ( $soonest > dt_from_string() ){
        return (0, "too_soon", { soonest_renew_date => $soonest } ) unless $override_limit;
    }


When we have auto_too_soon, we might be calling GetSoonestRenewDate twice here?
Comment 34 Marcel de Rooy 2024-03-01 10:17:00 UTC
Koha::Checkout

sub attempt_auto_renew {
    my ( $self, $params ) = @_;
    my $confirm = $params->{confirm} // 0;

    # CanBookBeRenewed returns 'auto_renew' when the renewal should be done by this script
    my ( $ok, $error ) = C4::Circulation::CanBookBeRenewed( $self->patron, $self, undef, 1 );

After this call the $ok result is not even checked?!
Could be 'fine' but does not look good in general.


Related observation, going out of scope. In the same routine:
        my $updated = 0;
        if ( !$self->auto_renew_error || $error ne $self->auto_renew_error ) {
            $updated = 1
                unless (
                $self->auto_renew_error
                && (   $self->auto_renew_error eq 'auto_renew_final' && $error eq 'too_many'
                    || $self->auto_renew_error eq 'auto_unseen_final' && $error eq 'too_unseen' )
                );
            $self->auto_renew_error($error)->store if $confirm;
        }
        return ( 0, $error, $updated );
This does not look good either. $updated is passed back but $confirm is not checked?
Comment 35 Marcel de Rooy 2024-03-01 10:33:13 UTC
Created attachment 162656 [details] [review]
Bug 31427: Get auto renewal errors before other renewal errors

This patch changes CanBookBeRenewed so that automatic renewal
errors pop up before other renewal errors. This means that a book
will be considered "auto_too_soon" before things like "too_many" or
"restricted". (Otherwise, you'll get an email saying you can't renew
a book the day after using your last auto renewal, even though the
earliest renewal isn't available until later.)

Test plan:
0. Apply patch
1. prove t/db_dependent/Circulation.t
2. prove t/db_dependent/Holds.t
3. prove t/db_dependent/Koha/Account/Line.t
4. prove t/db_dependent/Koha/Account.t

Additional tests:
5. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=RestrictionBlockRenewing
6. Change to "block"
7. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=AutoRenewalNotices
8. Change to "according to patron messaging preferences"
9. Go to http://localhost:8081/cgi-bin/koha/admin/smart-rules.pl
10. Set "Automatic renewal" to "Yes" and "No renewal before" to 4
11. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51
12. Checkout 39999000001310 with a due date 4 days in the future
13. Add a manual restriction
14. Run `perl ./misc/cronjobs/automatic_renewals.pl`
15. Note that it says something like the following:
Issue id: 1237 for borrower: 51 and item: 73 would not be renewed. (auto_too_soon)

Instead of something like the following:
Issue id: 1237 for borrower: 51 and item: 73 would not be renewed. (restriction)

Signed-off-by: Sam Lau <samalau@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Marcel de Rooy 2024-03-01 10:33:16 UTC
Created attachment 162657 [details] [review]
Bug 31427: (follow-up) Unit tests

This patch adds a unit test for error precidence where autorenewals is
involved.

It is not comprehensive however, and I'm a little confused by the logic
around cron vs non-cron handling...

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 37 Marcel de Rooy 2024-03-01 10:33:19 UTC
Created attachment 162658 [details] [review]
Bug 31427: Remove a few refs to CanBookBeRenewed

Actually, the module is not even needed anymore here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 38 Marcel de Rooy 2024-03-01 10:34:10 UTC
Looks good to me. Checked references to CanBookBeRenewed and ->attempt_auto_renew.

[OK] prove t/db_dependent/Holds.t t/db_dependent/Koha/Account/Line.t t/db_dependent/Koha/Account.t

Could be in test plan too:
prove t/db_dependent/Koha/Checkouts.t
t/db_dependent/Koha/Checkouts.t .. ok
All tests successful.
Files=1, Tests=12,  7 wallclock secs ( 0.03 usr  0.01 sys +  5.42 cusr  0.87 csys =  6.33 CPU)
Result: PASS

C4/SIP/ILS/Transaction/Renew.pm
sub do_renew_for
The auto_xxx codes from CanBookBeRenewed are not listed here, but seem to be passed through as-is.

Added a follow-up where CanBookBeRenewed is no longer called.
Comment 39 Katrin Fischer 2024-03-07 15:37:42 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 40 Fridolin Somers 2024-03-11 10:04:53 UTC
Pushed to 23.11.x for 23.11.04
Comment 41 Lucas Gass (lukeg) 2024-03-19 19:41:02 UTC
Doesn't apply cleanly to 23.05.x, no backport. Please rebase if needed/wanted in 23.05.