[Bug 32684] New: Implement SIP patron status field "too many items lost"
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 Bug ID: 32684 Summary: Implement SIP patron status field "too many items lost" Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: SIP2 Assignee: koha-bugs@lists.koha-community.org Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org 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. -- 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=32684 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kyle@bywatersolutions.com |ity.org | -- 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=32684 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 145491 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145491&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145491|0 |1 is obsolete| | --- Comment #2 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 145492 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145492&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145492|0 |1 is obsolete| | --- Comment #3 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 145493 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145493&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145493|0 |1 is obsolete| | --- Comment #4 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 145494 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145494&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145494|0 |1 is obsolete| | --- Comment #5 from David Nind <david@davidnind.com> --- Created attachment 145786 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145786&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 --- Comment #7 from Kyle M Hall <kyle@bywatersolutions.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145786|0 |1 is obsolete| | --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 147883 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=147883&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 147884 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=147884&action=edit Bug 32684: (follow-up) Move logic to SIP::ILS::Patron -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #147883|0 |1 is obsolete| | Attachment #147884|0 |1 is obsolete| | --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 147919 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=147919&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 147920 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=147920&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Fixing POD warn -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #147920|0 |1 is obsolete| | --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 147924 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=147924&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 --- Comment #16 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Kyle: If you agree with the follow-up, I will pass qa -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #147924|0 |1 is obsolete| | --- Comment #17 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 147969 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=147969&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 --- Comment #18 from 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |23.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 --- Comment #19 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.05. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 Jacob O'Mara <jacob.omara@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|23.05.00 |23.05.00,22.11.04 released in| | Status|Pushed to master |Pushed to stable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 --- Comment #20 from Jacob O'Mara <jacob.omara@ptfs-europe.com> --- Nice work, thanks everyone! Pushed to 22.11.x for the next release. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32684 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Resolution|--- |FIXED Status|Pushed to stable |RESOLVED --- Comment #21 from Lucas Gass <lucas@bywatersolutions.com> --- Enhancement will not be backported to 22.05.x. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org