Bug 26108 - Checkins should not require item to have been checked out
Summary: Checkins should not require item to have been checked out
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 25855
Blocks:
  Show dependency treegraph
 
Reported: 2020-07-30 19:02 UTC by Andrew Fuerste-Henry
Modified: 2020-11-30 21:52 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
20.11.00


Attachments
Bug 26108: (bug 25855 follow-up) Call after_circ_action hook only if issue exists (1.49 KB, patch)
2020-07-31 07:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26108: Regression tests (1.97 KB, patch)
2020-07-31 12:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26108: Only call the plugin hook if a return took place (1.82 KB, patch)
2020-07-31 12:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2020-07-30 19:02:40 UTC
Koha is giving an internal server error when trying to check in an item that was not checked out. Possibly due to bug 26063?
Comment 1 Jonathan Druart 2020-07-31 07:47:24 UTC
It's coming from bug 25855 actually.
Comment 2 Jonathan Druart 2020-07-31 07:51:55 UTC
Created attachment 107618 [details] [review]
Bug 26108: (bug 25855 follow-up) Call after_circ_action hook only if issue exists

AddReturn can be called on an item even if it's not checked out (to
trigger holds for instance).
The hook should (?) not be called in that situation

Test plan:
Confirm the above and that the following tests are now passing:
 t/db_dependent/SIP/Message.t
 t/db_dependent/Reserves.t
 t/db_dependent/Circulation/issue.t
 t/db_dependent/SIP/Transaction.t
 t/db_dependent/Circulation.t
Comment 3 Jonathan Druart 2020-07-31 08:57:32 UTC
I have pushed this patch to fix master and not block people who would test other patches today. But a SO and QA is required to confirm it's the expected behaviour.
Comment 4 Tomás Cohen Arazi 2020-07-31 12:56:07 UTC
Created attachment 107640 [details] [review]
Bug 26108: Regression tests

The current implementation makes the assumption that if $issue is
defined, then it is the 'return' use case. This is correct, but
incomplete: after verifying that there's actually an issue, the return
can be rejected for various reasons.

This patch introduces a regression test for one of those situations
(which I consider enough for testing purposes) . It highlights that the
hook shouldn't be called if there hasn't been an actual checkin.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Plugins/Circulation_hooks.t
=> FAIL: Tests fail because there's an unexpected warining printed by
the erroneous plugin call (i.e. there's been no return but the plugin is
called anyway).

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2020-07-31 12:56:11 UTC
Created attachment 107641 [details] [review]
Bug 26108: Only call the plugin hook if a return took place

This patch makes the hooks be called only if the checkin is effective
(i.e. if it is not rejected for some reason.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Plugins/Circulation_hooks.t
=> FAIL: Tests fail
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Jonathan Druart 2020-08-03 09:29:27 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 7 Lucas Gass 2020-08-13 16:20:29 UTC
Missing dependency for 20.05.x, no backport