Bug 27808

Summary: Item's onloan column remains unset if a checked out item is issued to another patron without being returned first
Product: Koha Reporter: Kyle M Hall <kyle>
Component: CirculationAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: major    
Priority: P5 - low CC: andrewfh, bugzilla, fridolin.somers, gmcharlt, jonathan.druart, kyle.m.hall, lisettepalouse+koha, techservspec, tomascohen, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.04,20.05.10
Bug Depends on: 23463    
Bug Blocks:    
Attachments: Bug 27808: Add unit tests
Bug 27808: Mark the onloan column as dirty in AddIssue
Bug 27808: Mark the onloan column as dirty in AddIssue
Bug 27808: Add unit tests
Bug 27808: Mark the onloan column as dirty in AddIssue
Bug 27808: Add unit tests
Bug 27808: Mark the onloan column as dirty in AddIssue
Bug 27808: Add unit tests
Bug 27808: Mark the onloan column as dirty in AddIssue
[OPTIONAL] Bug 27808: Refresh the item object when AddReturn is called
Bug 27808: DB changes - adjust items.onloan when needed
Bug 27808: Refresh the item object when AddReturn is called
Bug 27808: DB changes - adjust items.onloan when needed
Bug 27808: (QA follow-up) Make sure the update catches the items we want
Bug 27808: (QA follow-up) Make sure the update catches the items we want

Description Kyle M Hall 2021-02-26 14:55:27 UTC
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.
Comment 1 Kyle M Hall 2021-02-26 14:57:56 UTC
Created attachment 117380 [details] [review]
Bug 27808: Add unit tests
Comment 2 Kyle M Hall 2021-02-26 14:58:04 UTC
Created attachment 117381 [details] [review]
Bug 27808: Mark the onloan column as dirty in AddIssue
Comment 3 Kyle M Hall 2021-02-26 15:03:13 UTC
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
Comment 4 ByWater Sandboxes 2021-02-26 16:16:27 UTC
Created attachment 117384 [details] [review]
Bug 27808: Add unit tests

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 5 ByWater Sandboxes 2021-02-26 16:16:29 UTC
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>
Comment 6 Lisette Scheer 2021-02-26 16:18:45 UTC
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
Comment 7 Andrew Fuerste-Henry 2021-02-26 17:22:07 UTC
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>
Comment 8 Andrew Fuerste-Henry 2021-02-26 17:22:10 UTC
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>
Comment 9 Andrew Fuerste-Henry 2021-02-26 17:44:26 UTC
Kicking this up to major, as it makes a mess.
Comment 10 Tomás Cohen Arazi 2021-02-26 20:06:36 UTC
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>
Comment 11 Tomás Cohen Arazi 2021-02-26 20:06:42 UTC
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>
Comment 12 Tomás Cohen Arazi 2021-02-26 20:06:47 UTC
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>
Comment 13 Tomás Cohen Arazi 2021-02-26 20:10:08 UTC
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.
Comment 14 Jonathan Druart 2021-03-01 09:45:18 UTC
Don't we need a DBrev to adjust items.onloan?
Comment 15 Tomás Cohen Arazi 2021-03-01 11:23:35 UTC
(In reply to Jonathan Druart from comment #14)
> Don't we need a DBrev to adjust items.onloan?

Totally agree!
Comment 16 Jonathan Druart 2021-03-01 12:35:47 UTC
Created attachment 117443 [details] [review]
Bug 27808: DB changes - adjust items.onloan when needed
Comment 17 Kyle M Hall 2021-03-01 12:55:27 UTC
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>
Comment 18 Kyle M Hall 2021-03-01 12:55:43 UTC
Created attachment 117446 [details] [review]
Bug 27808: DB changes - adjust items.onloan when needed

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Tomás Cohen Arazi 2021-03-01 14:21:31 UTC
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>
Comment 20 Jonathan Druart 2021-03-01 14:24:37 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 21 Jonathan Druart 2021-03-01 14:29:09 UTC
I missed your patch Tomas, can you resubmit for updatedatabase.pl please?
Comment 22 Tomás Cohen Arazi 2021-03-01 14:57:16 UTC
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>
Comment 23 Jonathan Druart 2021-03-02 09:40:40 UTC
follow-up pushed to master.
Comment 24 Fridolin Somers 2021-03-04 12:15:23 UTC
Pushed to 20.11.x for 20.11.04
Comment 25 Andrew Fuerste-Henry 2021-03-14 18:55:00 UTC
Pushed to 20.05.x for 20.05.10
Comment 26 Victor Grousset/tuxayo 2021-03-22 16:18:36 UTC
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)
Comment 27 Jonathan Druart 2021-03-23 11:24:33 UTC
(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