[Bug 27808] New: Item's onloan column remains unset if a checked out item is issued to another patron without being returned first
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 Bug ID: 27808 Summary: Item's onloan column remains unset if a checked out item is issued to another patron without being returned first Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com If any item is currently checked out to a patron, and the item is then checked out directly to another patron without manually checking the item in first, the items.onloan column will remain NULL. This will only happen if the new checkout will be due the same day as the previous checked. This is caused by the item being returned without updating the item object from storage afterward. Even though AddIssue will call AddReturn which sets the value of onloan to NULL in the database, we are not passing in the item object by reference, so it's onloan value remains set to a date. Then we set the onloan value to the same date. Because the value does not change in the object, the column does not get marked dirty. We could update the object from storage first, but it seems more efficient to mark the column as dirty manually to avoid that otherwise unnecessary fetch. -- 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=27808 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=27808 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 117380 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117380&action=edit Bug 27808: Add unit tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 --- Comment #2 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 117381 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117381&action=edit Bug 27808: Mark the onloan column as dirty in AddIssue -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117381|0 |1 is obsolete| | --- Comment #3 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 117382 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117382&action=edit Bug 27808: Mark the onloan column as dirty in AddIssue If any item is currently checked out to a patron, and the item is then checked out directly to another patron without manually checking the item in first, the items.onloan column will remain NULL. This will only happen if the new checkout will be due the same day as the previous checked. This is caused by the item being returned without updating the item object from storage afterward. Even though AddIssue will call AddReturn which sets the value of onloan to NULL in the database, we are not passing in the item object by reference, so it's onloan value remains set to a date. Then we set the onloan value to the same date. Because the value does not change in the object, the column does not get marked dirty. We could update the object from storage first, but it seems more efficient to mark the column as dirty manually to avoid that otherwise unnecessary fetch. Test Plan: 1) Apply these patches 2) prove t/db_dependent/Circulation.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 ByWater Sandboxes <bws.sandboxes@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117380|0 |1 is obsolete| | --- Comment #4 from ByWater Sandboxes <bws.sandboxes@gmail.com> --- Created attachment 117384 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117384&action=edit Bug 27808: Add unit tests Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 ByWater Sandboxes <bws.sandboxes@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117382|0 |1 is obsolete| | --- Comment #5 from ByWater Sandboxes <bws.sandboxes@gmail.com> --- Created attachment 117385 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117385&action=edit Bug 27808: Mark the onloan column as dirty in AddIssue If any item is currently checked out to a patron, and the item is then checked out directly to another patron without manually checking the item in first, the items.onloan column will remain NULL. This will only happen if the new checkout will be due the same day as the previous checked. This is caused by the item being returned without updating the item object from storage afterward. Even though AddIssue will call AddReturn which sets the value of onloan to NULL in the database, we are not passing in the item object by reference, so it's onloan value remains set to a date. Then we set the onloan value to the same date. Because the value does not change in the object, the column does not get marked dirty. We could update the object from storage first, but it seems more efficient to mark the column as dirty manually to avoid that otherwise unnecessary fetch. Test Plan: 1) Apply these patches 2) prove t/db_dependent/Circulation.t Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 Lisette Scheer <lisetteslatah@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lisetteslatah@gmail.com Status|Needs Signoff |Signed Off --- Comment #6 from Lisette Scheer <lisetteslatah@gmail.com> --- I couldn't do the unit tests in the sand box but the item didn't show as available when I did the checkout. Lisette -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117384|0 |1 is obsolete| | --- Comment #7 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 117386 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117386&action=edit Bug 27808: Add unit tests Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org> Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117385|0 |1 is obsolete| | --- Comment #8 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 117387 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117387&action=edit Bug 27808: Mark the onloan column as dirty in AddIssue If any item is currently checked out to a patron, and the item is then checked out directly to another patron without manually checking the item in first, the items.onloan column will remain NULL. This will only happen if the new checkout will be due the same day as the previous checked. This is caused by the item being returned without updating the item object from storage afterward. Even though AddIssue will call AddReturn which sets the value of onloan to NULL in the database, we are not passing in the item object by reference, so it's onloan value remains set to a date. Then we set the onloan value to the same date. Because the value does not change in the object, the column does not get marked dirty. We could update the object from storage first, but it seems more efficient to mark the column as dirty manually to avoid that otherwise unnecessary fetch. Test Plan: 1) Apply these patches 2) prove t/db_dependent/Circulation.t Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org> Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com Severity|normal |major --- Comment #9 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Kicking this up to major, as it makes a mess. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117386|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117387|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 --- Comment #10 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 117398 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117398&action=edit Bug 27808: Add unit tests Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org> Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 --- Comment #11 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 117399 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117399&action=edit Bug 27808: Mark the onloan column as dirty in AddIssue If any item is currently checked out to a patron, and the item is then checked out directly to another patron without manually checking the item in first, the items.onloan column will remain NULL. This will only happen if the new checkout will be due the same day as the previous checked. This is caused by the item being returned without updating the item object from storage afterward. Even though AddIssue will call AddReturn which sets the value of onloan to NULL in the database, we are not passing in the item object by reference, so it's onloan value remains set to a date. Then we set the onloan value to the same date. Because the value does not change in the object, the column does not get marked dirty. We could update the object from storage first, but it seems more efficient to mark the column as dirty manually to avoid that otherwise unnecessary fetch. Test Plan: 1) Apply these patches 2) prove t/db_dependent/Circulation.t Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org> Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 117400 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117400&action=edit [OPTIONAL] Bug 27808: Refresh the item object when AddReturn is called This patch changes the original implementation so the item object is refreshed altogether instead of explicitly pinpointing a specific field we identified an edge case can leave out from ->store. I propose this alterate implementation because what this bug highlights is the fact we don't code thinking calls to things can have side-effects (like this case, with AddReturn updating the onloan status (and maybe other things?). To test: 1. Make sure circ tests pass with and without this patch Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |tomascohen@gmail.com |y.org | Status|Signed Off |Passed QA --- Comment #13 from Tomás Cohen Arazi <tomascohen@gmail.com> --- It works as expected, and the proposed solution is correct-ish. It does the job and actually is very optimal! Anyways, I submitted a follow-up that can be dismissed, which makes the AddIssue method actually refresh the $item_object, because I fear this might not be the only side-effect (from calling AddReturn) that might present edge cases (like overwriting the wrong things, etc). Food for thought, and more opinions. As I said, my follow-up can be skipped. It actually makes an extra DB call compared to your patch, Kyle. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 David Kuhn <techservspec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |techservspec@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Don't we need a DBrev to adjust items.onloan? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 --- Comment #15 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #14)
Don't we need a DBrev to adjust items.onloan?
Totally agree! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed 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=27808 --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 117443 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117443&action=edit Bug 27808: DB changes - adjust items.onloan when needed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117400|0 |1 is obsolete| | Attachment #117443|0 |1 is obsolete| | --- Comment #17 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 117445 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117445&action=edit Bug 27808: Refresh the item object when AddReturn is called This patch changes the original implementation so the item object is refreshed altogether instead of explicitly pinpointing a specific field we identified an edge case can leave out from ->store. I propose this alterate implementation because what this bug highlights is the fact we don't code thinking calls to things can have side-effects (like this case, with AddReturn updating the onloan status (and maybe other things?). To test: 1. Make sure circ tests pass with and without this patch Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <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=27808 --- Comment #18 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 117446 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117446&action=edit Bug 27808: DB changes - adjust items.onloan when needed Signed-off-by: Kyle M Hall <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=27808 --- Comment #19 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 117454 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117454&action=edit Bug 27808: (QA follow-up) Make sure the update catches the items we want This query: SELECT * FROM items LEFT JOIN issues ON issues.itemnumber=items.itemnumber WHERE items.onloan IS NULL; returns 961 rows on the sample data, with 2 items checked out and onloan set to NULL. With this tweak, the query only matches the 2 checkout items with onloan set to NULL: SELECT * FROM items LEFT JOIN issues ON issues.itemnumber=items.itemnumber WHERE items.onloan IS NULL AND issues.issue_id IS NOT NULL; This is the query that needs to be used on the atomic update for filtering the items to be updated. This patch does that. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |21.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 21.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I missed your patch Tomas, can you resubmit for updatedatabase.pl please? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117454|0 |1 is obsolete| | --- Comment #22 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 117456 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117456&action=edit Bug 27808: (QA follow-up) Make sure the update catches the items we want This query: SELECT * FROM items LEFT JOIN issues ON issues.itemnumber=items.itemnumber WHERE items.onloan IS NULL; returns 961 rows on the sample data, with 2 items checked out and onloan set to NULL. With this tweak, the query only matches the 2 checkout items with onloan set to NULL: SELECT * FROM items LEFT JOIN issues ON issues.itemnumber=items.itemnumber WHERE items.onloan IS NULL AND issues.issue_id IS NOT NULL; This is the query that needs to be used on the atomic update for filtering the items to be updated. This patch does that. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 --- Comment #23 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- follow-up 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=27808 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable CC| |fridolin.somers@biblibre.co | |m Version(s)|21.05.00 |21.05.00,20.11.04 released in| | --- Comment #24 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 20.11.x for 20.11.04 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Pushed to oldstable Version(s)|21.05.00,20.11.04 |21.05.00,20.11.04,20.05.10 released in| | --- Comment #25 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Pushed to 20.05.x for 20.05.10 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor@tuxayo.net Status|Pushed to oldstable |RESOLVED Resolution|--- |FIXED --- Comment #26 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Can't backport to 19.11.x: can't solve a conflict. <<<<<<< HEAD (19.11.x) ModItem( { issues => $item_object->issues + 1, holdingbranch => C4::Context->userenv->{'branch'}, itemlost => 0, onloan => $datedue->ymd(), datelastborrowed => dt_from_string(), }, $item_object->biblionumber, $item_object->itemnumber, { log_action => 0 } ); ModDateLastSeen( $item_object->itemnumber ); ||||||| parent of f9d204bd18 (Bug 27808: Mark the onloan column as dirty in AddIssue) (↓code before the patch in 20.05 ↓) $item_object->issues( ( $item_object->issues || 0 ) + 1); $item_object->holdingbranch(C4::Context->userenv->{'branch'}); $item_object->itemlost(0); $item_object->onloan($datedue->ymd()); $item_object->datelastborrowed( dt_from_string()->ymd() ); $item_object->datelastseen( dt_from_string()->ymd() ); $item_object->store({log_action => 0}); ======= (↓code before after the patch in 20.05 ↓) $item_object->issues( ( $item_object->issues || 0 ) + 1); $item_object->holdingbranch(C4::Context->userenv->{'branch'}); $item_object->itemlost(0); $item_object->onloan($datedue->ymd()); $item_object->make_column_dirty('onloan'); # Force write onloan so we don't need to fetch from db $item_object->datelastborrowed( dt_from_string()->ymd() ); $item_object->datelastseen( dt_from_string()->ymd() ); $item_object->store({log_action => 0});
> f9d204bd18 (Bug 27808: Mark the onloan column as dirty in AddIssue)
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |23463 --- Comment #27 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Victor Grousset/tuxayo from comment #26)
Can't backport to 19.11.x: can't solve a conflict.
Caused by bug 23463, not in 19.11.x Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23463 [Bug 23463] Move C4::Items CRUD subroutines to Koha::Item -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27808 koha-US bug tracker <bugzilla@koha-us.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bugzilla@koha-us.org -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org