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.
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>
Created attachment 82311 [details] [review] Bug 21826: (QA follow-up) Add UNIMARC support Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signoff carried across from bug 19365 where it has been extracted from.
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.
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.
(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.
No problem, I'll add some tests.
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>
Created attachment 82480 [details] [review] Bug 21826: (QA follow-up) Add UNIMARC support Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 82481 [details] [review] Bug 21826: Add tests Adds tests for new Heading::valid_bib_heading_subfield and ModBiblio with auto-created authorities.
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>
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>
Created attachment 82516 [details] [review] Bug 21826: (QA follow-up) Add UNIMARC support Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
Tests look food and comprehensive enough to me.. Passing QA
s/food/good/
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>
Created attachment 82536 [details] [review] Bug 21826: (QA follow-up) Add UNIMARC support Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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;
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 ?
"SELECT tagfield, authtypecode FROM marc_subfield_structure WHERE frameworkcode = '' AND authtypecode <> ''"
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>
Created attachment 82577 [details] [review] Bug 21826: (QA follow-up) Add UNIMARC support Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
Created attachment 82579 [details] [review] Bug 21826: Move Heading tests to db_dependent since they require database connection
Created attachment 82580 [details] [review] Bug 21826: Remove strict and warning from Heading.pm
Thanks for catching those. I moved Heading.t to t/db_dependent and removed strict and warning from Heading.pm.
(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.
Ere: Why this: int($biblio->{'copyrightdate'})
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.
(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 ;)
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Rebased to apply cleanly.
Biblio.t fails for me on master with this applied
Erm.. I can't get Biblio.t to fail here.. any further guidance on getting it to fail Nick?
Tests all pass for me now, not sure what was happening
Awesome work all! Pushed to master for 19.05
I love this enhancement, but alas it didn't make 18.11 and can wait for 19.05.
*** Bug 21224 has been marked as a duplicate of this bug. ***
*** Bug 11462 has been marked as a duplicate of this bug. ***
*** Bug 17811 has been marked as a duplicate of this bug. ***