Bug 32684 - Implement SIP patron status field "too many items lost"
Summary: Implement SIP patron status field "too many items lost"
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-01-19 18:45 UTC by Kyle M Hall
Modified: 2023-12-28 20:42 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.04


Attachments
Bug 32684: Implement SIP patron status field "too many items lost" (6.88 KB, patch)
2023-01-19 18:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32684: Implement SIP patron status field "too many items lost" (7.36 KB, patch)
2023-01-19 19:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32684: Implement SIP patron status field "too many items lost" (7.36 KB, patch)
2023-01-19 19:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32684: Implement SIP patron status field "too many items lost" (8.44 KB, patch)
2023-01-19 19:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32684: Implement SIP patron status field "too many items lost" (8.49 KB, patch)
2023-01-28 18:42 UTC, David Nind
Details | Diff | Splinter Review
Bug 32684: Implement SIP patron status field "too many items lost" (8.28 KB, patch)
2023-03-07 20:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32684: (follow-up) Move logic to SIP::ILS::Patron (7.63 KB, patch)
2023-03-07 20:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32684: Implement SIP patron status field "too many items lost" (8.38 KB, patch)
2023-03-08 12:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32684: (QA follow-up) Move too_many_lost to Patron (3.20 KB, patch)
2023-03-08 12:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32684: (QA follow-up) Move too_many_lost to Patron (4.50 KB, patch)
2023-03-08 13:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32684: (QA follow-up) Move too_many_lost to Patron (4.52 KB, patch)
2023-03-08 17:38 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2023-01-19 18:45:46 UTC
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.
Comment 1 Kyle M Hall 2023-01-19 18:46:19 UTC
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
Comment 2 Kyle M Hall 2023-01-19 19:00:46 UTC
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
Comment 3 Kyle M Hall 2023-01-19 19:01:44 UTC
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
Comment 4 Kyle M Hall 2023-01-19 19:02:43 UTC
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
Comment 5 David Nind 2023-01-28 18:42:08 UTC
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>
Comment 6 Marcel de Rooy 2023-03-07 13:05:00 UTC
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
Comment 7 Kyle M Hall 2023-03-07 20:01:03 UTC
(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.
Comment 8 Kyle M Hall 2023-03-07 20:02:31 UTC
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>
Comment 9 Kyle M Hall 2023-03-07 20:02:38 UTC
Created attachment 147884 [details] [review]
Bug 32684: (follow-up) Move logic to SIP::ILS::Patron
Comment 10 Marcel de Rooy 2023-03-08 12:45:42 UTC
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.
Comment 11 Marcel de Rooy 2023-03-08 12:47:08 UTC
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>
Comment 12 Marcel de Rooy 2023-03-08 12:47:11 UTC
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>
Comment 13 Marcel de Rooy 2023-03-08 13:05:10 UTC
Fixing POD warn
Comment 14 Marcel de Rooy 2023-03-08 13:25:32 UTC
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.
Comment 15 Marcel de Rooy 2023-03-08 13:27:40 UTC
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>
Comment 16 Marcel de Rooy 2023-03-08 13:29:14 UTC
Kyle: If you agree with the follow-up, I will pass qa
Comment 17 Kyle M Hall 2023-03-08 17:38:37 UTC
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>
Comment 18 Kyle M Hall 2023-03-08 17:38:52 UTC
(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!
Comment 19 Tomás Cohen Arazi 2023-03-14 12:10:38 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 20 Jacob O'Mara 2023-03-16 19:49:06 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 21 Lucas Gass 2023-04-14 18:13:56 UTC
Enhancement will not be backported to 22.05.x.