Bug 29380 - Auto renewing, batch due date extension tool and checkout note previews are broken
Summary: Auto renewing, batch due date extension tool and checkout note previews are b...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical
Assignee: Joonas Kylmälä
QA Contact: Testopia
URL:
Keywords:
Depends on: 29290
Blocks:
  Show dependency treegraph
 
Reported: 2021-10-30 19:02 UTC by Joonas Kylmälä
Modified: 2022-06-06 20:24 UTC (History)
8 users (show)

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


Attachments
Bug 29380: Correct table name in joins to prevent errors (3.14 KB, patch)
2021-10-30 19:14 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29380: Correct table name in joins to prevent errors (3.19 KB, patch)
2021-10-31 10:04 UTC, David Nind
Details | Diff | Splinter Review
Bug 29380: Correct table name in joins to prevent errors (3.25 KB, patch)
2021-11-01 08:16 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 29380: (follow-up) Fix renewal feature in staff interface (1.20 KB, patch)
2021-11-02 20:29 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2021-10-30 19:02:21 UTC
The table joins were not changed to use the new name in bug 29290 so a few scripts got broken.
Comment 1 Joonas Kylmälä 2021-10-30 19:14:34 UTC
Created attachment 127109 [details] [review]
Bug 29380: Correct table name in joins to prevent errors

The commit "Bug 29290: Rename relationships borrower =>
patron" (d46492ac23) renamed the relation for the borrowers table from
'borrower' to 'patron' but the joins were not updated accordingly so a
few scripts got broken.

To test:
 1) Notice that
    $ perl misc/cronjobs/automatic_renewals.pl -c -s -v
    returns 255 error code on exit
 2) Apply patch
 3) Notice the automatic_renewals.pl works now and exit code is 0
 4) Make sure /cgi-bin/koha/tools/batch_extend_due_dates.pl works.
 5) Try to grep for 'borrower' in Koha source code and see if there
    are any other join done using the Koha::Checkout or
    Koha::Old::Checkout objects.
Comment 2 David Nind 2021-10-31 10:04:28 UTC
Created attachment 127126 [details] [review]
Bug 29380: Correct table name in joins to prevent errors

The commit "Bug 29290: Rename relationships borrower =>
patron" (d46492ac23) renamed the relation for the borrowers table from
'borrower' to 'patron' but the joins were not updated accordingly so a
few scripts got broken.

To test:
 1) Notice that
    $ perl misc/cronjobs/automatic_renewals.pl -c -s -v
    returns 255 error code on exit
 2) Apply patch
 3) Notice the automatic_renewals.pl works now and exit code is 0
 4) Make sure /cgi-bin/koha/tools/batch_extend_due_dates.pl works.
 5) Try to grep for 'borrower' in Koha source code and see if there
    are any other join done using the Koha::Checkout or
    Koha::Old::Checkout objects.

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Nind 2021-10-31 10:13:52 UTC
Testing notes (using koha-testing-docker in strict SQL mode - the default when starting with ku).

Setup for testing
~~~~~~~~~~~~~~~~~

1. AutoRenewalNotices system preference - set to "according to patron messaging preferences".

2. Checked out some items to patrons (I checked out one item to three different patrons) - when checking out expanded "Checkout settings" and selected automatic renewal and added a date before the current date for "Specify the due date".

3. For those patrons added an email address (name@example.com for example) and changed their patron messaging preferences to enable email notification for auto renewal.

Error message before patch applied
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I didn't get "returns 255 error code on exit", but it clearly wasn't working as I got:

perl misc/cronjobs/automatic_renewals.pl -c -s -v
getting auto renewals
No method count found for Koha::Checkouts DBIx::Class::ResultSource::_resolve_join(): No such relationship borrower on Issue at /kohadevbox/koha/Koha/Objects.pm line 601
 at misc/cronjobs/automatic_renewals.pl line 136.
Use of uninitialized value in concatenation (.) or string at misc/cronjobs/automatic_renewals.pl line 136.
found  auto renewals
DBIx::Class::ResultSource::_resolve_join(): No such relationship borrower on Issue at /kohadevbox/koha/Koha/Objects.pm line 335

After applying patch and running automatic renewals
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

1. Checked the check outs for patrons and made sure items were renewed - noted new due dates.

2. Checked notices for patrons and made sure a notice for automatic renewal was added.

3. Used the batch extend due dates tool (Tools > Patrons and circulation > Batch extend due dates) to extend the due dates for items issued to the 31 December 2021 - the preview listed the items checked out to patrons and the dates were extended.

Step 5 - grep for borrower
~~~~~~~~~~~~~~~~~~~~~~~~~~

Not exactly sure what I was looking for, feel free to change back to sign off if this is not sufficient:

1. Did git greps for Koha::Old::Checkout and Koha::Checkout and saved results to a file. Then searched those files for join and borrower.

2. Did a git grep for borrower and saved results to a file. Searched that file for occurrences of Koha::Old::Checkout and Koha::Checkout that mentioned join and borrower. Only one item I came across but there is no join, so I don't think tat counts:
t/db_dependent/Members/IssueSlip.t:            or diag(Dumper(Koha::Checkouts->search({borrowernumber => $borrower->{borrowernumber}})->unblessed));
Comment 4 Martin Renvoize (ashimema) 2021-11-01 08:16:20 UTC
Created attachment 127143 [details] [review]
Bug 29380: Correct table name in joins to prevent errors

The commit "Bug 29290: Rename relationships borrower =>
patron" (d46492ac23) renamed the relation for the borrowers table from
'borrower' to 'patron' but the joins were not updated accordingly so a
few scripts got broken.

To test:
 1) Notice that
    $ perl misc/cronjobs/automatic_renewals.pl -c -s -v
    returns 255 error code on exit
 2) Apply patch
 3) Notice the automatic_renewals.pl works now and exit code is 0
 4) Make sure /cgi-bin/koha/tools/batch_extend_due_dates.pl works.
 5) Try to grep for 'borrower' in Koha source code and see if there
    are any other join done using the Koha::Checkout or
    Koha::Old::Checkout objects.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize (ashimema) 2021-11-01 08:17:19 UTC
Damn, annoyed I missed this during QA of the cause bug :(.

This appears to catch all cases, I couldn't find any more myself.

Passing QA, thanks for the quick spot and fix Joonas!
Comment 6 Jonathan Druart 2021-11-02 12:16:06 UTC
This one seems valid as well:
circ/renew.pl
66             $borrower = $issue->borrower();

Can't locate object method "borrower" via package "Koha::Schema::Result::Issue" at /kohadevbox/koha/circ/renew.pl line 66
Comment 7 Joonas Kylmälä 2021-11-02 20:29:24 UTC
Created attachment 127226 [details] [review]
Bug 29380: (follow-up) Fix renewal feature in staff interface

The commit "Bug 29290: Rename relationships borrower =>
patron" (d46492ac23) renamed the relation for the borrowers table from
'borrower' to 'patron' but the usage in the renew.pl script was not
updated.

To test:
 1) Try to renew a book in intranet through the renewal tab
 2) Notice it gives error
 3) Apply patch
 4) Notice the error is now gone

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 8 Joonas Kylmälä 2021-11-02 20:30:15 UTC
(In reply to Jonathan Druart from comment #6)
> This one seems valid as well:
> circ/renew.pl

fixed. I grepped another time for similar ones to this but couldn't find more.
Comment 9 Jonathan Druart 2021-11-03 12:30:19 UTC
Patches will be squashed when pushed.
Comment 10 Jonathan Druart 2021-11-03 14:32:20 UTC
Pushed to master for 21.11, thanks to everybody involved!