Bug 28472 - UpdateItemLocationOnCheckin not updating items where location is null
Summary: UpdateItemLocationOnCheckin not updating items where location is null
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: master
Hardware: All All
: P5 - low normal with 10 votes (vote)
Assignee: Andreas Roussos
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on: 26581
Blocks:
  Show dependency treegraph
 
Reported: 2021-05-27 11:59 UTC by Andrew Fuerste-Henry
Modified: 2021-10-16 02:18 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.05,20.11.11


Attachments
Bug 28472: handle items with NULL shelving location (2.05 KB, patch)
2021-07-02 11:34 UTC, Andreas Roussos
Details | Diff | Splinter Review
Bug 28472: handle items with NULL shelving location (2.12 KB, patch)
2021-07-02 12:16 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28472: handle items with NULL shelving location (2.17 KB, patch)
2021-07-09 13:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28472: (follow-up) add unit test for the case where location = NULL (2.07 KB, patch)
2021-09-17 14:03 UTC, Andreas Roussos
Details | Diff | Splinter Review
Bug 28472: handle items with NULL shelving location (2.24 KB, patch)
2021-09-17 16:25 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28472: (follow-up) add unit test for the case where location = NULL (2.13 KB, patch)
2021-09-17 16:25 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28472: handle items with NULL shelving location (2.38 KB, patch)
2021-09-19 12:36 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28472: (follow-up) add unit test for the case where location = NULL (2.28 KB, patch)
2021-09-19 12:36 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28472: (follow-up) add unit test for the case where location = NULL (2.32 KB, patch)
2021-09-20 10:31 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2021-05-27 11:59:36 UTC
UpdateItemLocationOnCheckin can be set to update ALL items at checkin. But that ALL is excluding items where location is null.

To recreate:
- have a location CART
- in UpdateItemLocationOnCheckin, enter "_ALL_: CART"
- find an item with a shelving location, check it in, confirm it changes to CART
- find an item with a NULL shelving location, check it in, confirm it doesn't go to CART
Comment 1 Nick Clemens 2021-05-27 12:20:33 UTC
I broke this trying to stop a warning:

Was:
if ( $item->location ne $update_loc_rules->{_ALL_}) {

Became:
if ( defined $item->location && $item->location ne $update_loc_rules->{_ALL_}) {

Should be:
if (
    ( defined $item->location && $item->location ne $update_loc_rules->{_ALL_}) ||
    (!defined $item->location && $update_loc_rules->{_ALL_} ne "")
    ) {
Comment 2 Andreas Roussos 2021-07-02 11:34:51 UTC
Created attachment 122541 [details] [review]
Bug 28472: handle items with NULL shelving location

The UpdateItemLocationOnCheckin System Preference can be set to update the
location of ALL items during check in, regardless of their shelving location.
However, this does not currently work 100% as it is excluding items with no
shelving location (i.e. value of NULL in the corresponding database field).

This patch, based on the comment made by Nick Clemens, fixes that.

Test plan (based on the original Bug Description by Andrew Fuerste-Henry):

1) Have a shelving location CART
2) In UpdateItemLocationOnCheckin, enter "_ALL_: CART" (without the quotes)
3) Check in an item that has a shelving location, confirm it changes to CART
4) Check in an item with a NULL shelving location, confirm it doesn't go to CART
5) Apply this patch
6) Repeat step 4): this time the item should move to the CART shelving location
Comment 3 Andrew Fuerste-Henry 2021-07-02 12:16:04 UTC
Created attachment 122547 [details] [review]
Bug 28472: handle items with NULL shelving location

The UpdateItemLocationOnCheckin System Preference can be set to update the
location of ALL items during check in, regardless of their shelving location.
However, this does not currently work 100% as it is excluding items with no
shelving location (i.e. value of NULL in the corresponding database field).

This patch, based on the comment made by Nick Clemens, fixes that.

Test plan (based on the original Bug Description by Andrew Fuerste-Henry):

1) Have a shelving location CART
2) In UpdateItemLocationOnCheckin, enter "_ALL_: CART" (without the quotes)
3) Check in an item that has a shelving location, confirm it changes to CART
4) Check in an item with a NULL shelving location, confirm it doesn't go to CART
5) Apply this patch
6) Repeat step 4): this time the item should move to the CART shelving location

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 4 Nick Clemens 2021-07-09 13:41:48 UTC
Created attachment 122749 [details] [review]
Bug 28472: handle items with NULL shelving location

The UpdateItemLocationOnCheckin System Preference can be set to update the
location of ALL items during check in, regardless of their shelving location.
However, this does not currently work 100% as it is excluding items with no
shelving location (i.e. value of NULL in the corresponding database field).

This patch, based on the comment made by Nick Clemens, fixes that.

Test plan (based on the original Bug Description by Andrew Fuerste-Henry):

1) Have a shelving location CART
2) In UpdateItemLocationOnCheckin, enter "_ALL_: CART" (without the quotes)
3) Check in an item that has a shelving location, confirm it changes to CART
4) Check in an item with a NULL shelving location, confirm it doesn't go to CART
5) Apply this patch
6) Repeat step 4): this time the item should move to the CART shelving location

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 5 Jonathan Druart 2021-07-12 09:44:45 UTC
The change is missing test coverage.
Comment 6 Hannah Co 2021-08-19 23:46:04 UTC
I don't believe this patch catches items in "processing" that should end up with no shelving location. I have a freshly updated 21.05, with this patch included. My system pref UpdateItemLocationOnCheckin contains "PROC: _PERM_". Check-in does not make any changes to a record with "PROC" location and no other shelving location assigned. 

New items are automatically created with location AND permanent_location both set as PROC. It seems that "PROC" and "CART" should never populate permanent_location. Can we prevent "PROC" from populating the permanent_location?

Or, can we handle the situation where "PROC" exists in both permanent_location AND location, so the location is completely removed and replaced with NULL?
Comment 7 Andreas Roussos 2021-09-17 14:03:17 UTC
Created attachment 124989 [details] [review]
Bug 28472: (follow-up) add unit test for the case where location = NULL

This patch adds an extra unit test, to cover the case where the
UpdateItemLocationOnCheckin System Preference is set to "_ALL_: CART"
and the item being returned has no shelving location set.

Test plan:

1) Apply the patch provided earlier
2) prove -v t/db_dependent/Circulation/issue.t
   ...and sign off if all tests pass.
Comment 8 Andreas Roussos 2021-09-17 14:05:10 UTC
(In reply to Jonathan Druart from comment #5)
> The change is missing test coverage.
Hi Jonathan!
I think I managed to write a test that covers this case, see my follow-up.
Comment 9 Andreas Roussos 2021-09-17 14:11:15 UTC
(In reply to Hannah Co from comment #6)
> I don't believe this patch catches items in "processing" that should end up
> with no shelving location. I have a freshly updated 21.05, with this patch
> included. My system pref UpdateItemLocationOnCheckin contains "PROC:
> _PERM_". Check-in does not make any changes to a record with "PROC" location
> and no other shelving location assigned. 
> 
> New items are automatically created with location AND permanent_location
> both set as PROC. It seems that "PROC" and "CART" should never populate
> permanent_location. Can we prevent "PROC" from populating the
> permanent_location?
> 
> Or, can we handle the situation where "PROC" exists in both
> permanent_location AND location, so the location is completely removed and
> replaced with NULL?
Hi Hannah!
Perhaps I've misunderstood what you're describing in your comment, but to me
it looks like it's a different scenario than the one in the Description of
this bug report. Perhaps your report deserves to be submitted as a new bug?
Comment 10 Andrew Fuerste-Henry 2021-09-17 16:25:40 UTC
Created attachment 125011 [details] [review]
Bug 28472: handle items with NULL shelving location

The UpdateItemLocationOnCheckin System Preference can be set to update the
location of ALL items during check in, regardless of their shelving location.
However, this does not currently work 100% as it is excluding items with no
shelving location (i.e. value of NULL in the corresponding database field).

This patch, based on the comment made by Nick Clemens, fixes that.

Test plan (based on the original Bug Description by Andrew Fuerste-Henry):

1) Have a shelving location CART
2) In UpdateItemLocationOnCheckin, enter "_ALL_: CART" (without the quotes)
3) Check in an item that has a shelving location, confirm it changes to CART
4) Check in an item with a NULL shelving location, confirm it doesn't go to CART
5) Apply this patch
6) Repeat step 4): this time the item should move to the CART shelving location

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 11 Andrew Fuerste-Henry 2021-09-17 16:25:45 UTC
Created attachment 125012 [details] [review]
Bug 28472: (follow-up) add unit test for the case where location = NULL

This patch adds an extra unit test, to cover the case where the
UpdateItemLocationOnCheckin System Preference is set to "_ALL_: CART"
and the item being returned has no shelving location set.

Test plan:

1) Apply the patch provided earlier
2) prove -v t/db_dependent/Circulation/issue.t
   ...and sign off if all tests pass.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 12 Hannah Co 2021-09-17 16:30:51 UTC
(In reply to Andreas Roussos from comment #9)
> (In reply to Hannah Co from comment #6)
> > I don't believe this patch catches items in "processing" that should end up
> > with no shelving location. I have a freshly updated 21.05, with this patch
> > included. My system pref UpdateItemLocationOnCheckin contains "PROC:
> > _PERM_". Check-in does not make any changes to a record with "PROC" location
> > and no other shelving location assigned. 
> > 
> > New items are automatically created with location AND permanent_location
> > both set as PROC. It seems that "PROC" and "CART" should never populate
> > permanent_location. Can we prevent "PROC" from populating the
> > permanent_location?
> > 
> > Or, can we handle the situation where "PROC" exists in both
> > permanent_location AND location, so the location is completely removed and
> > replaced with NULL?
> Hi Hannah!
> Perhaps I've misunderstood what you're describing in your comment, but to me
> it looks like it's a different scenario than the one in the Description of
> this bug report. Perhaps your report deserves to be submitted as a new bug?

Hi Andreas, I can certainly start a new bug report - I can report a bug for my situation, but would it be better to create a bug that covers the whole issue of "UpdateItemLocationOnCheckin not updating items where location is null"? The _ALL_: CART issue discussed above is another highly specific case, and I don't quite understand why the fix for that doesn't help in my situation. I feel like there's a larger issue of location = NULL needing to be handled in all situations.
Comment 13 Andrew Fuerste-Henry 2021-09-17 17:00:55 UTC
Hannah, just making sure you saw bug 28895.
Comment 14 Joonas Kylmälä 2021-09-19 12:36:10 UTC
Created attachment 125033 [details] [review]
Bug 28472: handle items with NULL shelving location

The UpdateItemLocationOnCheckin System Preference can be set to update the
location of ALL items during check in, regardless of their shelving location.
However, this does not currently work 100% as it is excluding items with no
shelving location (i.e. value of NULL in the corresponding database field).

This patch, based on the comment made by Nick Clemens, fixes that.

Test plan (based on the original Bug Description by Andrew Fuerste-Henry):

1) Have a shelving location CART
2) In UpdateItemLocationOnCheckin, enter "_ALL_: CART" (without the quotes)
3) Check in an item that has a shelving location, confirm it changes to CART
4) Check in an item with a NULL shelving location, confirm it doesn't go to CART
5) Apply this patch
6) Repeat step 4): this time the item should move to the CART shelving location

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 15 Joonas Kylmälä 2021-09-19 12:36:14 UTC
Created attachment 125034 [details] [review]
Bug 28472: (follow-up) add unit test for the case where location = NULL

This patch adds an extra unit test, to cover the case where the
UpdateItemLocationOnCheckin System Preference is set to "_ALL_: CART"
and the item being returned has no shelving location set.

Test plan:

1) Apply the patch provided earlier
2) prove -v t/db_dependent/Circulation/issue.t
   ...and sign off if all tests pass.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 16 Joonas Kylmälä 2021-09-19 12:41:54 UTC
(In reply to Hannah Co from comment #12)
> Hi Andreas, I can certainly start a new bug report - I can report a bug for
> my situation, but would it be better to create a bug that covers the whole
> issue of "UpdateItemLocationOnCheckin not updating items where location is
> null"? The _ALL_: CART issue discussed above is another highly specific
> case, and I don't quite understand why the fix for that doesn't help in my
> situation. I feel like there's a larger issue of location = NULL needing to
> be handled in all situations.

Opening a new bug report for your issue seems like the best idea, the problem here (by reading the code) is just specific to the _ALL_ configuration value and not to the other ones.

Passing QA.
Comment 17 Jonathan Druart 2021-09-20 10:31:20 UTC
Created attachment 125050 [details] [review]
Bug 28472: (follow-up) add unit test for the case where location = NULL

This patch adds an extra unit test, to cover the case where the
UpdateItemLocationOnCheckin System Preference is set to "_ALL_: CART"
and the item being returned has no shelving location set.

Test plan:

1) Apply the patch provided earlier
2) prove -v t/db_dependent/Circulation/issue.t
   ...and sign off if all tests pass.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

JD amended patch: use 'is' instead of 'ok'
Comment 18 Jonathan Druart 2021-09-20 12:47:32 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 19 Kyle M Hall 2021-10-01 17:39:13 UTC
Pushed to 21.05.x for 21.05.05
Comment 20 Fridolin Somers 2021-10-06 20:12:31 UTC
Pushed to 20.11.x for 20.11.11
Comment 21 Victor Grousset/tuxayo 2021-10-16 02:18:30 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.