|
Description
Christopher Brannon
2021-10-26 16:19:17 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. 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. 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. 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 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. Created attachment 188271 [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 Created attachment 188272 [details] [review] Bug 29326: (QA follow-up) Fix tidiness 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 Created attachment 188274 [details] [review] Bug 29326: (QA follow-up) Fix tidiness Created attachment 188275 [details] [review] Bug 29326: Add more tests Tests for more complex preference values To test: 1. prove t/db_dependent/Koha/Item.t Comment on attachment 188043 [details] [review] 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 > Created attachment 188276 [details] [review] Bug 29326: Document system preference change 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 Created attachment 188278 [details] [review] Bug 29326: Document system preference change (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_. 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. 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. |