From da26e8654b20a4d831c5d82c0d27126c621c333e Mon Sep 17 00:00:00 2001
From: Nick Clemens <nick@bywatersolutions.com>
Date: Wed, 1 Jun 2022 14:40:49 +0000
Subject: [PATCH] Bug 30327: 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 <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
---
 Koha/Biblio.pm                                | 22 +++++++----
 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 ++++++++++++-------
 9 files changed, 80 insertions(+), 25 deletions(-)
 create mode 100755 installer/data/mysql/atomicupdate/bug_30327_add_sortComponents_syspref.pl

diff --git a/Koha/Biblio.pm b/Koha/Biblio.pm
index b7431f5c26..52cc36f2d6 100644
--- a/Koha/Biblio.pm
+++ b/Koha/Biblio.pm
@@ -516,8 +516,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
 
@@ -526,19 +526,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',
@@ -547,7 +547,7 @@ sub get_marc_components {
                 }
             );
         }
-        $components = $results if defined($results) && @$results;
+        $components = $results->{biblioserver}->{RECORDS} if defined($results) && $results->{biblioserver}->{hits};
     }
 
     return $components // [];
@@ -565,6 +565,7 @@ sub get_components_query {
     my $builder = Koha::SearchEngine::QueryBuilder->new(
         { index => $Koha::SearchEngine::BIBLIOS_INDEX } );
     my $marc = $self->metadata->record;
+    my $sort = C4::Context->preference('ComponentSortField') . "_" . C4::Context->preference('ComponentSortOrder');
 
     my $searchstr;
     if ( C4::Context->preference('UseControlNumber') ) {
@@ -597,8 +598,13 @@ sub get_components_query {
         $cleaned_title = $builder->clean_search_term($cleaned_title);
         $searchstr = "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 990d108cf3..459fe6433f 100755
--- a/catalogue/detail.pl
+++ b/catalogue/detail.pl
@@ -219,7 +219,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..0cb8f16dac
--- /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 ComponentsSortField and ComponentsSortOrder sysprefs",
+    up => sub {
+        my ($args) = @_;
+        my ($dbh, $out) = @$args{qw(dbh out)};
+        $dbh->do(q{
+            INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `type` ) VALUES
+            ('ComponentsSortField','title','call_number|pubdate|acqdate|title|author','Specify the default field used for sorting','Choice'),
+            ('ComponentsSortOrder','asc','asc|dsc|az|za','Specify the default sort order','Choice')
+        });
+        say $out "Added ComponentsSortField and ComponentsSortOrder sysprefs";
+    },
+};
diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql
index d2c7ee3bec..0ea470bcd3 100644
--- a/installer/data/mysql/mandatory/sysprefs.sql
+++ b/installer/data/mysql/mandatory/sysprefs.sql
@@ -144,6 +144,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'),
+('ComponentsSortField','title','call_number|pubdate|acqdate|title|author','Specify the default field used for sorting','Choice'),
+('ComponentsSortOrder','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 41399d7c97..6f66761c37 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."
             - "<br/> 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 c24c861c03..1fae17084f 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
@@ -707,7 +707,7 @@ Note that permanent location is a code, and location may be an authval.
             [% END %]
         </table>
         [% IF ComponentParts.size == Koha.Preference('MaxComponentRecords')%]
-        <p>Only [% ComponentParts.size | html %] results are shown: <a href="/cgi-bin/koha/catalogue/search.pl?q=[% ComponentPartsQuery | uri %]"/>show all component parts</a></p>
+        <p>Only [% ComponentParts.size | html %] results are shown: <a href="/cgi-bin/koha/catalogue/search.pl?q=[% ComponentPartsQuery | url %]"/>show all component parts</a></p>
         [% END %]
     </div> <!-- /.content_set -->
 </div> <!-- /#components -->
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 7f49236e42..f6756d6a59 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 %]
                             </table>
                             [% IF ComponentParts.size == Koha.Preference('MaxComponentRecords')%]
-                            <p>Only [% ComponentParts.size | html %] results are shown: <a href="/cgi-bin/koha/opac-search.pl?q=[% ComponentPartsQuery | uri %]"/>show all component parts</a></p>
+                            <p>Only [% ComponentParts.size | html %] results are shown: <a href="/cgi-bin/koha/opac-search.pl?q=[% ComponentPartsQuery | url %]"/>show all component parts</a></p>
                             [% END %]
                         </div>
                     </div>
diff --git a/opac/opac-detail.pl b/opac/opac-detail.pl
index 000eac3a51..7219eef18e 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 fc8445545d..890d4e9b90 100755
--- a/t/db_dependent/Koha/Biblio.t
+++ b/t/db_dependent/Koha/Biblio.t
@@ -518,7 +518,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' );
@@ -529,8 +529,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(
@@ -538,13 +538,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::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,
@@ -556,35 +556,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 {
@@ -1014,12 +1025,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