Bug 30813 - Refactor TransformMarcToKoha to remove TransformMarcToKohaOneField
Summary: Refactor TransformMarcToKoha to remove TransformMarcToKohaOneField
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on: 30789
Blocks: 30824
  Show dependency treegraph
 
Reported: 2022-05-19 19:29 UTC by Nick Clemens
Modified: 2023-12-28 20:42 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 30813: Update TransformMarcToKoha to accept a hashref (15.47 KB, patch)
2022-05-20 10:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30813: Adjust TransformMarcToKoha to take kohafields parameter (2.38 KB, patch)
2022-05-20 10:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30813: Remove TransformMarcToKohaOneField (12.09 KB, patch)
2022-05-20 10:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30813: Remove Koha::MetadataRecord::getKohaField (1014 bytes, patch)
2022-05-20 10:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30813: Update TransformMarcToKoha to accept a hashref (15.53 KB, patch)
2022-05-20 11:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30813: Adjust TransformMarcToKoha to take kohafields parameter (2.44 KB, patch)
2022-05-20 11:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30813: Remove TransformMarcToKohaOneField (12.15 KB, patch)
2022-05-20 11:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30813: Remove Koha::MetadataRecord::getKohaField (1.05 KB, patch)
2022-05-20 11:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30813: (follow-up) ImportBatch updates (1.34 KB, patch)
2022-05-20 11:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30813: Update TransformMarcToKoha to accept a hashref (15.54 KB, patch)
2022-05-24 14:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30813: Adjust TransformMarcToKoha to take kohafields parameter (2.44 KB, patch)
2022-05-24 14:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30813: Remove TransformMarcToKohaOneField (12.15 KB, patch)
2022-05-24 14:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30813: Remove Koha::MetadataRecord::getKohaField (1.05 KB, patch)
2022-05-24 14:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30813: (follow-up) ImportBatch updates (1.41 KB, patch)
2022-05-24 14:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30813: (follow-up) Correct C4::Breeding call (2.94 KB, patch)
2022-05-26 11:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30813: Update TransformMarcToKoha to accept a hashref (15.69 KB, patch)
2022-05-26 12:15 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 30813: Adjust TransformMarcToKoha to take kohafields parameter (2.59 KB, patch)
2022-05-26 12:15 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 30813: Remove TransformMarcToKohaOneField (12.29 KB, patch)
2022-05-26 12:16 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 30813: Remove Koha::MetadataRecord::getKohaField (1.20 KB, patch)
2022-05-26 12:16 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 30813: (follow-up) ImportBatch updates (1.55 KB, patch)
2022-05-26 12:16 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 30813: (follow-up) Correct C4::Breeding call (3.08 KB, patch)
2022-05-26 12:16 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 30813: Update TransformMarcToKoha to accept a hashref (15.78 KB, patch)
2022-06-07 12:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30813: Adjust TransformMarcToKoha to take kohafields parameter (2.68 KB, patch)
2022-06-07 12:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30813: Remove TransformMarcToKohaOneField (12.39 KB, patch)
2022-06-07 13:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30813: Remove Koha::MetadataRecord::getKohaField (1.29 KB, patch)
2022-06-07 13:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30813: (follow-up) ImportBatch updates (1.65 KB, patch)
2022-06-07 13:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30813: (follow-up) Correct C4::Breeding call (3.18 KB, patch)
2022-06-07 13:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30813: (QA follow-up) Adjust three use statements (1.69 KB, patch)
2022-06-07 13:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30813: (QA follow-up) Adjust three use statements (1.87 KB, patch)
2022-06-08 12:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30813: (follow-up) Fix tests import (757 bytes, patch)
2022-06-08 16:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2022-05-19 19:29:40 UTC
While testing bug 30789 it was found that passing the already retrieved marc subfield structure into TransformMarckToKohaOneField increased performance by ~20%

This seemed odd as the routine is already retrieving from cache unsafe

Rather than pass around this shared variable, it seems to make sense to allow TransformMarcToKoha to take a list of fields to process, rather than have a seperate routine that must be called
Comment 1 Nick Clemens 2022-05-20 10:24:06 UTC
Created attachment 135232 [details] [review]
Bug 30813: Update TransformMarcToKoha to accept a hashref

This patch updates all the calls to pass a hasref rather than an array

It also removes the no longer used framework parameter

To test:
prove -v t/Biblio.t t/db_dependent/Biblio/TransformMarcToKoha.t
Comment 2 Nick Clemens 2022-05-20 10:24:09 UTC
Created attachment 135233 [details] [review]
Bug 30813: Adjust TransformMarcToKoha to take kohafields parameter

This adjusts the routine to accept an arrayref of koha fields to process

To test:
prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/Biblio.t
Comment 3 Nick Clemens 2022-05-20 10:24:12 UTC
Created attachment 135234 [details] [review]
Bug 30813: Remove TransformMarcToKohaOneField

To test:
1 - prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/db_dependent/Breeding.t
2 - Test Z3950 search in advanced catalog editor
Comment 4 Nick Clemens 2022-05-20 10:24:15 UTC
Created attachment 135235 [details] [review]
Bug 30813: Remove Koha::MetadataRecord::getKohaField

This was essentially a wrapper around TransformKohaToMarcOneField

After changing and updating, it seems easier not to create the intermediary
object and to just transform the data
Comment 5 Martin Renvoize 2022-05-20 11:17:16 UTC
Created attachment 135239 [details] [review]
Bug 30813: Update TransformMarcToKoha to accept a hashref

This patch updates all the calls to pass a hasref rather than an array

It also removes the no longer used framework parameter

To test:
prove -v t/Biblio.t t/db_dependent/Biblio/TransformMarcToKoha.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2022-05-20 11:17:20 UTC
Created attachment 135240 [details] [review]
Bug 30813: Adjust TransformMarcToKoha to take kohafields parameter

This adjusts the routine to accept an arrayref of koha fields to process

To test:
prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/Biblio.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2022-05-20 11:17:24 UTC
Created attachment 135241 [details] [review]
Bug 30813: Remove TransformMarcToKohaOneField

To test:
1 - prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/db_dependent/Breeding.t
2 - Test Z3950 search in advanced catalog editor

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2022-05-20 11:17:28 UTC
Created attachment 135242 [details] [review]
Bug 30813: Remove Koha::MetadataRecord::getKohaField

This was essentially a wrapper around TransformKohaToMarcOneField

After changing and updating, it seems easier not to create the intermediary
object and to just transform the data

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2022-05-20 11:18:38 UTC
This works great and serves as another nice cleanup.

QA script is happy, all unit tests for biblios and items are passing great.

Signing off.
Comment 10 Nick Clemens 2022-05-20 11:27:43 UTC
Created attachment 135243 [details] [review]
Bug 30813: (follow-up) ImportBatch updates

Missed these in the original commits
Comment 11 Martin Renvoize 2022-05-24 14:19:23 UTC
Created attachment 135302 [details] [review]
Bug 30813: Update TransformMarcToKoha to accept a hashref

This patch updates all the calls to pass a hasref rather than an array

It also removes the no longer used framework parameter

To test:
prove -v t/Biblio.t t/db_dependent/Biblio/TransformMarcToKoha.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2022-05-24 14:19:27 UTC
Created attachment 135303 [details] [review]
Bug 30813: Adjust TransformMarcToKoha to take kohafields parameter

This adjusts the routine to accept an arrayref of koha fields to process

To test:
prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/Biblio.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2022-05-24 14:19:31 UTC
Created attachment 135304 [details] [review]
Bug 30813: Remove TransformMarcToKohaOneField

To test:
1 - prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/db_dependent/Breeding.t
2 - Test Z3950 search in advanced catalog editor

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2022-05-24 14:19:35 UTC
Created attachment 135305 [details] [review]
Bug 30813: Remove Koha::MetadataRecord::getKohaField

This was essentially a wrapper around TransformKohaToMarcOneField

After changing and updating, it seems easier not to create the intermediary
object and to just transform the data

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2022-05-24 14:19:39 UTC
Created attachment 135306 [details] [review]
Bug 30813: (follow-up) ImportBatch updates

Missed these in the original commits

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Joonas Kylmälä 2022-05-26 10:29:37 UTC
If you go to

> /cgi-bin/koha/cataloguing/addbooks.pl

and perform a search using "New from Z39.50" you get:

> Undefined subroutine &C4::Breeding::TransformMarcToKoha called at /kohadevbox/koha/C4/Breeding.pm line 311

to reproduce the error in the search pop-up put as the Title "aa". If you fix the missing import then the search results are still empty however (they should not be empty for this keyword), so there is a bit more to fix this, I will leave it to the author of the patch set.

- Koha/MetaSearcher.pm doesn't import C4::Biblio
- Koha/MetadataRecord.pm imports now C4::Biblio unnecessarily
Comment 17 Nick Clemens 2022-05-26 11:58:48 UTC
Created attachment 135371 [details] [review]
Bug 30813: (follow-up) Correct C4::Breeding call

This patch changes the call to use the fully qualified name and
adds import to C4::Breeding, Koha::MetaSearcher, and removes import
from Koha::MetadataRecord

Additionally it replaces missing fields from the update to using TransformMarcToKoha

Lastly, it reduces the fields used when saving the bredding record to the reservoir
Comment 18 Joonas Kylmälä 2022-05-26 12:15:52 UTC
Created attachment 135374 [details] [review]
Bug 30813: Update TransformMarcToKoha to accept a hashref

This patch updates all the calls to pass a hasref rather than an array

It also removes the no longer used framework parameter

To test:
prove -v t/Biblio.t t/db_dependent/Biblio/TransformMarcToKoha.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 19 Joonas Kylmälä 2022-05-26 12:15:56 UTC
Created attachment 135375 [details] [review]
Bug 30813: Adjust TransformMarcToKoha to take kohafields parameter

This adjusts the routine to accept an arrayref of koha fields to process

To test:
prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/Biblio.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 20 Joonas Kylmälä 2022-05-26 12:16:00 UTC
Created attachment 135376 [details] [review]
Bug 30813: Remove TransformMarcToKohaOneField

To test:
1 - prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/db_dependent/Breeding.t
2 - Test Z3950 search in advanced catalog editor

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 21 Joonas Kylmälä 2022-05-26 12:16:04 UTC
Created attachment 135377 [details] [review]
Bug 30813: Remove Koha::MetadataRecord::getKohaField

This was essentially a wrapper around TransformKohaToMarcOneField

After changing and updating, it seems easier not to create the intermediary
object and to just transform the data

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 22 Joonas Kylmälä 2022-05-26 12:16:09 UTC
Created attachment 135378 [details] [review]
Bug 30813: (follow-up) ImportBatch updates

Missed these in the original commits

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 23 Joonas Kylmälä 2022-05-26 12:16:13 UTC
Created attachment 135379 [details] [review]
Bug 30813: (follow-up) Correct C4::Breeding call

This patch changes the call to use the fully qualified name and
adds import to C4::Breeding, Koha::MetaSearcher, and removes import
from Koha::MetadataRecord

Additionally it replaces missing fields from the update to using TransformMarcToKoha

Lastly, it reduces the fields used when saving the bredding record to the reservoir

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 24 Marcel de Rooy 2022-05-26 12:56:43 UTC
RM: I like to have another look at this one too still.
Comment 25 Marcel de Rooy 2022-06-03 09:46:33 UTC
Taking a look here too
Comment 26 Marcel de Rooy 2022-06-03 09:56:07 UTC Comment hidden (obsolete)
Comment 27 Marcel de Rooy 2022-06-03 09:57:09 UTC Comment hidden (obsolete)
Comment 28 Marcel de Rooy 2022-06-03 10:01:46 UTC
Coming back here very soon. Please keep status.
Comment 29 Tomás Cohen Arazi 2022-06-06 16:56:41 UTC
(In reply to Marcel de Rooy from comment #28)
> Coming back here very soon. Please keep status.

Ok, do you have an idea of the timing for this one?
Comment 30 Marcel de Rooy 2022-06-07 11:07:10 UTC
(In reply to Tomás Cohen Arazi from comment #29)
> (In reply to Marcel de Rooy from comment #28)
> > Coming back here very soon. Please keep status.
> 
> Ok, do you have an idea of the timing for this one?

Will be going further this afternoon.
Comment 31 Marcel de Rooy 2022-06-07 12:59:54 UTC
Created attachment 135763 [details] [review]
Bug 30813: Update TransformMarcToKoha to accept a hashref

This patch updates all the calls to pass a hasref rather than an array

It also removes the no longer used framework parameter

To test:
prove -v t/Biblio.t t/db_dependent/Biblio/TransformMarcToKoha.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2022-06-07 12:59:58 UTC
Created attachment 135764 [details] [review]
Bug 30813: Adjust TransformMarcToKoha to take kohafields parameter

This adjusts the routine to accept an arrayref of koha fields to process

To test:
prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/Biblio.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 33 Marcel de Rooy 2022-06-07 13:00:03 UTC
Created attachment 135765 [details] [review]
Bug 30813: Remove TransformMarcToKohaOneField

To test:
1 - prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/db_dependent/Breeding.t
2 - Test Z3950 search in advanced catalog editor

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Marcel de Rooy 2022-06-07 13:00:08 UTC
Created attachment 135766 [details] [review]
Bug 30813: Remove Koha::MetadataRecord::getKohaField

This was essentially a wrapper around TransformKohaToMarcOneField

After changing and updating, it seems easier not to create the intermediary
object and to just transform the data

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Marcel de Rooy 2022-06-07 13:00:13 UTC
Created attachment 135767 [details] [review]
Bug 30813: (follow-up) ImportBatch updates

Missed these in the original commits

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Marcel de Rooy 2022-06-07 13:00:18 UTC
Created attachment 135768 [details] [review]
Bug 30813: (follow-up) Correct C4::Breeding call

This patch changes the call to use the fully qualified name and
adds import to C4::Breeding, Koha::MetaSearcher, and removes import
from Koha::MetadataRecord

Additionally it replaces missing fields from the update to using TransformMarcToKoha

Lastly, it reduces the fields used when saving the bredding record to the reservoir

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 37 Marcel de Rooy 2022-06-07 13:00:22 UTC
Created attachment 135769 [details] [review]
Bug 30813: (QA follow-up) Adjust three use statements

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 38 Marcel de Rooy 2022-06-07 13:03:24 UTC
Great work, Nick. Removing that framework parameter was still something far down on my todo list ;) Might still be a report for; will close it.
Comment 39 Nick Clemens 2022-06-07 17:43:29 UTC Comment hidden (obsolete)
Comment 40 Marcel de Rooy 2022-06-08 11:05:11 UTC Comment hidden (obsolete)
Comment 41 Nick Clemens 2022-06-08 11:34:35 UTC Comment hidden (obsolete)
Comment 42 Marcel de Rooy 2022-06-08 11:40:47 UTC Comment hidden (obsolete)
Comment 43 Jonathan Druart 2022-06-08 11:55:14 UTC
Please explicitly import the subroutines that are used in the scripts/modules, that's the point of bug 17600. That helps to clean the import (remove the 'use' statements when no subroutines are imported).
Comment 44 Marcel de Rooy 2022-06-08 11:56:20 UTC Comment hidden (obsolete)
Comment 45 Jonathan Druart 2022-06-08 12:09:10 UTC Comment hidden (obsolete)
Comment 46 Marcel de Rooy 2022-06-08 12:12:55 UTC Comment hidden (obsolete)
Comment 47 Marcel de Rooy 2022-06-08 12:31:12 UTC
Ok see IRC for a lively discussion on how 17600 was meant ;)

The implications here are:
The last patch that 'corrected' a few use statements needs tweaking (or its predecessors).
Although we do not need to import TransformMarcToKoha there since we call with prefix C4::Biblio, we should do it anyway for transparency/documentation etc.

Will adjust the patch.
Comment 48 Marcel de Rooy 2022-06-08 12:34:47 UTC
Created attachment 135803 [details] [review]
Bug 30813: (QA follow-up) Adjust three use statements

Altough we used the namespace when calling, the intention of
bug 17600 was that we do import the routines that we are
calling here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 49 Tomás Cohen Arazi 2022-06-08 14:51:04 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 50 Tomás Cohen Arazi 2022-06-08 16:41:56 UTC
Created attachment 135825 [details] [review]
Bug 30813: (follow-up) Fix tests import

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>