Bug 30879 - Add option to sort components by biblionumber
Summary: Add option to sort components by biblionumber
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Zebra (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact:
URL:
Keywords: release-notes-needed
Depends on: 30327
Blocks: 31756
  Show dependency treegraph
 
Reported: 2022-06-01 14:52 UTC by Nick Clemens
Modified: 2022-10-28 20:08 UTC (History)
5 users (show)

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


Attachments
Bug 30879: Add option to sort components by biblionumber (6.88 KB, patch)
2022-06-17 11:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30879: Add biblionumber as a sorting option in MARC21 (5.21 KB, patch)
2022-06-22 14:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30879: Allow biblionumber as sort option in Elasticsearch (2.12 KB, patch)
2022-06-22 14:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30879: Handle index/sorting for UNIMARC (3.28 KB, patch)
2022-06-22 14:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30879: Add option to syspref and update display (2.07 KB, patch)
2022-06-22 14:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30879: Add biblionumber as a sorting option in MARC21 (5.27 KB, patch)
2022-06-24 15:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30879: Allow biblionumber as sort option in Elasticsearch (2.19 KB, patch)
2022-06-24 15:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30879: Handle index/sorting for UNIMARC (3.35 KB, patch)
2022-06-24 15:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30879: Add option to syspref and update display (2.13 KB, patch)
2022-06-24 15:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30879: Add biblionumber as a sorting option in MARC21 (5.33 KB, patch)
2022-06-25 12:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30879: Allow biblionumber as sort option in Elasticsearch (2.25 KB, patch)
2022-06-25 12:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30879: Handle index/sorting for UNIMARC (3.41 KB, patch)
2022-06-25 12:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30879: Add option to syspref and update display (2.19 KB, patch)
2022-06-25 12:40 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2022-06-01 14:52:30 UTC
Zebra sorting options are hardcoded - but trying to add local-number failed 
 375             elsif ( $sort eq "local-number_asc" ) {
 376                 $sort_by .= "1=12  <i ";
 377             }
 378             elsif ( $sort eq "local-number_dsc" ) {
 379                 $sort_by .= "1=12 >i ";
 380             }

I think it is because we index both as word and number
   5   <!--record.abs line 24: melm 001        Local-number,Local-number:n-->
   6   <index_control_field tag="001">
   7     <target_index>Local-number:w</target_index>
   8   </index_control_field>
   9   <index_control_field tag="001">
  10     <target_index>Local-number:n</target_index>
  11   </index_control_field>

I am not sure how to specify sorting by number index, and not word
Comment 1 David Cook 2022-06-01 23:31:39 UTC
(In reply to Nick Clemens from comment #0)
> I am not sure how to specify sorting by number index, and not word

The short answer is that you don't. 

If you look at ./etc/zebradb/marc_defs/marc21/biblios/biblio-zebra-indexdefs.xsl, you'll see that "pubdate" has a word, number, year, and sort index. You would just need to add Local-number:s for the target index.

<z:index name="pubdate:w">
  <xslo:value-of select="substring(., 8, 4)"/>
</z:index>
<z:index name="pubdate:n">
  <xslo:value-of select="substring(., 8, 4)"/>
</z:index>
<z:index name="pubdate:y">
  <xslo:value-of select="substring(., 8, 4)"/>
</z:index>
<z:index name="pubdate:s">
  <xslo:value-of select="substring(., 8, 4)"/>
</z:index>
Comment 2 Nick Clemens 2022-06-17 11:32:45 UTC
Created attachment 136255 [details] [review]
Bug 30879: Add option to sort components by biblionumber

WIP

This works for ES, but zebra sorts the biblionumber as a string, s=109 was my attempt based on
https://software.indexdata.com/yaz/doc/tools.html#ccl.syntax
which I don't fully understand
Comment 3 David Cook 2022-06-20 00:45:08 UTC
(In reply to Nick Clemens from comment #2)
> Created attachment 136255 [details] [review] [review]
> Bug 30879: Add option to sort components by biblionumber
> 
> WIP
> 
> This works for ES, but zebra sorts the biblionumber as a string, s=109 was
> my attempt based on
> https://software.indexdata.com/yaz/doc/tools.html#ccl.syntax
> which I don't fully understand

Sorry but I think you gotten it a bit backwards 😅.

First, it looks like you've updated the UNIMARC files for biblio-koha-indexdefs.xml and biblio-zebra-indexdefs.xsl instead of the MARC21 files.

Second, you don't need that change to ccl.properties. 

--

And now that I'm looking more closely... it looks like we do already index 999$c into the following indexes/registers for MARC 21: Local-Number:n Local-Number:w Local-Number:s

So I think you might be going down the wrong path...
Comment 4 David Cook 2022-06-20 01:45:22 UTC
Ok I understand what you're saying now... 

It's not that the sort fails completely. Rather, it's doing a lexicographic sort... so it'll go 1, 10, 2 for ascending 1=12 sort. Yeah that's annoying...

Btw, one way to do tests for this is to use "yaz-client".

Search on something like "e" in koha-testing-docker, and then run "sort 1=4 >i"
Comment 5 David Cook 2022-06-20 03:31:09 UTC
I think this is just a flaw in Zebra.

I tried sorting by "itemnumber" which is just defined in the "n" and "s" registers, and it just lexically sorts as well.

I've tried reading through the YAZ code which seems to be responsible for sorting, but the sort code is from 1998 and very difficult to understand. It looks like sorting updates the query... but I don't see anything that handles the actual "sorting".
Comment 6 David Cook 2022-06-20 03:51:54 UTC
(In reply to David Cook from comment #5)
> I've tried reading through the YAZ code which seems to be responsible for
> sorting, but the sort code is from 1998 and very difficult to understand. It
> looks like sorting updates the query... but I don't see anything that
> handles the actual "sorting".

Ah I was looking at the wrong Zebra folder which is why my grepping was failing :|

According to https://software.indexdata.com/zebra/doc/features.html#features-sort-rank:

"Sorting on the basis of alpha-numeric and numeric data is supported."

"Sorting on the basis of combined sorts ­ e.g. combinations of ascending/descending sorts of lexicographical/numeric/date field data is supported"

"All ordering operations are based on a lexicographical ordering, except when the structure attribute numeric (109) is used. In this case, ordering is numerical. See Section 2.4.3, “Structure Attributes (type 4)”."

And that's why you've used that s=109 there...

Their documentation sure is ambiguous...
Comment 7 David Cook 2022-06-20 04:23:26 UTC
So I think that suggestion with the 4=109 probably only works when providing the 7 attribute:

Ascending order:
find @or @attr 1=1016 e @attr 7=1 @attr 1=Local-Number @attr 4=109 0

Descending order:
find @or @attr 1=1016 e @attr 7=2 @attr 1=Local-Number @attr 4=109 0

(Highest bib is 389 in this case)

--

But in this case we're using the ZOOM "sort" method to run the sort query separately from the main query, so I don't think it'll work, as the sort spec can't support it currently. 

I tried adding 4=109 to "Local-number" in ccl.properties and just sending "Local-number" via the sort and hoping it might do something but nope...

--

So ultimately I'm thinking a bug/shortcoming in Zebra.
Comment 8 David Cook 2022-06-20 04:26:29 UTC
https://github.com/indexdata/idzebra/issues/37
Comment 9 Katrin Fischer 2022-06-20 05:19:43 UTC
Thanks for taking a look at this, David!
Comment 10 Nick Clemens 2022-06-21 13:59:19 UTC
Our IDs all seem to be INT(11) in the DB - would it be reasonable to zero pad our ids into a new column to fix the sorting?
Comment 11 David Cook 2022-06-21 23:54:37 UTC
(In reply to Nick Clemens from comment #10)
> Our IDs all seem to be INT(11) in the DB - would it be reasonable to zero
> pad our ids into a new column to fix the sorting?

Zero padding the ID is smart thinking I reckon.

In theory, we wouldn't need a new DB column. We'd just need to add something like the following to our Zebra indexing XSLT for a new custom index (e.g. Local-number-sort):
<xsl:value-of select='format-number(1, "00000000000")' />

I think to update "./etc/zebradb/marc_defs/marc21/biblios/biblio-zebra-indexdefs.xsl", we'd need to update the template "handle-index-subfields" in ./etc/zebradb/xsl/koha-indexdefs-to-zebra.xsl
Comment 12 Nick Clemens 2022-06-22 14:57:29 UTC
Created attachment 136425 [details] [review]
Bug 30879: Add biblionumber as a sorting option in MARC21

This patch updates the Local-Number indexing by adding a zeropad option
to Zebra indexing and adding this to the mapping files

It also updates C4/Search.pm to allow biblionumber as an option

To test:
1 - Apply patches
2 - copy etc/zebradb/marc_defs/marc21/biblios/biblio-zebra-indexdefs.xsl to /etc/koha/zebradb/marc_defs/marc21/biblios/biblio-zebra-indexdefs.xsl
3 - Restart all, reindex zebra
4 - Browse to: http://localhost:8081/cgi-bin/koha/catalogue/search.pl?idx=kw&q=a&sort_by=biblionumber_dsc&count=20
5 - Confirm records sorted correctly
6 - Browse to http://localhost:8081/cgi-bin/koha/catalogue/search.pl?idx=kw&q=a&sort_by=biblionumber_asc&count=20
7 - Confirm records sorted correctly
Comment 13 Nick Clemens 2022-06-22 14:57:33 UTC
Created attachment 136426 [details] [review]
Bug 30879: Allow biblionumber as sort option in Elasticsearch

Repeat previous tests with Elasticsearch engine
You will need to reindex and reset mappings to pickup the changes form the file
Comment 14 Nick Clemens 2022-06-22 14:57:37 UTC
Created attachment 136427 [details] [review]
Bug 30879: Handle index/sorting for UNIMARC

Same as before, but test with UNIMARC setup
Comment 15 Nick Clemens 2022-06-22 14:57:41 UTC
Created attachment 136428 [details] [review]
Bug 30879: Add option to syspref and update display

To test:
1 - Update ComponentSortField system preference and verify biblionumber is an option
2 - Repeat test plan from 30327 and confirm sorting with biblionumber works
Comment 16 David Cook 2022-06-23 01:21:58 UTC
Can't really test this at the moment since bug 30879 can't apply on master.
Comment 17 David Cook 2022-06-23 01:30:31 UTC
However, I'm pretty keen to test this Zebra indexing change, so I'll do a subtest.

My test plan:
1. Apply patches for 30879 (w/o 30327 and nixing merge conflict due to missing 30327)
2. cp etc/zebradb/marc_defs/marc21/biblios/biblio-zebra-indexdefs.xsl /etc/koha/zebradb/marc_defs/marc21/biblios/biblio-zebra-indexdefs.xsl
3. restart_all
4. koha-rebuild-zebra -b -f -v kohadev
5. yaz-client unix:/var/run/koha/kohadev/bibliosocket
5b. find e
5c. sort 1=12 >i
6. show (through whole 64 record result set to confirm desc sort)

--

It works 100% as you'd expect, so that's awesome. Great job, Nick! I'm so pleased how we got here using teamwork!
Comment 18 David Cook 2022-06-24 02:27:08 UTC
Since 30327 has been updated...

My test plan:
1 - Apply patches
2 - cp etc/zebradb/marc_defs/marc21/biblios/biblio-zebra-indexdefs.xsl /etc/koha/zebradb/marc_defs/marc21/biblios/biblio-zebra-indexdefs.xsl
3a - restart_all
3b - koha-rebuild-zebra -b -f -v kohadev
3c - koha-upgrade-schema kohadev
4 - Browse to: http://localhost:8081/cgi-bin/koha/catalogue/search.pl?idx=kw&q=a&sort_by=biblionumber_dsc&count=20
5 - Confirm records sorted correctly
6 - Browse to http://localhost:8081/cgi-bin/koha/catalogue/search.pl?idx=kw&q=a&sort_by=biblionumber_asc&count=20
7 - Confirm records sorted correctly

I don't know how to test the Elasticsearch or UNIMARC changes.

I've changed "SearchEngine" to "Elasticsearch" and run "koha-elasticsearch --rebuild kohadev" which spat out the following:
Cannot determine authority type for record: 1 at /kohadevbox/koha/Koha/SearchEngine/Elasticsearch.pm line 564.

In the logs I'm seeing this error:
[2022/06/24 02:25:42] [WARN] [Request] ** [http://es:9200]-[400] [query_shard_exception] No mapping found for [local-number__sort] in order to sort on, with: {"index":"koha_kohadev_biblios","index_uuid":"CVVpzugoRfqw7zsFft1pnQ"}

Will need more specific instructions to test this one. I'll leave it as "Needs Signoff" rather than "Failed QA" in the event that someone else can test it correctly. 

--

The test plan for bug 30327 wasn't very specific in its instruction "Add some components to a record by control number or title depending on above", so I tried a lot of things... many which didn't work. Would've been nice to have more explicit steps there. 

Additional test plan:
1 - Update ComponentSortField system preference and verify biblionumber is an option
2 - Repeat test plan from 30327 and confirm sorting with biblionumber works

This looks OK with Zebra.
Comment 19 Martin Renvoize 2022-06-24 15:35:43 UTC
Created attachment 136525 [details] [review]
Bug 30879: Add biblionumber as a sorting option in MARC21

This patch updates the Local-Number indexing by adding a zeropad option
to Zebra indexing and adding this to the mapping files

It also updates C4/Search.pm to allow biblionumber as an option

To test:
1 - Apply patches
2 - copy etc/zebradb/marc_defs/marc21/biblios/biblio-zebra-indexdefs.xsl to /etc/koha/zebradb/marc_defs/marc21/biblios/biblio-zebra-indexdefs.xsl
3 - Restart all, reindex zebra
4 - Browse to: http://localhost:8081/cgi-bin/koha/catalogue/search.pl?idx=kw&q=a&sort_by=biblionumber_dsc&count=20
5 - Confirm records sorted correctly
6 - Browse to http://localhost:8081/cgi-bin/koha/catalogue/search.pl?idx=kw&q=a&sort_by=biblionumber_asc&count=20
7 - Confirm records sorted correctly

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2022-06-24 15:35:47 UTC
Created attachment 136526 [details] [review]
Bug 30879: Allow biblionumber as sort option in Elasticsearch

Repeat previous tests with Elasticsearch engine
You will need to reindex and reset mappings to pickup the changes form the file

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2022-06-24 15:35:51 UTC
Created attachment 136527 [details] [review]
Bug 30879: Handle index/sorting for UNIMARC

Same as before, but test with UNIMARC setup

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2022-06-24 15:35:55 UTC
Created attachment 136528 [details] [review]
Bug 30879: Add option to syspref and update display

To test:
1 - Update ComponentSortField system preference and verify biblionumber is an option
2 - Repeat test plan from 30327 and confirm sorting with biblionumber works

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2022-06-24 15:36:43 UTC
Awesome teamwork here guys, thankyou very much.

All tested and working as expected from my side.. signing off.
Comment 24 Katrin Fischer 2022-06-25 11:54:43 UTC
We need a rebase here please!

Applying: Bug 30327: Add options for sorting components
Using index info to reconstruct a base tree...
M	Koha/Biblio.pm
M	catalogue/detail.pl
M	installer/data/mysql/mandatory/sysprefs.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref
M	koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
M	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt
M	opac/opac-detail.pl
M	t/db_dependent/Koha/Biblio.t
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/mandatory/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/mandatory/sysprefs.sql
CONFLICT (add/add): Merge conflict in installer/data/mysql/atomicupdate/bug_30327_add_sortComponents_syspref.pl
Auto-merging installer/data/mysql/atomicupdate/bug_30327_add_sortComponents_syspref.pl
Auto-merging Koha/Biblio.pm
CONFLICT (content): Merge conflict in Koha/Biblio.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 30327: Add options for sorting components
Comment 25 Katrin Fischer 2022-06-25 12:40:39 UTC
Created attachment 136545 [details] [review]
Bug 30879: Add biblionumber as a sorting option in MARC21

This patch updates the Local-Number indexing by adding a zeropad option
to Zebra indexing and adding this to the mapping files

It also updates C4/Search.pm to allow biblionumber as an option

To test:
1 - Apply patches
2 - copy etc/zebradb/marc_defs/marc21/biblios/biblio-zebra-indexdefs.xsl to /etc/koha/zebradb/marc_defs/marc21/biblios/biblio-zebra-indexdefs.xsl
3 - Restart all, reindex zebra
4 - Browse to: http://localhost:8081/cgi-bin/koha/catalogue/search.pl?idx=kw&q=a&sort_by=biblionumber_dsc&count=20
5 - Confirm records sorted correctly
6 - Browse to http://localhost:8081/cgi-bin/koha/catalogue/search.pl?idx=kw&q=a&sort_by=biblionumber_asc&count=20
7 - Confirm records sorted correctly

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Katrin Fischer 2022-06-25 12:40:43 UTC
Created attachment 136546 [details] [review]
Bug 30879: Allow biblionumber as sort option in Elasticsearch

Repeat previous tests with Elasticsearch engine
You will need to reindex and reset mappings to pickup the changes form the file

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Katrin Fischer 2022-06-25 12:40:47 UTC
Created attachment 136547 [details] [review]
Bug 30879: Handle index/sorting for UNIMARC

Same as before, but test with UNIMARC setup

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 28 Katrin Fischer 2022-06-25 12:40:53 UTC
Created attachment 136548 [details] [review]
Bug 30879: Add option to syspref and update display

To test:
1 - Update ComponentSortField system preference and verify biblionumber is an option
2 - Repeat test plan from 30327 and confirm sorting with biblionumber works

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 29 Katrin Fischer 2022-06-25 12:42:08 UTC
Happy to add a motivational sign-off, but:

I wonder how we can make sure this feature works correctly for updated installations.

1) We should add a note to the release notes about this requiring to reindex.

2) For Zebra the update will usually just work, but what about Elasticsearch? We recently noticed that bug 15851 doesn't work for people because of the missing cni index.  I'd like to avoid that here. Could we add a database update for updating the mappings?

3) Unit tests?
Comment 30 Katrin Fischer 2022-07-26 15:04:48 UTC
No reply from Nick yet, I think we still need the release notes note, but dropping the other requirement from my side. I believe this is worth having.
Comment 31 Nick Clemens 2022-08-01 13:59:08 UTC
(In reply to Katrin Fischer from comment #30)
> No reply from Nick yet, I think we still need the release notes note, but
> dropping the other requirement from my side. I believe this is worth having.

I think I drafted and never posted

We need to solve the problem of differing settings in a larger way - if we are letting people define their own mappings we really can't update them without feedback - bug 30836 is the place to figure that out
Comment 32 Tomás Cohen Arazi 2022-08-02 12:43:47 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 33 Lucas Gass 2022-09-29 16:53:53 UTC
Backported to 22.05.x for 22.05.06
Comment 34 Arthur Suzuki 2022-10-12 13:17:31 UTC
depends on Bug30327 which i'm still waiting a valid patch for 21.11.x.
Will only backport if I get an update on the dependancy.
(don't know the status... on hold?)
Comment 35 David Cook 2022-10-12 23:41:18 UTC
So I've got an update from Indexdata...

It turns out that we could've just patched our Perl code to the following:

 375             elsif ( $sort eq "local-number_asc" ) {
 376                 $sort_by .= "1=12,4=109  <i ";
 377             }
 378             elsif ( $sort eq "local-number_dsc" ) {
 379                 $sort_by .= "1=12,4=109 >i ";
 380             }

I've already tested it using yaz-client and it worked a treat. 

It's an undocumented feature that Adam told me about in the Github issues: https://github.com/indexdata/idzebra/issues/37

I don't know if we want to undo the XSLT changes or keep them and do the Perl change as well. It might be worth doing if only to document what the change would be. It might be useful in other sorting contexts as well...
Comment 36 David Cook 2022-10-12 23:43:02 UTC
(In reply to Lucas Gass from comment #33)
> Backported to 22.05.x for 22.05.06

I don't think that this has gone out yet, so we could actually still revert these changes too?
Comment 37 David Cook 2022-10-12 23:54:18 UTC
(In reply to Katrin Fischer from comment #29)
> Happy to add a motivational sign-off, but:
> 
> I wonder how we can make sure this feature works correctly for updated
> installations.
> 
> 1) We should add a note to the release notes about this requiring to reindex.

If we replace the workaround using bug 31756, we won't need to require a reindex!
Comment 38 David Cook 2022-10-13 00:07:01 UTC
(In reply to David Cook from comment #37)
> (In reply to Katrin Fischer from comment #29)
> > Happy to add a motivational sign-off, but:
> > 
> > I wonder how we can make sure this feature works correctly for updated
> > installations.
> > 
> > 1) We should add a note to the release notes about this requiring to reindex.
> 
> If we replace the workaround using bug 31756, we won't need to require a
> reindex!

Actually, it looks like that may just be true for MARC21. It looks like "Local-number:s" was added for UNIMARC, so they'll probably still need to re-index...
Comment 39 Katrin Fischer 2022-10-14 18:36:20 UTC
(In reply to David Cook from comment #38)
> (In reply to David Cook from comment #37)
> > (In reply to Katrin Fischer from comment #29)
> > > Happy to add a motivational sign-off, but:
> > > 
> > > I wonder how we can make sure this feature works correctly for updated
> > > installations.
> > > 
> > > 1) We should add a note to the release notes about this requiring to reindex.
> > 
> > If we replace the workaround using bug 31756, we won't need to require a
> > reindex!
> 
> Actually, it looks like that may just be true for MARC21. It looks like
> "Local-number:s" was added for UNIMARC, so they'll probably still need to
> re-index...

It makes sense, but since this has been pushed to stable and we are already close to release we need to hurry up if we want to revert and replace with an alternative solution.