From cc0a68625dc139e458284cb569113683a38874a3 Mon Sep 17 00:00:00 2001 From: Pedro Amorim 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 --- 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 - '' - . __("ILL Request #") - . $request->illrequest_id . ''; + $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: text, av_category_code and bypass