Bug 34258 - Cannot renew item via SIP2
Summary: Cannot renew item via SIP2
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: 23.05
Hardware: All All
: P5 - low major (vote)
Assignee: Andreas Roussos
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 34846
  Show dependency treegraph
 
Reported: 2023-07-12 09:30 UTC by Andreas Roussos
Modified: 2023-09-20 13:36 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.03


Attachments
Bug 34258: pass an unblessed hash to AddIssue() (1.91 KB, patch)
2023-07-13 16:25 UTC, Andreas Roussos
Details | Diff | Splinter Review
Bug 34258: update SIP-related unit test (1.97 KB, patch)
2023-07-13 16:25 UTC, Andreas Roussos
Details | Diff | Splinter Review
Bug 34258: pass an unblessed hash to AddIssue() (1.98 KB, patch)
2023-07-13 18:38 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34258: update SIP-related unit test (2.04 KB, patch)
2023-07-13 18:38 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34258: pass an unblessed hash to AddIssue() (2.04 KB, patch)
2023-07-19 14:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34258: update SIP-related unit test (2.11 KB, patch)
2023-07-19 14:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34258: (QA follow-up) Tidy the test (1.87 KB, patch)
2023-07-19 14:21 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Roussos 2023-07-12 09:30:33 UTC
The ability to renew an item via SIP2 broke with version 23.05.

To reproduce, run these commands (tailored for a KTD instance,
so adjust them accordingly if necessary):

misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -su term1 -sp term1 -m checkout --patron koha --item 3999900000001
misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -su term1 -sp term1 -m renew --patron koha --item 3999900000001

The last command will fail, and in /var/log/koha/kohadev/sip.log
you will get:

Use of uninitialized value in string eq at /kohadevbox/koha/C4/Circulation.pm line 1540
Comment 1 Andreas Roussos 2023-07-12 09:32:00 UTC
The related commit is ddc2906b77 from Bug 31735, where the file
C4/SIP/ILS/Transaction/Renew.pm was modified in the following way:

@@ -64,7 +66,7 @@ sub do_renew {
     my $self = shift;
     my $patron = Koha::Patrons->find( $self->{patron}->borrowernumber );
     $patron or return; # FIXME we should log that
-    return $self->do_renew_for($patron->unblessed);
+    return $self->do_renew_for($patron);
 }

As per https://perldoc.koha-community.org/C4/Circulation.html#AddIssue,
AddIssue() expects its 1st argument ($borrower) to be " a hash with
borrower informations (from Koha::Patron->unblessed)."

This is trivial to fix, so expect a patch and updated unit tests soon.
Comment 2 Andreas Roussos 2023-07-13 07:12:09 UTC
Apparently, there's an earlier bug (Bug 32496, currently in "Signed Off"
status) that attempts to "Reduce unnecessary unblessings of objects in
Circulation.pm".

Applying the changes from Bug 32496 fixes the problem with SIP2 renewals,
so I'm marking my bug report as resolved.
Comment 3 Jonathan Druart 2023-07-13 07:57:33 UTC
Bug 32496 should not be backported IMO, so this is valid for stable versions.
Comment 4 Andreas Roussos 2023-07-13 16:22:11 UTC
(In reply to Jonathan Druart from comment #3)
> Bug 32496 should not be backported IMO, so this is valid for stable versions.
Thanks Jonathan, that makes sense. I'll re-open this report and upload my patches.
Comment 5 Andreas Roussos 2023-07-13 16:25:30 UTC
Created attachment 153416 [details] [review]
Bug 34258: pass an unblessed hash to AddIssue()

In Koha 23.05, we lost the ability to renew an item via SIP2.

The relevant commit is ddc2906b77 from Bug 31735, where the
file C4/SIP/ILS/Transaction/Renew.pm was modified to no longer
pass an unblessed $patron hash to C4::Circulation::AddIssue()

This patch fixes that.

Test plan:

1) Using the SIP emulator, check out an item to a patron, then
   try to renew it. Example commands for a KTD instance:

   $ misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -su term1 -sp term1 -m checkout --patron koha --item 3999900000001
   $ misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -su term1 -sp term1 -m renew --patron koha --item 3999900000001

   Notice that the second command will fail!

2) Apply this patch.

3) Repeat the 2nd command -- this time the renewal should work.

4) Run the SIP-related unit tests, they should all pass:

   $ prove t/db_dependent/SIP/
   t/db_dependent/SIP/ILS.t .......... ok
   t/db_dependent/SIP/Message.t ...... ok
   t/db_dependent/SIP/Patron.t ....... ok
   t/db_dependent/SIP/SIPServer.t .... ok
   t/db_dependent/SIP/Transaction.t .. ok
Comment 6 Andreas Roussos 2023-07-13 16:25:57 UTC
Created attachment 153417 [details] [review]
Bug 34258: update SIP-related unit test

Test plan:

1) Run the updated SIP-related unit test *without* having applied
   the other patch from this bug report -- it should fail:

   $ prove t/db_dependent/SIP/ILS.t

2) Apply the patch that fixes C4/SIP/ILS/Transaction/Renew.pm

3) Re-run the unit test -- it should pass.
Comment 7 Emily Lamancusa 2023-07-13 18:38:10 UTC
Created attachment 153427 [details] [review]
Bug 34258: pass an unblessed hash to AddIssue()

In Koha 23.05, we lost the ability to renew an item via SIP2.

The relevant commit is ddc2906b77 from Bug 31735, where the
file C4/SIP/ILS/Transaction/Renew.pm was modified to no longer
pass an unblessed $patron hash to C4::Circulation::AddIssue()

This patch fixes that.

Test plan:

1) Using the SIP emulator, check out an item to a patron, then
   try to renew it. Example commands for a KTD instance:

   $ misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -su term1 -sp term1 -m checkout --patron koha --item 3999900000001
   $ misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -su term1 -sp term1 -m renew --patron koha --item 3999900000001

   Notice that the second command will fail!

2) Apply this patch.

3) Repeat the 2nd command -- this time the renewal should work.

4) Run the SIP-related unit tests, they should all pass:

   $ prove t/db_dependent/SIP/
   t/db_dependent/SIP/ILS.t .......... ok
   t/db_dependent/SIP/Message.t ...... ok
   t/db_dependent/SIP/Patron.t ....... ok
   t/db_dependent/SIP/SIPServer.t .... ok
   t/db_dependent/SIP/Transaction.t .. ok

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 8 Emily Lamancusa 2023-07-13 18:38:12 UTC
Created attachment 153428 [details] [review]
Bug 34258: update SIP-related unit test

Test plan:

1) Run the updated SIP-related unit test *without* having applied
   the other patch from this bug report -- it should fail:

   $ prove t/db_dependent/SIP/ILS.t

2) Apply the patch that fixes C4/SIP/ILS/Transaction/Renew.pm

3) Re-run the unit test -- it should pass.

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 9 Martin Renvoize 2023-07-19 14:21:42 UTC
Created attachment 153663 [details] [review]
Bug 34258: pass an unblessed hash to AddIssue()

In Koha 23.05, we lost the ability to renew an item via SIP2.

The relevant commit is ddc2906b77 from Bug 31735, where the
file C4/SIP/ILS/Transaction/Renew.pm was modified to no longer
pass an unblessed $patron hash to C4::Circulation::AddIssue()

This patch fixes that.

Test plan:

1) Using the SIP emulator, check out an item to a patron, then
   try to renew it. Example commands for a KTD instance:

   $ misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -su term1 -sp term1 -m checkout --patron koha --item 3999900000001
   $ misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -su term1 -sp term1 -m renew --patron koha --item 3999900000001

   Notice that the second command will fail!

2) Apply this patch.

3) Repeat the 2nd command -- this time the renewal should work.

4) Run the SIP-related unit tests, they should all pass:

   $ prove t/db_dependent/SIP/
   t/db_dependent/SIP/ILS.t .......... ok
   t/db_dependent/SIP/Message.t ...... ok
   t/db_dependent/SIP/Patron.t ....... ok
   t/db_dependent/SIP/SIPServer.t .... ok
   t/db_dependent/SIP/Transaction.t .. ok

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2023-07-19 14:21:45 UTC
Created attachment 153664 [details] [review]
Bug 34258: update SIP-related unit test

Test plan:

1) Run the updated SIP-related unit test *without* having applied
   the other patch from this bug report -- it should fail:

   $ prove t/db_dependent/SIP/ILS.t

2) Apply the patch that fixes C4/SIP/ILS/Transaction/Renew.pm

3) Re-run the unit test -- it should pass.

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2023-07-19 14:21:47 UTC
Created attachment 153665 [details] [review]
Bug 34258: (QA follow-up) Tidy the test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2023-07-19 14:22:15 UTC
All working, tests passing.. Thanks for the quick backportable fix.

Passing QA
Comment 13 Tomás Cohen Arazi 2023-07-26 14:19:02 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 14 Fridolin Somers 2023-08-04 02:28:41 UTC
Pushed to 23.05.x for 23.05.03
Comment 15 Pedro Amorim 2023-08-18 11:51:21 UTC
Missing dependencies for 22.11.x. Not pushing.