Bug 30327 - Sort component parts
Summary: Sort component parts
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 11175
Blocks: 30879
  Show dependency treegraph
 
Reported: 2022-03-22 08:12 UTC by Johanna Räisä
Modified: 2022-10-12 13:38 UTC (History)
8 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00. 22.05.04


Attachments
Bug 30327: Sort component parts (1.13 KB, patch)
2022-03-22 09:24 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 30327: [Alternate] Add options for sorting components (16.27 KB, patch)
2022-06-01 14:47 UTC, Nick Clemens
Details | Diff | Splinter Review
Simple script to add all records as components to biblio 1 (1004 bytes, application/x-perl)
2022-06-01 14:49 UTC, Nick Clemens
Details
Bug 30327: [Alternate] Add options for sorting components (16.33 KB, patch)
2022-06-06 13:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30327: Fix tests (2.36 KB, patch)
2022-06-17 11:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30327: Fix tests (2.36 KB, patch)
2022-06-23 11:10 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30327: Add options for sorting components (16.32 KB, patch)
2022-06-23 11:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30327: Fix tests (2.36 KB, patch)
2022-06-23 11:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30327: Supply defaults if component sort fields not set (1.22 KB, patch)
2022-06-25 11:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30327: (follow-up) Fix inconsistencies in syspref names and suply defaults (4.24 KB, patch)
2022-06-25 11:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30327: Add options for sorting components (16.38 KB, patch)
2022-06-25 11:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30327: Fix tests (2.41 KB, patch)
2022-06-25 11:53 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30327: (follow-up) Fix inconsistencies in syspref names and supply defaults (4.30 KB, patch)
2022-06-25 11:53 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30327: [21.11.x] Add options for sorting components (17.86 KB, patch)
2022-10-12 13:38 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Johanna Räisä 2022-03-22 08:12:59 UTC
The component parts order seems to be random now. It would be good to sort them so the detail view would be more informative.
Comment 1 Johanna Räisä 2022-03-22 09:24:14 UTC
Created attachment 131998 [details] [review]
Bug 30327: Sort component parts

This patch adds ascending sorting by biblionumber to component parts.

Test plan:
1) Enable ShowComponentRecords on systempreference
2) Go to detail page of a record which has component parts
3) See that the component parts are in random order
4) Apply the patch
5) Reload the detail page
6) Now the component parts should be in ascending biblionumber order

Sponsored-by: Koha-Suomi Oy
Comment 2 Owen Leonard 2022-05-09 14:46:25 UTC
Is there a reason why we shouldn't use a DataTable here? Is a default order by title logical?
Comment 3 Martin Renvoize 2022-05-09 16:25:23 UTC
I wasn't really sure about this patch, whilst it makes the return sort consistently in biblionumber order I'm not sure if that order is actually correct.

We're relying on the underlying search engine for the results themselves and that will give us the order the indexer picks...

I have no idea if we can set the ordering to a marc field in the child records, but that would somehow feel preferable?

Adding Nick and David for discussion
Comment 4 Martin Renvoize 2022-05-09 16:41:30 UTC
So it looks to me like you can use search_compat instead of simple_search_compat and that allows passing more options.. including a sort_by index name (see C4::Search::GetRecords for the underlying list)...  though it appears the ES implementation of search_compat doesn't support such a sort_by as yet :(.

Must admit.. I'm not fully up to speed on how simple_search_compat and search_compat compare.

Do we have any idea on what index/field component results should actually be sorted on?
Comment 5 Johanna Räisä 2022-05-10 08:52:09 UTC
(In reply to Owen Leonard from comment #2)
> Is there a reason why we shouldn't use a DataTable here? Is a default order
> by title logical?

Usually the cataloguers want the order to be same as in the album or sheet. Ordering by the biblionumbers is the easiest way to make it happen as those are mostly catalogued in order.
Comment 6 Nick Clemens 2022-05-10 11:38:42 UTC
(In reply to Martin Renvoize from comment #4)
> So it looks to me like you can use search_compat instead of
> simple_search_compat and that allows passing more options.. including a
> sort_by index name (see C4::Search::GetRecords for the underlying list)... 
> though it appears the ES implementation of search_compat doesn't support
> such a sort_by as yet :(.
> 
> Must admit.. I'm not fully up to speed on how simple_search_compat and
> search_compat compare.
> 
> Do we have any idea on what index/field component results should actually be
> sorted on?

So the sort_by options are expected to be part of the query by the time we hit the search_compat routines - they should have been passed to and handled by the 'build_query_compat' routine - though we don't call that in 'get_components_query' - but I suspect we should add that call there and handle the sort correctly
Comment 7 Michal Urban 2022-06-01 01:24:46 UTC
Testing procedure:
- Enabled ShowComponentRecords
- Added 3 child records to record "E Street"
- There were varying biblionumbers for the child records, in the details of "E Street" they were not displayed in ascending biblionumber order, instead in the order they were attached.
- Applied patch and refreshed page
- Child record components displayed by ascending biblionumber order.
Comment 8 David Nind 2022-06-01 01:43:37 UTC
From IRC - oleonard, there is a component parts sample record attached to bug 11175 http://irc.koha-community.org/koha/2022-06-01#i_2427405

If you are using koha-testing-docker to sign off, these are the steps I normally use (once .env file for koha-testing-docker is setup with Bugzillar information) - this is so that your sign-off is added to the patch, and appears on the dashboard:

1. git bz apply bug-number
2. Test and make sure things work as per the test plan.
3. git so 1
4. git bz attach -e bug-number HEAD~1..
5. Remove the # in front of the signoff line (note that there is text added after the test plan, for example: "Signed-off-by: David Nind <david@davidnind.com>"), the :wq

Notes: 
- If more than one patch, change: 
  . git so 1 to the number of patches, for example: git so 2
  . HEAD~1.. to the number of patches, for example:  HEAD~2..
- see my cheat sheet https://gitlab.com/-/snippets/1893788
Comment 9 Michal Urban 2022-06-01 14:28:24 UTC
Okay, I'll use this procedure from now on, thanks for the help

(In reply to David Nind from comment #8)
> From IRC - oleonard, there is a component parts sample record attached to
> bug 11175 http://irc.koha-community.org/koha/2022-06-01#i_2427405
> 
> If you are using koha-testing-docker to sign off, these are the steps I
> normally use (once .env file for koha-testing-docker is setup with Bugzillar
> information) - this is so that your sign-off is added to the patch, and
> appears on the dashboard:
> 
> 1. git bz apply bug-number
> 2. Test and make sure things work as per the test plan.
> 3. git so 1
> 4. git bz attach -e bug-number HEAD~1..
> 5. Remove the # in front of the signoff line (note that there is text added
> after the test plan, for example: "Signed-off-by: David Nind
> <david@davidnind.com>"), the :wq
> 
> Notes: 
> - If more than one patch, change: 
>   . git so 1 to the number of patches, for example: git so 2
>   . HEAD~1.. to the number of patches, for example:  HEAD~2..
> - see my cheat sheet https://gitlab.com/-/snippets/1893788
Comment 10 Nick Clemens 2022-06-01 14:47:28 UTC
Created attachment 135550 [details] [review]
Bug 30327: [Alternate] 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
Comment 11 Nick Clemens 2022-06-01 14:49:28 UTC
Created attachment 135551 [details]
Simple script to add all records as components to biblio 1

Don't use this without backing up your db, it indiscriminately updates all biblios to point to biblionumber 1

Record 72 and 369 in sample database will throw errors
Remove duplicate 010 field in 72
Edit and save 369 to remove bad characters
Comment 12 Nick Clemens 2022-06-01 14:54:06 UTC
I provided an alternate - there was already discussion on how we should sort, I think we should make it configurable

I copied the defaultSearchOrder options, and removed relevancy, for components they should all be relevant :-)
Comment 13 David Nind 2022-06-05 23:58:55 UTC
I tested the alternate patch, but haven't signed it off. I like the idea that the library can configure depending on how they do things/want to do things.

After applying the patch the default sort order seems to be biblionumber, rather than title (did flush_memcached, restart_all, clear browser cache). System preference was showing as title and ascending. (I increased the MaxComponentRecords to 1,000 so that all sample records are shown.

If I change the preference to title, from A to Z it seems to work okay (chinese and other titles with different character encoding appear first, so I'm assuming there are some rules for how this works behind the scenes - once the "english" titles start, they appear in order).

I had mixed results trying the different combinations of sort orders, probably more to do with the sample record data and having 435 records listed:
- date added: it was almost impossible to check given most being 09/04/2014 - changing some seemed to sort in order.
- author: seems OK for later items when clear last name, for the records listed first seems pretty random - particularly where multople authors, editors, no author - I'm assuming there are some rules about how this works or will use the first author.
- call number: seems ok - added a few to some items to test.
- date of publication: based on 260$c?, seems a bit random sometimes - for example, dates with <1973-c1980>, sorting descending - Economics (record 434), with a 2014 publication date appears at the end of the list
- title: seems okay.

I'm assuming for sort order A-Z is for titles or for a mix of numbers and letters (like call numbers), and ascending and descending when dates.
Comment 14 David Cook 2022-06-06 00:19:38 UTC
(In reply to Martin Renvoize from comment #3)
> I wasn't really sure about this patch, whilst it makes the return sort
> consistently in biblionumber order I'm not sure if that order is actually
> correct.
> 
> We're relying on the underlying search engine for the results themselves and
> that will give us the order the indexer picks...
> 
> I have no idea if we can set the ordering to a marc field in the child
> records, but that would somehow feel preferable?
> 
> Adding Nick and David for discussion

I commented on Bug 30879 but regarding the default sort order for Zebra... it uses the internal Zebra ID in ascending order (and the internal Zebra ID is provided by 999$c).
Comment 15 Martin Renvoize 2022-06-06 13:55:53 UTC
Created attachment 135730 [details] [review]
Bug 30327: [Alternate] 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>
Comment 16 Martin Renvoize 2022-06-06 13:57:45 UTC
This alternative works great and allows for various options in the Marc.  Signing off.

If my understanding is correct we should perhaps be sorting on 773$g by default.. but I'm told that field is often not populated or rather open to interpretation as to what it should contain.  A follow-up bug could help to populate that field and add it as a sort option to the existing options added here.
Comment 17 Martin Renvoize 2022-06-06 13:59:13 UTC
I tested using the easypiano set on bug 11175 as well as Nicks script.
Comment 18 Jonathan Druart 2022-06-17 07:04:17 UTC
Tests are failing for me (second patch)

    #   Failed test at t/db_dependent/Koha/Biblio.t line 547.
    # found warning: Use of uninitialized value in concatenation (.) or string at /kohadevbox/koha/Koha/Biblio.pm line 568.
    # found warning: Use of uninitialized value in concatenation (.) or string at /kohadevbox/koha/Koha/Biblio.pm line 568.
    # found warning: Use of uninitialized value $limit in concatenation (.) or string at /kohadevbox/koha/C4/Search.pm line 1331.
    # found warning: Warning from search_compat: 'Exception 'Koha::Exception' thrown 'error searching analytics'
    # expected to find warning: (?^u:Warning from search_compat: .* 'error searching analytics')
    # Looks like you failed 1 test of 5.
Comment 19 Jonathan Druart 2022-06-17 07:05:09 UTC
Johanna, are you happy with Nick's patch?
Comment 20 Johanna Räisä 2022-06-17 07:11:37 UTC
I will test it today(In reply to Jonathan Druart from comment #19)
> Johanna, are you happy with Nick's patch?

I will test it today. I like that it is configurable but didn't check yet if there is an option to get it in biblionumber order which is the best option for us.
Comment 21 Johanna Räisä 2022-06-17 09:31:39 UTC
I tested this and there are some problems. Component parts don't have items so sorting with item related fields is quite useless. The items are in the host record and component parts are additional information about the host record.

Date of publication is problematic also since in most cases component parts have the same value. Author and title are quite messy also. The author field usually has the same value or is added only to the host record. The component parts are wanted to be always in the same order and same order the album or note has it.

As Martin mentioned the sorting order could be in 773$g but it is rarely used. So if there would be an option to sort by biblionumber it would be the most "correct" way to get what catalogers want.
Comment 22 Nick Clemens 2022-06-17 11:34:06 UTC
Created attachment 136256 [details] [review]
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
Comment 23 Nick Clemens 2022-06-17 11:34:59 UTC
(In reply to Jonathan Druart from comment #18)
> Tests are failing for me (second patch)
> 
Fixed, mismatch in update pref name and an undef value causing extra warnings
Comment 24 Nick Clemens 2022-06-17 11:38:43 UTC
(In reply to Johanna Räisä from comment #21)
> I tested this and there are some problems. Component parts don't have items
> so sorting with item related fields is quite useless. The items are in the
> host record and component parts are additional information about the host
> record.
> 
> Date of publication is problematic also since in most cases component parts
> have the same value. Author and title are quite messy also. The author field
> usually has the same value or is added only to the host record. The
> component parts are wanted to be always in the same order and same order the
> album or note has it.
> 
> As Martin mentioned the sorting order could be in 773$g but it is rarely
> used. So if there would be an option to sort by biblionumber it would be the
> most "correct" way to get what catalogers want.

Hi Johanna,

We often see analytics for 'bound' volumes of serials, which can have differing copy dates. We also have libraries with individual articles in serials cataloged, so the title and author can vary and these values are how they have requested sorting.

Biblionumber is a great addition, I simply cannot make it work in zebra other than the default ascending search.

I do think we want to handle the sorting in the search engine, rather than our code, so I have moved the addition of biblionumber sorting to bug 30879. I think we can move this one forward, and then continue the work there?

-Nick
Comment 25 Katrin Fischer 2022-06-17 13:28:04 UTC
I think we can only push this if the biblionumber sorting still works or we would break the existing behaviour. 

Could "relevance" sorting not work here? As you noted in an earlier comment, the relevance in this case is always the same, the fallback for Zebra at least, is sorting by biblionumber for same relevance.
Comment 26 Katrin Fischer 2022-06-17 13:29:00 UTC
(In reply to Katrin Fischer from comment #25)
> I think we can only push this if the biblionumber sorting still works or we
> would break the existing behaviour. 

Ok, was too fast - I realized later it was random now.

> Could "relevance" sorting not work here? As you noted in an earlier comment,
> the relevance in this case is always the same, the fallback for Zebra at
> least, is sorting by biblionumber for same relevance.
Comment 27 Johanna Räisä 2022-06-20 07:08:22 UTC
(In reply to Nick Clemens from comment #24)
> (In reply to Johanna Räisä from comment #21)
> > I tested this and there are some problems. Component parts don't have items
> > so sorting with item related fields is quite useless. The items are in the
> > host record and component parts are additional information about the host
> > record.
> > 
> > Date of publication is problematic also since in most cases component parts
> > have the same value. Author and title are quite messy also. The author field
> > usually has the same value or is added only to the host record. The
> > component parts are wanted to be always in the same order and same order the
> > album or note has it.
> > 
> > As Martin mentioned the sorting order could be in 773$g but it is rarely
> > used. So if there would be an option to sort by biblionumber it would be the
> > most "correct" way to get what catalogers want.
> 
> Hi Johanna,
> 
> We often see analytics for 'bound' volumes of serials, which can have
> differing copy dates. We also have libraries with individual articles in
> serials cataloged, so the title and author can vary and these values are how
> they have requested sorting.
> 
> Biblionumber is a great addition, I simply cannot make it work in zebra
> other than the default ascending search.
> 
> I do think we want to handle the sorting in the search engine, rather than
> our code, so I have moved the addition of biblionumber sorting to bug 30879.
> I think we can move this one forward, and then continue the work there?
> 
> -Nick

The new bug is fine for me.
Comment 28 David Cook 2022-06-23 01:20:51 UTC
Patch doesn't currently apply:

diff --cc Koha/Biblio.pm
index 4a4df845eb,a84b0e32f6..0000000000
--- a/Koha/Biblio.pm
+++ b/Koha/Biblio.pm
@@@ -547,7 -547,7 +547,11 @@@ sub get_marc_components
                  }
              );
          }
++<<<<<<< HEAD
 +        @$components = sort { $searcher->extract_biblionumber($a) <=> $searcher->extract_biblionumber($b) } @$results if defined($results) && @$results;
++=======
+         $components = $results->{biblioserver}->{RECORDS} if defined($results) && $results->{biblioserver}->{hits};
++>>>>>>> Bug 30327: [Alternate] Add options for sorting components
      }

      return $components // [];
Comment 29 Nick Clemens 2022-06-23 11:10:32 UTC
Created attachment 136446 [details] [review]
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
Comment 30 Nick Clemens 2022-06-23 11:11:10 UTC
Created attachment 136447 [details] [review]
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>
Comment 31 Nick Clemens 2022-06-23 11:11:14 UTC
Created attachment 136448 [details] [review]
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
Comment 32 Katrin Fischer 2022-06-25 11:09:52 UTC
I have an issue with one of the tests, but I think it just requires a small fix:

    #   Failed test at t/db_dependent/Koha/Biblio.t line 547.
    # found warning: Use of uninitialized value in concatenation (.) or string at /kohadevbox/koha/Koha/Biblio.pm line 568.
    # found warning: Use of uninitialized value in concatenation (.) or string at /kohadevbox/koha/Koha/Biblio.pm line 568.
    # found warning: Warning from search_compat: 'Exception 'Koha::Exception' thrown 'error searching analytics'
    # expected to find warning: (?^u:Warning from search_compat: .* 'error searching analytics')
    ok 5
    # Looks like you failed 1 test of 5.
not ok 11 - get_marc_components() tests
Comment 33 Katrin Fischer 2022-06-25 11:16:01 UTC
Why do we need ascending, descending, A-Z and Z-A as options? Is there a functional difference between those and are there any 'impossible' combinations one should not select?
Comment 34 Nick Clemens 2022-06-25 11:20:13 UTC
Created attachment 136540 [details] [review]
Bug 30327: Supply defaults if component sort fields not set

This patch fetches the new sysprefs into variables, providing default title ascending if
they are not set to avoid an undefined concatenation warning
Comment 35 Katrin Fischer 2022-06-25 11:33:38 UTC
Ok, time to do this properly and summarize in one comment :)

1) Unit tests are failing for me, although why is not obvious to me:

    #   Failed test at t/db_dependent/Koha/Biblio.t line 547.
    # found warning: Use of uninitialized value in concatenation (.) or string at /kohadevbox/koha/Koha/Biblio.pm line 568.
    # found warning: Use of uninitialized value in concatenation (.) or string at /kohadevbox/koha/Koha/Biblio.pm line 568.
    # found warning: Warning from search_compat: 'Exception 'Koha::Exception' thrown 'error searching analytics'
    # expected to find warning: (?^u:Warning from search_compat: .* 'error searching analytics')
    ok 5
    # Looks like you failed 1 test of 5.
not ok 11 - get_marc_components() tests


2) Database update is not idempotent, missing the IGNORE (it will explode if you run it a second time):

+            INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `type` ) VALUES


3) I feel like the sorting options in combination with the sort criterias are not totally clear. 
Why do we need ascending, descending, A-Z and Z-A as separate options? Is there a functional difference between those and are there any 'impossible' combinations one should not select?


4) There is a mismatch that needs to be fixed (additional s):
.yml: ComponentSortField
syspref.sql: ComponentsSortField
update: both versions are mixed in update and description.



Please fix, this works otherwise nicely and we are very close!
Comment 36 Katrin Fischer 2022-06-25 11:34:14 UTC
Hi Nick, now we overlapped - please take a look at the longer comment above!
Comment 37 Nick Clemens 2022-06-25 11:43:10 UTC
Created attachment 136541 [details] [review]
Bug 30327: (follow-up) Fix inconsistencies in syspref names and suply 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
Comment 38 Nick Clemens 2022-06-25 11:44:33 UTC
(In reply to Katrin Fischer from comment #35)

> 3) I feel like the sorting options in combination with the sort criterias
> are not totally clear. 
> Why do we need ascending, descending, A-Z and Z-A as separate options? Is
> there a functional difference between those and are there any 'impossible'
> combinations one should not select?
> 
This copies the 'DefaultSortOrder' pref - I agree we should clarify/fix this, but lets fix all the prefs on another report
Comment 39 Katrin Fischer 2022-06-25 11:52:57 UTC
Created attachment 136542 [details] [review]
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>
Comment 40 Katrin Fischer 2022-06-25 11:53:02 UTC
Created attachment 136543 [details] [review]
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 <katrin.fischer.83@web.de>
Comment 41 Katrin Fischer 2022-06-25 11:53:07 UTC
Created attachment 136544 [details] [review]
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 <katrin.fischer.83@web.de>
Comment 42 Tomás Cohen Arazi 2022-06-25 18:31:27 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 43 Lucas Gass 2022-07-29 16:39:17 UTC
Backported to 22.05.x for 22.05.04
Comment 44 Katrin Fischer 2022-07-30 12:50:47 UTC
(In reply to Lucas Gass from comment #43)
> Backported to 22.05.x for 22.05.04

I am not sure about this one, if the sorting currently was by biblionumber that might have been the expected sorting - and it won't work now as long as we don't have bug 30879 in.
Comment 45 Lucas Gass 2022-08-01 13:44:27 UTC
(In reply to Katrin Fischer from comment #44)
> (In reply to Lucas Gass from comment #43)
> > Backported to 22.05.x for 22.05.04
> 
> I am not sure about this one, if the sorting currently was by biblionumber
> that might have been the expected sorting - and it won't work now as long as
> we don't have bug 30879 in.

Cait,

Do you think I should revert this patchset?
Comment 46 Katrin Fischer 2022-08-02 09:14:41 UTC
I am not sure really, it might be best to get feedback from Johanna and others using this feature. The follow-up with bug 30879 requires re-indexing, which makes it unsuitable for backporting.
Comment 47 Johanna Räisä 2022-08-02 09:54:32 UTC
(In reply to Katrin Fischer from comment #46)
> I am not sure really, it might be best to get feedback from Johanna and
> others using this feature. The follow-up with bug 30879 requires
> re-indexing, which makes it unsuitable for backporting.

We use 21.11 but I added my first patch on top of it so we are fine for now. Updating for us is not a problem since we bring the features to our version separately.
Comment 48 Arthur Suzuki 2022-08-11 12:56:03 UTC
(In reply to Johanna Räisä from comment #47)
> (In reply to Katrin Fischer from comment #46)
> > I am not sure really, it might be best to get feedback from Johanna and
> > others using this feature. The follow-up with bug 30879 requires
> > re-indexing, which makes it unsuitable for backporting.
> 
> We use 21.11 but I added my first patch on top of it so we are fine for now.
> Updating for us is not a problem since we bring the features to our version
> separately.

Hi Johanna,
I have some conflicts when trying to apply to the 21.11.x community branch.
Please provide patches for 21.11.x if needed. Would be nice to share the patch you made to solve this issue on 21.11.
Arthur
Comment 49 Nick Clemens 2022-10-12 13:38:45 UTC
Created attachment 141731 [details] [review]
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 <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

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 <katrin.fischer.83@web.de>

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 <katrin.fischer.83@web.de>