Bugzilla – Attachment 180045 Details for
Bug 38441
Allow for an ILL history check workflow stage
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 38441: (QA follow-up): HistoryCheck now only considers same patron
Bug-38441-QA-follow-up-HistoryCheck-now-only-consi.patch (text/plain), 15.87 KB, created by
Lisette Scheer
on 2025-03-31 13:47:21 UTC
(
hide
)
Description:
Bug 38441: (QA follow-up): HistoryCheck now only considers same patron
Filename:
MIME Type:
Creator:
Lisette Scheer
Created:
2025-03-31 13:47:21 UTC
Size:
15.87 KB
patch
obsolete
>From 68f30ce14556348c9d8c2e86374f7dc6313cf8e7 Mon Sep 17 00:00:00 2001 >From: Pedro Amorim <pedro.amorim@ptfs-europe.com> >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 <lisette@bywatersolutions.com> >--- > 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 . '</a>'; > } > >-=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 %] > </table> > >- <h3>This interlibrary loan has been requested before:</h3> > [% IF matching_requests_for_patron.size %] >- <h4>By this patron [%- INCLUDE 'patron-title.inc' patron => request_patron_obj hide_patron_infos_if_needed => 1 -%]:</h4> >+ <h4>This interlibrary loan has already been placed by this patron [%- INCLUDE 'patron-title.inc' patron => request_patron_obj hide_patron_infos_if_needed => 1 -%]:</h4> > <ul class="list-unstyled"> > [% FOREACH matching_request_for_patron IN matching_requests_for_patron %] > <li> >- <a href="/cgi-bin/koha/ill/ill-requests.pl?op=illview&illrequest_id=[% matching_request_for_patron.illrequest_id %]">ILL Request #[% matching_request_for_patron.illrequest_id | html %]</a> >- </li> >- [% END %] >- </ul> >- [% END %] >- [% IF remaining_matching_requests.size %] >- [% IF matching_requests_for_patron.size %] >- <h4>By other patrons:</h4> >- [% END %] >- <ul class="list-unstyled"> >- [% FOREACH remaining_matching_request IN remaining_matching_requests %] >- <li> >- <a href="/cgi-bin/koha/ill/ill-requests.pl?op=illview&illrequest_id=[% remaining_matching_request.illrequest_id %]">ILL Request #[% remaining_matching_request.illrequest_id | html %]</a> by [%- INCLUDE 'patron-title.inc' patron => remaining_matching_request.patron hide_patron_infos_if_needed => 1 -%] >+ <a target="_blank" href="/cgi-bin/koha/ill/ill-requests.pl?op=illview&illrequest_id=[% matching_request_for_patron.illrequest_id | uri %]" >+ ><i class="fa fa-fw fa-external-link-square"></i>Request [% matching_request_for_patron.id_prefix | html %][% matching_request_for_patron.illrequest_id | html %] >+ ([% matching_request_for_patron.status | html %]) on [% matching_request_for_patron.placed | html %]</a >+ > > </li> > [% END %] > </ul> >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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 38441
:
174604
|
174605
|
174606
|
174607
|
174608
|
174609
|
174610
|
174611
|
175300
|
175301
|
175302
|
175303
|
175304
|
175305
|
175306
|
175307
|
176507
|
176508
|
176509
|
176510
|
176511
|
176512
|
176513
|
176514
|
176561
|
176566
|
176573
|
176829
|
179800
|
179801
|
179802
|
179803
|
179804
|
179805
|
179806
|
179807
|
179808
|
179809
|
179810
|
179811
|
180034
|
180035
|
180036
|
180037
|
180038
|
180039
|
180040
|
180041
|
180042
|
180043
|
180044
|
180045
|
180907
|
180908
|
180909
|
180910
|
180911
|
180912
|
180913
|
180914
|
180915
|
180916
|
180917
|
180918
|
180919
|
181072
|
181111
|
181112
|
181113
|
181114
|
181115
|
181116
|
181117
|
181118
|
181119
|
181120
|
181121
|
181122
|
181123
|
181124
|
181125
|
181126
|
181593
|
181594
|
181595
|
181596
|
181597
|
181598
|
181599
|
181600
|
181601
|
181602
|
181603
|
181604
|
181605
|
181606
|
181607