Bug 30167

Summary: Return soonest renewal date when CanBookBeRenewed returns %too_soon
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: Architecture, internals, and plumbingAssignee: Nick Clemens (kidclamp) <nick>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P5 - low CC: david, fridolin.somers, lucas, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30004
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29537
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00
Circulation function:
Bug Depends on:    
Bug Blocks: 30168    
Attachments: Bug 30167: Return soonest renewal date when renewal is to soon
Bug 30167: Use returned renewal date rather than fetching
Bug 30167: Return soonest renewal date when renewal is to soon
Bug 30167: Use returned renewal date rather than fetching
Bug 30167: Return soonest renewal date when renewal is to soon
Bug 30167: Use returned renewal date rather than fetching
Bug 30167: (follow-up) Return a hash with soonest_renew_date
Bug 30167: Return soonest renewal date when renewal is to soon
Bug 30167: Use returned renewal date rather than fetching
Bug 30167: (follow-up) Return a hash with soonest_renew_date
Bug 30167: Return soonest renewal date when renewal is to soon
Bug 30167: Use returned renewal date rather than fetching
Bug 30167: (follow-up) Return a hash with soonest_renew_date
Bug 30167: (follow-up) Compare dt objects
Bug 30167: [21.11.x] Return soonest renewal date when renewal is to soon

Description Nick Clemens (kidclamp) 2022-02-23 16:01:44 UTC
There are several places where we fetch the soonest renewal date when CanBookBeRenewed returns too soon

We have already calculated this date and should pass it back
Comment 1 Nick Clemens (kidclamp) 2022-02-23 16:15:36 UTC
Created attachment 131046 [details] [review]
Bug 30167: Return soonest renewal date when renewal is to soon

This patch adds an 'info' return param to CanBookBeRenewed and passes
the soonest renewal date when returning too_soon errors

To test:
1 - prove -v t/db_dependent/Circulation.t

Fix whitespace
Comment 2 Nick Clemens (kidclamp) 2022-02-23 16:15:39 UTC
Created attachment 131047 [details] [review]
Bug 30167: Use returned renewal date rather than fetching

This patch updates the three scripts that fetched the soonest renewal date
to use the return from CanBookBeRenewed

To test:
1 - Set a circulation rule with a 'no renewal before' set to 3, loan length set to 5
2 - Check out an item to a patron that uses this rule
3 - Verify the checkouts for the patron show the correct 'No renewal before' date
4 - Sign in to the patron's opac account
5 - Verify the item shows it cannot be renewed, and shows the correct date
6 - Go to Circulation->Renew
7 - Attempt to renew using barcode
8 - Confirm error shows the soonest renewal date
Comment 3 David Nind 2022-04-27 13:50:58 UTC
Patch no longer applies 8-(..

git bz apply 30167

Bug 30167 - Return soonest renewal date when CanBookBeRenewed returns %too_soon

131046 - Bug 30167: Return soonest renewal date when renewal is to soon
131047 - Bug 30167: Use returned renewal date rather than fetching

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 30167: Return soonest renewal date when renewal is to soon
Using index info to reconstruct a base tree...
M	C4/Circulation.pm
M	t/db_dependent/Circulation.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Circulation.t
CONFLICT (content): Merge conflict in t/db_dependent/Circulation.t
Auto-merging C4/Circulation.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 30167: Return soonest renewal date when renewal is to soon
Comment 4 Nick Clemens (kidclamp) 2022-04-27 15:44:24 UTC
Created attachment 134082 [details] [review]
Bug 30167: Return soonest renewal date when renewal is to soon

This patch adds an 'info' return param to CanBookBeRenewed and passes
the soonest renewal date when returning too_soon errors

To test:
1 - prove -v t/db_dependent/Circulation.t

Fix whitespace
Comment 5 Nick Clemens (kidclamp) 2022-04-27 15:44:28 UTC
Created attachment 134083 [details] [review]
Bug 30167: Use returned renewal date rather than fetching

This patch updates the three scripts that fetched the soonest renewal date
to use the return from CanBookBeRenewed

To test:
1 - Set a circulation rule with a 'no renewal before' set to 3, loan length set to 5
2 - Check out an item to a patron that uses this rule
3 - Verify the checkouts for the patron show the correct 'No renewal before' date
4 - Sign in to the patron's opac account
5 - Verify the item shows it cannot be renewed, and shows the correct date
6 - Go to Circulation->Renew
7 - Attempt to renew using barcode
8 - Confirm error shows the soonest renewal date
Comment 6 David Nind 2022-04-28 00:15:06 UTC
Created attachment 134197 [details] [review]
Bug 30167: Return soonest renewal date when renewal is to soon

This patch adds an 'info' return param to CanBookBeRenewed and passes
the soonest renewal date when returning too_soon errors

To test:
1 - prove -v t/db_dependent/Circulation.t

Fix whitespace

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2022-04-28 00:15:11 UTC
Created attachment 134198 [details] [review]
Bug 30167: Use returned renewal date rather than fetching

This patch updates the three scripts that fetched the soonest renewal date
to use the return from CanBookBeRenewed

To test:
1 - Set a circulation rule with a 'no renewal before' set to 3, loan length set to 5
2 - Check out an item to a patron that uses this rule
3 - Verify the checkouts for the patron show the correct 'No renewal before' date
4 - Sign in to the patron's opac account
5 - Verify the item shows it cannot be renewed, and shows the correct date
6 - Go to Circulation->Renew
7 - Attempt to renew using barcode
8 - Confirm error shows the soonest renewal date

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2022-04-28 00:24:05 UTC
Testing notes (koha-testing-docker):

- Tested on 04/28/2022 (American date format) with 
  . 'no renewal before' set to 3
  . 'loan period' set to 5
- Checked out on date is 04/28/2022 00:09 
- For the patron from the checkouts: No renewal before 04/30/2022
- When trying to renew, the message is: "Cannot renew:
    Programming Perl / ( 39999000011418 ) cannot be renewed before 04/30/2022. "
- Date is calculated correctly including the current day (3 days - 28th, 29th + 30th)
Comment 9 Marcel de Rooy 2022-04-29 09:46:29 UTC
+  ($ok,$error,$info) = &CanBookBeRenewed($borrowernumber, $itemnumber[, $override_limit]);

 Find out whether a borrowed item may be renewed.

+    $info will contain the soonest renewal date if the error is 'too soon'

I would suggest to let $info either be a hash containing a key for soonest renewal date (among others, room for extension) in order to honor its name or rename it to soonestrenewaldate. This is somewhat obsure.
Comment 10 Katrin Fischer 2022-04-29 11:00:47 UTC
Changing status to get some feedback on Marcel's last comment.
Comment 11 Nick Clemens (kidclamp) 2022-04-29 13:01:35 UTC
Created attachment 134380 [details] [review]
Bug 30167: (follow-up) Return a hash with soonest_renew_date
Comment 12 Marcel de Rooy 2022-05-02 06:56:25 UTC
 FAIL   koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt
   FAIL   valid_template
                Attempt to reload Koha/Template/Plugin/Branches.pm aborted.
Compilation failed in require at /usr/lib/x86_64-linux-gnu/perl5/5.28/Template/Plugins.pm line 206.

I am going to ignore this one.
It pops up now and then on some reports. And several people have not succeeded in finding the exact cause.
See also https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27944#c46
Comment 13 Marcel de Rooy 2022-05-02 07:19:14 UTC
Created attachment 134454 [details] [review]
Bug 30167: Return soonest renewal date when renewal is to soon

This patch adds an 'info' return param to CanBookBeRenewed and passes
the soonest renewal date when returning too_soon errors

To test:
1 - prove -v t/db_dependent/Circulation.t

Fix whitespace

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2022-05-02 07:19:18 UTC
Created attachment 134455 [details] [review]
Bug 30167: Use returned renewal date rather than fetching

This patch updates the three scripts that fetched the soonest renewal date
to use the return from CanBookBeRenewed

To test:
1 - Set a circulation rule with a 'no renewal before' set to 3, loan length set to 5
2 - Check out an item to a patron that uses this rule
3 - Verify the checkouts for the patron show the correct 'No renewal before' date
4 - Sign in to the patron's opac account
5 - Verify the item shows it cannot be renewed, and shows the correct date
6 - Go to Circulation->Renew
7 - Attempt to renew using barcode
8 - Confirm error shows the soonest renewal date

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2022-05-02 07:19:23 UTC
Created attachment 134456 [details] [review]
Bug 30167: (follow-up) Return a hash with soonest_renew_date

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2022-05-02 07:20:53 UTC
Not sure about 'major' btw. More like an enhancement.
Comment 17 Nick Clemens (kidclamp) 2022-05-03 11:16:28 UTC
(In reply to Marcel de Rooy from comment #16)
> Not sure about 'major' btw. More like an enhancement.

could be less than major - but can affect performance, so not an enh, should be eligible for backport
Comment 18 Fridolin Somers 2022-05-03 21:18:56 UTC
Sorry, does not apply any anymore since Bug 29537 was pushed
Comment 19 Nick Clemens (kidclamp) 2022-05-04 11:38:40 UTC
Created attachment 134576 [details] [review]
Bug 30167: Return soonest renewal date when renewal is to soon

This patch adds an 'info' return param to CanBookBeRenewed and passes
the soonest renewal date when returning too_soon errors

To test:
1 - prove -v t/db_dependent/Circulation.t

Fix whitespace

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Nick Clemens (kidclamp) 2022-05-04 11:38:45 UTC
Created attachment 134577 [details] [review]
Bug 30167: Use returned renewal date rather than fetching

This patch updates the three scripts that fetched the soonest renewal date
to use the return from CanBookBeRenewed

To test:
1 - Set a circulation rule with a 'no renewal before' set to 3, loan length set to 5
2 - Check out an item to a patron that uses this rule
3 - Verify the checkouts for the patron show the correct 'No renewal before' date
4 - Sign in to the patron's opac account
5 - Verify the item shows it cannot be renewed, and shows the correct date
6 - Go to Circulation->Renew
7 - Attempt to renew using barcode
8 - Confirm error shows the soonest renewal date

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Nick Clemens (kidclamp) 2022-05-04 11:38:49 UTC
Created attachment 134578 [details] [review]
Bug 30167: (follow-up) Return a hash with soonest_renew_date

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Nick Clemens (kidclamp) 2022-05-04 11:38:54 UTC
Created attachment 134579 [details] [review]
Bug 30167: (follow-up) Compare dt objects
Comment 23 Fridolin Somers 2022-05-05 00:32:34 UTC
Pushed to master for 22.05, thanks to everybody involved ﷐[U+1F984]﷑
Comment 24 Nick Clemens (kidclamp) 2022-05-16 11:08:47 UTC
Created attachment 135016 [details] [review]
Bug 30167: [21.11.x] Return soonest renewal date when renewal is to soon

This patch adds an 'info' return param to CanBookBeRenewed and passes
the soonest renewal date when returning too_soon errors

To test:
1 - prove -v t/db_dependent/Circulation.t

Fix whitespace

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Bug 30167: Use returned renewal date rather than fetching

This patch updates the three scripts that fetched the soonest renewal date
to use the return from CanBookBeRenewed

To test:
1 - Set a circulation rule with a 'no renewal before' set to 3, loan length set to 5
2 - Check out an item to a patron that uses this rule
3 - Verify the checkouts for the patron show the correct 'No renewal before' date
4 - Sign in to the patron's opac account
5 - Verify the item shows it cannot be renewed, and shows the correct date
6 - Go to Circulation->Renew
7 - Attempt to renew using barcode
8 - Confirm error shows the soonest renewal date

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Bug 30167: (follow-up) Return a hash with soonest_renew_date

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Bug 30167: (follow-up) Compare dt objects