The component parts order seems to be random now. It would be good to sort them so the detail view would be more informative.
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
Is there a reason why we shouldn't use a DataTable here? Is a default order by title logical?
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
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?
(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.
(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
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.
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
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
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
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
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 :-)
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.
(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).
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>
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.
I tested using the easypiano set on bug 11175 as well as Nicks script.
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.
Johanna, are you happy with Nick's patch?
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.
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.
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
(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
(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
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.
(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.
(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.
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 // [];
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
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>
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
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
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?
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
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!
Hi Nick, now we overlapped - please take a look at the longer comment above!
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
(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
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>
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>
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>
Pushed to master for 22.11. Nice work everyone, thanks!
Backported to 22.05.x for 22.05.04
(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.
(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?
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.
(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.
(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
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>