Bug 24267

Summary: C4::Breeding::ImportBreedingAuth is ineffective
Product: Koha Reporter: Nick Clemens <nick>
Component: MARC Authority data supportAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: 1joynelson, hayleypelham, jonathan.druart, lucas, m.de.rooy, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24286
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.02
Attachments: Bug 24267: Improve ImportBreedingAuth
Bug 24267: Improve ImportBreedingAuth
Bug 24267: Unit tests
Bug 24267: Unit tests
Bug 24267: Improve ImportBreedingAuth
Bug 24267: Unit tests
Bug 24267: (QA follow-up) Remove two calls, add transaction
Bug 24267: Improve ImportBreedingAuth
Bug 24267: Unit tests
Bug 24267: (QA follow-up) Remove two calls, add transaction

Description Nick Clemens 2019-12-18 18:12:10 UTC
git grep ImportBreedingAuth - there is only one call to this routine from SearchZ3950Auth

We pass it a MARC record, '2' for overwrite_auth

We then check for this record in the DB and get the breeding id, however, when overwrite_auth is 2 we always add the auth to the batch and return the new breeding id.

We don't actually use any of the other parameters returned here either

To recreate:
1 - Browse to Authorities
2 - Select New form Z3950
3 - Perform a search that returns results
4 - SELECT COUNT(*) FROM import_auths
5 - Repeat the search
6 - SELECT COUNT(*) FROM import_auths
7 - There are 20 more records
8 - SELECT * FROM import_auths - note the repeated rows
Comment 1 Nick Clemens 2019-12-18 18:27:54 UTC
Created attachment 96453 [details] [review]
Bug 24267: Improve ImportBreedingAuth
Comment 2 Marcel de Rooy 2019-12-19 07:32:33 UTC
Looks good at first glance.

You add:
+#    warn Data::Dumper::Dumper( $query );
Comment 3 Nick Clemens 2019-12-19 12:30:33 UTC
Created attachment 96482 [details] [review]
Bug 24267: Improve ImportBreedingAuth
Comment 4 Nick Clemens 2019-12-19 12:31:14 UTC
(In reply to Marcel de Rooy from comment #2)
> Looks good at first glance.
> 
> You add:
> +#    warn Data::Dumper::Dumper( $query );

Heh, bum for bug 24103
Comment 5 Marcel de Rooy 2019-12-19 13:05:53 UTC
Any chance to extend Breeding.t for this sub ;) Sorry..
Comment 6 Nick Clemens 2019-12-19 17:37:40 UTC
Created attachment 96496 [details] [review]
Bug 24267: Unit tests
Comment 7 Nick Clemens 2019-12-19 17:38:33 UTC
(In reply to Marcel de Rooy from comment #5)
> Any chance to extend Breeding.t for this sub ;) Sorry..

I did very brief, let me know if there are more cases to cover
Comment 8 Nick Clemens 2019-12-19 17:44:06 UTC
Created attachment 96497 [details] [review]
Bug 24267: Unit tests
Comment 9 Marcel de Rooy 2019-12-20 07:20:21 UTC
Created attachment 96502 [details] [review]
Bug 24267: Improve ImportBreedingAuth

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2019-12-20 07:20:26 UTC
Created attachment 96503 [details] [review]
Bug 24267: Unit tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2019-12-20 07:20:30 UTC
Created attachment 96504 [details] [review]
Bug 24267: (QA follow-up) Remove two calls, add transaction

Call to GetAuthorizedHeading is already done just before calling ImportBreedingAuth.
Call to GuessAuthTypeCode is not used.
Adding transaction to test (check your database, kidclamp ;)

Test plan:
Add new authority via Z3950 in the interface.
Run t/db_dependent/Breeding.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Jonathan Druart 2020-01-02 13:06:06 UTC
Created attachment 96741 [details] [review]
Bug 24267: Improve ImportBreedingAuth

git grep ImportBreedingAuth - there is only one call to this routine
from SearchZ3950Auth

We pass it a MARC record, '2' for overwrite_auth

We then check for this record in the DB and get the breeding id,
however, when overwrite_auth is 2 we always add the auth to the batch
and return the new breeding id.

We don't actually use any of the other parameters returned here either

To recreate:
1 - Browse to Authorities
2 - Select New form Z3950
3 - Perform a search that returns results
4 - SELECT COUNT(*) FROM import_auths
5 - Repeat the search
6 - SELECT COUNT(*) FROM import_auths
7 - There are 20 more records
8 - SELECT * FROM import_auths - note the repeated rows

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Jonathan Druart 2020-01-02 13:06:09 UTC
Created attachment 96742 [details] [review]
Bug 24267: Unit tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Jonathan Druart 2020-01-02 13:06:13 UTC
Created attachment 96743 [details] [review]
Bug 24267: (QA follow-up) Remove two calls, add transaction

Call to GetAuthorizedHeading is already done just before calling ImportBreedingAuth.
Call to GuessAuthTypeCode is not used.
Adding transaction to test (check your database, kidclamp ;)

Test plan:
Add new authority via Z3950 in the interface.
Run t/db_dependent/Breeding.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Martin Renvoize 2020-01-02 14:05:42 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 16 Joy Nelson 2020-01-12 21:22:02 UTC
Pushed to 19.11.x branch for 19.11.02
Comment 17 Lucas Gass 2020-01-15 18:51:09 UTC
backported to 19.05.x for 19.05.07
Comment 18 Hayley Pelham 2020-01-15 21:50:15 UTC
Enhancement will not be backported to 18.11.x series.