Bug 15869

Summary: Change framework on overlay
Product: Koha Reporter: Nicole C. Engard <nengard>
Component: CatalogingAssignee: Nick Clemens <nick>
Status: Passed QA --- QA Contact: Kyle M Hall <kyle>
Severity: normal    
Priority: P5 - low CC: andrewfh, emjhet, jzairo, katrin.fischer, kyle, m.de.rooy, marjorie.barry-vila, martin.renvoize, mtompset, nick, sbcornell
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This change fixes a long-standing bug where the framework specified during import only applied to new records and not overlaid matches.
Version(s) released in:
Attachments: Bug 15869 - change framework on overlay
Bug 15869 - change framework on overlay
Bug 15869: Change framework on overlay
Bug 15869: Change framework on overlay
Bug 15869: Unit test
Bug 15869: Change framework on overlay
Bug 15869: Unit test
Bug 15869: (follow-up) Don't mock undef return value
Bug 15869: Change framework on overlay
Bug 15869: Unit test
Bug 15869: (follow-up) Don't mock undef return value
Bug 15869: (QA follow-up) Add some padding around the pulldowns
Bug 15869: (QA follow-up) Don't pass placeholder framework '_USE_ORIG'

Description Nicole C. Engard 2016-02-19 19:44:09 UTC
When you stage files for import you can choose how to handle matches.  One option is to replace the existing record.  The problem is that many libraries use this option to load a fuller record over a quick or fast record and the framework doesn't change.  So you can choose the framework before importing but that framework only applies to new records, it doesn't update records that are being replaced or overlaid.  We should fix that to update the framework for all items, not just new ones.

Nicole
Comment 1 Nick Clemens 2016-09-13 15:28:35 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2016-09-13 15:29:47 UTC
Needs unit tests - wanted comments on approach first.
Comment 3 Katrin Fischer 2017-10-07 22:05:08 UTC Comment hidden (obsolete)
Comment 4 Nick Clemens 2018-01-22 16:02:16 UTC Comment hidden (obsolete)
Comment 5 Mark Tompsett 2018-01-31 02:13:47 UTC
I don't think this is Academy, because line 567 of C4/ImportBatch.pm in master is probably the wrong condition. I think it should be >= 1, not ==1. Read lines 251/252 of tools/manage-marc-import.pl where 4 parameters are passed. This means adding a parameter in the middle is messed up. If the code is cleaned up, then perhaps, but I'm marking this Failed QA, because it reads awful and probably explains why imports are flaky sometimes. Also, it fails to meet the "must have tests" coding guidelines.
Comment 6 emjhet 2018-05-30 20:13:14 UTC
This is an issue with overlaying any record, not just staged records. 

I should be able to select that all Imported records are imported in a specific Framework, whether imported singularly or as a batch, and whether overlaid onto a brief order record or as the first bibliographic content. 

Indeed, libraries should be able to select any of their Frameworks as THEIR default Framework, instead of having to use the Koha Default.
Comment 7 Nick Clemens 2018-07-16 17:22:23 UTC
Created attachment 77018 [details] [review]
Bug 15869: Change framework on overlay

This patch allows for selection of framework to use when overlaying
records - by default it is set to keep the initial framework

To test:
 1 - Create some records using one framework
 2 - Export the records
 3 - Edit the records to add fields not in original framework
 4 - Stage records using a rule that will find matches
 5 - Import
 6 - Note records contain new fields on display, but they are lost on edit
 7 - Apply patch
 8 - Stage records again
 9 - Select a framework that contains the new fields on import
10 - Import records
11 - Note records now use selected framework and are displayed/edited
correctly
Comment 8 Nick Clemens 2018-07-16 17:24:38 UTC
Patches updated just to see.

I think the condition was right (it is checking if there are two variables left in the parameters) but agree it is ugly - changed to take a hash

It still needs unit tests, feel free to help :-)
Comment 9 Andrew Fuerste-Henry 2021-09-08 20:21:46 UTC
+1, this would be handy!
Comment 10 Nick Clemens 2022-09-22 19:15:57 UTC
Created attachment 140857 [details] [review]
Bug 15869: Change framework on overlay

This patch allows for selection of framework to use when overlaying
records - by default it is set to keep the initial framework

To test:
 1 - Create some records using one framework
 2 - Export the records
 3 - Edit the records to add fields not in original framework
 4 - Stage records using a rule that will find matches
 5 - Import
 6 - Note records contain new fields on display, but they are lost on edit
 7 - Apply patch
 8 - Stage records again
 9 - Select a framework that contains the new fields on import
10 - Import records
11 - Note records now use selected framework and are displayed/edited
correctly
Comment 11 Nick Clemens 2022-09-22 19:16:02 UTC
Created attachment 140858 [details] [review]
Bug 15869: Unit test
Comment 12 ByWater Sandboxes 2022-10-20 14:46:51 UTC
Created attachment 142257 [details] [review]
Bug 15869: Change framework on overlay

This patch allows for selection of framework to use when overlaying
records - by default it is set to keep the initial framework

To test:
 1 - Create some records using one framework
 2 - Export the records
 3 - Edit the records to add fields not in original framework
 4 - Stage records using a rule that will find matches
 5 - Import
 6 - Note records contain new fields on display, but they are lost on edit
 7 - Apply patch
 8 - Stage records again
 9 - Select a framework that contains the new fields on import
10 - Import records
11 - Note records now use selected framework and are displayed/edited
correctly

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 13 ByWater Sandboxes 2022-10-20 14:46:57 UTC
Created attachment 142258 [details] [review]
Bug 15869: Unit test

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 14 Katrin Fischer 2022-10-28 19:33:49 UTC
There is a new warning in the output of the unit test with this patch set applied:

t/db_dependent/ImportBatch.t .. 9/19 Use of uninitialized value $item_result in string eq at /kohadevbox/koha/C4/ImportBatch.pm line 672.
Use of uninitialized value $item_result in string eq at /kohadevbox/koha/C4/ImportBatch.pm line 672.

Can you please check?
Comment 15 Nick Clemens 2022-11-02 19:34:08 UTC
Created attachment 143033 [details] [review]
Bug 15869: (follow-up) Don't mock undef return value
Comment 16 Kyle M Hall 2023-01-13 13:24:32 UTC
Created attachment 145271 [details] [review]
Bug 15869: Change framework on overlay

This patch allows for selection of framework to use when overlaying
records - by default it is set to keep the initial framework

To test:
 1 - Create some records using one framework
 2 - Export the records
 3 - Edit the records to add fields not in original framework
 4 - Stage records using a rule that will find matches
 5 - Import
 6 - Note records contain new fields on display, but they are lost on edit
 7 - Apply patch
 8 - Stage records again
 9 - Select a framework that contains the new fields on import
10 - Import records
11 - Note records now use selected framework and are displayed/edited
correctly

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2023-01-13 13:24:45 UTC
Created attachment 145272 [details] [review]
Bug 15869: Unit test

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2023-01-13 13:24:50 UTC
Created attachment 145273 [details] [review]
Bug 15869: (follow-up) Don't mock undef return value

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Kyle M Hall 2023-01-13 13:24:55 UTC
Created attachment 145274 [details] [review]
Bug 15869: (QA follow-up) Add some padding around the pulldowns

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Nick Clemens 2023-01-18 19:58:16 UTC
Created attachment 145406 [details] [review]
Bug 15869: (QA follow-up) Don't pass placeholder framework '_USE_ORIG'

This param in the dropdown is meant to signify no change, distinct from
"" when means to set to default. In rebasing this condition was lost, this patch
restores undef of the variable to avoid setting framework to an invalid value

To test:
1 - Stage a record with a match (export and import a record)
2 - Attempt to import and overlay with option 'When replacing records use this framework:' 'Keep original framework'
3 - It fails, logs note 'value too long for field frameworkcode'
4 - Apply this patch
5 - Try again, it succeeds - framework is not changed