Bug 24267 - C4::Breeding::ImportBreedingAuth is ineffective
Summary: C4::Breeding::ImportBreedingAuth is ineffective
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-12-18 18:12 UTC by Nick Clemens
Modified: 2020-01-15 21:50 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.02


Attachments
Bug 24267: Improve ImportBreedingAuth (5.13 KB, patch)
2019-12-18 18:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24267: Improve ImportBreedingAuth (4.67 KB, patch)
2019-12-19 12:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24267: Unit tests (2.37 KB, patch)
2019-12-19 17:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24267: Unit tests (2.16 KB, patch)
2019-12-19 17:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24267: Improve ImportBreedingAuth (4.77 KB, patch)
2019-12-20 07:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24267: Unit tests (2.00 KB, patch)
2019-12-20 07:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24267: (QA follow-up) Remove two calls, add transaction (5.91 KB, patch)
2019-12-20 07:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24267: Improve ImportBreedingAuth (5.46 KB, patch)
2020-01-02 13:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24267: Unit tests (2.03 KB, patch)
2020-01-02 13:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24267: (QA follow-up) Remove two calls, add transaction (5.94 KB, patch)
2020-01-02 13:06 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 Mapley 2020-01-15 21:50:15 UTC
Enhancement will not be backported to 18.11.x series.