|
Description
Nick Clemens (kidclamp)
2018-12-05 21:18:26 UTC
Maybe not. Taking. Created attachment 84283 [details] [review] Bug 21958: Fix bibliographic record field comparison with authority This makes the comparison between bibliographic field and authority field more robust and per subfield. This makes the comparison not consider the same e.g. the following fields: $a Test User $a Test $b User The actual issue cannot be as easily reproduced with the patches for bug 21826 applied, but here's a test plan anyway: 1. Make sure tests pass (especially t/db_dependent/AuthoritiesMarc*) 2. Make sure authority linking still works properly 3. Make sure authority and biblio frameworks allow subfield i 4. Make sure that even if you add subfield i to 700 in biblio, authority link is kept the same 5. Make sure that even if you add subfield i to the authority record, the authority link is kept the same Hi Ere, I get error: Applying: Bug 21958: Fix bibliographic record field comparison with authority Using index info to reconstruct a base tree... M C4/Biblio.pm M installer/data/mysql/en/marcflavour/marc21/mandatory/authorities_normal_marc21.sql Falling back to patching base and 3-way merge... Auto-merging installer/data/mysql/en/marcflavour/marc21/mandatory/authorities_normal_marc21.sql Auto-merging C4/Biblio.pm CONFLICT (content): Merge conflict in C4/Biblio.pm error: Failed to merge in the changes. Can you rebase this patch on master? Thank you. Created attachment 91150 [details] [review] Bug 21958: Fix bibliographic record field comparison with authority This makes the comparison between bibliographic field and authority field more robust and per subfield. This makes the comparison not consider the same e.g. the following fields: $a Test User $a Test $b User The actual issue cannot be as easily reproduced with the patches for bug 21826 applied, but here's a test plan anyway: 1. Make sure tests pass (especially t/db_dependent/AuthoritiesMarc*) 2. Make sure authority linking still works properly 3. Make sure authority and biblio frameworks allow subfield i 4. Make sure that even if you add subfield i to 700 in biblio, authority link is kept the same 5. Make sure that even if you add subfield i to the authority record, the authority link is kept the same Hello, can you rebase on master? Thank you. Applying: Bug 21958: Fix bibliographic record field comparison with authority Using index info to reconstruct a base tree... M C4/AuthoritiesMarc.pm M C4/Biblio.pm M C4/Heading.pm M C4/Heading/MARC21.pm M installer/data/mysql/en/marcflavour/marc21/mandatory/authorities_normal_marc21.sql M t/db_dependent/AuthoritiesMarc.t Falling back to patching base and 3-way merge... Auto-merging C4/Biblio.pm CONFLICT (content): Merge conflict in C4/Biblio.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 21958: Fix bibliographic record field comparison with authority The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-21958-Fix-bibliographic-record-field-compariso-DE87lj.patch Created attachment 92611 [details] [review] Bug 21958: Fix bibliographic record field comparison with authority This makes the comparison between bibliographic field and authority field more robust and per subfield. This makes the comparison not consider the same e.g. the following fields: $a Test User $a Test $b User The actual issue cannot be as easily reproduced with the patches for bug 21826 applied, but here's a test plan anyway: 1. Make sure tests pass (especially t/db_dependent/AuthoritiesMarc*) 2. Make sure authority linking still works properly 3. Make sure authority and biblio frameworks allow subfield i 4. Make sure that even if you add subfield i to 700 in biblio, authority link is kept the same 5. Make sure that even if you add subfield i to the authority record, the authority link is kept the same Afraid this one no longer applies.. I'm not close enough to MARC practices at the moment to rebase.. looks reasonably trivial for someone who is. Created attachment 99528 [details] [review] Bug 21958: Fix bibliographic record field comparison with authority This makes the comparison between bibliographic field and authority field more robust and per subfield. This makes the comparison not consider the same e.g. the following fields: $a Test User $a Test $b User The actual issue cannot be as easily reproduced with the patches for bug 21826 applied, but here's a test plan anyway: 1. Make sure tests pass (especially t/db_dependent/AuthoritiesMarc*) 2. Make sure authority linking still works properly 3. Make sure authority and biblio frameworks allow subfield i 4. Make sure that even if you add subfield i to 700 in biblio, authority link is kept the same 5. Make sure that even if you add subfield i to the authority record, the authority link is kept the same Hi, this patch does not apply. Some comments: * If want to add new auth types then we need to add some definitions in authorities_normal_marc21.yml, I can help with that * For the new auth types we need new codes, you are using the same as existing ones, perhaps we can use 147 NAME_EVENT Named event 162 MED_PERFRM Medium of Performance Term 180 GENRL_SUBD General Subdivision 181 GEOGR_SUBD Geographic Subdivision 182 CHRON_SUBD Chronological Subdivision 185 FORM_SUBD Form Subdivision Created attachment 108182 [details] [review] Bug 21958: Fix bibliographic record field comparison with authority This makes the comparison between bibliographic field and authority field more robust and per subfield. This makes the comparison not consider the same e.g. the following fields: $a Test User $a Test $b User The actual issue cannot be as easily reproduced with the patches for bug 21826 applied, but here's a test plan anyway: 1. Make sure tests pass (especially t/db_dependent/AuthoritiesMarc*) 2. Make sure authority linking still works properly 3. Make sure authority and biblio frameworks allow subfield i 4. Make sure that even if you add subfield i to 700 in biblio, authority link is kept the same 5. Make sure that even if you add subfield i to the authority record, the authority link is kept the same (In reply to Bernardo Gonzalez Kriegel from comment #10) > Hi, > this patch does not apply. > > Some comments: > * If want to add new auth types then we need to add some definitions in > authorities_normal_marc21.yml, I can help with that > * For the new auth types we need new codes, you are using the same as > existing ones, perhaps we can use > 147 NAME_EVENT Named event > 162 MED_PERFRM Medium of Performance Term > 180 GENRL_SUBD General Subdivision > 181 GEOGR_SUBD Geographic Subdivision > 182 CHRON_SUBD Chronological Subdivision > 185 FORM_SUBD Form Subdivision I removed the addition of the new types, though the code will support them when they are added. If you can help here, or on another patch that would be excellent :-) Trivial but: diff --cc t/db_dependent/AuthoritiesMarc.t index 50bbd6edb4,97f118ccf4..0000000000 --- a/t/db_dependent/AuthoritiesMarc.t +++ b/t/db_dependent/AuthoritiesMarc.t @@@ -5,9 -5,10 +5,14 @@@ use Modern::Perl; ++<<<<<<< HEAD +use Test::More tests => 11; ++======= + use Test::More tests => 10; ++>>>>>>> Bug 21958: Fix bibliographic record field comparison with authority use Test::MockModule; use Test::Warn; + use MARC::Field; use MARC::Record; use t::lib::Mocks; Created attachment 114303 [details] [review] Bug 21958: Fix bibliographic record field comparison with authority This makes the comparison between bibliographic field and authority field more robust and per subfield. This makes the comparison not consider the same e.g. the following fields: $a Test User $a Test $b User The actual issue cannot be as easily reproduced with the patches for bug 21826 applied, but here's a test plan anyway: 1. Make sure tests pass (especially t/db_dependent/AuthoritiesMarc*) 2. Make sure authority linking still works properly 3. Make sure authority and biblio frameworks allow subfield i 4. Make sure that even if you add subfield i to 700 in biblio, authority link is kept the same 5. Make sure that even if you add subfield i to the authority record, the authority link is kept the same The patch works fine for me. Signing off. Created attachment 118795 [details] [review] Bug 21958: Fix bibliographic record field comparison with authority This makes the comparison between bibliographic field and authority field more robust and per subfield. This makes the comparison not consider the same e.g. the following fields: $a Test User $a Test $b User The actual issue cannot be as easily reproduced with the patches for bug 21826 applied, but here's a test plan anyway: 1. Make sure tests pass (especially t/db_dependent/AuthoritiesMarc*) 2. Make sure authority linking still works properly 3. Make sure authority and biblio frameworks allow subfield i 4. Make sure that even if you add subfield i to 700 in biblio, authority link is kept the same 5. Make sure that even if you add subfield i to the authority record, the authority link is kept the same Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 118796 [details] [review] Bug 21958: (QA follow-up) Correct indirect object notation Our coding guidelines now forbit indirect object notation calling. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> All works as expected now.. would love to see a followup for the types addition in full. QA scripts happy and no regressions found. Passing QA Pushed to master for 21.05, thanks to everybody involved! There is a test failing, please fix ASAP
t/db_dependent/Heading.t .. 1/3
# Failed test '100e not valid for authority'
# at t/db_dependent/Heading.t line 38.
# Looks like you failed 1 test of 9.
Created attachment 119217 [details] [review] Bug 21958: Allow 'e' subfield for authorities This bug specifically changed the code to allow for e in authority record This is a tricky issue, see link for discussion: https://www.loc.gov/marc/marbi/2008/2008-dp05-1.html It seems to be allowed, but possibly not useful? In either case I think for now we can allow in the test to fix this error and possibly open a new bug for further discussion Follow-up pushed to master. Nick and I discussed the follow-up and agreed this was the best approach.. the caveat here is that in this case you wouldn't be able to link a bib with a relator to an auth with the relator.. but an auth with a relator would be created by a bib with the relator.. I think.. Actually not backported to stable. To much impact. *** Bug 17805 has been marked as a duplicate of this bug. *** |