The SIP patron status and information responses always return false foe "too many items lost". It would be reasonable to check the count of lost items still checked out to the patron and compare that to a threshold set in the sip config file. Though not all libraries operate in this way, it seems like a good and reasonable implementation as long is it is properly documented.
Created attachment 145491 [details] [review] Bug 32684: Implement SIP patron status field "too many items lost" The SIP patron status and information responses always return false foe "too many items lost". It would be reasonable to check the count of lost items still checked out to the patron and compare that to a threshold set in the sip config file. Though not all libraries operate in this way, it seems like a good and reasonable implementation as long is it is properly documented. Test Plan: 1) Apply this patch 2) prove t/db_dependent/SIP/Message.t
Created attachment 145492 [details] [review] Bug 32684: Implement SIP patron status field "too many items lost" The SIP patron status and information responses always return false foe "too many items lost". It would be reasonable to check the count of lost items still checked out to the patron and compare that to a threshold set in the sip config file. Though not all libraries operate in this way, it seems like a good and reasonable implementation as long is it is properly documented. This patch adds the ability to set the SIP "too many items lost" flag for a patron based on the number of lost checkouts the patron has where the lost flag on those items is greater than the given flag value. For example, one could specify that the flag be set if the patron has more than 2 items checked out where itemlost is greater than 3. By default the feature is disabled to retain the existing functionality. If enabled, the default itemlost minimum flag value is 1 unless specified. Test Plan: 1) Apply this patch 2) prove t/db_dependent/SIP/Message.t
Created attachment 145493 [details] [review] Bug 32684: Implement SIP patron status field "too many items lost" The SIP patron status and information responses always return false foe "too many items lost". It would be reasonable to check the count of lost items still checked out to the patron and compare that to a threshold set in the sip config file. Though not all libraries operate in this way, it seems like a good and reasonable implementation as long is it is properly documented. This patch adds the ability to set the SIP "too many items lost" flag for a patron based on the number of lost checkouts the patron has where the lost flag on those items is greater than the given flag value. For example, one could specify that the flag be set if the patron has more than 2 items checked out where itemlost is greater than 3. By default the feature is disabled to retain the existing functionality. If enabled, the default itemlost minimum flag value is 1 unless specified. Test Plan: 1) Apply this patch 2) prove t/db_dependent/SIP/Message.t
Created attachment 145494 [details] [review] Bug 32684: Implement SIP patron status field "too many items lost" The SIP patron status and information responses always return false foe "too many items lost". It would be reasonable to check the count of lost items still checked out to the patron and compare that to a threshold set in the sip config file. Though not all libraries operate in this way, it seems like a good and reasonable implementation as long is it is properly documented. This patch adds the ability to set the SIP "too many items lost" flag for a patron based on the number of lost checkouts the patron has where the lost flag on those items is greater than the given flag value. For example, one could specify that the flag be set if the patron has more than 2 items checked out where itemlost is greater than 3. By default the feature is disabled to retain the existing functionality. If enabled, the default itemlost minimum flag value is 1 unless specified. Test Plan: 1) Apply this patch 2) prove t/db_dependent/SIP/Message.t
Created attachment 145786 [details] [review] Bug 32684: Implement SIP patron status field "too many items lost" The SIP patron status and information responses always return false foe "too many items lost". It would be reasonable to check the count of lost items still checked out to the patron and compare that to a threshold set in the sip config file. Though not all libraries operate in this way, it seems like a good and reasonable implementation as long is it is properly documented. This patch adds the ability to set the SIP "too many items lost" flag for a patron based on the number of lost checkouts the patron has where the lost flag on those items is greater than the given flag value. For example, one could specify that the flag be set if the patron has more than 2 items checked out where itemlost is greater than 3. By default the feature is disabled to retain the existing functionality. If enabled, the default itemlost minimum flag value is 1 unless specified. Test Plan: 1) Apply this patch 2) prove t/db_dependent/SIP/Message.t Signed-off-by: David Nind <david@davidnind.com>
The crux is here: + my $too_many_lost = 0; + if ( my $lost_block_checkout = $server->{account}->{lost_block_checkout} ) { + my $lost_block_checkout_value = $server->{account}->{lost_block_checkout_value} // 1; + my $lost_checkouts = Koha::Checkouts->search({ borrowernumber => $patron->borrowernumber, 'itemlost' => { '>=', $lost_block_checkout_value } }, { join => 'item'} )->count; + $too_many_lost = $lost_checkouts >= $lost_block_checkout; + } But we still have: C4/SIP/ILS/Patron.pm: too_many_lost => 0, # for patron_status[9] C4/SIP/ILS/Patron.pod: $bool = $patron->too_many_lost; C4/SIP/ILS/Patron.pod: $bool = $patron-E<gt>too_many_lost; So we should probably move the block to ILS/Patron and leave MsgType as-is, or we should remove the obsoleted too_many_lost from ILS/Patron. Although it is not a big deal, it looks like moving to Patron was expected design-wise. But this has a performance penalty since we dont always need it. Needs feedback
(In reply to Marcel de Rooy from comment #6) > The crux is here: > > + my $too_many_lost = 0; > + if ( my $lost_block_checkout = > $server->{account}->{lost_block_checkout} ) { > + my $lost_block_checkout_value = > $server->{account}->{lost_block_checkout_value} // 1; > + my $lost_checkouts = Koha::Checkouts->search({ borrowernumber => > $patron->borrowernumber, 'itemlost' => { '>=', $lost_block_checkout_value } > }, { join => 'item'} )->count; > + $too_many_lost = $lost_checkouts >= $lost_block_checkout; > + } > > > But we still have: > C4/SIP/ILS/Patron.pm: too_many_lost => 0, # for > patron_status[9] > C4/SIP/ILS/Patron.pod: $bool = $patron->too_many_lost; > C4/SIP/ILS/Patron.pod: $bool = $patron-E<gt>too_many_lost; > > So we should probably move the block to ILS/Patron and leave MsgType as-is, > or we should remove the obsoleted too_many_lost from ILS/Patron. > Although it is not a big deal, it looks like moving to Patron was expected > design-wise. But this has a performance penalty since we dont always need it. > > Needs feedback I think the encapsulation is worth the "possible" single extra query here which only happens if the feature in enabled.
Created attachment 147883 [details] [review] Bug 32684: Implement SIP patron status field "too many items lost" The SIP patron status and information responses always return false foe "too many items lost". It would be reasonable to check the count of lost items still checked out to the patron and compare that to a threshold set in the sip config file. Though not all libraries operate in this way, it seems like a good and reasonable implementation as long is it is properly documented. This patch adds the ability to set the SIP "too many items lost" flag for a patron based on the number of lost checkouts the patron has where the lost flag on those items is greater than the given flag value. For example, one could specify that the flag be set if the patron has more than 2 items checked out where itemlost is greater than 3. By default the feature is disabled to retain the existing functionality. If enabled, the default itemlost minimum flag value is 1 unless specified. Test Plan: 1) Apply this patch 2) prove t/db_dependent/SIP/Message.t Signed-off-by: David Nind <david@davidnind.com>
Created attachment 147884 [details] [review] Bug 32684: (follow-up) Move logic to SIP::ILS::Patron
The $server param addition looks good for the case at hand, but we still have some calls in ILS.pm that do not have it now. Could be a potential cause for confusion in the future.. # git grep "ILS::Patron\->new" [OK] ILS.pm: return C4::SIP::ILS::Patron->new(@_); [checkout] ILS.pm: $circ->patron( $patron = C4::SIP::ILS::Patron->new($patron_id) ); [checkin] ILS.pm: $circ->patron( $patron = C4::SIP::ILS::Patron->new( { borrowernumber => $item->{borrowernumber} } ) ); [pay_fee] ILS.pm: $trans->patron($patron = C4::SIP::ILS::Patron->new($patron_id)); [add_hold] ILS.pm: $patron = C4::SIP::ILS::Patron->new( $patron_id ); [cancel_hold] ILS.pm: $patron = C4::SIP::ILS::Patron->new( $patron_id ); [alter_hold] ILS.pm: $patron = C4::SIP::ILS::Patron->new( $patron_id ); [renew] ILS.pm: $trans->patron($patron = C4::SIP::ILS::Patron->new( $patron_id )); [renew_all] ILS.pm: $trans->patron($patron = C4::SIP::ILS::Patron->new( $patron_id )); Apart from that, this statement will produce a warning: + if ( my $lost_block_checkout = $server->{account}->{lost_block_checkout} ) { Let me submit a simple counter proposal creating an method for it.
Created attachment 147919 [details] [review] Bug 32684: Implement SIP patron status field "too many items lost" The SIP patron status and information responses always return false foe "too many items lost". It would be reasonable to check the count of lost items still checked out to the patron and compare that to a threshold set in the sip config file. Though not all libraries operate in this way, it seems like a good and reasonable implementation as long is it is properly documented. This patch adds the ability to set the SIP "too many items lost" flag for a patron based on the number of lost checkouts the patron has where the lost flag on those items is greater than the given flag value. For example, one could specify that the flag be set if the patron has more than 2 items checked out where itemlost is greater than 3. By default the feature is disabled to retain the existing functionality. If enabled, the default itemlost minimum flag value is 1 unless specified. Test Plan: 1) Apply this patch 2) prove t/db_dependent/SIP/Message.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 147920 [details] [review] Bug 32684: (QA follow-up) Move too_many_lost to Patron Alternative implementation outside sub new. Test plan: Run t/db_dependent/SIP/Message.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Fixing POD warn
Interesting indeed: perl -MPod::Coverage=C4::SIP::ILS::Patron -e 1 C4::SIP::ILS::Patron has a Pod::Coverage rating of 0.214285714285714 The following are uncovered: build_custom_field_string, build_patron_attributes_string, charge_denied, charged_items, excessive_fees, excessive_fines, expired, fine_items, fines_amount, format, hold_items, holds_blocked_by_excessive_fees, inet_privileges, invalid_patron, library_name, new, overdue_items, recall_items, too_many_lost, unavail_holds, update_lastseen, x_items root@master:/usr/share/koha# rm C4/SIP/ILS/Patron.pod root@master:/usr/share/koha# perl -MPod::Coverage=C4::SIP::ILS::Patron -e 1 C4::SIP::ILS::Patron has a Pod::Coverage rating of 0.178571428571429 The following are uncovered: block, charge_denied, charged_items, check_password, drop_hold, enable, excessive_fees, excessive_fines, expired, fee_amount, fine_items, fines_amount, hold_items, holds_blocked_by_excessive_fees, inet_privileges, invalid_patron, language, library_name, new, overdue_items, recall_items, unavail_holds, x_items => Deleting the Patron.pod makes a difference in the POD::Coverage output used by qa tools. Note for QA/RM: The warn on too_many_lost is a false negative.
Created attachment 147924 [details] [review] Bug 32684: (QA follow-up) Move too_many_lost to Patron Alternative implementation outside sub new. Test plan: Run t/db_dependent/SIP/Message.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Kyle: If you agree with the follow-up, I will pass qa
Created attachment 147969 [details] [review] Bug 32684: (QA follow-up) Move too_many_lost to Patron Alternative implementation outside sub new. Test plan: Run t/db_dependent/SIP/Message.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(In reply to Marcel de Rooy from comment #16) > Kyle: If you agree with the follow-up, I will pass qa Looks good to me!
Pushed to master for 23.05. Nice work everyone, thanks!
Nice work, thanks everyone! Pushed to 22.11.x for the next release.
Enhancement will not be backported to 22.05.x.