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
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 "") ) {
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
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>
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>
The change is missing test coverage.
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?
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.
(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.
(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?
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>
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>
(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.
Hannah, just making sure you saw bug 28895.
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>
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>
(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.
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'
Pushed to master for 21.11, thanks to everybody involved!
Pushed to 21.05.x for 21.05.05
Pushed to 20.11.x for 20.11.11
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.
*** Bug 29981 has been marked as a duplicate of this bug. ***