Bug 21958 - _check_valid_auth_link checks too many subfields
Summary: _check_valid_auth_link checks too many subfields
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Ere Maijala
QA Contact: Testopia
URL:
Keywords:
Depends on: 21826
Blocks:
  Show dependency treegraph
 
Reported: 2018-12-05 21:18 UTC by Nick Clemens
Modified: 2021-08-27 18:54 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00


Attachments
Bug 21958: Fix bibliographic record field comparison with authority (18.08 KB, patch)
2019-01-22 14:50 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 21958: Fix bibliographic record field comparison with authority (18.15 KB, patch)
2019-07-01 14:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21958: Fix bibliographic record field comparison with authority (18.15 KB, patch)
2019-09-05 19:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21958: Fix bibliographic record field comparison with authority (19.88 KB, patch)
2020-02-24 14:17 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 21958: Fix bibliographic record field comparison with authority (18.61 KB, patch)
2020-08-13 13:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21958: Fix bibliographic record field comparison with authority (18.64 KB, patch)
2020-12-10 12:08 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21958: Fix bibliographic record field comparison with authority (18.73 KB, patch)
2021-03-25 13:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21958: (QA follow-up) Correct indirect object notation (1.12 KB, patch)
2021-03-25 13:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21958: Allow 'e' subfield for authorities (1.37 KB, patch)
2021-04-06 12:58 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2018-12-05 21:18:26 UTC
In the sub we call C4::AuthoritiesMarc::GetAuthorizedHeading on the auth record

for the field we use all subfields a-z - these are not all valid authorized heading fields

To recreate - add a 700 field with $i - link it to an authority with the $i

Run the linker in a way that it ends up checking the validity of this link, it will be rejected as not matching

This can lead to multiple duplicate authorities being created depending upon settings
Comment 1 Ere Maijala 2018-12-07 07:16:51 UTC
Duplicate of bug 21826?
Comment 2 Ere Maijala 2018-12-07 11:30:41 UTC
Maybe not. Taking.
Comment 3 Ere Maijala 2019-01-22 14:50:38 UTC
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
Comment 4 Michal Denar 2019-05-03 19:14:19 UTC
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.
Comment 5 Nick Clemens 2019-07-01 14:21:14 UTC
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
Comment 6 Michal Denar 2019-08-30 20:01:33 UTC
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
Comment 7 Nick Clemens 2019-09-05 19:36:42 UTC
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
Comment 8 Martin Renvoize 2020-02-24 12:55:54 UTC
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.
Comment 9 Ere Maijala 2020-02-24 14:17:46 UTC
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
Comment 10 Bernardo Gonzalez Kriegel 2020-03-21 13:54:43 UTC
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
Comment 11 Nick Clemens 2020-08-13 13:30:46 UTC
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
Comment 12 Nick Clemens 2020-08-13 13:31:57 UTC
(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 :-)
Comment 13 Didier Gautheron 2020-11-24 22:40:31 UTC
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;
Comment 14 Nick Clemens 2020-12-10 12:08:57 UTC
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
Comment 15 Frank Hansen 2021-02-05 14:25:52 UTC
The patch works fine for me. Signing off.
Comment 16 Martin Renvoize 2021-03-25 13:38:15 UTC
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>
Comment 17 Martin Renvoize 2021-03-25 13:38:19 UTC
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>
Comment 18 Martin Renvoize 2021-03-25 13:40:10 UTC
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
Comment 19 Jonathan Druart 2021-04-01 16:53:28 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 20 Jonathan Druart 2021-04-06 08:52:48 UTC
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.
Comment 21 Nick Clemens 2021-04-06 12:58:56 UTC
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
Comment 22 Jonathan Druart 2021-04-06 13:58:03 UTC
Follow-up pushed to master.
Comment 23 Martin Renvoize 2021-04-06 14:53:34 UTC
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..
Comment 24 Fridolin Somers 2021-04-07 13:21:37 UTC
Actually not backported to stable. To much impact.