Bugzilla – Attachment 181124 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): Hackfest feedback
Bug-38441-QA-follow-up-Hackfest-feedback.patch (text/plain), 22.77 KB, created by
Tomás Cohen Arazi (tcohen)
on 2025-04-17 18:16:28 UTC
(
hide
)
Description:
Bug 38441: (QA follow-up): Hackfest feedback
Filename:
MIME Type:
Creator:
Tomás Cohen Arazi (tcohen)
Created:
2025-04-17 18:16:28 UTC
Size:
22.77 KB
patch
obsolete
>From cc0a68625dc139e458284cb569113683a38874a3 Mon Sep 17 00:00:00 2001 >From: Pedro Amorim <pedro.amorim@openfifth.co.uk> >Date: Mon, 14 Apr 2025 11:11:48 +0000 >Subject: [PATCH] Bug 38441: (QA follow-up): Hackfest feedback > >This commit addresses all Hackfest QA related feedback: >1) Update of all strings that required update, including UI messages and > sys pref description >2) Moved the history check information from staffnotes to its own > pipe-separated illrequestattribute 'historycheck_requests' >3) Fix display of ILL request information on the history check screen > (Staff), including proper label naming (e.g. branchcode -> library), >show status description instead of code, uppercase labels. >4) Updated OPAC display to match Staff UI's display of matching history > requests >5) Removed ISSN criteria for matching requests check >6) Update tests accordingly > >Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> >--- > Koha/ILL/Backend/Standard.pm | 2 +- > Koha/ILL/Request.pm | 17 +++++ > Koha/ILL/Request/Workflow/HistoryCheck.pm | 67 ++++++++++--------- > .../data/mysql/atomicupdate/bug_38441.pl | 2 +- > installer/data/mysql/mandatory/sysprefs.sql | 2 +- > .../admin/preferences/interlibrary_loans.pref | 2 +- > .../prog/en/modules/ill/ill-requests.tt | 31 +++++++-- > .../bootstrap/en/modules/opac-illrequests.tt | 6 +- > .../Koha/ILL/Request/Workflow/HistoryCheck.t | 39 +++++------ > 9 files changed, 104 insertions(+), 64 deletions(-) > >diff --git a/Koha/ILL/Backend/Standard.pm b/Koha/ILL/Backend/Standard.pm >index 4ebc1f8311d..08891086b07 100644 >--- a/Koha/ILL/Backend/Standard.pm >+++ b/Koha/ILL/Backend/Standard.pm >@@ -149,7 +149,7 @@ sub metadata { > my $metadata = {}; > my @ignore = ( > 'requested_partners', 'type', 'type_disclaimer_value', 'type_disclaimer_date', 'unauthenticated_first_name', >- 'unauthenticated_last_name', 'unauthenticated_email' >+ 'unauthenticated_last_name', 'unauthenticated_email', 'historycheck_requests' > ); > my $core_fields = _get_core_fields(); > while ( my $attr = $attrs->next ) { >diff --git a/Koha/ILL/Request.pm b/Koha/ILL/Request.pm >index 265c2b8f876..979b975368d 100644 >--- a/Koha/ILL/Request.pm >+++ b/Koha/ILL/Request.pm >@@ -2267,6 +2267,23 @@ sub can_patron_place_ill_in_opac { > return 1; > } > >+=head3 get_history_check_requests >+ >+ $self->get_history_check_requests(); >+ >+Returns a list of history check requests for this request >+ >+=cut >+ >+sub get_history_check_requests { >+ my ($self) = @_; >+ >+ my $historycheck_attribute = $self->extended_attributes->find( { 'type' => 'historycheck_requests' } ); >+ return unless $historycheck_attribute; >+ >+ return Koha::ILL::Requests->search( { 'me.illrequest_id' => [ split qr{\|}, $historycheck_attribute->value ] } ); >+} >+ > =head3 get_op_param_deprecation > > my $op = $req->check_url_param_deprecation($params); >diff --git a/Koha/ILL/Request/Workflow/HistoryCheck.pm b/Koha/ILL/Request/Workflow/HistoryCheck.pm >index 00a0f85012a..5a3eefbddf4 100644 >--- a/Koha/ILL/Request/Workflow/HistoryCheck.pm >+++ b/Koha/ILL/Request/Workflow/HistoryCheck.pm >@@ -85,16 +85,32 @@ sub history_check_template_params { > > $params->{cardnumber} = C4::Context->userenv->{'cardnumber'} || 0 if $self->{ui_context} eq 'opac'; > >- my $backend = Koha::ILL::Request->new->load_backend( $params->{backend} ); >+ my $backend = Koha::ILL::Request->new->load_backend( $params->{backend} ); >+ my $fields_mapping = { >+ cardnumber => __('Cardnumber'), >+ branchcode => __('Library'), >+ backend => __('Backend'), >+ }; > my $mapping_function; > eval { >- $mapping_function = sub { return $backend->{_my_backend}->_get_core_fields()->{ $_[0] } }; >- $backend->{_my_backend}->_get_core_fields(); >+ my $backend_mapping = $backend->{_my_backend}->_get_core_fields(); >+ $fields_mapping = { %$fields_mapping, %$backend_mapping }; >+ $mapping_function = sub { return $fields_mapping->{ $_[0] } }; > }; > if ($@) { >- $mapping_function = sub { return $_[0] }; >+ $mapping_function = sub { return $fields_mapping->{ $_[0] } }; > } > >+ my @sorted_params = sort { >+ my $a_val = $mapping_function->($a) // $a; >+ my $b_val = $mapping_function->($b) // $b; >+ lc($a_val) cmp lc($b_val) >+ } keys %$params; >+ >+ @sorted_params = map { >+ { $_ => $params->{$_} } >+ } @sorted_params; >+ > my $matching_requests_for_patron = $self->_find_matching_requests(); > > return ( >@@ -103,6 +119,7 @@ sub history_check_template_params { > matching_requests_for_patron => $matching_requests_for_patron, > $self->{ui_context} eq 'staff' > ? ( >+ sorted_params => \@sorted_params, > key_mapping => $mapping_function, > request_patron_obj => Koha::Patrons->find( { cardnumber => $params->{cardnumber} } ) > ) >@@ -133,53 +150,37 @@ sub after_request_created { > my $patron_cardnumber = C4::Context->userenv->{'cardnumber'} || 0; > my $matching_requests_for_patron = $self->_find_matching_requests(); > >- my $staffnotes; >- >+ my $historycheck_requests; > if ($matching_requests_for_patron) { >- my $appended_self_note = 0; > foreach my $matching_request ( @{$matching_requests_for_patron} ) { > next if $matching_request->illrequest_id eq $request->illrequest_id; >- if ( $appended_self_note == 0 ) { >- $staffnotes .= __("Request has been submitted by this patron in the past:"); >- $appended_self_note = 1; >- } >- $staffnotes .= ' ' . $self->_get_request_staff_link($matching_request); >+ $historycheck_requests .= >+ $historycheck_requests ? '|' . $matching_request->illrequest_id : $matching_request->illrequest_id; > } > } > >- $request->append_to_note($staffnotes)->store() if $staffnotes; >-} >- >-=head3 _get_request_staff_link >- >- $self->_get_request_staff_link($matching_request); >- >-Returns an HTML staff link to the provided request >- >-=cut >- >-sub _get_request_staff_link { >- my ( $self, $request ) = @_; >- >- return >- '<a href="/cgi-bin/koha/ill/ill-requests.pl?op=illview&illrequest_id=' >- . $request->illrequest_id . '">' >- . __("ILL Request #") >- . $request->illrequest_id . '</a>'; >+ $request->extended_attributes( >+ [ >+ { >+ type => 'historycheck_requests', >+ value => $historycheck_requests, >+ }, >+ ] >+ ) if $historycheck_requests; > } > > =head3 _find_matching_requests > > my $matching_requests = $self->_find_matching_requests(); > >-Returns a list of matching requests (match is done by doi, issn, isbn, pubmedid) >+Returns a list of matching requests (match is done by doi, isbn, pubmedid) > > =cut > > sub _find_matching_requests { > my ($self) = @_; > >- my @id_fields = ( 'doi', 'issn', 'isbn', 'pubmedid' ); >+ my @id_fields = ( 'doi', 'isbn', 'pubmedid' ); > > my @existing_id_fields = grep { $self->{metadata}->{$_} } @id_fields; > return 0 unless scalar @existing_id_fields; >diff --git a/installer/data/mysql/atomicupdate/bug_38441.pl b/installer/data/mysql/atomicupdate/bug_38441.pl >index 3b56b6852d4..9a584ebc3d1 100755 >--- a/installer/data/mysql/atomicupdate/bug_38441.pl >+++ b/installer/data/mysql/atomicupdate/bug_38441.pl >@@ -14,7 +14,7 @@ return { > q{ > INSERT IGNORE INTO systempreferences (variable,value,options,explanation,type) VALUES ( > 'ILLHistoryCheck', '', '', >- 'If ON, during the ILL request process, a check is performed to see if the ILL request has already been previously placed', >+ 'If ON, a verification is performed to check if the ILL request has previously been placed by the same patron. Verification is done using one of the following identifier fields: DOI, Pubmed ID or ISBN', > 'YesNo' > ); > } >diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql >index 06a5d28e868..ab840c09344 100644 >--- a/installer/data/mysql/mandatory/sysprefs.sql >+++ b/installer/data/mysql/mandatory/sysprefs.sql >@@ -324,7 +324,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` > ('ILLCheckAvailability', 0, '', 'If ON, during the ILL request process third party sources will be checked for current availability', 'YesNo'), > ('ILLDefaultStaffEmail', '', NULL, 'Fallback email address for staff ILL notices to be sent to in the absence of a branch address', 'Free'), > ('ILLHiddenRequestStatuses', NULL, NULL, 'ILL statuses that are considered finished and should not be displayed in the ILL module', 'multiple'), >-('ILLHistoryCheck', 0, '', 'If ON, during the ILL request process, a check is performed to see if the ILL request has already been previously placed', 'YesNo'), >+('ILLHistoryCheck', 0, '', 'If ON, a verification is performed to check if the ILL request has previously been placed by the same patron. Verification is done using one of the following identifier fields: DOI, Pubmed ID or ISBN', 'YesNo'), > ('IllLog', 0, '', 'If ON, log information about ILL requests', 'YesNo'), > ('ILLModule','0','If ON, enables the interlibrary loans module.','','YesNo'), > ('ILLModuleDisclaimerByType','','','YAML defining disclaimer settings for each ILL request type','Textarea'), >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/interlibrary_loans.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/interlibrary_loans.pref >index 3763ec93530..858ee775541 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/interlibrary_loans.pref >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/interlibrary_loans.pref >@@ -69,7 +69,7 @@ Interlibrary loans: > choices: > 1: Check > 0: "Don't check" >- - if an ILL request has already been placed during the request process. >+ - "if an ILL request has previously been placed by the same patron. Verification is done using one of the following identifier fields: DOI, Pubmed ID or ISBN." > - > - "If the ILL backend supports it, adding YAML will enable the request type disclaimer stage in request creation, prompting the user with a request type specific disclaimer as defined (staff interface and OPAC)." > - "For each request type, you can have 3 configuration options: <code>text</code>, <code>av_category_code</code> and <code>bypass</code><ul>" >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 2a4b4e1d564..dd8dfa5b72b 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 >@@ -630,6 +630,19 @@ > [% type | html %] > </li> > [% END %] >+ [% historycheck_requests = request.get_history_check_requests %] >+ [% IF historycheck_requests.count %] >+ <li class="historycheck_requests_value"> >+ <span class="label historycheck_requests_value">History check requests:</span> >+ [% FOREACH historycheck_request IN historycheck_requests %] >+ [% SET status = historycheck_request.status %] >+ <a target="_blank" href="/cgi-bin/koha/ill/ill-requests.pl?op=illview&illrequest_id=[% historycheck_request.illrequest_id | uri %]" >+ ><i class="fa fa-fw fa-external-link-square"></i>Request [% historycheck_request.id_prefix | html %][% historycheck_request.illrequest_id | html %] >+ ([% historycheck_request.capabilities.$status.name | html %]) on [% historycheck_request.placed | $KohaDates %]</a >+ > >+ [% END %] >+ </li> >+ [% END %] > [% type_disclaimer_value = request.get_type_disclaimer_value %] > [% IF type_disclaimer_value %] > <li class="type_disclaimer_value"> >@@ -896,25 +909,31 @@ > [% INCLUDE 'csrf-token.inc' %] > <h3>You are attempting to place the following request:</h3> > <table> >- [% FOREACH key IN whole.keys %] >- [% value = whole.$key %] >+ [% FOREACH param IN sorted_params %] >+ [% key = param.keys.0 %] >+ [% value = param.$key %] > [% IF key != 'op' && key != 'method' && key != 'custom_key' && key != 'custom_value' && key != 'csrf_token' && value %] > <tr> >- <td>[% key_mapping(key) || key | html %]</td> >- <td>[% value | html %]</td> >+ <td>[% key_mapping(key) || key | ucfirst | html %]</td> >+ [% IF key == 'branchcode' %] >+ <td>[% Branches.GetName(value) | html %]</td> >+ [% ELSE %] >+ <td>[% value | html %]</td> >+ [% END %] > </tr> > [% END %] > [% END %] > </table> > > [% IF matching_requests_for_patron.size %] >- <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> >+ <h4>This request has already been placed by 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 %] >+ [% SET matching_req_status = matching_request_for_patron.status %] > <li> > <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 >+ ([% matching_request_for_patron.capabilities.$matching_req_status.name | html %]) on [% matching_request_for_patron.placed | $KohaDates %]</a > > > </li> > [% END %] >diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt >index 676214a2763..3d93f7d7246 100644 >--- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt >+++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt >@@ -333,9 +333,11 @@ > [% IF matching_requests_for_patron.size %] > <ul class="list-unstyled"> > [% FOREACH matching_request_for_patron IN matching_requests_for_patron %] >+ [% SET matching_req_status = matching_request_for_patron.status %] > <li> >- <a href="/cgi-bin/koha/opac-illrequests.pl?op=view&illrequest_id=[% matching_request_for_patron.illrequest_id | uri %]" >- >ILL Request #[% matching_request_for_patron.illrequest_id | html %]</a >+ <a target="_blank" href="/cgi-bin/koha/opac-illrequests.pl?op=view&illrequest_id=[% matching_request_for_patron.illrequest_id | uri %]" >+ ><i aria-hidden="true" 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.capabilities.$matching_req_status.name | html %]) on [% matching_request_for_patron.placed | $KohaDates %]</a > > > </li> > [% END %] >diff --git a/t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t b/t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t >index 0e2af3b5549..e1f7a3a2fef 100755 >--- a/t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t >+++ b/t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t >@@ -30,7 +30,7 @@ use Koha::Database; > > my $schema = Koha::Database->new->schema; > >-my $issn = '321321321'; >+my $isbn = '321321321'; > > subtest 'show_history_check' => sub { > >@@ -53,7 +53,7 @@ subtest 'show_history_check' => sub { > my $metadata = { > title => 'This is a title', > author => 'This is an author', >- issn => $issn, >+ isbn => $isbn, > cardnumber => $fake_cardnumber, > }; > >@@ -70,7 +70,7 @@ subtest 'show_history_check' => sub { > > is( > $history_check->prep_metadata($sorted), >- 'eyJhdXRob3IiOiJUaGlzIGlzIGFuIGF1dGhvciIsImNhcmRudW1iZXIiOiIxMjM0NTY3ODkiLCJp%0Ac3NuIjoiMzIxMzIxMzIxIiwidGl0bGUiOiJUaGlzIGlzIGEgdGl0bGUifQ%3D%3D%0A', >+ 'eyJhdXRob3IiOiJUaGlzIGlzIGFuIGF1dGhvciIsImNhcmRudW1iZXIiOiIxMjM0NTY3ODkiLCJp%0Ac2JuIjoiMzIxMzIxMzIxIiwidGl0bGUiOiJUaGlzIGlzIGEgdGl0bGUifQ%3D%3D%0A', > 'prep_metadata works' > ); > >@@ -89,19 +89,19 @@ subtest 'show_history_check' => sub { > 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' >+ 0, 'Request with ISBN ' . $isbn . ' does not exist even though syspref is on. Not showing history check screen' > ); > > $builder->build( > { > source => 'Illrequestattribute', >- value => { illrequest_id => $ill_request->illrequest_id, type => 'issn', value => $issn } >+ value => { illrequest_id => $ill_request->illrequest_id, type => 'isbn', value => $isbn } > } > ); > > is( > $history_check->show_history_check($ill_request), >- 1, 'Request with ISSN ' . $issn . ' exists, syspref is on and is same patron. Able to show history check screen' >+ 1, 'Request with ISBN ' . $isbn . ' exists, syspref is on and is same patron. Able to show history check screen' > ); > > # Mock ILLHistoryCheck disabled >@@ -128,7 +128,7 @@ subtest 'after_request_created' => sub { > my $metadata = { > title => 'This is a title', > author => 'This is an author', >- issn => $issn >+ isbn => $isbn > }; > > my $authenticated_patron = $builder->build_object( >@@ -139,48 +139,49 @@ subtest 'after_request_created' => sub { > > t::lib::Mocks::mock_userenv( { patron => $authenticated_patron } ); > >- # Create a new request with new issn >+ # Create a new request with new isbn > my $new_ill_request = > $builder->build_sample_ill_request( { borrowernumber => $authenticated_patron->borrowernumber } ); > my $original_staff_notes = $new_ill_request->notesstaff; >- $metadata->{issn} = 'nonexistentissn'; >+ $metadata->{isbn} = 'nonexistentisbn'; > $builder->build( > { > source => 'Illrequestattribute', >- value => { illrequest_id => $new_ill_request->illrequest_id, type => 'issn', value => $metadata->{issn} } >+ value => { illrequest_id => $new_ill_request->illrequest_id, type => 'isbn', value => $metadata->{isbn} } > } > ); > my $opac_history_check = Koha::ILL::Request::Workflow::HistoryCheck->new( $metadata, 'opac' ); > $opac_history_check->after_request_created( $metadata, $new_ill_request ); > > is( >- $new_ill_request->notesstaff, >- $original_staff_notes, >- 'History check didn\'t find any matching requests. Staff notes have not been updated.' >+ $new_ill_request->extended_attributes->find( { 'type' => 'historycheck_requests' } ), >+ undef, >+ 'History check didn\'t find any matching requests. historycheck_requests illrequestattribute has not been updated.' > ); > >- # Create a second request with preexisting issn by self patron >+ # Create a second request with preexisting isbn 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; >+ $metadata->{isbn} = $isbn; > $builder->build( > { > source => 'Illrequestattribute', >- value => { illrequest_id => $second_ill_request->illrequest_id, type => 'issn', value => $issn } >+ value => { illrequest_id => $second_ill_request->illrequest_id, type => 'isbn', value => $isbn } > } > ); > $builder->build( > { > source => 'Illrequestattribute', >- value => { illrequest_id => $third_ill_request->illrequest_id, type => 'issn', value => $issn } >+ value => { illrequest_id => $third_ill_request->illrequest_id, type => 'isbn', value => $isbn } > } > ); > $opac_history_check->after_request_created( $metadata, $third_ill_request ); > >- like( >- $third_ill_request->notesstaff, qr/Request has been submitted by this patron in the past/, >+ is( >+ $third_ill_request->extended_attributes->find( { 'type' => 'historycheck_requests' } )->value, >+ $second_ill_request->illrequest_id, > 'Contains staffnotes related submissions by self patron' > ); > >-- >2.49.0
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