Summary: | CheckPrevCheckout should also check current loans | ||
---|---|---|---|
Product: | Koha | Reporter: | Tuomas Kunttu <tuomas.kunttu> |
Component: | Circulation | Assignee: | Emmi Takkinen <emmi.takkinen> |
Status: | Pushed to main --- | QA Contact: | Lucas Gass (lukeg) <lucas> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | emmi.takkinen, felicie.thiery, gmcharlt, jonathan.druart, kyle.m.hall, lisette, lucas |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | Small patch | Documentation contact: | |
Documentation submission: | Text to go in the release notes: |
If `AllowMultipleIssuesOnABiblio` is enabled patrons can checkout multiple items from same record but these check outs were not checked by Koha. This patch changes `CheckPrevCheckout` system preference functionality so that it also checks patrons current check outs and displays new confirmation message "Patron has this title currently checked out:..." so that librarians now have to confirm if item will be check out to patron.
|
|
Version(s) released in: |
25.05.00
|
Circulation function: | |
Attachments: |
Bug 38356: Check patrons current checkouts separately
Bug 38356: Check patrons current checkouts separately Bug 38356: Check patrons current checkouts separately Bug 38356: (QA follow-up) Fix spelling mistake (follow-up) Bug 38356: Fix failing Circulation.t tests Bug 38356: (follow-up) Fix failing Circulation.t tests Bug 38356: (follow-up) Add code block to handle CURRENTISSUE message |
Description
Tuomas Kunttu
2024-11-05 13:04:09 UTC
Created attachment 180239 [details] [review] Bug 38356: Check patrons current checkouts separately If syspref CheckPrevCheckout is enabled, Koha checks if patron has previously checked out item within timeperiod set in syspref CheckPrevCheckoutDelay. This however doesn't take into account cases where item is currently checked out for patron and they are trying to check out another item from same record (can be done if syspref AllowMultipleIssuesOnABiblio is enabled). We should always check if item is currently checked out for the patron if CheckPrevCheckout is enabled. To test: 1. Enable syspref CheckPrevCheckout and set CheckPrevCheckoutDelay as e.g. 10. Also enable syspref AllowMultipleIssuesOnABiblio. 2. Find a record that has been checked out for patron over 10 days ago. 3. Attempt to check out item from same record for same patron. => Note that you have to confirm if you want to check out item for the patron. 4. Find record that is currently checked out for a patron. 5. Attempt to check out item from same record for same patron. => Note that item is checked out for patron without confirm message. 6. Apply this patch and restart services if needed. 7. Repeat steps 2. and 3. => Confirm that you still have to confirm if you want to check out item for the patron. 8. Repeat steps 4. and 5. => New confirm message "Patron has this title currently checked out:..." should be displayed and you have to confirm if you want to check out item for the patron. Also prove t/db_dependent/Patron/Borrower_PrevCheckout.t Sponsored-by: Koha-Suomi Oy Created attachment 180305 [details] [review] Bug 38356: Check patrons current checkouts separately If syspref CheckPrevCheckout is enabled, Koha checks if patron has previously checked out item within timeperiod set in syspref CheckPrevCheckoutDelay. This however doesn't take into account cases where item is currently checked out for patron and they are trying to check out another item from same record (can be done if syspref AllowMultipleIssuesOnABiblio is enabled). We should always check if item is currently checked out for the patron if CheckPrevCheckout is enabled. To test: 1. Enable syspref CheckPrevCheckout and set CheckPrevCheckoutDelay as e.g. 10. Also enable syspref AllowMultipleIssuesOnABiblio. 2. Find a record that has been checked out for patron over 10 days ago. 3. Attempt to check out item from same record for same patron. => Note that you have to confirm if you want to check out item for the patron. 4. Find record that is currently checked out for a patron. 5. Attempt to check out item from same record for same patron. => Note that item is checked out for patron without confirm message. 6. Apply this patch and restart services if needed. 7. Repeat steps 2. and 3. => Confirm that you still have to confirm if you want to check out item for the patron. 8. Repeat steps 4. and 5. => New confirm message "Patron has this title currently checked out:..." should be displayed and you have to confirm if you want to check out item for the patron. Also prove t/db_dependent/Patron/Borrower_PrevCheckout.t Sponsored-by: Koha-Suomi Oy Signed-off-by: Felicie <felicie.thiery@biblibre.com> Created attachment 181883 [details] [review] Bug 38356: Check patrons current checkouts separately If syspref CheckPrevCheckout is enabled, Koha checks if patron has previously checked out item within timeperiod set in syspref CheckPrevCheckoutDelay. This however doesn't take into account cases where item is currently checked out for patron and they are trying to check out another item from same record (can be done if syspref AllowMultipleIssuesOnABiblio is enabled). We should always check if item is currently checked out for the patron if CheckPrevCheckout is enabled. To test: 1. Enable syspref CheckPrevCheckout and set CheckPrevCheckoutDelay as e.g. 10. Also enable syspref AllowMultipleIssuesOnABiblio. 2. Find a record that has been checked out for patron over 10 days ago. 3. Attempt to check out item from same record for same patron. => Note that you have to confirm if you want to check out item for the patron. 4. Find record that is currently checked out for a patron. 5. Attempt to check out item from same record for same patron. => Note that item is checked out for patron without confirm message. 6. Apply this patch and restart services if needed. 7. Repeat steps 2. and 3. => Confirm that you still have to confirm if you want to check out item for the patron. 8. Repeat steps 4. and 5. => New confirm message "Patron has this title currently checked out:..." should be displayed and you have to confirm if you want to check out item for the patron. Also prove t/db_dependent/Patron/Borrower_PrevCheckout.t Sponsored-by: Koha-Suomi Oy Signed-off-by: Felicie <felicie.thiery@biblibre.com> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Created attachment 181884 [details] [review] Bug 38356: (QA follow-up) Fix spelling mistake Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Pushed for 25.05! Well done everyone, thank you! git bisect blames this patch, it broke t/db_dependent/SIP/Message.t t/db_dependent/SIP/Message.t .. 1/23 [15/727] # Failed test 'Found AH field as timestamp in response' # at t/db_dependent/SIP/Message.t line 1320. # Failed test 'Found AH field as SQL date in response' # at t/db_dependent/SIP/Message.t line 1325. # Looks like you failed 2 tests of 8. t/db_dependent/SIP/Message.t .. 3/23 # Failed test 'Checkout V2' # at t/db_dependent/SIP/Message.t line 80. # Failed test 'Desensitize flag was set for patron category not in inhouse_patron_categories' # at t/db_dependent/SIP/Message.t line 1717. # got: 'N' # expected: 'Y' # Failed test 'Desensitize flag was set for empty inhouse_patron_categories' # at t/db_dependent/SIP/Message.t line 1723. # got: 'N' # expected: 'Y' # Failed test 'Desensitize flag was set for item type not in inhouse_item_types' # at t/db_dependent/SIP/Message.t line 1737. # got: 'N' # expected: 'Y' # Failed test 'Desensitize flag was set for empty inhouse_item_types' # at t/db_dependent/SIP/Message.t line 1743. # got: 'N' # expected: 'Y' # Looks like you failed 4 tests of 6. t/db_dependent/SIP/Message.t .. 4/23 # Failed test 'Test checkout desensitize' # at t/db_dependent/SIP/Message.t line 89. # Failed test 'Desensitize flag was set for patron category not in inhouse_patron_categories' # at t/db_dependent/SIP/Message.t line 1917. # got: 'N' # expected: 'Y' # Failed test 'Desensitize flag was set for empty inhouse_patron_categories' # at t/db_dependent/SIP/Message.t line 1923. # got: 'N' # expected: 'Y' # Failed test 'Desensitize flag was set for item type not in inhouse_item_types' # at t/db_dependent/SIP/Message.t line 1931. # got: 'N' # expected: 'Y' # Failed test 'Desensitize flag was set for empty inhouse_item_types' # at t/db_dependent/SIP/Message.t line 1937. # got: 'N' # expected: 'Y' # Looks like you failed 4 tests of 6. t/db_dependent/SIP/Message.t .. 5/23 # Failed test 'Test renew desensitize' # at t/db_dependent/SIP/Message.t line 98. t/db_dependent/SIP/Message.t .. 21/23 # Looks like you failed 3 tests of 23. Also t/db_dependent/Circulation.t .. 68/76 # No tests run! t/db_dependent/Circulation.t .. 70/76 # Failed test 'No tests run for subtest "CanBookBeIssued + needsconfirmation message"' # at t/db_dependent/Circulation.t line 7094. DBIC result _type isn't of the _type BiblioMetadata at /kohadevbox/koha/Koha/Patron.pm line 854. # Looks like your test exited with 11 just after 71. Emmi, can you provide a fix for the failing tests? (In reply to Jonathan Druart from comment #7) > Also > t/db_dependent/Circulation.t .. 68/76 # No tests run! > t/db_dependent/Circulation.t .. 70/76 > # Failed test 'No tests run for subtest "CanBookBeIssued + > needsconfirmation message"' > # at t/db_dependent/Circulation.t line 7094. > DBIC result _type isn't of the _type BiblioMetadata at > /kohadevbox/koha/Koha/Patron.pm line 854. > # Looks like your test exited with 11 just after 71. As far as I can tell t/db_dependent/Circulation.t tests are now failing randomly. They passed for me 3 times out of 5. But if I backtrack to the previous patch added to the main before these patches (bug 39775: Remove no-sort def), tests seem to pass every time. Also the line where they fail points to $biblio->is_serial which tries to check biblios metadata and is not related to the changes made here. Could it be possible that build_object sometimes fails to create metadata for the biblio object? If that's the case here it's weird that moving code around in the do_check_for_previous_checkout method has this kind of effect to building the test objects. (In reply to Emmi Takkinen from comment #9) > (In reply to Jonathan Druart from comment #7) > > Also > > t/db_dependent/Circulation.t .. 68/76 # No tests run! > > t/db_dependent/Circulation.t .. 70/76 > > # Failed test 'No tests run for subtest "CanBookBeIssued + > > needsconfirmation message"' > > # at t/db_dependent/Circulation.t line 7094. > > DBIC result _type isn't of the _type BiblioMetadata at > > /kohadevbox/koha/Koha/Patron.pm line 854. > > # Looks like your test exited with 11 just after 71. > > As far as I can tell t/db_dependent/Circulation.t tests are now failing > randomly. They passed for me 3 times out of 5. But if I backtrack to the > previous patch added to the main before these patches (bug 39775: Remove > no-sort def), tests seem to pass every time. Also the line where they fail > points to $biblio->is_serial which tries to check biblios metadata and is > not related to the changes made here. Could it be possible that build_object > sometimes fails to create metadata for the biblio object? If that's the case > here it's weird that moving code around in the > do_check_for_previous_checkout method has this kind of effect to building > the test objects. Hi Emmi, thanks! I wonder if is_serial breaks because of Bug 39835 - Tidy is_boolean / tinyint? (In reply to Katrin Fischer from comment #10) > (In reply to Emmi Takkinen from comment #9) > > (In reply to Jonathan Druart from comment #7) > > > Also > > > t/db_dependent/Circulation.t .. 68/76 # No tests run! > > > t/db_dependent/Circulation.t .. 70/76 > > > # Failed test 'No tests run for subtest "CanBookBeIssued + > > > needsconfirmation message"' > > > # at t/db_dependent/Circulation.t line 7094. > > > DBIC result _type isn't of the _type BiblioMetadata at > > > /kohadevbox/koha/Koha/Patron.pm line 854. > > > # Looks like your test exited with 11 just after 71. > > > > As far as I can tell t/db_dependent/Circulation.t tests are now failing > > randomly. They passed for me 3 times out of 5. But if I backtrack to the > > previous patch added to the main before these patches (bug 39775: Remove > > no-sort def), tests seem to pass every time. Also the line where they fail > > points to $biblio->is_serial which tries to check biblios metadata and is > > not related to the changes made here. Could it be possible that build_object > > sometimes fails to create metadata for the biblio object? If that's the case > > here it's weird that moving code around in the > > do_check_for_previous_checkout method has this kind of effect to building > > the test objects. > > Hi Emmi, thanks! I wonder if is_serial breaks because of Bug 39835 - Tidy > is_boolean / tinyint? My guess is that probably not. I changed the way test item is build in "CanBookBeIssued + needsconfirmation message" tests and now tests pass every time I run them. Will provide patch shortly so someone else can also confirm this. Thanks! t/db_dependent/SIP/Message.t seems to fail "reliably" at least, so hopefully easier. Created attachment 182064 [details] [review] (follow-up) Bug 38356: Fix failing Circulation.t tests After pushing bug 38356 to main, Circulation.t tests started to fail. Fail happened randomly and was related to sub is_serial trying to fetch biblios metadata. This patch changes the way test item is build in "CanBookBeIssued + needsconfirmation message" tests. It seems more reliable to use build_sample_item rather than building item using biblio and biblioitem data. To test prove t/db_dependent/Circulation.t. Sorry, hope not bothering you: patch looks good to me. But I'd like to push together with the fixes for SIP if possible to save some Jenkins time. Are you working on these as well? (In reply to Katrin Fischer from comment #14) > Sorry, hope not bothering you: patch looks good to me. But I'd like to push > together with the fixes for SIP if possible to save some Jenkins time. Are > you working on these as well? Ah, forgot to say that I'm now checking those SIP tests. I'll add separate patch when I'm done. (In reply to Jonathan Druart from comment #6) > git bisect blames this patch, it broke t/db_dependent/SIP/Message.t > > t/db_dependent/SIP/Message.t .. 1/23 > [15/727] > # Failed test 'Found AH field as timestamp in response' > # at t/db_dependent/SIP/Message.t line 1320. > > # Failed test 'Found AH field as SQL date in response' > # at t/db_dependent/SIP/Message.t line 1325. > > # Looks like you failed 2 tests of 8. > > t/db_dependent/SIP/Message.t .. 3/23 > # Failed test 'Checkout V2' > # at t/db_dependent/SIP/Message.t line 80. > > # Failed test 'Desensitize flag was set for patron category not in > inhouse_patron_categories' > # at t/db_dependent/SIP/Message.t line 1717. > # got: 'N' > # expected: 'Y' > > > # Failed test 'Desensitize flag was set for empty > inhouse_patron_categories' > # at t/db_dependent/SIP/Message.t line 1723. > # got: 'N' > # expected: 'Y' > > # Failed test 'Desensitize flag was set for item type not in > inhouse_item_types' > # at t/db_dependent/SIP/Message.t line 1737. > # got: 'N' > # expected: 'Y' > > # Failed test 'Desensitize flag was set for empty inhouse_item_types' > # at t/db_dependent/SIP/Message.t line 1743. > # got: 'N' > # expected: 'Y' > # Looks like you failed 4 tests of 6. > t/db_dependent/SIP/Message.t .. 4/23 > # Failed test 'Test checkout desensitize' > # at t/db_dependent/SIP/Message.t line 89. > > # Failed test 'Desensitize flag was set for patron category not in > inhouse_patron_categories' > # at t/db_dependent/SIP/Message.t line 1917. > # got: 'N' > # expected: 'Y' > > # Failed test 'Desensitize flag was set for empty > inhouse_patron_categories' > # at t/db_dependent/SIP/Message.t line 1923. > # got: 'N' > # expected: 'Y' > > # Failed test 'Desensitize flag was set for item type not in > inhouse_item_types' > # at t/db_dependent/SIP/Message.t line 1931. > # got: 'N' > # expected: 'Y' > > # Failed test 'Desensitize flag was set for empty inhouse_item_types' > # at t/db_dependent/SIP/Message.t line 1937. > # got: 'N' > # expected: 'Y' > # Looks like you failed 4 tests of 6. > t/db_dependent/SIP/Message.t .. 5/23 > # Failed test 'Test renew desensitize' > # at t/db_dependent/SIP/Message.t line 98. > t/db_dependent/SIP/Message.t .. 21/23 # Looks like you failed 3 tests of 23. Package C4::SIP::ILS::Transaction::Checkout is missing block to handle new "CURRENTISSUE" message and thus tests fail. Created attachment 182065 [details] [review] Bug 38356: (follow-up) Fix failing Circulation.t tests After pushing bug 38356 to main, Circulation.t tests started to fail. Fail happened randomly and was related to sub is_serial trying to fetch biblios metadata. This patch changes the way test item is build in "CanBookBeIssued + needsconfirmation message" tests. It seems more reliable to use build_sample_item rather than building item using biblio and biblioitem data. To test prove t/db_dependent/Circulation.t. Created attachment 182066 [details] [review] Bug 38356: (follow-up) Add code block to handle CURRENTISSUE message Package C4::SIP::ILS::Transaction::Checkout was missing block to handle new "CURRENTISSUE" message. This patch adds this block so tests should no longer fail. To test prove t/db_dependent/SIP/Message.t Both tests should now pass. Had to modify Circulation.t patches title so that QA tool is happy with it. Hi Emmi, thanks a lot! Please check you are running latest ktd and maybe "nuke" your node_modules directory once to avoid these: OK C4/SIP/ILS/Transaction/Checkout.pm FAIL t/db_dependent/Circulation.t FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl t/db_dependent/Circulation.t` I've tidied the patch before pushing, but it should happen automatically on commit with the hooks now. Follow-ups pushed to main. (In reply to Katrin Fischer from comment #20) > Hi Emmi, > > thanks a lot! > > Please check you are running latest ktd and maybe "nuke" your node_modules > directory once to avoid these: > > OK C4/SIP/ILS/Transaction/Checkout.pm > > FAIL t/db_dependent/Circulation.t > FAIL tidiness > File is not tidy, please run `perl misc/devel/tidy.pl > t/db_dependent/Circulation.t` > > I've tidied the patch before pushing, but it should happen automatically on > commit with the hooks now. > > Follow-ups pushed to main. Thanks for pointing this out. I'm not using ktd but my qa tool was indeed pointing out those messy lines. Thank you for fixing them! Added release notes. |