SQL queries looking at authority records are complicated by the fact that authorities don't have an equivalent to a biblio's title. It would be helpful to create a new database field (auth_header.main_heading) in which we record a text string. The specific value we record here would be determined by the authority type, following the mappings defined in authtypes.pl.
+1
Created attachment 161208 [details] [review] Bug 30047: DB Updates
Created attachment 161209 [details] [review] Bug 30047: DO NOT PUSH - Schema updates
Created attachment 161210 [details] [review] Bug 30047: Add new heading field to auth_header table This patch adds a new heading field containing the display form of the authority record NOTE: If trying to save an auhority in the 'DEFAULT' framework, you will get an error, you should not be using DEFAULT for authorities and we should remove from the list on another bug To test: 1 - Apply patches 2 - Update database 3 - Restart all 4 - Create a new authority, save. 5 - Do this for various types 6 - View the db records: SELECT * FROM auth_header\G 7 - Note new heading field is populated correctly 8 - Edit your new authorities 9 - Confirm the heading field is updated correctly 10 - Import some authorities and confirm heading generated correctly 11 - Import auth via Z39.50 and confirm heading generated correctly
Created attachment 161211 [details] [review] Bug 30047: Unit tests
Created attachment 161213 [details] [review] Bug 30047: DB Updates Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Created attachment 161214 [details] [review] Bug 30047: DO NOT PUSH - Schema updates Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Created attachment 161215 [details] [review] Bug 30047: Add new heading field to auth_header table This patch adds a new heading field containing the display form of the authority record NOTE: If trying to save an auhority in the 'DEFAULT' framework, you will get an error, you should not be using DEFAULT for authorities and we should remove from the list on another bug To test: 1 - Apply patches 2 - Update database 3 - Restart all 4 - Create a new authority, save. 5 - Do this for various types 6 - View the db records: SELECT * FROM auth_header\G 7 - Note new heading field is populated correctly 8 - Edit your new authorities 9 - Confirm the heading field is updated correctly 10 - Import some authorities and confirm heading generated correctly 11 - Import auth via Z39.50 and confirm heading generated correctly Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Created attachment 161216 [details] [review] Bug 30047: Unit tests Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
We do not have a script touching all authorities? That could fill the table for existing records? Another idea to accomplish that and mention in release notes?
Created attachment 166522 [details] [review] Bug 30047: DB Updates Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 166523 [details] [review] Bug 30047: DO NOT PUSH - Schema updates Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 166524 [details] [review] Bug 30047: Add new heading field to auth_header table This patch adds a new heading field containing the display form of the authority record NOTE: If trying to save an auhority in the 'DEFAULT' framework, you will get an error, you should not be using DEFAULT for authorities and we should remove from the list on another bug To test: 1 - Apply patches 2 - Update database 3 - Restart all 4 - Create a new authority, save. 5 - Do this for various types 6 - View the db records: SELECT * FROM auth_header\G 7 - Note new heading field is populated correctly 8 - Edit your new authorities 9 - Confirm the heading field is updated correctly 10 - Import some authorities and confirm heading generated correctly 11 - Import auth via Z39.50 and confirm heading generated correctly Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 166525 [details] [review] Bug 30047: Unit tests Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #10) > We do not have a script touching all authorities? That could fill the table > for existing records? > Another idea to accomplish that and mention in release notes? Something with misc/maintenance/update_authorities.pl ?
This looks a bit like a change I didn't see advertised here? It's obv correct... but I don't like sneaky code ;) '151' => { - auth_type => 'GEOG_NAME', + auth_type => 'GEOGR_NAME', subfields => 'avxyz68', main_entry => 1 We have some open bugs for problems with this authority type. Including a fix in an enh patch means it won't be fixed in stable versions. RMaints will pass over this one.
Pushed for 24.05! Well done everyone, thank you!
This broke t/db_dependent/Biblio.t
(In reply to Tomás Cohen Arazi from comment #18) > This broke t/db_dependent/Biblio.t I suspect more than Biblio.t, a lot of the authority related tests are failing: * t_db_dependent_api_v1_authorities_t * t_db_dependent_Koha_Authorities_t * t_db_dependent_Koha_Plugins_authority_hooks_t * t_db_dependent_Authority_Merge_t (copied from Jenkins, translate _)
Created attachment 166644 [details] [review] Bug 30047: (follow-up) Fix failing tests This patch updates the test files to correctly create authorities and/or correctly mock the needed data for authorities. the tests in t/db_dependent/Authority/Merge.t cover the case of 'Default' authorities which don't have valid headings - so I had to add handling for blanking the heading field when Default type is used.
Picked follow-up for pushing to main, thx!
Not backported to 23.11.x
This is awesome. I can't believe I didn't know about it until now! -- Thinking about indexing... On one hand, it's a shame that an index wasn't added for the heading column. On the other hand, off the top of my head I'm not sure if the index should be on the heading or on the heading and authtypecode. Additionally, "heading" is longtext, which can't be indexed without specifying a (limited) length. Of course, we wouldn't need a unique index. Just an index that could help search performance. Anyway... just going to leave that here for now. We can always see how it goes in production and then improve it if necessary...
(In reply to Marcel de Rooy from comment #15) > (In reply to Marcel de Rooy from comment #10) > > We do not have a script touching all authorities? That could fill the table > > for existing records? > > Another idea to accomplish that and mention in release notes? > > Something with misc/maintenance/update_authorities.pl ? This was not addressed, unless I'm missing something? Upon upgrade, is there something to do to populate this column in existing systems?
(In reply to Caroline Cyr La Rose from comment #24) > (In reply to Marcel de Rooy from comment #15) > > (In reply to Marcel de Rooy from comment #10) > > > We do not have a script touching all authorities? That could fill the table > > > for existing records? > > > Another idea to accomplish that and mention in release notes? > > > > Something with misc/maintenance/update_authorities.pl ? > > This was not addressed, unless I'm missing something? Upon upgrade, is there > something to do to populate this column in existing systems? Yeah, it doesn't look like it was addressed. I reckon a new touch_all_authorities.pl might be a good way to go.
I created(In reply to David Cook from comment #25) > (In reply to Caroline Cyr La Rose from comment #24) > > (In reply to Marcel de Rooy from comment #15) > > > (In reply to Marcel de Rooy from comment #10) > > > > We do not have a script touching all authorities? That could fill the table > > > > for existing records? > > > > Another idea to accomplish that and mention in release notes? > > > > > > Something with misc/maintenance/update_authorities.pl ? > > > > This was not addressed, unless I'm missing something? Upon upgrade, is there > > something to do to populate this column in existing systems? > > Yeah, it doesn't look like it was addressed. I reckon a new > touch_all_authorities.pl might be a good way to go. I created Bug 38133. Unfortunately, this is useless to existing systems right now without a script to populate the new column.