Bug 29326

Summary: _ALL_ should not override other rules in UpdateItemLocationOnCheckin
Product: Koha Reporter: Christopher Brannon <cbrannon>
Component: CirculationAssignee: Lari Taskula <lari.taskula>
Status: Needs Signoff --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: barbara.johnson, gmcharlt, kyle, lari.taskula, olli-antti.kivilahti
Version: Main   
Hardware: All   
OS: All   
GIT URL: Initiative type: ---
Sponsorship status: --- Comma delimited list of Sponsors:
Crowdfunding goal: 0 Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Code commit to upgrade this feature
Bug 29326: Add test for _BLANK_ key
Bug 29326 - _ALL_ should not override other rules in UpdateItemLocationOnCheckin. Backend.
Bug 29326: Fix _BLANK_ test failing
Bug 29326: (QA follow-up) Fix tidiness
Bug 29326: Fix _BLANK_ test failing
Bug 29326: (QA follow-up) Fix tidiness
Bug 29326: Add more tests
Bug 29326: Document system preference change
Bug 29326: Add more tests
Bug 29326: Document system preference change
Bug 29326 - _ALL_ should not override other rules in UpdateItemLocationOnCheckin. C4::Context->interface -specific configurations. Backend.

Description Christopher Brannon 2021-10-26 16:19:17 UTC
_ALL_ is stated to override all other rules.  However, if you use the CART functionality, that is all this preference is good for.  You are not allowed special cases.

It would be nice if there were specific rules, that it would override _ALL_.  This is how we are used to most circ rules working, and this would be more consistent with behavior.
Comment 1 Barbara Johnson 2021-11-24 15:16:07 UTC
+1  We use the CART functionality but we also have some special collections such as Library of Things and STEM kits that we would like to handle differently.
Comment 2 Katrin Fischer 2023-06-24 20:57:09 UTC
This sill seems to be the case, as there is a note in bold on the pref:

The special term _ALL_ is used on the left side of the colon (:) to affect all items, and overrides all other rules.
Comment 3 Olli-Antti Kivilahti 2025-10-17 04:02:41 UTC
Created attachment 188043 [details] [review]
Code commit to upgrade this feature

Hi!
We have had this same problem also with a few clients.
Always a bit embarassing to demo this very old CartToShelf feature, which never works reasonably.

One client has 200+ shelving locations so we cannot really define all of those here at 'UpdateItemLocationOnCheckin'.

Attached is one solution.
Comment 4 Lari Taskula 2025-10-22 08:12:08 UTC
Created attachment 188269 [details] [review]
Bug 29326: Add test for _BLANK_ key

To test:

1. prove t/db_dependent/Koha/Item.t
2. Observe success
Comment 5 Lari Taskula 2025-10-22 08:12:10 UTC
Created attachment 188270 [details] [review]
Bug 29326 - _ALL_ should not override other rules in UpdateItemLocationOnCheckin. Backend.

Refactored for a more efficient testing of rules.
Added _DEFAULT_ to fall back to if _ALL_ or a specific location rule is not defined.
Comment 6 Lari Taskula 2025-10-22 08:12:12 UTC Comment hidden (obsolete)
Comment 7 Lari Taskula 2025-10-22 08:12:14 UTC Comment hidden (obsolete)
Comment 8 Lari Taskula 2025-10-22 08:25:25 UTC
Created attachment 188273 [details] [review]
Bug 29326: Fix _BLANK_ test failing

To test:
Before this patch:
1. prove t/db_dependent/Koha/Item.t
2. Observe failure
After this patch:
3. prove t/db_dependent/Koha/Item.t
4. Observe success
Comment 9 Lari Taskula 2025-10-22 08:25:28 UTC
Created attachment 188274 [details] [review]
Bug 29326: (QA follow-up) Fix tidiness
Comment 10 Lari Taskula 2025-10-22 08:25:30 UTC Comment hidden (obsolete)
Comment 11 Lari Taskula 2025-10-22 08:26:14 UTC Comment hidden (obsolete)
Comment 12 Lari Taskula 2025-10-22 08:33:09 UTC Comment hidden (obsolete)
Comment 13 Lari Taskula 2025-10-22 08:34:10 UTC
Created attachment 188277 [details] [review]
Bug 29326: Add more tests

Tests for more complex preference values

To test:
1. prove t/db_dependent/Koha/Item.t
Comment 14 Lari Taskula 2025-10-22 08:34:12 UTC
Created attachment 188278 [details] [review]
Bug 29326: Document system preference change
Comment 15 Lari Taskula 2025-10-22 08:47:50 UTC
(In reply to Olli-Antti Kivilahti from comment #3)
> Created attachment 188043 [details] [review] [review]

This Bugzilla comment did not explain the change (the feature is explained in the patch commit message), so I will explain it here.

It maintains _ALL_ as it is, ie. forcing _ALL_ to overwrite any other setting in the preference. It instead adds a new key, _DEFAULT_, that can be used for a fallback when no other setting was applied.

I've added unit tests and while doing so, noticed a Bug in the original patch, so I fixed that as well.

This "fix" could be further enhanced. This change renders _ALL_ useless, because the same functionality can be now set with _DEFAULT_.

We could replace _ALL_ altogether with _DEFAULT_ with a database update, but it comes with some risk. If a library has currently defined _ALL_ together with other settings, it could cause sudden unexpected consequences. We could add some logic into the replacement, ie. checking if _ALL_ is the only setting (in which case it is OK to replace), or if there are other settings together with _ALL_, remove the others and replace _ALL_ with _DEFAULT_.
Comment 16 Christopher Brannon 2025-10-24 20:03:39 UTC
I can't wait to get this.  This will be a HUGE help!  Thank you for the work on this.

In the case of updating _ALL_ with _DEFAULT_ I would opt for the first suggestion:  only if _ALL_ is the only rule you should replace it.  I would not suggest wiping out other rules.  Can notes be added (like remarks)?  That would be SO HANDY.  Not asking for that in this but, but it would be great in general if we could remark what we are doing.  Then you would have a way to let people that upgrade know, if the change could not be made, why and what they can do.
Comment 17 Olli-Antti Kivilahti 2025-11-04 20:38:35 UTC
Created attachment 189063 [details] [review]
Bug 29326 - _ALL_ should not override other rules in UpdateItemLocationOnCheckin. C4::Context->interface -specific configurations. Backend.

Attached is a preview of an extra functionality, where each Koha instance can define the sysprefs:
 -  UpdateItemLocationOnCheckin
 -  UpdateItemLocationOnCheckout
for each C4::Context->interface, like:
 - opac
 - intranet
 - commandline
 - cron
 - rest
 - sip
and a fallback default.