|
Description
Kyle M Hall (khall)
2021-02-26 14:55:27 UTC
Created attachment 117380 [details] [review] Bug 27808: Add unit tests Created attachment 117381 [details] [review] Bug 27808: Mark the onloan column as dirty in AddIssue Created attachment 117382 [details] [review] 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 Created attachment 117384 [details] [review] Bug 27808: Add unit tests Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org> Created attachment 117385 [details] [review] 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> 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 Created attachment 117386 [details] [review] Bug 27808: Add unit tests Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org> Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org> Created attachment 117387 [details] [review] 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> Kicking this up to major, as it makes a mess. Created attachment 117398 [details] [review] 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> Created attachment 117399 [details] [review] 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> Created attachment 117400 [details] [review] [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> 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. Don't we need a DBrev to adjust items.onloan? (In reply to Jonathan Druart from comment #14) > Don't we need a DBrev to adjust items.onloan? Totally agree! Created attachment 117443 [details] [review] Bug 27808: DB changes - adjust items.onloan when needed Created attachment 117445 [details] [review] 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> Created attachment 117446 [details] [review] Bug 27808: DB changes - adjust items.onloan when needed Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 117454 [details] [review] 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> Pushed to master for 21.05, thanks to everybody involved! I missed your patch Tomas, can you resubmit for updatedatabase.pl please? Created attachment 117456 [details] [review] 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> follow-up pushed to master. Pushed to 20.11.x for 20.11.04 Pushed to 20.05.x for 20.05.10 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)
(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 |