Bug 21826 - Automatic authority record generation improvements
Summary: Automatic authority record generation improvements
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Ere Maijala
QA Contact: Martin Renvoize
URL:
Keywords: Manual, release-notes-needed
Depends on:
Blocks: 21958
  Show dependency treegraph
 
Reported: 2018-11-13 16:31 UTC by Martin Renvoize
Modified: 2019-05-29 11:46 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
This enhancement makes improvements to automatic authority record generation, including using only allowable subfields when creating authorities from bibliographic records.
Version(s) released in:


Attachments
Bug 21826: Use subfield map in LinkBibHeadingsToAuthorities (3.42 KB, patch)
2018-11-13 16:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21826: (QA follow-up) Add UNIMARC support (994 bytes, patch)
2018-11-13 16:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21826: Use subfield map in LinkBibHeadingsToAuthorities (3.40 KB, patch)
2018-11-19 12:28 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 21826: (QA follow-up) Add UNIMARC support (987 bytes, patch)
2018-11-19 12:28 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 21826: Add tests (6.75 KB, patch)
2018-11-19 12:28 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 21826: Add tests (6.81 KB, patch)
2018-11-19 15:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21826: Use subfield map in LinkBibHeadingsToAuthorities (3.42 KB, patch)
2018-11-19 15:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21826: (QA follow-up) Add UNIMARC support (994 bytes, patch)
2018-11-19 15:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21826: Add tests (6.81 KB, patch)
2018-11-19 15:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21826: Use subfield map in LinkBibHeadingsToAuthorities (3.46 KB, patch)
2018-11-20 10:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21826: (QA follow-up) Add UNIMARC support (994 bytes, patch)
2018-11-20 10:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21826: Add tests (6.81 KB, patch)
2018-11-20 10:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21826: Use subfield map in LinkBibHeadingsToAuthorities (3.44 KB, patch)
2018-11-22 09:27 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 21826: (QA follow-up) Add UNIMARC support (987 bytes, patch)
2018-11-22 09:27 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 21826: Add tests (6.81 KB, patch)
2018-11-22 09:27 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 21826: Move Heading tests to db_dependent since they require database connection (535 bytes, patch)
2018-11-22 09:27 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 21826: Remove strict and warning from Heading.pm (550 bytes, patch)
2018-11-22 09:27 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 21826: Use subfield map in LinkBibHeadingsToAuthorities (3.54 KB, patch)
2018-11-26 12:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21826: (QA follow-up) Add UNIMARC support (1.06 KB, patch)
2018-11-26 12:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21826: Add tests (6.90 KB, patch)
2018-11-26 12:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21826: Move Heading tests to db_dependent since they require database connection (632 bytes, patch)
2018-11-26 12:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21826: Remove strict and warning from Heading.pm (647 bytes, patch)
2018-11-26 12:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21826: Use subfield map in LinkBibHeadingsToAuthorities (3.51 KB, patch)
2018-12-07 12:50 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 21826: (QA follow-up) Add UNIMARC support (1.03 KB, patch)
2018-12-07 12:50 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 21826: Add tests (6.86 KB, patch)
2018-12-07 12:50 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 21826: Move Heading tests to db_dependent since they require database connection (593 bytes, patch)
2018-12-07 12:50 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 21826: Remove strict and warning from Heading.pm (631 bytes, patch)
2018-12-07 12:51 UTC, Ere Maijala
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2018-11-13 16:31:09 UTC
Whilst QAing bug 19365 I saw that some general improvements had been made to the generation of authorities from bibliographic records routines.

I felt these should get their own mention in the release notes and so have split them out into this bug.
Comment 1 Martin Renvoize 2018-11-13 16:50:36 UTC
Created attachment 82310 [details] [review]
Bug 21826: Use subfield map in LinkBibHeadingsToAuthorities

Use only allowable subfields when creating authorities from
bibliographic records.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 2 Martin Renvoize 2018-11-13 16:50:39 UTC
Created attachment 82311 [details] [review]
Bug 21826: (QA follow-up) Add UNIMARC support

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 3 Martin Renvoize 2018-11-13 16:51:34 UTC
Signoff carried across from bug 19365 where it has been extracted from.
Comment 4 Martin Renvoize 2018-11-13 17:08:14 UTC
I'd really like to see some minimal test coverage adding for the change to C4::LinkBibHeadingsToAuthorities here before I feel I can pass qa.
Comment 5 Martin Renvoize 2018-11-16 16:59:30 UTC
Setting back to 'Signed Off' to get a second QA opinion here.. I think the code is solid but it would certainly be nice to have tests.. but given the lack of tests for this whole module it's a difficult one to get going with.
Comment 6 Marcel de Rooy 2018-11-19 07:58:46 UTC
(In reply to Martin Renvoize from comment #5)
> Setting back to 'Signed Off' to get a second QA opinion here.. I think the
> code is solid but it would certainly be nice to have tests.. but given the
> lack of tests for this whole module it's a difficult one to get going with.

If we are changing the module, we have to add tests. We are not asking complete test coverage of the whole module, but some tests to prove the suggested changes. Too bad that there was nothing yet, but we will have a good place to start..

Since this is an enh and not something for the release of this week, there is enough time to do so. Changing status accordingly.
Comment 7 Ere Maijala 2018-11-19 08:12:06 UTC
No problem, I'll add some tests.
Comment 8 Ere Maijala 2018-11-19 12:28:37 UTC
Created attachment 82479 [details] [review]
Bug 21826: Use subfield map in LinkBibHeadingsToAuthorities

Use only allowable subfields when creating authorities from
bibliographic records.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Ere Maijala 2018-11-19 12:28:41 UTC
Created attachment 82480 [details] [review]
Bug 21826: (QA follow-up) Add UNIMARC support

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Ere Maijala 2018-11-19 12:28:44 UTC
Created attachment 82481 [details] [review]
Bug 21826: Add tests

Adds tests for new Heading::valid_bib_heading_subfield and ModBiblio with auto-created authorities.
Comment 11 Martin Renvoize 2018-11-19 15:52:37 UTC
Created attachment 82514 [details] [review]
Bug 21826: Add tests

Adds tests for new Heading::valid_bib_heading_subfield and ModBiblio with auto-created authorities.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2018-11-19 15:55:51 UTC
Created attachment 82515 [details] [review]
Bug 21826: Use subfield map in LinkBibHeadingsToAuthorities

Use only allowable subfields when creating authorities from
bibliographic records.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2018-11-19 15:55:55 UTC
Created attachment 82516 [details] [review]
Bug 21826: (QA follow-up) Add UNIMARC support

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2018-11-19 15:55:58 UTC
Created attachment 82517 [details] [review]
Bug 21826: Add tests

Adds tests for new Heading::valid_bib_heading_subfield and ModBiblio with auto-created authorities.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2018-11-19 15:57:05 UTC
Tests look food and comprehensive enough to me.. Passing QA
Comment 16 Martin Renvoize 2018-11-19 16:01:38 UTC
s/food/good/
Comment 17 Martin Renvoize 2018-11-20 10:10:08 UTC
Created attachment 82535 [details] [review]
Bug 21826: Use subfield map in LinkBibHeadingsToAuthorities

Use only allowable subfields when creating authorities from
bibliographic records.

Sponsored-by: National Library of Finland
Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2018-11-20 10:10:13 UTC
Created attachment 82536 [details] [review]
Bug 21826: (QA follow-up) Add UNIMARC support

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2018-11-20 10:10:17 UTC
Created attachment 82537 [details] [review]
Bug 21826: Add tests

Adds tests for new Heading::valid_bib_heading_subfield and ModBiblio with auto-created authorities.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Marcel de Rooy 2018-11-21 14:47:44 UTC
Minor detail, but remove strict and warnings if you add modern perl

--- a/C4/Heading.pm
+++ b/C4/Heading.pm
@@ -17,6 +17,8 @@ package C4::Heading;
 # You should have received a copy of the GNU General Public License
 # along with Koha; if not, see <http://www.gnu.org/licenses>.

+use Modern::Perl;
+
 use strict;
 use warnings;
Comment 21 Marcel de Rooy 2018-11-21 15:01:16 UTC
t/Heading.t
# Subtest: UNIMARC tests
Can't call method "config" on unblessed reference at /usr/share/koha/devclone/Koha/Database.pm line 56.

I think we are doing things in Heading.pm and subordinate modules that expect a database. So failing on that one now. Move to t/db or merge ?
Comment 22 Marcel de Rooy 2018-11-21 15:02:51 UTC
        "SELECT tagfield, authtypecode
         FROM marc_subfield_structure
         WHERE frameworkcode = '' AND authtypecode <> ''"
Comment 23 Ere Maijala 2018-11-22 09:27:27 UTC
Created attachment 82576 [details] [review]
Bug 21826: Use subfield map in LinkBibHeadingsToAuthorities

Use only allowable subfields when creating authorities from
bibliographic records.

Sponsored-by: National Library of Finland
Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Ere Maijala 2018-11-22 09:27:31 UTC
Created attachment 82577 [details] [review]
Bug 21826: (QA follow-up) Add UNIMARC support

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Ere Maijala 2018-11-22 09:27:34 UTC
Created attachment 82578 [details] [review]
Bug 21826: Add tests

Adds tests for new Heading::valid_bib_heading_subfield and ModBiblio with auto-created authorities.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Ere Maijala 2018-11-22 09:27:39 UTC
Created attachment 82579 [details] [review]
Bug 21826: Move Heading tests to db_dependent since they require database connection
Comment 27 Ere Maijala 2018-11-22 09:27:43 UTC
Created attachment 82580 [details] [review]
Bug 21826: Remove strict and warning from Heading.pm
Comment 28 Ere Maijala 2018-11-22 09:29:30 UTC
Thanks for catching those. I moved Heading.t to t/db_dependent and removed strict and warning from Heading.pm.
Comment 29 Marcel de Rooy 2018-11-22 09:53:18 UTC
(In reply to Ere Maijala from comment #28)
> Thanks for catching those. I moved Heading.t to t/db_dependent and removed
> strict and warning from Heading.pm.

I would suggest to move it back to SO now. Martin or I will have another look.
Comment 30 Marcel de Rooy 2018-11-26 10:52:09 UTC
Ere: Why this:

int($biblio->{'copyrightdate'})
Comment 31 Ere Maijala 2018-11-26 12:02:17 UTC
Marcel, the field is smallint, so we need to make sure we don't write non-integer data to it. MySQL may silently accept it, but the result doesn't stick. I'd have to dig up an example if necessary since I don't remember the exact situation where it goes wrong otherwise.
Comment 32 Marcel de Rooy 2018-11-26 12:38:43 UTC
(In reply to Ere Maijala from comment #31)
> Marcel, the field is smallint, so we need to make sure we don't write
> non-integer data to it. MySQL may silently accept it, but the result doesn't
> stick. I'd have to dig up an example if necessary since I don't remember the
> exact situation where it goes wrong otherwise.

OK. Calling an int field ..date was a good choice ;)
Comment 33 Marcel de Rooy 2018-11-26 12:43:35 UTC
Created attachment 82634 [details] [review]
Bug 21826: Use subfield map in LinkBibHeadingsToAuthorities

Use only allowable subfields when creating authorities from
bibliographic records.

Sponsored-by: National Library of Finland
Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Marcel de Rooy 2018-11-26 12:43:39 UTC
Created attachment 82635 [details] [review]
Bug 21826: (QA follow-up) Add UNIMARC support

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Marcel de Rooy 2018-11-26 12:43:43 UTC
Created attachment 82636 [details] [review]
Bug 21826: Add tests

Adds tests for new Heading::valid_bib_heading_subfield and ModBiblio with auto-created authorities.

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Marcel de Rooy 2018-11-26 12:43:47 UTC
Created attachment 82637 [details] [review]
Bug 21826: Move Heading tests to db_dependent since they require database connection

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 37 Marcel de Rooy 2018-11-26 12:43:52 UTC
Created attachment 82638 [details] [review]
Bug 21826: Remove strict and warning from Heading.pm

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 38 Ere Maijala 2018-12-07 12:50:43 UTC
Created attachment 82961 [details] [review]
Bug 21826: Use subfield map in LinkBibHeadingsToAuthorities

Use only allowable subfields when creating authorities from
bibliographic records.

Sponsored-by: National Library of Finland
Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 39 Ere Maijala 2018-12-07 12:50:48 UTC
Created attachment 82962 [details] [review]
Bug 21826: (QA follow-up) Add UNIMARC support

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 40 Ere Maijala 2018-12-07 12:50:53 UTC
Created attachment 82963 [details] [review]
Bug 21826: Add tests

Adds tests for new Heading::valid_bib_heading_subfield and ModBiblio with auto-created authorities.

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 41 Ere Maijala 2018-12-07 12:50:58 UTC
Created attachment 82964 [details] [review]
Bug 21826: Move Heading tests to db_dependent since they require database connection

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 42 Ere Maijala 2018-12-07 12:51:02 UTC
Created attachment 82965 [details] [review]
Bug 21826: Remove strict and warning from Heading.pm

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 43 Ere Maijala 2018-12-07 12:51:22 UTC
Rebased to apply cleanly.
Comment 44 Nick Clemens 2018-12-26 16:23:33 UTC
Biblio.t fails for me on master with this applied
Comment 45 Martin Renvoize 2019-01-24 14:25:51 UTC
Erm.. I can't get Biblio.t to fail here.. any further guidance on getting it to fail Nick?
Comment 46 Nick Clemens 2019-01-24 14:43:00 UTC
Tests all pass for me now, not sure what was happening
Comment 47 Nick Clemens 2019-01-25 20:51:26 UTC
Awesome work all!

Pushed to master for 19.05
Comment 48 Martin Renvoize 2019-01-31 16:51:08 UTC
I love this enhancement, but alas it didn't make 18.11 and can wait for 19.05.