[Bug 29326] New: _ALL_ should not override other rules in UpdateItemLocationOnCheckin
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 Bug ID: 29326 Summary: _ALL_ should not override other rules in UpdateItemLocationOnCheckin Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: cbrannon@cdalibrary.org QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com _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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 Barbara Johnson <barbara.johnson@bedfordtx.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |barbara.johnson@bedfordtx.g | |ov --- Comment #1 from Barbara Johnson <barbara.johnson@bedfordtx.gov> --- +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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 --- Comment #2 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 Olli-Antti Kivilahti <olli-antti.kivilahti@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |olli-antti.kivilahti@hypern | |ova.fi --- Comment #3 from Olli-Antti Kivilahti <olli-antti.kivilahti@hypernova.fi> --- Created attachment 188043 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188043&action=edit 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 --- Comment #4 from Lari Taskula <lari.taskula@hypernova.fi> --- Created attachment 188269 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188269&action=edit Bug 29326: Add test for _BLANK_ key To test: 1. prove t/db_dependent/Koha/Item.t 2. Observe success -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 --- Comment #5 from Lari Taskula <lari.taskula@hypernova.fi> --- Created attachment 188270 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188270&action=edit 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 --- Comment #6 from Lari Taskula <lari.taskula@hypernova.fi> --- Created attachment 188271 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188271&action=edit 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 --- Comment #7 from Lari Taskula <lari.taskula@hypernova.fi> --- Created attachment 188272 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188272&action=edit Bug 29326: (QA follow-up) Fix tidiness -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lari.taskula@hypernova.fi Assignee|koha-bugs@lists.koha-commun |lari.taskula@hypernova.fi |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #188271|0 |1 is obsolete| | --- Comment #8 from Lari Taskula <lari.taskula@hypernova.fi> --- Created attachment 188273 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188273&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #188272|0 |1 is obsolete| | --- Comment #9 from Lari Taskula <lari.taskula@hypernova.fi> --- Created attachment 188274 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188274&action=edit Bug 29326: (QA follow-up) Fix tidiness -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 --- Comment #10 from Lari Taskula <lari.taskula@hypernova.fi> --- Created attachment 188275 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188275&action=edit Bug 29326: Add more tests Tests for more complex preference values To test: 1. prove t/db_dependent/Koha/Item.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #188043|0 |1 is obsolete| | --- Comment #11 from Lari Taskula <lari.taskula@hypernova.fi> --- Comment on attachment 188043 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188043 Code commit to upgrade this feature
From 62a09afe9a2954e22d54e95e8001562fcf05ae7f Mon Sep 17 00:00:00 2001 From: Olli-Antti Kivilahti <Olli-Antti.Kivilahti@Hypernova.fi> Date: Fri, 17 Oct 2025 06:58:49 +0300 Subject: [PATCH] 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. --- lib/Koha/Item.pm | 100 +++++++++++++++++++++-------------------------- 1 file changed, 44 insertions(+), 56 deletions(-)
diff --git a/lib/Koha/Item.pm b/lib/Koha/Item.pm index 5012584..98170ac 100644 --- a/lib/Koha/Item.pm +++ b/lib/Koha/Item.pm @@ -2704,6 +2704,27 @@ FIXME: It should return I<$self>. See bug 35270. sub location_update_trigger { my ( $self, $action ) = @_;
+ if (my $new_location = $self->_check_location_update_rules($action)) { + my $messages = { + 'ItemLocationUpdated' => { + from => $self->location, + to => $new_location, + } + }; + $self->location($new_location)->store( + { + log_action => 0, + skip_record_index => 1, + skip_holds_queue => 1 + } + ); + return $messages; + } + return undef; +} + +sub _check_location_update_rules { + my ($self, $action) = @_; my ( $update_loc_rules, $messages ); if ( $action eq 'checkin' ) { $update_loc_rules = C4::Context->yaml_preference('UpdateItemLocationOnCheckin'); @@ -2712,66 +2733,33 @@ sub location_update_trigger { }
if ($update_loc_rules) { - if ( defined $update_loc_rules->{_ALL_} ) { - if ( $update_loc_rules->{_ALL_} eq '_PERM_' ) { - $update_loc_rules->{_ALL_} = $self->permanent_location; - } - if ( $update_loc_rules->{_ALL_} eq '_BLANK_' ) { - $update_loc_rules->{_ALL_} = ''; - } - if ( - ( defined $self->location && $self->location ne $update_loc_rules->{_ALL_} ) - || ( !defined $self->location - && $update_loc_rules->{_ALL_} ne "" ) - ) - { - $messages->{'ItemLocationUpdated'} = - { from => $self->location, to => $update_loc_rules->{_ALL_} }; - $self->location( $update_loc_rules->{_ALL_} )->store( - { - log_action => 0, - skip_record_index => 1, - skip_holds_queue => 1 - } - ); - } - } else { - foreach my $key ( keys %$update_loc_rules ) { - if ( $update_loc_rules->{$key} eq '_PERM_' ) { - $update_loc_rules->{$key} = $self->permanent_location; - } elsif ( $update_loc_rules->{$key} eq '_BLANK_' ) { - $update_loc_rules->{$key} = ''; - } - if ( - ( - defined $self->location - && $self->location eq $key - && $self->location ne $update_loc_rules->{$key} - ) - || ( $key eq '_BLANK_' - && ( !defined $self->location || $self->location eq '' ) - && $update_loc_rules->{$key} ne '' ) - ) - { - $messages->{'ItemLocationUpdated'} = { - from => $self->location, - to => $update_loc_rules->{$key} - }; - $self->location( $update_loc_rules->{$key} )->store( - { - log_action => 0, - skip_record_index => 1, - skip_holds_queue => 1 - } - ); - last; - } - } + if (defined($update_loc_rules->{'_ALL_'})) { + return $self->_check_location_update_rule($update_loc_rules->{'_ALL_'}); + } + elsif (defined($update_loc_rules->{$self->location//''})) { + return $self->_check_location_update_rule($update_loc_rules->{$self->location}); + } + elsif (defined($update_loc_rules->{'_DEFAULT_'})) { + return $self->_check_location_update_rule($update_loc_rules->{'_DEFAULT_'}); } } - return $messages; }
+sub _check_location_update_rule { + my ($self, $update_loc_rule) = @_; + + if ($update_loc_rule eq '_PERM_') { + $update_loc_rule = $self->permanent_location; + } + if ($update_loc_rule eq '_BLANK_') { + $update_loc_rule = ''; + } + if ((defined($self->location) && $self->location ne $update_loc_rule) || + (not(defined($self->location)) && $update_loc_rule ne "" )) { + return $update_loc_rule; + } + return undef; +}
=head3 z3950_status
-- 2.43.0
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 --- Comment #12 from Lari Taskula <lari.taskula@hypernova.fi> --- Created attachment 188276 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188276&action=edit Bug 29326: Document system preference change -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #188275|0 |1 is obsolete| | --- Comment #13 from Lari Taskula <lari.taskula@hypernova.fi> --- Created attachment 188277 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188277&action=edit Bug 29326: Add more tests Tests for more complex preference values To test: 1. prove t/db_dependent/Koha/Item.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #188276|0 |1 is obsolete| | --- Comment #14 from Lari Taskula <lari.taskula@hypernova.fi> --- Created attachment 188278 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188278&action=edit Bug 29326: Document system preference change -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 --- Comment #15 from Lari Taskula <lari.taskula@hypernova.fi> --- (In reply to Olli-Antti Kivilahti from comment #3)
Created attachment 188043 [details] [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_. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 --- Comment #16 from Christopher Brannon <cbrannon@cdalibrary.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 --- Comment #17 from Olli-Antti Kivilahti <olli-antti.kivilahti@hypernova.fi> --- Created attachment 189063 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189063&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29326 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #18 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- The last patch here no longer applies: Error: Apply failed: Git command (git am -3 /tmp/hz4NDyXKHT/0007-189063.patch) failed: error: sha1 information is lacking or useless (lib/Koha/Item.pm). error: could not build fake ancestor Looking at Olli-Antti's comment it's a "preview of an extra functionality" so I am not sure if it should be applied/tested or maybe moved to a separate bug report? Lari or Olli-Antti, can you please clarify? Looking at the commit messages, we are also missing a test plan that goes beyond running the unit tests. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org