Bug 25306 - Unnecessary update of framework in ModBiblioMarc
Summary: Unnecessary update of framework in ModBiblioMarc
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-04-28 18:36 UTC by Nick Clemens
Modified: 2021-01-18 14:41 UTC (History)
5 users (show)

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


Attachments
Bug 25306: Remove framework paramter from ModBiblioMarc (8.74 KB, patch)
2020-04-28 18:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25306: Remove framework paramter from ModBiblioMarc (8.78 KB, patch)
2021-01-06 19:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25306: Remove framework paramter from ModBiblioMarc (9.09 KB, patch)
2021-01-07 15:02 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 25306: Remove framework paramter from ModBiblioMarc (9.16 KB, patch)
2021-01-11 11:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25306: (QA follow-up) Remove framework from bulkmarcimport (1.31 KB, patch)
2021-01-11 11:24 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-04-28 18:36:16 UTC
All calls to this function are either passing the framework obtained from the bib, or precede or are preceded by a bib modification which sets the framework code.

We do not need this parameter in this function
Comment 1 Nick Clemens 2020-04-28 18:37:08 UTC
Created attachment 103876 [details] [review]
Bug 25306: Remove framework paramter from ModBiblioMarc
Comment 2 Frédéric Demians 2020-04-30 09:07:02 UTC
I suppose you have searched for all occurrences of ModBiblio, but why is you patch also modifying a AddBiblio call? This one:

- eval { ( $biblionumber, $biblioitemnumber ) = AddBiblio( $record, '', { defer_marc_save => 1 } ) };
+ eval { ( $biblionumber, $biblioitemnumber ) = AddBiblio( $record, $framework, { defer_marc_save => 1 } ) };
Comment 3 Nick Clemens 2020-04-30 10:23:23 UTC
(In reply to Frédéric Demians from comment #2)
> I suppose you have searched for all occurrences of ModBiblio, but why is you
> patch also modifying a AddBiblio call? This one:
> 
> - eval { ( $biblionumber, $biblioitemnumber ) = AddBiblio( $record, '', {
> defer_marc_save => 1 } ) };
> + eval { ( $biblionumber, $biblioitemnumber ) = AddBiblio( $record,
> $framework, { defer_marc_save => 1 } ) };

In that bit of code we are passing the framework to ModBiblioMarc after this call - the $framework variable is the same during both calls, moving it to the AddBiblio means we can remove it from the later call
Comment 4 Fridolin Somers 2020-12-30 16:16:49 UTC
Sorry does not apply on current master.

I get :

Applying: Bug 25306: Remove framework paramter from ModBiblioMarc
error: sha1 information is lacking or useless (C4/Biblio.pm).
error: could not build fake ancestor
Comment 5 Nick Clemens 2021-01-06 19:04:46 UTC
Created attachment 114900 [details] [review]
Bug 25306: Remove framework paramter from ModBiblioMarc
Comment 6 Fridolin Somers 2021-01-07 15:01:10 UTC
Tested with :

For a framework (not the default) :

Creation of a biblio record
Edition of this biblio record
Creation of an item of this record
Creation of an item of this record

./misc/batchRepairMissingBiblionumbers.pl OK

prove t/db_dependent/Biblio/ModBiblioMarc.t OK
Comment 7 Fridolin Somers 2021-01-07 15:02:41 UTC
Created attachment 114923 [details] [review]
Bug 25306: Remove framework paramter from ModBiblioMarc

Tested with :

For a framework (not the default) :

Creation of a biblio record
Edition of this biblio record
Creation of an item of this record
Creation of an item of this record

./misc/batchRepairMissingBiblionumbers.pl OK

prove t/db_dependent/Biblio/ModBiblioMarc.t OK

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 8 Martin Renvoize 2021-01-11 11:23:57 UTC
Created attachment 115010 [details] [review]
Bug 25306: Remove framework paramter from ModBiblioMarc

Tested with :

For a framework (not the default) :

Creation of a biblio record
Edition of this biblio record
Creation of an item of this record
Creation of an item of this record

./misc/batchRepairMissingBiblionumbers.pl OK

prove t/db_dependent/Biblio/ModBiblioMarc.t OK

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2021-01-11 11:24:00 UTC
Created attachment 115011 [details] [review]
Bug 25306: (QA follow-up) Remove framework from bulkmarcimport

One last instance of ModBiblioMarc in bulkmarcimport still passed the
frameworkcode.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2021-01-11 11:25:13 UTC
Works as expected, tests pass and no regressions found.. I added a follow up for the final case where ModBiblioMarc was being called with a frameworkcode.

Passing QA
Comment 11 Jonathan Druart 2021-01-12 15:47:56 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 12 Fridolin Somers 2021-01-18 14:41:56 UTC
Enhancement not pushed to 20.11.x