From 68f30ce14556348c9d8c2e86374f7dc6313cf8e7 Mon Sep 17 00:00:00 2001 From: Pedro Amorim Date: Mon, 20 Jan 2025 16:07:40 +0000 Subject: [PATCH] Bug 38441: (QA follow-up): HistoryCheck now only considers same patron Initial implementation checked existing ILLs for same patron and other patrons. User testing reported ILLs for other patrons are not helpful, and makes this stage screen show up much more frequently than it should. This patch simplifies the original implementation to only consider the patron of the request being placed for an ILL history check. It also changes the ID matching to only 'DOI' or only 'Pubmed ID' if any of those is set. This is to minimize scenarios where different articles from the same journal come up as matching results e.g. same ISSN but different DOI. It is also updating tests accordingly. Other QA improvements Improve HistoryCheck request list: - Make link external and show icon (to prevent staff member from clicking away from the historyCheck screen unintentionally) - Show requests' status - Show requests' placed date - Add id_prefix to request ID if possible Signed-off-by: Lisette Scheer --- Koha/ILL/Request/Workflow/HistoryCheck.pm | 88 +++++-------------- .../prog/en/modules/ill/ill-requests.tt | 20 ++--- .../Koha/ILL/Request/Workflow/HistoryCheck.t | 70 ++++++--------- 3 files changed, 50 insertions(+), 128 deletions(-) diff --git a/Koha/ILL/Request/Workflow/HistoryCheck.pm b/Koha/ILL/Request/Workflow/HistoryCheck.pm index 421fd01f184..00a0f85012a 100644 --- a/Koha/ILL/Request/Workflow/HistoryCheck.pm +++ b/Koha/ILL/Request/Workflow/HistoryCheck.pm @@ -52,23 +52,11 @@ Given $params, return true if history check should be shown sub show_history_check { my ( $self, $request ) = @_; - my $opac_no_matching_requests_for_patron = 0; - if ( $self->{ui_context} eq 'opac' ) { - my $patron_cardnumber = C4::Context->userenv ? C4::Context->userenv->{'cardnumber'} || 0 : 0; - my ( $matching_requests_for_patron, $remaining_matching_requests ) = - $self->_get_split_matching_requests($patron_cardnumber); - $opac_no_matching_requests_for_patron = 1 - if $matching_requests_for_patron && !scalar @{$matching_requests_for_patron}; - } - return # ILLHistoryCheck is enabled C4::Context->yaml_preference("ILLHistoryCheck") - # It's not OPAC with no matching requests for patron - && !$opac_no_matching_requests_for_patron - # Matching requests were found && $self->_find_matching_requests() @@ -107,14 +95,12 @@ sub history_check_template_params { $mapping_function = sub { return $_[0] }; } - my ( $matching_requests_for_patron, $remaining_matching_requests ) = - $self->_get_split_matching_requests( $params->{cardnumber} ); + my $matching_requests_for_patron = $self->_find_matching_requests(); return ( whole => $params, metadata => $self->prep_metadata($params), matching_requests_for_patron => $matching_requests_for_patron, - remaining_matching_requests => $remaining_matching_requests, $self->{ui_context} eq 'staff' ? ( key_mapping => $mapping_function, @@ -144,9 +130,8 @@ sub after_request_created { return if ( $self->{ui_context} ne 'opac' ); - my $patron_cardnumber = C4::Context->userenv->{'cardnumber'} || 0; - my ( $matching_requests_for_patron, $remaining_matching_requests ) = - $self->_get_split_matching_requests($patron_cardnumber); + my $patron_cardnumber = C4::Context->userenv->{'cardnumber'} || 0; + my $matching_requests_for_patron = $self->_find_matching_requests(); my $staffnotes; @@ -162,19 +147,6 @@ sub after_request_created { } } - if ($remaining_matching_requests) { - my $appended_others_note = 0; - foreach my $matching_request ( @{$remaining_matching_requests} ) { - next if $matching_request->illrequest_id eq $request->illrequest_id; - if ( $appended_others_note == 0 ) { - $staffnotes .= - ( $staffnotes ? "\n" : '' ) . __("Request has been submitted by other patrons in the past:"); - $appended_others_note = 1; - } - $staffnotes .= ' ' . $self->_get_request_staff_link($matching_request); - } - } - $request->append_to_note($staffnotes)->store() if $staffnotes; } @@ -196,38 +168,6 @@ sub _get_request_staff_link { . $request->illrequest_id . ''; } -=head3 _get_split_matching_requests - - my ( $matching_requests_for_patron, $remaining_matching_requests ) - = $self->_get_split_matching_requests( $cardnumber ); - -Splits the matching requests from _find_matching_requests into two arrays. - -One array contains ILL requests made by the patron with the cardnumber -specified, and the other contains the rest of the matching requests. - -=cut - -sub _get_split_matching_requests { - my ( $self, $cardnumber ) = @_; - - my $all_matching_requests = $self->_find_matching_requests(); - my @matching_requests_for_patron; - my @remaining_matching_requests; - - return ( undef, undef ) if !$all_matching_requests; - - foreach my $request ( @{$all_matching_requests} ) { - if ( $request->patron && $request->patron->cardnumber eq $cardnumber ) { - push @matching_requests_for_patron, $request; - } else { - push @remaining_matching_requests, $request; - } - } - return ( \@matching_requests_for_patron, \@remaining_matching_requests ); - -} - =head3 _find_matching_requests my $matching_requests = $self->_find_matching_requests(); @@ -241,18 +181,30 @@ sub _find_matching_requests { my @id_fields = ( 'doi', 'issn', 'isbn', 'pubmedid' ); - return 0 unless grep { $self->{metadata}->{$_} } @id_fields; + my @existing_id_fields = grep { $self->{metadata}->{$_} } @id_fields; + return 0 unless scalar @existing_id_fields; + + if ( grep { $_ eq 'doi' } @existing_id_fields ) { + @existing_id_fields = grep { $_ eq 'doi' } @existing_id_fields; + } + + if ( grep { $_ eq 'pubmedid' } @existing_id_fields ) { + @existing_id_fields = grep { $_ eq 'pubmedid' } @existing_id_fields; + } + + my $patron = Koha::Patrons->find( { cardnumber => $self->{metadata}->{cardnumber} } ); - my @query = (); - foreach my $id_field (@id_fields) { - push @query, { + my $query; + $query->{'-and'} = [ { 'me.borrowernumber' => $patron->borrowernumber } ] if $patron; + foreach my $id_field (@existing_id_fields) { + push @{ $query->{'-or'} }, { 'illrequestattributes.type' => $id_field, 'illrequestattributes.value' => $self->{metadata}->{$id_field}, }; } my $matching_requests = Koha::ILL::Requests->search( - \@query, + $query, { join => 'illrequestattributes', distinct => 'illrequest_id', diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt index 371d24cda04..e7eb52cb423 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt @@ -879,25 +879,15 @@ [% END %]   -

This interlibrary loan has been requested before:

[% IF matching_requests_for_patron.size %] -

By this patron [%- INCLUDE 'patron-title.inc' patron => request_patron_obj hide_patron_infos_if_needed => 1 -%]:

+

This interlibrary loan has already been placed by this patron [%- INCLUDE 'patron-title.inc' patron => request_patron_obj hide_patron_infos_if_needed => 1 -%]:

- [% END %] - [% IF remaining_matching_requests.size %] - [% IF matching_requests_for_patron.size %] -

By other patrons:

- [% END %] - diff --git a/t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t b/t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t index c819e8fd8f7..0e2af3b5549 100755 --- a/t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t +++ b/t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t @@ -42,10 +42,19 @@ subtest 'show_history_check' => sub { use_ok('Koha::ILL::Request::Workflow::HistoryCheck'); + my $fake_cardnumber = '123456789'; + my $ill_patron = $builder->build_object( + { + class => 'Koha::Patrons', + value => { cardnumber => $fake_cardnumber } + } + ); + my $metadata = { - title => 'This is a title', - author => 'This is an author', - issn => $issn + title => 'This is a title', + author => 'This is an author', + issn => $issn, + cardnumber => $fake_cardnumber, }; # Because hashes can reorder themselves, we need to make sure ours is in a @@ -61,7 +70,7 @@ subtest 'show_history_check' => sub { is( $history_check->prep_metadata($sorted), - 'eyJhdXRob3IiOiJUaGlzIGlzIGFuIGF1dGhvciIsImlzc24iOiIzMjEzMjEzMjEiLCJ0aXRsZSI6%0AIlRoaXMgaXMgYSB0aXRsZSJ9%0A', + 'eyJhdXRob3IiOiJUaGlzIGlzIGFuIGF1dGhvciIsImNhcmRudW1iZXIiOiIxMjM0NTY3ODkiLCJp%0Ac3NuIjoiMzIxMzIxMzIxIiwidGl0bGUiOiJUaGlzIGlzIGEgdGl0bGUifQ%3D%3D%0A', 'prep_metadata works' ); @@ -77,7 +86,7 @@ subtest 'show_history_check' => sub { # Mock ILLHistoryCheck enabled t::lib::Mocks::mock_preference( 'ILLHistoryCheck', 1 ); - my $ill_request = $builder->build_sample_ill_request(); + my $ill_request = $builder->build_sample_ill_request( { borrowernumber => $ill_patron->borrowernumber } ); is( $history_check->show_history_check($ill_request), 0, 'Request with ISSN ' . $issn . ' does not exist even though syspref is on. Not showing history check screen' @@ -92,7 +101,7 @@ subtest 'show_history_check' => sub { is( $history_check->show_history_check($ill_request), - 1, 'Request with ISSN ' . $issn . ' exists and syspref is on. Able to show history check screen' + 1, 'Request with ISSN ' . $issn . ' exists, syspref is on and is same patron. Able to show history check screen' ); # Mock ILLHistoryCheck disabled @@ -109,13 +118,14 @@ subtest 'show_history_check' => sub { subtest 'after_request_created' => sub { - plan tests => 4; + plan tests => 2; $schema->storage->txn_begin; my $builder = t::lib::TestBuilder->new; - my $metadata = { + my $fake_cardnumber = '123456789'; + my $metadata = { title => 'This is a title', author => 'This is an author', issn => $issn @@ -127,24 +137,8 @@ subtest 'after_request_created' => sub { } ); - my $other_patron = $builder->build_object( - { - class => 'Koha::Patrons', - } - ); - t::lib::Mocks::mock_userenv( { patron => $authenticated_patron } ); - # Create an old request - my $existing_ill_request = - $builder->build_sample_ill_request( { borrowernumber => $other_patron->borrowernumber } ); - $builder->build( - { - source => 'Illrequestattribute', - value => { illrequest_id => $existing_ill_request->illrequest_id, type => 'issn', value => $issn } - } - ); - # Create a new request with new issn my $new_ill_request = $builder->build_sample_ill_request( { borrowernumber => $authenticated_patron->borrowernumber } ); @@ -165,42 +159,28 @@ subtest 'after_request_created' => sub { 'History check didn\'t find any matching requests. Staff notes have not been updated.' ); - # Create a third request with preexisting issn by other patron + # Create a second request with preexisting issn by self patron + my $second_ill_request = + $builder->build_sample_ill_request( { borrowernumber => $authenticated_patron->borrowernumber } ); my $third_ill_request = $builder->build_sample_ill_request( { borrowernumber => $authenticated_patron->borrowernumber } ); $metadata->{issn} = $issn; $builder->build( { source => 'Illrequestattribute', - value => { illrequest_id => $third_ill_request->illrequest_id, type => 'issn', value => $issn } + value => { illrequest_id => $second_ill_request->illrequest_id, type => 'issn', value => $issn } } ); - $opac_history_check->after_request_created( $metadata, $third_ill_request ); - - like( - $third_ill_request->notesstaff, qr/Request has been submitted by other patrons in the past/, - 'Contains staffnotes related submissions by other patrons' - ); - - # Create a fourth request with preexisting issn by self patron and others - my $fourth_ill_request = - $builder->build_sample_ill_request( { borrowernumber => $authenticated_patron->borrowernumber } ); - $metadata->{issn} = $issn; $builder->build( { source => 'Illrequestattribute', - value => { illrequest_id => $fourth_ill_request->illrequest_id, type => 'issn', value => $issn } + value => { illrequest_id => $third_ill_request->illrequest_id, type => 'issn', value => $issn } } ); - $opac_history_check->after_request_created( $metadata, $fourth_ill_request ); - - like( - $fourth_ill_request->notesstaff, qr/Request has been submitted by other patrons in the past/, - 'Contains staffnotes related submissions by other patrons' - ); + $opac_history_check->after_request_created( $metadata, $third_ill_request ); like( - $fourth_ill_request->notesstaff, qr/Request has been submitted by this patron in the past/, + $third_ill_request->notesstaff, qr/Request has been submitted by this patron in the past/, 'Contains staffnotes related submissions by self patron' ); -- 2.39.5