Bug 35115 - ERM - Potential MARC data loss when importing titles from list
Summary: ERM - Potential MARC data loss when importing titles from list
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ERM (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical (vote)
Assignee: Pedro Amorim
QA Contact:
URL:
Keywords:
: 34195 (view as bug list)
Depends on:
Blocks:
 
Reported: 2023-10-20 10:19 UTC by Pedro Amorim
Modified: 2024-04-17 09:04 UTC (History)
10 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:
23.11.00


Attachments
Bug 35115: Prefix biblio hash with table name (2.30 KB, patch)
2023-10-20 11:57 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35115 [Alternative]: Skip Resource store logic if resource was imported from list (2.67 KB, patch)
2023-10-30 16:32 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35115: Move store routine from Resource to Title and update code (7.48 KB, patch)
2023-11-08 20:20 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35115: Move store routine from Resource to Title and update code (8.09 KB, patch)
2023-11-09 16:13 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35115: Move store routine from Resource to Title and update code (8.15 KB, patch)
2023-11-10 15:20 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35115: (QA follow-up): (3.97 KB, patch)
2023-11-10 15:20 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35115: Move store routine from Resource to Title and update code (8.20 KB, patch)
2023-11-26 05:19 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35115: (QA follow-up): (4.02 KB, patch)
2023-11-26 05:19 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2023-10-20 10:19:31 UTC
To reproduce:
1) Enable ERMModule sys pref
2) Create a new public list, visit:
/cgi-bin/koha/virtualshelves/shelves.pl
3) Click "New list" enter name, set public -> public
4) Click "Add items", enter 112\n113\n114 (new line for each), in "Biblio numbers"
5) Notice that all added biblios have quite a few MARC fields
6) Go to packages, visit:
/cgi-bin/koha/erm/eholdings/local/packages
7) Create a new package, add a name and hit 'Submit'
8) Go to titles, visit:
/cgi-bin/koha/erm/eholdings/local/titles
9) Click "import from list"
10) Pick the package created in 7)
11) On the row of the list created in 2), click "Import"
12) Go back to the list, visit:
/cgi-bin/koha/virtualshelves/shelves.pl?op=view&shelfnumber=1
13) Notice all the biblios have been stripped of their MARC data, and only title remains.

Setting this as 'major' for now as I don't think this is intended or desired behavour.
Comment 1 Pedro Amorim 2023-10-20 11:57:00 UTC
Created attachment 157507 [details] [review]
Bug 35115: Prefix biblio hash with table name

It seems TransformKohaToMarc requires the input to be prefixed with the table name, like this:
    {
          'biblio.abstract' => undef,
          'biblio.subtitle' => undef,
          'biblio.frameworkcode' => 'BKS',
          'biblio.medium' => undef,
          'biblio.part_name' => undef,
          'biblio.copyrightdate' => 2008,
          'biblio.seriestitle' => undef,
          'biblio.notes' => 'Originally published: 2008.',
          'biblio.biblionumber' => 299,
          'biblio.serial' => undef,
          'biblio.title' => 'Kluge :',
          'biblio.datecreated' => '2014-05-07',
          'biblio.part_number' => undef,
          'biblio.author' => 'Marcus, Gary F.',
          'biblio.timestamp' => '2020-04-21 09:38:26',
          'biblio.unititle' => undef
    };

but we were using this:

    {
          'abstract' => undef,
          'notes' => 'Originally published: 2008.',
          'medium' => undef,
          'subtitle' => undef,
          'title' => 'Kluge :',
          'author' => 'Marcus, Gary F.',
          'serial' => undef,
          'timestamp' => '2020-04-21 09:38:26',
          'biblionumber' => 299,
          'part_name' => undef,
          'datecreated' => '2014-05-07',
          'part_number' => undef,
          'seriestitle' => undef,
          'unititle' => undef,
          'frameworkcode' => 'BKS',
          'copyrightdate' => 2008
    };

ATTENTION: This does not solve the whole issue, i.e. MARC fields not mapped to Koha fields will still be stripped.
More work required.
Comment 2 Pedro Amorim 2023-10-20 11:57:38 UTC
@Jonathan Druart could use your help here!
Comment 3 Pedro Amorim 2023-10-20 12:04:46 UTC
This should depend on bug 35095 or vice-versa, whichever moves first.
Comment 4 Jonathan Druart 2023-10-24 14:20:56 UTC
ok for me, please provide tests.

Also you can replace the loop with

  $biblio = { map { ( "biblio.$_" => $biblio->{$_} ) } keys %$biblio };
Comment 5 Pedro Amorim 2023-10-24 14:44:17 UTC
*** Bug 34195 has been marked as a duplicate of this bug. ***
Comment 6 Pedro Amorim 2023-10-24 14:48:53 UTC
(In reply to Jonathan Druart from comment #4)
> ok for me, please provide tests.
> 
> Also you can replace the loop with
> 
>   $biblio = { map { ( "biblio.$_" => $biblio->{$_} ) } keys %$biblio };

Thank you for taking a look Jonathan, this is great.
I'm not sure when I'll be able to get back to this, but there is still the matter of:

ATTENTION: This does not solve the whole issue, i.e. MARC fields not mapped to Koha fields will still be stripped.
More work required.
Comment 7 Pedro Amorim 2023-10-30 16:32:57 UTC
Created attachment 158085 [details] [review]
Bug 35115 [Alternative]: Skip Resource store logic if resource was imported from list

This patch fixes the data loss issue when importing titles from a list
of biblios. It does NOT fix the data loss issue when saving a title that
is linked to a biblio, that will still caused data loss on the biblio.

I believe store in Resource.pm is trying to do a couple things:
1) Updating Biblio when a title is saved, or
2) Adding a biblio when a title is created

The problem is that it's updating the biblio when the title is created
from an import list, as if new changes were made to the title and need
to be reflected to the biblio.
This is not the case, a title created from an imported list of biblios
should have no business updating their respective biblio's MARC at that time.

----

The biblio MARC being updated when the title is saved through the edit form
should perhaps be a different bug. Should all possible fields be updated on the biblio when saving the
title?
Comment 8 Nick Clemens (kidclamp) 2023-11-08 17:42:44 UTC
Raising to critical - this causes severe data loss
Comment 9 Nick Clemens (kidclamp) 2023-11-08 20:20:43 UTC Comment hidden (obsolete)
Comment 10 Nick Clemens (kidclamp) 2023-11-08 20:27:13 UTC
Talked to Pedro, trying my hand here.

I found that the routine was really only handling Title changes, not resource, so I moved the code over to the module and added tests. Before this, an update to the title that didn't pass the resources did not trigger the changes, this felt incorrect.


We will need to handle more fields on bug 35095, but this should fix data loss both when adding from a list, or editing an individual title

NOTE: Changing the title of a biblio record will not be reflected in the eHoldings title - this may be another bug
Comment 11 Nick Clemens (kidclamp) 2023-11-09 16:13:50 UTC
Created attachment 158748 [details] [review]
Bug 35115: Move store routine from Resource to Title and update code

These patches move the store routine from Koha::ERM::EHoldings::Resource to Koha::ERM::EHoldings::Title as the code deals exclusively with title fields.

It updates the code to ensure that records are created when a title is not attched to a biblio, and that only the biblio title field is updated when updating an eholdings title.

To test:
 1 - Enable ERMModule sys pref
 2 - Create a new public list, visit:
     /cgi-bin/koha/virtualshelves/shelves.pl
 3 - Click "New list" enter name, set public -> public
 4 - Click "Add items", enter 112\n113\n114 (new line for each), in "Biblio numbers"
 5 - Notice that all added biblios have quite a few MARC fields
 6 - Go to packages, visit:
     /cgi-bin/koha/erm/eholdings/local/packages
 7 - Create a new package, add a name and hit 'Submit'
 8 - Go to titles, visit:
     /cgi-bin/koha/erm/eholdings/local/titles
 9 - Click "import from list"
10 - Pick the package created in 7)
11 - On the row of the list created in 2), click "Import"
12 - Go back to the list, visit:
     /cgi-bin/koha/virtualshelves/shelves.pl?op=view&shelfnumber=1
13 - Notice all the biblios have been stripped of their MARC data, and only title remains.
14 - Go to details page for one of the bibs
15 - Edit -> Replace record via Z39.50
16 - You can search for anything, just make sure to import a mostly full record
17 - Go back to ERM - eHoldings - Local - Titles
18 - Edit the title for the record that was replaced
19 - Save
20 - Go to record details - note the record is gone again
21 - Apply patches
22 - Search the catalog
23 - Check some titles
24 - Add to a new list
25 - repeat 8 - 11 with the new list
26 - View and confirm records are intact
27 - Edit the eholdings title for one of the records, changing the title field
28 - Save and view record details
29 - Confirm record is intact and title updated


Current status: Needs Signoff
Comment 12 Pedro Amorim 2023-11-10 15:20:25 UTC
Created attachment 158806 [details] [review]
Bug 35115: Move store routine from Resource to Title and update code

These patches move the store routine from Koha::ERM::EHoldings::Resource to Koha::ERM::EHoldings::Title as the code deals exclusively with title fields.

It updates the code to ensure that records are created when a title is not attched to a biblio, and that only the biblio title field is updated when updating an eholdings title.

To test:
 1 - Enable ERMModule sys pref
 2 - Create a new public list, visit:
     /cgi-bin/koha/virtualshelves/shelves.pl
 3 - Click "New list" enter name, set public -> public
 4 - Click "Add items", enter 112\n113\n114 (new line for each), in "Biblio numbers"
 5 - Notice that all added biblios have quite a few MARC fields
 6 - Go to packages, visit:
     /cgi-bin/koha/erm/eholdings/local/packages
 7 - Create a new package, add a name and hit 'Submit'
 8 - Go to titles, visit:
     /cgi-bin/koha/erm/eholdings/local/titles
 9 - Click "import from list"
10 - Pick the package created in 7)
11 - On the row of the list created in 2), click "Import"
12 - Go back to the list, visit:
     /cgi-bin/koha/virtualshelves/shelves.pl?op=view&shelfnumber=1
13 - Notice all the biblios have been stripped of their MARC data, and only title remains.
14 - Go to details page for one of the bibs
15 - Edit -> Replace record via Z39.50
16 - You can search for anything, just make sure to import a mostly full record
17 - Go back to ERM - eHoldings - Local - Titles
18 - Edit the title for the record that was replaced
19 - Save
20 - Go to record details - note the record is gone again
21 - Apply patches
22 - Search the catalog
23 - Check some titles
24 - Add to a new list
25 - repeat 8 - 11 with the new list
26 - View and confirm records are intact
27 - Edit the eholdings title for one of the records, changing the title field
28 - Save and view record details
29 - Confirm record is intact and title updated

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 13 Pedro Amorim 2023-11-10 15:20:29 UTC
Created attachment 158807 [details] [review]
Bug 35115: (QA follow-up):

Spelling.
Tidyness.
Removal of leftover warn

Nick's patch fixes the issue as described, and I agree that it is the ideal solution here.
prove t/db_dependent/api/v1/erm_eholdings* passes
prove t/db_dependent/Koha/BackgroundJob/CreateEHoldingsFromBiblios.t passes

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 14 Victor Grousset/tuxayo 2023-11-24 20:25:14 UTC
Is comment 6 still valid?

> ATTENTION: This does not solve the whole issue, i.e.
> MARC fields not mapped to Koha fields will still be stripped.
Comment 15 Nick Clemens (kidclamp) 2023-11-24 20:39:36 UTC
(In reply to Victor Grousset/tuxayo from comment #14)
> Is comment 6 still valid?
> 
> > ATTENTION: This does not solve the whole issue, i.e.
> > MARC fields not mapped to Koha fields will still be stripped.

No, the most recent patch should address this
Comment 16 Victor Grousset/tuxayo 2023-11-25 21:26:53 UTC
Great :D "important" tag removed then.
Comment 17 Victor Grousset/tuxayo 2023-11-26 05:17:54 UTC
> 27 - Edit the eholdings title for one of the records, changing the title field
> 28 - Save and view record details
> 29 - Confirm record is intact and title updated

Just to be sure, title update should only be from eholding to record, right? And not from record to eholding.
Comment 18 Victor Grousset/tuxayo 2023-11-26 05:19:45 UTC
Created attachment 159254 [details] [review]
Bug 35115: Move store routine from Resource to Title and update code

These patches move the store routine from Koha::ERM::EHoldings::Resource to Koha::ERM::EHoldings::Title as the code deals exclusively with title fields.

It updates the code to ensure that records are created when a title is not attched to a biblio, and that only the biblio title field is updated when updating an eholdings title.

To test:
 1 - Enable ERMModule sys pref
 2 - Create a new public list, visit:
     /cgi-bin/koha/virtualshelves/shelves.pl
 3 - Click "New list" enter name, set public -> public
 4 - Click "Add items", enter 112\n113\n114 (new line for each), in "Biblio numbers"
 5 - Notice that all added biblios have quite a few MARC fields
 6 - Go to packages, visit:
     /cgi-bin/koha/erm/eholdings/local/packages
 7 - Create a new package, add a name and hit 'Submit'
 8 - Go to titles, visit:
     /cgi-bin/koha/erm/eholdings/local/titles
 9 - Click "import from list"
10 - Pick the package created in 7)
11 - On the row of the list created in 2), click "Import"
12 - Go back to the list, visit:
     /cgi-bin/koha/virtualshelves/shelves.pl?op=view&shelfnumber=1
13 - Notice all the biblios have been stripped of their MARC data, and only title remains.
14 - Go to details page for one of the bibs
15 - Edit -> Replace record via Z39.50
16 - You can search for anything, just make sure to import a mostly full record
17 - Go back to ERM - eHoldings - Local - Titles
18 - Edit the title for the record that was replaced
19 - Save
20 - Go to record details - note the record is gone again
21 - Apply patches
22 - Search the catalog
23 - Check some titles
24 - Add to a new list
25 - repeat 8 - 11 with the new list
26 - View and confirm records are intact
27 - Edit the eholdings title for one of the records, changing the title field
28 - Save and view record details
29 - Confirm record is intact and title updated

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 19 Victor Grousset/tuxayo 2023-11-26 05:19:48 UTC
Created attachment 159255 [details] [review]
Bug 35115: (QA follow-up):

Spelling.
Tidyness.
Removal of leftover warn

Nick's patch fixes the issue as described, and I agree that it is the ideal solution here.
prove t/db_dependent/api/v1/erm_eholdings* passes
prove t/db_dependent/Koha/BackgroundJob/CreateEHoldingsFromBiblios.t passes

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 20 Victor Grousset/tuxayo 2023-11-26 05:20:39 UTC
In the hope to help, here is a 2nd signoff.
Comment 21 Pedro Amorim 2023-11-27 11:15:37 UTC
(In reply to Victor Grousset/tuxayo from comment #17)
> > 27 - Edit the eholdings title for one of the records, changing the title field
> > 28 - Save and view record details
> > 29 - Confirm record is intact and title updated
> 
> Just to be sure, title update should only be from eholding to record, right?
> And not from record to eholding.

Correct, yes.

I'm converting my SO to PQA here.
Comment 22 Victor Grousset/tuxayo 2023-11-28 20:54:18 UTC
(In reply to Pedro Amorim from comment #21)
> (In reply to Victor Grousset/tuxayo from comment #17)
> > > 27 - Edit the eholdings title for one of the records, changing the title field
> > > 28 - Save and view record details
> > > 29 - Confirm record is intact and title updated
> > 
> > Just to be sure, title update should only be from eholding to record, right?
> > And not from record to eholding.
> 
> Correct, yes.

Great, no issues then.


> I'm converting my SO to PQA here.

Perfect, that was the plan :)
Comment 23 Tomás Cohen Arazi 2023-11-29 20:39:24 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!