Bug 15869 - Change framework on overlay
Summary: Change framework on overlay
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal with 15 votes (vote)
Assignee: Nick Clemens
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on:
Blocks: 33412
  Show dependency treegraph
 
Reported: 2016-02-19 19:44 UTC by Nicole C. Engard
Modified: 2023-10-28 21:14 UTC (History)
13 users (show)

See Also:
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:
23.05.00,22.11.03


Attachments
Bug 15869 - change framework on overlay (4.31 KB, patch)
2016-09-13 15:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15869 - change framework on overlay (3.93 KB, patch)
2018-01-22 16:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15869: Change framework on overlay (8.12 KB, patch)
2018-07-16 17:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15869: Change framework on overlay (8.96 KB, patch)
2022-09-22 19:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15869: Unit test (2.64 KB, patch)
2022-09-22 19:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15869: Change framework on overlay (9.02 KB, patch)
2022-10-20 14:46 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 15869: Unit test (2.70 KB, patch)
2022-10-20 14:46 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 15869: (follow-up) Don't mock undef return value (1.00 KB, patch)
2022-11-02 19:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15869: Change framework on overlay (9.08 KB, patch)
2023-01-13 13:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15869: Unit test (2.76 KB, patch)
2023-01-13 13:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15869: (follow-up) Don't mock undef return value (1.05 KB, patch)
2023-01-13 13:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15869: (QA follow-up) Add some padding around the pulldowns (4.25 KB, patch)
2023-01-13 13:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15869: (QA follow-up) Don't pass placeholder framework '_USE_ORIG' (1.39 KB, patch)
2023-01-18 19:58 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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
Comment 21 Tomás Cohen Arazi 2023-02-07 13:49:51 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 22 Jacob O'Mara 2023-02-08 16:37:21 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 23 Lucas Gass 2023-02-15 21:29:56 UTC
Doesn't apply cleanly to 22.05.x, no backport. If needed please rebase.
Comment 24 Holly 2023-02-24 10:38:09 UTC
*** Bug 24843 has been marked as a duplicate of this bug. ***
Comment 25 David Roberts 2023-07-27 16:36:38 UTC
I'm pretty sure that 

$overlay_framework = undef if $overlay_framework = '_USE_ORIG';

needs to be 

$overlay_framework = undef if $overlay_framework == '_USE_ORIG';

The import fails until this is changed.
Comment 26 Katrin Fischer 2023-10-28 21:14:19 UTC
(In reply to David Roberts from comment #25)
> I'm pretty sure that 
> 
> $overlay_framework = undef if $overlay_framework = '_USE_ORIG';
> 
> needs to be 
> 
> $overlay_framework = undef if $overlay_framework == '_USE_ORIG';
> 
> The import fails until this is changed.

I believe this was fixed on another bug report.