From bd4cbf0f7c24e1a492aa92596c353cff2c5010a0 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Wed, 1 Jun 2022 14:40:49 +0000 Subject: [PATCH] Bug 30327: [21.11.x] Add options for sorting components This patch adds two new sysprefs: ComponentSortField ComponentSortOrder These allow the user to choose how components should be sorted when displaying on the details page of a record, and the corresponding search for all components This also updates our search from simple_search_compat to search_compat to allow for sorting options Note: Some sorting under ES is unclear - this is a separate issue to be invesitgated Our Zebra index does not offer 'record number' sorting, I will file a bug for that To test: 1 - Enable UseControlNumber (or not) 2 - Add some components to a record by control number or title depending on above 3 - Enable ShowComponentRecords syspref 4 - View the record that has components 5 - Note they are not sorted 6 - Apply patch, updatedatabase 7 - reload record 8 - Note components are sorted by title ascending 9 - Try different values for ComponentSortField and ComponentSortOrder 10 - Confirm sorting changes with system preferences 11 - Repeat test on staff and opac, with ES and Zebra search engines Signed-off-by: Martin Renvoize Signed-off-by: Katrin Fischer Bug 30327: Fix tests Corrected variable name on update to match everywhere else Added a default value for limit in buildQuery and only append limit if it has content Signed-off-by: Katrin Fischer Bug 30327: (follow-up) Fix inconsistencies in syspref names and supply defaults This patch fetches the new sysprefs into variables, providing default title ascending if they are not set to avoid an undefined concatenation warning I also make the update idempotent and fix confusion of plural/singular names Signed-off-by: Katrin Fischer --- C4/Search.pm | 4 +- Koha/Biblio.pm | 24 ++++++++---- catalogue/detail.pl | 4 +- .../bug_30327_add_sortComponents_syspref.pl | 16 ++++++++ installer/data/mysql/mandatory/sysprefs.sql | 2 + .../admin/preferences/cataloguing.pref | 16 ++++++++ .../prog/en/modules/catalogue/detail.tt | 2 +- .../bootstrap/en/modules/opac-detail.tt | 2 +- opac/opac-detail.pl | 4 +- t/db_dependent/Koha/Biblio.t | 37 ++++++++++++------- 10 files changed, 84 insertions(+), 27 deletions(-) create mode 100755 installer/data/mysql/atomicupdate/bug_30327_add_sortComponents_syspref.pl diff --git a/C4/Search.pm b/C4/Search.pm index d805a50450..c7608f5715 100644 --- a/C4/Search.pm +++ b/C4/Search.pm @@ -1216,7 +1216,7 @@ sub buildQuery { my $query_cgi; my $query_type; - my $limit; + my $limit = q{}; my $limit_cgi; my $limit_desc; @@ -1327,7 +1327,7 @@ sub buildQuery { # This is needed otherwise ccl= and &limit won't work together, and # this happens when selecting a subject on the opac-detail page my $original_q = $q; # without available part - $q .= $limit; + $q .= $limit if $limit; return ( undef, $q, $q, "q=ccl=".uri_escape_utf8($q), $original_q, '', '', '', 'ccl' ); } if ( $query =~ /^cql=/ ) { diff --git a/Koha/Biblio.pm b/Koha/Biblio.pm index deb3c1812d..17f7cdcae0 100644 --- a/Koha/Biblio.pm +++ b/Koha/Biblio.pm @@ -515,8 +515,8 @@ sub suggestions { my $components = $self->get_marc_components(); -Returns an array of MARCXML data, which are component parts of -this object (MARC21 773$w points to this) +Returns an array of search results data, which are component parts of +this object (MARC21 773 points to this) =cut @@ -525,19 +525,19 @@ sub get_marc_components { return [] if (C4::Context->preference('marcflavour') ne 'MARC21'); - my $searchstr = $self->get_components_query; + my ( $searchstr, $sort ) = $self->get_components_query; my $components; if (defined($searchstr)) { my $searcher = Koha::SearchEngine::Search->new({index => $Koha::SearchEngine::BIBLIOS_INDEX}); - my ( $error, $results, $total_hits ); + my ( $error, $results, $facets ); eval { - ( $error, $results, $total_hits ) = $searcher->simple_search_compat( $searchstr, 0, $max_results ); + ( $error, $results, $facets ) = $searcher->search_compat( $searchstr, undef, [$sort], ['biblioserver'], $max_results, 0, undef, undef, 'ccl', 0 ); }; if( $error || $@ ) { $error //= q{}; $error .= $@ if $@; - warn "Warning from simple_search_compat: '$error'"; + warn "Warning from search_compat: '$error'"; $self->add_message( { type => 'error', @@ -546,7 +546,7 @@ sub get_marc_components { } ); } - $components = $results if defined($results) && @$results; + $components = $results->{biblioserver}->{RECORDS} if defined($results) && $results->{biblioserver}->{hits}; } return $components // []; @@ -564,6 +564,9 @@ sub get_components_query { my $builder = Koha::SearchEngine::QueryBuilder->new( { index => $Koha::SearchEngine::BIBLIOS_INDEX } ); my $marc = $self->metadata->record; + my $component_sort_field = C4::Context->preference('ComponentSortField') // "title"; + my $component_sort_order = C4::Context->preference('ComponentSortOrder') // "asc"; + my $sort = $component_sort_field . "_" . $component_sort_order; my $searchstr; if ( C4::Context->preference('UseControlNumber') ) { @@ -596,8 +599,13 @@ sub get_components_query { $cleaned_title = $builder->clean_search_term($cleaned_title); $searchstr = qq#Host-item:("$cleaned_title")#; } + my ($error, $query_str) = $builder->build_query_compat( undef, [$searchstr], undef, undef, [$sort], 0 ); + if( $error ){ + warn $error; + return; + } - return $searchstr; + return ($query_str, $sort); } =head3 subscriptions diff --git a/catalogue/detail.pl b/catalogue/detail.pl index 0ee6da03fa..6c8550b154 100755 --- a/catalogue/detail.pl +++ b/catalogue/detail.pl @@ -216,7 +216,9 @@ if ( $showcomp eq 'both' || $showcomp eq 'staff' ) { ); } $template->param( ComponentParts => $parts ); - $template->param( ComponentPartsQuery => $biblio->get_components_query ); + my ( $comp_query, $comp_sort ) = $biblio->get_components_query; + my $cpq = $comp_query . "&sort_by=" . $comp_sort; + $template->param( ComponentPartsQuery => $cpq ); } } else { # check if we should show analytics anyway $show_analytics = 1 if $marc_record && @{$biblio->get_marc_components(1)}; # count matters here, results does not diff --git a/installer/data/mysql/atomicupdate/bug_30327_add_sortComponents_syspref.pl b/installer/data/mysql/atomicupdate/bug_30327_add_sortComponents_syspref.pl new file mode 100755 index 0000000000..cbc9416f9b --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_30327_add_sortComponents_syspref.pl @@ -0,0 +1,16 @@ +use Modern::Perl; + +return { + bug_number => "30327", + description => "Add ComponentSortField and ComponentSortOrder sysprefs", + up => sub { + my ($args) = @_; + my ($dbh, $out) = @$args{qw(dbh out)}; + $dbh->do(q{ + INSERT IGNORE INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `type` ) VALUES + ('ComponentSortField','title','call_number|pubdate|acqdate|title|author','Specify the default field used for sorting','Choice'), + ('ComponentSortOrder','asc','asc|dsc|az|za','Specify the default sort order','Choice') + }); + say $out "Added ComponentSortField and ComponentSortOrder sysprefs"; + }, +}; diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql index 2ffc076e55..3905ded291 100644 --- a/installer/data/mysql/mandatory/sysprefs.sql +++ b/installer/data/mysql/mandatory/sysprefs.sql @@ -142,6 +142,8 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('CoceProviders', '', 'aws,gb,ol', 'Coce providers', 'multiple'), ('COinSinOPACResults','1','','If ON, use COinS in OPAC search results page. NOTE: this can slow down search response time significantly','YesNo'), ('CollapseFieldsPatronAddForm','',NULL,'Collapse these fields by default when adding a new patron. These fields can still be expanded.','Multiple'), +('ComponentSortField','title','call_number|pubdate|acqdate|title|author','Specify the default field used for sorting','Choice'), +('ComponentSortOrder','asc','asc|dsc|az|za','Specify the default sort order','Choice'), ('ConfirmFutureHolds','0','','Number of days for confirming future holds','Integer'), ('ConsiderOnSiteCheckoutsAsNormalCheckouts','1',NULL,'Consider on-site checkouts as normal checkouts','YesNo'), ('CreateAVFromCataloguing', '1', '', 'Ability to create authorized values from the cataloguing module', 'YesNo'), diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref index 86840eff5a..801608607d 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref @@ -280,6 +280,22 @@ Cataloging: - pref: MaxComponentRecords - "records will be displayed." - "
UNIMARC is not supported." + - By default, sort component results in the staff interface by + - pref: ComponentSortField + default: title + choices: + call_number: call number + pubdate: date of publication + acqdate: date added + title: title + author: author + - ',' + - pref: ComponentSortOrder + choices: + asc: ascending. + dsc: descending. + az: from A to Z. + za: from Z to A. Importing: - - When matching on ISBN with the record import tool, diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt index 42bd6bd025..2a003b2c3a 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt @@ -680,7 +680,7 @@ Note that permanent location is a code, and location may be an authval. [% END %] [% IF ComponentParts.size == Koha.Preference('MaxComponentRecords')%] -

Only [% ComponentParts.size | html %] results are shown: show all component parts

+

Only [% ComponentParts.size | html %] results are shown: show all component parts

[% END %] diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt index 687f7fa8c3..9c153bee60 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt +++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt @@ -600,7 +600,7 @@ [% END %] [% IF ComponentParts.size == Koha.Preference('MaxComponentRecords')%] -

Only [% ComponentParts.size | html %] results are shown: show all component parts

+

Only [% ComponentParts.size | html %] results are shown: show all component parts

[% END %] diff --git a/opac/opac-detail.pl b/opac/opac-detail.pl index 2eb7ceaa92..6a55283ee7 100755 --- a/opac/opac-detail.pl +++ b/opac/opac-detail.pl @@ -650,7 +650,9 @@ if ( $showcomp eq 'both' || $showcomp eq 'opac' ) { ); } $template->param( ComponentParts => $parts ); - $template->param( ComponentPartsQuery => $biblio->get_components_query ); + my ( $comp_query, $comp_sort ) = $biblio->get_components_query; + my $cpq = $comp_query . "&sort_by=" . $comp_sort; + $template->param( ComponentPartsQuery => $cpq ); } } else { # check if we should show analytics anyway $show_analytics = 1 if @{$biblio->get_marc_components(1)}; # count matters here, results does not diff --git a/t/db_dependent/Koha/Biblio.t b/t/db_dependent/Koha/Biblio.t index ef0c66e275..df692a86e4 100755 --- a/t/db_dependent/Koha/Biblio.t +++ b/t/db_dependent/Koha/Biblio.t @@ -521,7 +521,7 @@ subtest 'get_marc_components() tests' => sub { my $host_biblio = Koha::Biblios->find($host_bibnum); t::lib::Mocks::mock_preference( 'SearchEngine', 'Zebra' ); my $search_mod = Test::MockModule->new( 'Koha::SearchEngine::Zebra::Search' ); - $search_mod->mock( 'simple_search_compat', \&search_component_record2 ); + $search_mod->mock( 'search_compat', \&search_component_record2 ); my $components = $host_biblio->get_marc_components; is( ref($components), 'ARRAY', 'Return type is correct' ); @@ -532,8 +532,8 @@ subtest 'get_marc_components() tests' => sub { '->get_marc_components returns an empty ARRAY' ); - $search_mod->unmock( 'simple_search_compat'); - $search_mod->mock( 'simple_search_compat', \&search_component_record1 ); + $search_mod->unmock( 'search_compat'); + $search_mod->mock( 'search_compat', \&search_component_record1 ); my $component_record = component_record1()->as_xml(); is_deeply( @@ -541,13 +541,13 @@ subtest 'get_marc_components() tests' => sub { [$component_record], '->get_marc_components returns the related component part record' ); - $search_mod->unmock( 'simple_search_compat'); + $search_mod->unmock( 'search_compat'); - $search_mod->mock( 'simple_search_compat', + $search_mod->mock( 'search_compat', sub { Koha::Exceptions::Exception->throw("error searching analytics") } ); warning_like { $components = $host_biblio->get_marc_components } - qr{^Warning from simple_search_compat: 'error searching analytics'}; + qr{^Warning from search_compat: 'error searching analytics'}; is_deeply( $host_biblio->object_messages, @@ -559,35 +559,46 @@ subtest 'get_marc_components() tests' => sub { } ] ); - $search_mod->unmock( 'simple_search_compat'); + $search_mod->unmock( 'search_compat'); $schema->storage->txn_rollback; }; subtest 'get_components_query' => sub { - plan tests => 3; + plan tests => 6; my $biblio = $builder->build_sample_biblio(); my $biblionumber = $biblio->biblionumber; my $record = $biblio->metadata->record; t::lib::Mocks::mock_preference( 'UseControlNumber', '0' ); - is($biblio->get_components_query, 'Host-item:("Some boring read")', "UseControlNumber disabled"); + t::lib::Mocks::mock_preference( 'ComponentSortField', 'author' ); + t::lib::Mocks::mock_preference( 'ComponentSortOrder', 'za' ); + my ( $comp_q, $comp_s ) = $biblio->get_components_query; + is($comp_q, 'Host-item:("Some boring read")', "UseControlNumber disabled"); + is($comp_s, "author_za", "UseControlNumber disabled sort is correct"); t::lib::Mocks::mock_preference( 'UseControlNumber', '1' ); + t::lib::Mocks::mock_preference( 'ComponentSortOrder', 'az' ); my $marc_001_field = MARC::Field->new('001', $biblionumber); $record->append_fields($marc_001_field); C4::Biblio::ModBiblio( $record, $biblio->biblionumber ); $biblio = Koha::Biblios->find( $biblio->biblionumber); - is($biblio->get_components_query, "(rcn:$biblionumber AND (bib-level:a OR bib-level:b))", "UseControlNumber enabled without MarcOrgCode"); + ( $comp_q, $comp_s ) = $biblio->get_components_query; + is($comp_q, "(rcn:$biblionumber AND (bib-level:a OR bib-level:b))", "UseControlNumber enabled without MarcOrgCode"); + is($comp_s, "author_az", "UseControlNumber enabled without MarcOrgCode sort is correct"); my $marc_003_field = MARC::Field->new('003', 'OSt'); $record->append_fields($marc_003_field); C4::Biblio::ModBiblio( $record, $biblio->biblionumber ); $biblio = Koha::Biblios->find( $biblio->biblionumber); - is($biblio->get_components_query, "(((rcn:$biblionumber AND cni:OSt) OR rcn:\"OSt $biblionumber\") AND (bib-level:a OR bib-level:b))", "UseControlNumber enabled with MarcOrgCode"); + t::lib::Mocks::mock_preference( 'ComponentSortField', 'title' ); + t::lib::Mocks::mock_preference( 'ComponentSortOrder', 'asc' ); + ( $comp_q, $comp_s ) = $biblio->get_components_query; + is($comp_q, "(((rcn:$biblionumber AND cni:OSt) OR rcn:\"OSt $biblionumber\") AND (bib-level:a OR bib-level:b))", "UseControlNumber enabled with MarcOrgCode"); + is($comp_s, "title_asc", "UseControlNumber enabled with MarcOrgCode sort if correct"); }; subtest 'orders() and active_orders() tests' => sub { @@ -883,12 +894,12 @@ sub component_record1 { } sub search_component_record1 { my @results = ( component_record1()->as_xml() ); - return ( undef, \@results, 1 ); + return ( undef, { biblioserver => { RECORDS => \@results, hits => 1 } }, 1 ); } sub search_component_record2 { my @results; - return ( undef, \@results, 0 ); + return ( undef, { biblioserver => { RECORDS => \@results, hits => 0 } }, 0 ); } sub host_record { -- 2.30.2