https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 160851 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=160851&action=edit Bug 35357: Add unit test -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kyle@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 --- Comment #4 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 161010 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161010&action=edit Bug 35357: Remove item from holds queue when it is checked out Test Plan: 1) Place a hold on an item 2) Build the holds queue 3) Check out the item to a different patron than the one targeted in the holds queue 4) Verify the holds queue viewer still shows that item and patron 5) Apply this patch 6) Repeat stepts 1 through 3 7) Verify the holds queue viewer no longer shows that patron and item! Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #5 from Victor Grousset/tuxayo <victor@tuxayo.net> ---

7) Verify the holds queue viewer no longer shows that patron and item!

The next run of misc/cronjobs/holds/build_holds_queue.pl brings back that patron and item in the Holds queue page. So back with the initial issue :( ---- QA script unhappy: WARN t/db_dependent/HoldsQueue.t WARN tidiness The file is less tidy than before (bad/messy lines before: 484, now: 488) ---- minor finding: The test uses a record level hold instead of an item level one. It works because there is one item for the record. Otherwise reproducing the bug would depend on which item is picked. IIUC it has to be the one suggested by the hold queue to get the issue. So maybe a comment is needed to say that the test tests the case where the checked out item is the one suggested by the hold queue. Or make it do an item level hold like in the test plan. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 --- Comment #7 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to Victor Grousset/tuxayo from comment #6)

While testing, found this that is not due to this patch but related:

When canceling a hold, it stays in the hold queue. Is that bad? It stays until the next queue rebuild (15min is said to be the usual delay)

Please keep in mind that build_holdsqueue is no longer the only use case. We have RTHoldsqueue without people running the script or only running it nightly now. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 161790 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161790&action=edit Bug 35357: Remove item from holds queue when it is checked out Test Plan: 1) Place a hold on an item 2) Build the holds queue 3) Check out the item to a different patron than the one targeted in the holds queue 4) Verify the holds queue viewer still shows that item and patron 5) Apply this patch 6) Repeat stepts 1 through 3 7) Verify the holds queue viewer no longer shows that patron and item! Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 --- Comment #12 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Victor Grousset/tuxayo from comment #5)

...

7) Verify the holds queue viewer no longer shows that patron and item!

The next run of misc/cronjobs/holds/build_holds_queue.pl brings back that patron and item in the Holds queue page. So back with the initial issue :(

I am unable to recreate that. If you can please submit a new bug report as that would be a different bug!

QA script unhappy: WARN t/db_dependent/HoldsQueue.t WARN tidiness The file is less tidy than before (bad/messy lines before: 484, now: 488)

Fixed!

minor finding: The test uses a record level hold instead of an item level one. It works because there is one item for the record. Otherwise reproducing the bug would depend on which item is picked. IIUC it has to be the one suggested by the hold queue to get the issue. So maybe a comment is needed to say that the test tests the case where the checked out item is the one suggested by the hold queue. Or make it do an item level hold like in the test plan.

That is correct, it is always the one suggested by the holds queue. I have enhanced the unit tests to prove this is the case. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 --- Comment #14 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to Katrin Fischer from comment #7)

(In reply to Victor Grousset/tuxayo from comment #6)

...

While testing, found this that is not due to this patch but related:

When canceling a hold, it stays in the hold queue. Is that bad? It stays until the next queue rebuild (15min is said to be the usual delay)

Please keep in mind that build_holdsqueue is no longer the only use case. We have RTHoldsqueue without people running the script or only running it nightly now.

Indeed thanks for pointing this out. (confirmed that the RTHQ doesn't suffer from the bug here) --- Thanks Kyle for the followups, I'll looks at them asap and again at the issue at step 7. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #161790|0 |1 is obsolete| | --- Comment #16 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 161850 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161850&action=edit Bug 35357: Remove item from holds queue when it is checked out Test Plan: 1) Place a hold on an item 2) Build the holds queue 3) Check out the item to a different patron than the one targeted in the holds queue 4) Verify the holds queue viewer still shows that item and patron 5) Apply this patch 6) Repeat stepts 1 through 3 7) Verify the holds queue viewer no longer shows that patron and item! Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #18 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to Kyle M Hall from comment #12)

(In reply to Victor Grousset/tuxayo from comment #5)

...

...

7) Verify the holds queue viewer no longer shows that patron and item!

The next run of misc/cronjobs/holds/build_holds_queue.pl brings back that patron and item in the Holds queue page. So back with the initial issue :(

I am unable to recreate that. If you can please submit a new bug report as that would be a different bug!

I can't create either, my bad, I might have checked in the item and forgot when testing various stuff. So the reappearance in the queue would be expected there. Thanks for the followup that fixes the rest. Works, makes sense, QA script happy, code looks good, passing QA :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |24.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 --- Comment #21 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to Katrin Fischer from comment #19)

Just wondering about the direct SQL in this patch. We do have HoldsQueueItems.pm - should we not be able to use a delete?

Indeed it seems so , thanks for seeing that. @Kyle: up for a follow up patch? I can open the ticket. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 --- Comment #23 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to Kyle M Hall from comment #22)

(In reply to Victor Grousset/tuxayo from comment #21)

...

(In reply to Katrin Fischer from comment #19)

...

Just wondering about the direct SQL in this patch. We do have HoldsQueueItems.pm - should we not be able to use a delete?

Indeed it seems so , thanks for seeing that.

@Kyle: up for a follow up patch? I can open the ticket.

Using DBIC to find and delete the lines in the two tables will result in 4 queries compared to the one query we have now.

Hi Kyle, can you explain a bit more about those extra queries if you can? I'd like to understand the issue better and learn. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 --- Comment #25 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to Kyle M Hall from comment #22)

Using DBIC to find and delete the lines in the two tables will result in 4 queries compared to the one query we have now.

The manual query should still do in practice two lookups via itemnumber. And then two deletes. But maybe for simple queries the overhead of doing individual queries is larger than the work done by the simple queries themselves. I don't know enough about real world DBMS behavior. --- (In reply to Katrin Fischer from comment #23)

Hi Kyle, can you explain a bit more about those extra queries if you can? I'd like to understand the issue better and learn.

IIUC, we don't have the ORM objects on hand so we have to do a find first. Then do a delete on the object. So 4 queries. (maybe Kyle had something else in mind) --- (In reply to Fridolin Somers from comment #24)

Seeing comments, I wait for backport

No worries, you can backport. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 --- Comment #27 from Kyle M Hall <kyle@bywatersolutions.com> ---

(In reply to Katrin Fischer from comment #23)

...

Hi Kyle, can you explain a bit more about those extra queries if you can? I'd like to understand the issue better and learn.

IIUC, we don't have the ORM objects on hand so we have to do a find first. Then do a delete on the object. So 4 queries. (maybe Kyle had something else in mind)

This exactly what I meant! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #29 from Lucas Gass <lucas@bywatersolutions.com> --- Tests fail on 23.05.x: # Failed test 'No tests run for subtest "Remove item from holds queue on checkout"' # at /kohadevbox/koha/t/db_dependent/HoldsQueue.t line 2160. Can't locate object method "search" via package "Koha::Hold::HoldsQueueItems" (perhaps you forgot to load "Koha::Hold::HoldsQueueItems"?) at /kohadevbox/koha/t/db_dependent/HoldsQueue.t line 2135. # Looks like your test exited with 11 just after 62. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 --- Comment #31 from Victor Grousset/tuxayo <victor@tuxayo.net> --- @Andrii Where is the error? In C4/Circulation.pm in AddIssue() ? --- @Lucas HoldsQueueItems didn't exist back then. If someone wants this for 23.05.x they will need to get a backport that use the appropriate way at the time to access tmp_holdsqueue -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 --- Comment #33 from Victor Grousset/tuxayo <victor@tuxayo.net> --- This is weird, 23.11.x has $dbh declared at the beginning of the function. https://gitlab.com/koha-community/Koha/-/blob/23.11.x/C4/Circulation.pm#L154... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35357 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.