Bug 25539 - Remove AddBiblio "defer_marc_save" option
Summary: Remove AddBiblio "defer_marc_save" option
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P3 normal (vote)
Assignee: David Gustafsson
QA Contact: Martin Renvoize
URL:
Keywords:
: 35075 (view as bug list)
Depends on: 29440
Blocks: 30349 35075
  Show dependency treegraph
 
Reported: 2020-05-19 09:50 UTC by David Gustafsson
Modified: 2024-01-05 13:38 UTC (History)
8 users (show)

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


Attachments
Bug 25539: Remove AddBiblio option "defer_marc_save" (7.42 KB, patch)
2020-05-19 10:05 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Remove AddBiblio option "defer_marc_save" (6.97 KB, patch)
2020-05-19 10:08 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Remove AddBiblio option "defer_marc_save" (7.04 KB, patch)
2020-09-09 10:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Strip items when adding new biblio to preserve previous behaviour of the script (1.75 KB, patch)
2020-09-17 12:11 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script (1.71 KB, patch)
2020-09-17 12:13 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Remove AddBiblio option "defer_marc_save" (8.99 KB, patch)
2021-11-12 22:45 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script (1.48 KB, patch)
2021-11-12 22:45 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Remove AddBiblio option "defer_marc_save" (8.99 KB, patch)
2021-11-12 22:46 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script (1.48 KB, patch)
2021-11-12 22:46 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script (1.59 KB, patch)
2021-11-12 22:49 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Remove AddBiblio option "defer_marc_save" (8.86 KB, patch)
2021-11-24 13:46 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script (1.59 KB, patch)
2021-11-24 13:46 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Remove AddBiblio option "defer_marc_save" (4.87 KB, patch)
2021-11-24 14:32 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script (1.59 KB, patch)
2021-11-24 14:33 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Remove AddBiblio option "defer_marc_save" (5.69 KB, patch)
2021-11-24 15:36 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script (1.59 KB, patch)
2021-11-24 15:36 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Remove AddBiblio option "defer_marc_save" (6.04 KB, patch)
2022-04-06 14:05 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script (1.59 KB, patch)
2022-04-06 14:05 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Enable bulkmarcimport overlay context (1.03 KB, patch)
2022-04-06 14:05 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Remove AddBiblio option "defer_marc_save" (6.07 KB, patch)
2022-04-06 14:20 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script (1.59 KB, patch)
2022-04-06 14:20 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Enable bulkmarcimport overlay context (1.03 KB, patch)
2022-04-06 14:20 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Remove AddBiblio option "defer_marc_save" (5.46 KB, patch)
2022-09-26 15:20 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script (1.95 KB, patch)
2022-09-26 15:20 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Enable bulkmarcimport overlay context (1.03 KB, patch)
2022-09-26 15:20 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Remove AddBiblio option "defer_marc_save" (5.50 KB, patch)
2023-09-26 13:45 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script (1.95 KB, patch)
2023-09-26 13:45 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Enable bulkmarcimport overlay context (1.01 KB, patch)
2023-09-26 13:45 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Remove AddBiblio option "defer_marc_save" (5.59 KB, patch)
2023-12-20 18:46 UTC, M
Details | Diff | Splinter Review
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script (2.04 KB, patch)
2023-12-20 18:46 UTC, M
Details | Diff | Splinter Review
Bug 25539: Enable bulkmarcimport overlay context (1.10 KB, patch)
2023-12-20 18:46 UTC, M
Details | Diff | Splinter Review
Bug 25539: Remove AddBiblio option "defer_marc_save" (5.15 KB, patch)
2024-01-03 16:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script (2.16 KB, patch)
2024-01-03 16:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25539: Enable bulkmarcimport overlay context (1.23 KB, patch)
2024-01-03 16:38 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 David Gustafsson 2020-05-19 09:50:09 UTC
In bulkmarcimoprt.pl AddBiblio is called with defer_marc_save => 1. This perhaps made sense sometime when items where saved embedded with the MARCXML, but since that is no longer the case(?) there is no need for saving the MARC-data once more after processing items in bulkmarcimport.pl. Thus this option can/should be removed. The current behavior actually blocks bug 14957, we recently discovered, since calling 
ModBiblioMarc after the record has been saved bypasses marc merge rules on updates (which are applied in ModBiblio) and that save will ignore the possible rules that has been setup.

I also discovered some other odd things that will not address in this bug. One of the being that since ModBiblio runs _strip_item_fields no items will ever be imported though bulkmarcimport on updates (but will be in inserts). I'm pretty sure this is the case (have tested and verified, but seems odd this kind of bug has survived undetected for so long).
Comment 1 David Gustafsson 2020-05-19 10:05:34 UTC Comment hidden (obsolete)
Comment 2 David Gustafsson 2020-05-19 10:08:34 UTC Comment hidden (obsolete)
Comment 3 Victor Grousset/tuxayo 2020-05-31 21:20:03 UTC
I would like to try to test this. But I need a test plan as I don't know well enough this part of Koha to improvise one.

It can looks like this:
1. Step to prepare some stuff
2. Another step
3. Do something else that exposes the current issue
4. Apply the patch
5. Redo some of the above steps
6. See that the issue is gone
Comment 4 David Gustafsson 2020-06-22 08:48:27 UTC
Yes, I usually include a test-plan, this fix will not produce any change of behaviour with the current code, but causes a bug together with bug 14957. So in that case the test-plan would have to include applying that patch.
Comment 5 David Gustafsson 2020-06-22 08:49:51 UTC
Bad working that, this patch does not cause a bug together with bug 14957, but rather it resolves a bug with the current code base.
Comment 6 Katrin Fischer 2020-06-22 21:09:11 UTC
(In reply to David Gustafsson from comment #4)
> Yes, I usually include a test-plan, this fix will not produce any change of
> behaviour with the current code, but causes a bug together with bug 14957.
> So in that case the test-plan would have to include applying that patch.

No change in behaviour can still be tested with and without the patch. I think it would really help to get this moving and unlock bug 14957. But looking at the patch I get an idea of why this is hard. Some example uses might still be good to note and test.
Comment 7 Martin Renvoize 2020-09-09 10:59:56 UTC Comment hidden (obsolete)
Comment 8 Martin Renvoize 2020-09-09 11:02:49 UTC
It took a little getting my head around, but I believe this is all correct.

For reference, the commit adding the defer_marc_save option had the message:

====
item rework: replaced AddBiblioAndItems

Replace C4::Biblio::AddBiblioAndItems with two
things:

* An option to C4::Biblio::AddBiblio to defer writing
  biblioitems.marc and biblioitems.marcxml.  This
  option was created to give a significant
  speed boost to bulkmarcimport.pl, but is *not*
  recommended for general use.
* C4::Items::AddItemBatchFromMarc

This refactoring removes the need to have functions
in C4::Biblio and C4::Items that call each other's
private functions.

Signed-off-by: Chris Cormack <crc@liblime.com>
Signed-off-by: Joshua Ferraro <jmf@liblime.com>
====

I believe it is no defunk as David suggests. I tried a few runs of bulkmarcimport with and without the patch and found no really noticeable slowdown in performance (though performance of this script really isn't very good these days :( )

Signing off.
Comment 9 Nick Clemens 2020-09-11 13:09:15 UTC
Small things:
Please put change for ModBiblio in its own patch, appreciated, but not entirely related :-)

You have some stray whitespace changes

Blocker:
Before this patch the items are stripped before saving the marc. After this patch they are not.

To test:
1 - Export a record with items from Koha into a file: test.mrc
2 - perl misc/migration_tools/bulkmarcimport.pl -b -file test.mrc
3 - You will get error on adding items, but not important for this test
4 - Check the biblio_metadata table:
    SELECT metadata FROM biblio_metadata ORDER BY biblionumber DESC LIMIT 1\G
5 - Note no item fields
6 - Apply patch
7 - perl misc/migration_tools/bulkmarcimport.pl -b -file test.mrc
8 - Check the biblio_metadata table as above
9 - Item fields are stored in the db
Comment 10 David Gustafsson 2020-09-16 12:12:57 UTC
I had a look at this, and I think this is a bit of a mess to sort out. You are calling bulkmarcimport.pl without -update or -insert set, which seems to default to -insert ($insert = 1, $update = undef), and without -match with a matchpoint.

bulkmarcimport will correctly get original id through:

$originalid = GetRecordId( $record, $tagid, $subfieldid );

But will not use $originalid as a match ($biblionumber will have no value on line 453 where record is either inserted or updated). Because of this and $update = undef the block inside of if ($insert) {... on line 470 will run inserting a new record with defer_marc_save => 1. After this block:

eval { ( $itemnumbers_ref, $errors_ref ) = AddItemBatchFromMarc( $record, $biblionumber, $biblioitemnumber, '' ); };

Will be run with a $biblionumber set to a biblio without any marc data yet saved.

Inside of AddItemBatchFromMarc

my $item_object = Koha::Item->new($item)->store;

will be run.

Inside Koha/Item.pm Koha::Item::store this will get executed:

C4::Biblio::ModZebra( $self->biblionumber, "specialUpdate", "biblioserver" )
            unless $params->{skip_modzebra_update}

ModZebra will attempt to load the Biblio Marc data:

$record = GetMarcBiblio({
    biblionumber => $biblionumber,
    embed_items  => 1 });

But this will return an undefined record since the marc-data has not yet been saved.

This will cause the import in later processing of this undefined $record to crash which will not propagate to bulkmarcimport.pl since run within an eval.

I get this when running bulkmarcimport without the patch. The new record marc data is later saved in the following block which comes shortly after:

   my $clone_record = $record->clone();
   C4::Biblio::_strip_item_fields($clone_record, '');
   # This sets the marc fields if there was an error, and also calls
   # defer_marc_save.
   ModBiblioMarc( $clone_record, $biblionumber, $framework );

which I removed in the patch (was not aware that the marc data was loaded as a side effect of AddItemBarchFromMarc).

This code needs to be removed somehow though since it will bypass the marc merge rules in bug 14957.

I think by removing this block a new bug is exposed which is that AddBiblio will not strip items before saving. Perhaps this could be addressed by stripping items before calling AddBiblio in bulkmarcimport.pl. I will look further into this.
Comment 11 David Gustafsson 2020-09-17 12:11:31 UTC Comment hidden (obsolete)
Comment 12 David Gustafsson 2020-09-17 12:13:09 UTC Comment hidden (obsolete)
Comment 13 David Gustafsson 2020-09-17 12:38:14 UTC
I have now modified the patch so that items will be stripped also when adding new biblios. You will still get an error message when adding an exported biblio with existing items since the item ids will cause a duplicate primary key constraint error.

If understanding this correctly this should be considered "better" or in line with the current behavior of the script since the unmodified script will bail out even earlier with a different error (triggered by not saving the marc when attempting to load it). Even if the current script would get past that (which it would if removing the "defer_marc_save" option) you would still get the exact same error since AddItemBatchFromMarc would be called on the $record with updated internal bibio ids but with the same item-ids.
Comment 14 David Gustafsson 2020-09-17 13:26:20 UTC
About the whitespace in the patch, I removed superfluous white space in some places, but sure, to beconsistent that should perhaps be another issue since there is also some other instances of tabs instead of spaces and other incorrect formatting that could warrant another issue. I can remove this from the patch in this case.
Comment 15 Martin Renvoize 2020-10-12 09:39:28 UTC
Er.. not sure what's being 'Discussed' here at the moment.. is this ready for testing again?
Comment 16 Martin Renvoize 2021-03-29 12:22:47 UTC
Removing bug 14957 as a dependant. We have PQA'd on that patchset and it doesn't appear to actually require this as a dependency.

I'd still be interested to see where this one is going, I'm not sure what discussion is in progress?
Comment 17 Martin Renvoize 2021-03-29 12:36:11 UTC
I commented out the ability to use overlay rules with the bulkmarcimport route.. that unblocks that bug so it can be pushed and allows us to continue work here to re-enable that part of the feature.
Comment 18 Victor Grousset/tuxayo 2021-03-29 16:30:24 UTC
> Removing bug 14957 as a dependant

Actually it was re-added ^^
Comment 19 Martin Renvoize 2021-03-29 17:04:06 UTC
(In reply to Victor Grousset/tuxayo from comment #18)
> > Removing bug 14957 as a dependant
> 
> Actually it was re-added ^^

Actually, it was a swap ;).. this was blocking the other bug, now it depends upon it ;)
Comment 20 David Gustafsson 2021-03-29 17:20:59 UTC
(In reply to Martin Renvoize from comment #16)
> Removing bug 14957 as a dependant. We have PQA'd on that patchset and it
> doesn't appear to actually require this as a dependency.
> 
> I'd still be interested to see where this one is going, I'm not sure what
> discussion is in progress?

Perhaps you are aware of this since commented out the bulkmarcimport source, but it does require it as a dependency for bulkmarcimport. We use a fork of bulkmarcimport.pl which has been largely rewritten and decided not to try getting this into Koha because of the huge task of splitting this into minor patches which probably was a mistake. But there was such a huge overhead of doing so at the time. Would like to contribute those changes since fixed a large number of issues and bugs, but the authorities part is largely untested (since we don't import those) and therefore I would guess largely broken. I'm quite quite puzzled how bulkmarcimport.pl works for anyone else since discovered many major issues/bugs which had to be addressed in our use case, but obviously it does. This is the current patch for bulkmarcimport.pl and it would probably be a nightmare to review: https://github.com/ub-digit/Koha/commit/4615cf59575bb32bf9aeb8eaa716fd926201327d
Comment 21 David Gustafsson 2021-03-29 17:25:48 UTC
I completely forgot to add why I brought the patch up. The reason being that it's hard to fix the issue without exposing other bugs as mentioned in comment #13. It would probably be better to perform a more thorough refactor, but then there is the issue that we already did that but does not have a clean patch-set to contribute. Perhaps I will have to take the time to go through our current patch and verify that the current options are still supported and the script behaves in more or less the same way, so no or minimal API breakage.
Comment 22 Martin Renvoize 2021-05-06 08:27:00 UTC
(In reply to David Gustafsson from comment #21)
> I completely forgot to add why I brought the patch up. The reason being that
> it's hard to fix the issue without exposing other bugs as mentioned in
> comment #13. It would probably be better to perform a more thorough
> refactor, but then there is the issue that we already did that but does not
> have a clean patch-set to contribute. Perhaps I will have to take the time
> to go through our current patch and verify that the current options are
> still supported and the script behaves in more or less the same way, so no
> or minimal API breakage.

I'm more than happy to take a look at a bulkmarcimport patch.. even if it is initially large and unwieldy. We use that script a lot at PTFS-E and have been contemplating working on it to improve performance for some time.. though I'm not aware of any real bugs.. I'll ask the migrations team as they're the one's that utilise it.. perhaps they're using it in a different way from yourselves.
Comment 23 David Gustafsson 2021-09-06 14:56:17 UTC
Sorry about the late reply, I posted the commit in comment #20, not sure it applies to the current Koha master, we regularly rebase our patch-set but have not done so in a while. Even though I think it could be useful to have a look at I hope to work more on cleaning this mess up, but not sure when will able to do so. Some changes might be easier to break out than others.
Comment 24 David Gustafsson 2021-11-09 12:54:09 UTC
I have now started to work on breaking up the patch earlier mentioned in this thread into more manageable parts. At least separating the new features from the major cleanup. Created a new issue (bug 29440) which is now a dependency for this. I will later update the patches in this issue so are rebased upon the patch-set in 29440.
Comment 25 Martin Renvoize 2021-11-09 14:04:57 UTC
That's great news David, thankyou for getting back onto this.. Do feel free to grab me to take a look when you need review etc :)
Comment 26 David Gustafsson 2021-11-10 15:43:39 UTC
(In reply to Martin Renvoize from comment #25)
> That's great news David, thankyou for getting back onto this.. Do feel free
> to grab me to take a look when you need review etc :)

Thanks a lot, that would be really helpful! Will let you know when is ready for review. Have pulled out code some code without properly testing that did not break things, so first try to do some manual testing. After that the patch in bug 29440 should be ready for review/sign off.
Comment 27 David Gustafsson 2021-11-12 22:45:04 UTC Comment hidden (obsolete)
Comment 28 David Gustafsson 2021-11-12 22:45:08 UTC Comment hidden (obsolete)
Comment 29 David Gustafsson 2021-11-12 22:46:34 UTC Comment hidden (obsolete)
Comment 30 David Gustafsson 2021-11-12 22:46:38 UTC Comment hidden (obsolete)
Comment 31 David Gustafsson 2021-11-12 22:47:06 UTC
Now rebased upon bug 29440
Comment 32 David Gustafsson 2021-11-12 22:49:51 UTC Comment hidden (obsolete)
Comment 33 David Gustafsson 2021-11-24 13:46:18 UTC Comment hidden (obsolete)
Comment 34 David Gustafsson 2021-11-24 13:46:23 UTC Comment hidden (obsolete)
Comment 35 David Gustafsson 2021-11-24 14:32:56 UTC Comment hidden (obsolete)
Comment 36 David Gustafsson 2021-11-24 14:33:01 UTC Comment hidden (obsolete)
Comment 37 David Gustafsson 2021-11-24 15:36:09 UTC
Created attachment 127980 [details] [review]
Bug 25539: Remove AddBiblio option "defer_marc_save"

Items are no longer embedded in the MARCXML and because of this the MARC data
does not need to be saved once more after changing record items data.
The "defer_marc_save" is no longer needed since bulkmarcimport.pl was the only
place this option was utilized in order to resave MARC data after possibly changing
items data. There is also a bug bulkmarcimport.pl where the record data is re-saved
without stripping items if duplicate items are found and the dedup barcodes
option is enabled that is resolved by this change.

This change enables MARC Overlay rules to be enabled for bulkmarkcimport.pl as using
the defer_marc option would previously effectively bypass any defined rules.

To test
1) Apply patch 29440 (which this depends on), but no not yet apply this
   patch
2) Enable MARC overlay rules and add a rule (with source => "*" or
   'bulkmarcimport') to protect some field.
3) Import a biblio using the bulkmarcimport script. Overlay rules will
   only be applied on updates so a match condition need to be supplied
   (for example -match "control-number,001")
4) Edit the imported biblio and change the value of the protected field
5) Run the import again and verify the the field that should have been
   protected has been overwritten
6) Apply the patch and repeat steps 4-5, verify that the field was
   protected and has not been overwritten
Comment 38 David Gustafsson 2021-11-24 15:36:15 UTC
Created attachment 127981 [details] [review]
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script
Comment 39 Frank Hansen 2022-02-08 13:37:16 UTC
(In reply to David Gustafsson from comment #37)
> Created attachment 127980 [details] [review] [review]
> Bug 25539: Remove AddBiblio option "defer_marc_save"
> 
> 2) Enable MARC overlay rules and add a rule (with source => "*" or
>    'bulkmarcimport') to protect some field.

Hi David. It is not possible to configure bulkmarcimport as source filter in home->administration->Marc overlay rules because bulkmarcimport.pl is commented out in koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc-overlay-rules.tt template. Is it commented out by intention?
Comment 40 David Cook 2022-03-18 00:08:12 UTC
I really want to get rid of "defer_marc_save" as it's a real pain (especially when using Koha plugins).

But I'm not sure I understand the test plan...
Comment 41 David Gustafsson 2022-04-06 14:05:12 UTC
Created attachment 133030 [details] [review]
Bug 25539: Remove AddBiblio option "defer_marc_save"

Items are no longer embedded in the MARCXML and because of this the MARC data
does not need to be saved once more after changing record items data.
The "defer_marc_save" is no longer needed since bulkmarcimport.pl was the only
place this option was utilized in order to resave MARC data after possibly changing
items data. There is also a bug bulkmarcimport.pl where the record data is re-saved
without stripping items if duplicate items are found and the dedup barcodes
option is enabled that is resolved by this change.

This change enables MARC Overlay rules to be enabled for bulkmarkcimport.pl as using
the defer_marc option would previously effectively bypass any defined rules.

To test
1) Apply patch 29440 (which this depends on), but no not yet apply this
   patch
2) Remove comments around "overlay_context => { source => 'bulkmarcimport' }"
   in misc/migration_tools/bulkmarcimport.pl line 129
3) Enable MARC overlay rules and add a rule (with source => "*" or
   'bulkmarcimport') to protect some field.
4) Import a biblio using the bulkmarcimport script. Overlay rules will
   only be applied on updates so a match condition matching the
   record in Koha when bulkmarcimport.pl is run the next time for the same
   record needs to be supplied (for example -match "control-number,001")
5) Edit the imported biblio and change the value of the protected field
6) Run bulkmarcimport with the same parameters as in 4) and verify the
   the field that should have been protected has been overwritten
7) Revert changes for line 129 in bulkmarcimport.pl (or will result in
   conflict applying the patch), apply the patch and repeat steps 4-5,
   verify that the field now was protected from being overwritten.
Comment 42 David Gustafsson 2022-04-06 14:05:17 UTC
Created attachment 133031 [details] [review]
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script
Comment 43 David Gustafsson 2022-04-06 14:05:21 UTC
Created attachment 133032 [details] [review]
Bug 25539: Enable bulkmarcimport overlay context
Comment 44 David Gustafsson 2022-04-06 14:17:06 UTC
@(In reply to David Cook from comment #40)
> I really want to get rid of "defer_marc_save" as it's a real pain
> (especially when using Koha plugins).
> 
> But I'm not sure I understand the test plan...

I sympathize with the fact the test-plan is probably not exactly crystal clear, compounded by or perhaps due to the fact that bulkmarcimport is not even selectable in the UI (as it's commented out because of the bug this patch should solve). I added a step to the testplan to remove the comments and enable bulkmarcimport as a source (now realizing i pointed to the wrong line in the wrong file, will fix this).

Also the bulkmarcimport.pl command to test this could look something like this:

./bulkmarcimport.pl -b -file /tmp/test_record.marc -l /tmp/bulkmarcimport.log -append  -insert -update -c=MARC21 -match "control-number,001"

It's not part of the test-plan, but it could be useful to use the logfile feature as to ensure that the biblio is really matched and updated on subsequent runs.
Comment 45 David Gustafsson 2022-04-06 14:20:29 UTC
Created attachment 133034 [details] [review]
Bug 25539: Remove AddBiblio option "defer_marc_save"

Items are no longer embedded in the MARCXML and because of this the MARC data
does not need to be saved once more after changing record items data.
The "defer_marc_save" is no longer needed since bulkmarcimport.pl was the only
place this option was utilized in order to resave MARC data after possibly changing
items data. There is also a bug bulkmarcimport.pl where the record data is re-saved
without stripping items if duplicate items are found and the dedup barcodes
option is enabled that is resolved by this change.

This change enables MARC Overlay rules to be enabled for bulkmarkcimport.pl as using
the defer_marc option would previously effectively bypass any defined rules.

To test
1) Apply patch 29440 (which this depends on), but no not yet apply this
   patch
2) Remove comments around "bulkmarcimport: _("bulkmarcimport.pl"),"
   in koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc-overlay-rules.tt
   line 463
3) Enable MARC overlay rules and add a rule (with source => "*" or
   'bulkmarcimport') to protect some field.
4) Import a biblio using the bulkmarcimport script. Overlay rules will
   only be applied on updates so a match condition matching the
   record in Koha when bulkmarcimport.pl is run the next time for the same
   record needs to be supplied (for example -match "control-number,001")
5) Edit the imported biblio and change the value of the protected field
6) Run bulkmarcimport with the same parameters as in 4) and verify the
   the field that should have been protected has been overwritten
7) Revert changes for line 463 in marc-overlay-rules.tt (or will result in
   conflict applying the patch), apply the patch and repeat steps 4-5,
   verify that the field now was protected from being overwritten.
Comment 46 David Gustafsson 2022-04-06 14:20:34 UTC
Created attachment 133035 [details] [review]
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script
Comment 47 David Gustafsson 2022-04-06 14:20:39 UTC
Created attachment 133036 [details] [review]
Bug 25539: Enable bulkmarcimport overlay context
Comment 48 David Gustafsson 2022-04-06 14:23:03 UTC
Also forgot to mention that also added a commit enabling bulkmarcimport context in the UI as part of this patch.
Comment 49 David Gustafsson 2022-09-26 15:20:44 UTC
Created attachment 140992 [details] [review]
Bug 25539: Remove AddBiblio option "defer_marc_save"

Items are no longer embedded in the MARCXML and because of this the MARC data
does not need to be saved once more after changing record items data.
The "defer_marc_save" is no longer needed since bulkmarcimport.pl was the only
place this option was utilized in order to resave MARC data after possibly changing
items data. There is also a bug bulkmarcimport.pl where the record data is re-saved
without stripping items if duplicate items are found and the dedup barcodes
option is enabled that is resolved by this change.

This change enables MARC Overlay rules to be enabled for bulkmarkcimport.pl as using
the defer_marc option would previously effectively bypass any defined rules.

To test
1) Apply patch 29440 (which this depends on), but no not yet apply this
   patch
2) Remove comments around "bulkmarcimport: _("bulkmarcimport.pl"),"
   in koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc-overlay-rules.tt
   line 463
3) Enable MARC overlay rules and add a rule (with source => "*" or
   'bulkmarcimport') to protect some field.
4) Import a biblio using the bulkmarcimport script. Overlay rules will
   only be applied on updates so a match condition matching the
   record in Koha when bulkmarcimport.pl is run the next time for the same
   record needs to be supplied (for example -match "control-number,001")
5) Edit the imported biblio and change the value of the protected field
6) Run bulkmarcimport with the same parameters as in 4) and verify the
   the field that should have been protected has been overwritten
7) Revert changes for line 463 in marc-overlay-rules.tt (or will result in
   conflict applying the patch), apply the patch and repeat steps 4-5,
   verify that the field now was protected from being overwritten.
Comment 50 David Gustafsson 2022-09-26 15:20:50 UTC
Created attachment 140993 [details] [review]
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script
Comment 51 David Gustafsson 2022-09-26 15:20:56 UTC
Created attachment 140994 [details] [review]
Bug 25539: Enable bulkmarcimport overlay context
Comment 52 David Gustafsson 2022-09-26 15:21:37 UTC
Rebased against master (including bug_29440)
Comment 53 Magnus Enger 2023-03-27 10:36:27 UTC
Depends on Bug 29440, which currently does not apply.
Comment 54 David Gustafsson 2023-09-26 13:45:03 UTC
Created attachment 156224 [details] [review]
Bug 25539: Remove AddBiblio option "defer_marc_save"

Items are no longer embedded in the MARCXML and because of this the MARC data
does not need to be saved once more after changing record items data.
The "defer_marc_save" is no longer needed since bulkmarcimport.pl was the only
place this option was utilized in order to resave MARC data after possibly changing
items data. There is also a bug bulkmarcimport.pl where the record data is re-saved
without stripping items if duplicate items are found and the dedup barcodes
option is enabled that is resolved by this change.

This change enables MARC Overlay rules to be enabled for bulkmarkcimport.pl as using
the defer_marc option would previously effectively bypass any defined rules.

To test
1) Apply patch 29440 (which this depends on), but no not yet apply this
   patch
2) Remove comments around "bulkmarcimport: _("bulkmarcimport.pl"),"
   in koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc-overlay-rules.tt
   line 463
3) Enable MARC overlay rules and add a rule (with source => "*" or
   'bulkmarcimport') to protect some field.
4) Import a biblio using the bulkmarcimport script. Overlay rules will
   only be applied on updates so a match condition matching the
   record in Koha when bulkmarcimport.pl is run the next time for the same
   record needs to be supplied (for example -match "control-number,001")
5) Edit the imported biblio and change the value of the protected field
6) Run bulkmarcimport with the same parameters as in 4) and verify the
   the field that should have been protected has been overwritten
7) Revert changes for line 463 in marc-overlay-rules.tt (or will result in
   conflict applying the patch), apply the patch and repeat steps 4-5,
   verify that the field now was protected from being overwritten.
Comment 55 David Gustafsson 2023-09-26 13:45:06 UTC
Created attachment 156225 [details] [review]
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script
Comment 56 David Gustafsson 2023-09-26 13:45:09 UTC
Created attachment 156226 [details] [review]
Bug 25539: Enable bulkmarcimport overlay context
Comment 57 David Gustafsson 2023-09-26 13:45:35 UTC
Rebased against master.
Comment 58 M 2023-10-25 11:39:13 UTC
> 7) Revert changes for line 463 in marc-overlay-rules.tt (or will result in
>    conflict applying the patch), apply the patch and repeat steps 4-5,
>   verify that the field now was protected from being overwritten.

Generally it works, but doesn't for field 942 for some reason... It just gets removed, while others like 866 are preserved (with exact same overlay rules...). Perhaps related to Bug 34191 (meaning that it might be unrelated to this patchset)
Comment 59 Katrin Fischer 2023-10-25 19:39:05 UTC
(In reply to M from comment #58)
> > 7) Revert changes for line 463 in marc-overlay-rules.tt (or will result in
> >    conflict applying the patch), apply the patch and repeat steps 4-5,
> >   verify that the field now was protected from being overwritten.
> 
> Generally it works, but doesn't for field 942 for some reason... It just
> gets removed, while others like 866 are preserved (with exact same overlay
> rules...). Perhaps related to Bug 34191 (meaning that it might be unrelated
> to this patchset)

Don't use the asterisk - then it should work. See: 

Bug 33268 - Overlay rules don't work correctly when source is set to *

But there should be no difference for different fields. Do you have a second rule for 942 by chance?
Comment 60 M 2023-10-30 07:13:04 UTC
> Don't use the asterisk - then it should work

Huh, this behavior is good to know. Though I actually had all rules with filter "bulkmarcimport", only then changed the one for 942 to "*" just for a test (after noticing what I described)...

> But there should be no difference for different fields. Do you have a second rule for 942 by chance?

Not a second rule either. It was all somewhat strange to me too, I couldn't notice anything too special about this field when reading code...

By the way, on an unrelated note, it'd be really nice to have this patchset and the dependent Bug 29440 merged for the next release soon, we want record updates from upstream central library, but it's only gonna work nicely if we can set the overlay rules while using the script...
Comment 61 M 2023-10-30 08:09:37 UTC
I ran another test and it does seem to be the case somehow still. Original record in Koha has fields 866 and 942 (the latter is Koha metadata, including required item type). The imported record has both of these fields missing. Both of these fields have exact same overlay rules setup up, the field ID being the only different thing between them. I did a dirty patch to the script to copy over field 942 from found record for matching, and only that seems to help...

Weird thing, it'd be nice if someone else could try to reproduce this situation, maybe I'm doing something wrong, but I can't seem to spot what's the difference either...
Comment 62 Katrin Fischer 2023-10-30 08:18:20 UTC
(In reply to M from comment #61)
> I ran another test and it does seem to be the case somehow still. Original
> record in Koha has fields 866 and 942 (the latter is Koha metadata,
> including required item type). The imported record has both of these fields
> missing. Both of these fields have exact same overlay rules setup up, the
> field ID being the only different thing between them. I did a dirty patch to
> the script to copy over field 942 from found record for matching, and only
> that seems to help...
> 
> Weird thing, it'd be nice if someone else could try to reproduce this
> situation, maybe I'm doing something wrong, but I can't seem to spot what's
> the difference either...

Could you post the rules from the database maybe? So we can make sure we set them up the same and it's not a display issue.
Comment 63 M 2023-10-30 08:48:47 UTC
"id","tag","module","filter","add","append","remove","delete"
"1","942","source","bulkmarcimport","1","1","0","0"
"2","967","source","bulkmarcimport","0","0","0","0"
"3","852","source","bulkmarcimport","1","1","0","0"
"4","866","source","bulkmarcimport","1","1","0","0"
"6","003","source","bulkmarcimport","1","0","0","0"

I initially had add+append set to 0, and I think setting them to 1 like 8xx was one of attempts at finding the root cause. Last few attempts were done with this exact settings, after changing the filter back to be the same.
Comment 64 Katrin Fischer 2023-10-30 12:55:03 UTC
(In reply to M from comment #63)
> "id","tag","module","filter","add","append","remove","delete"
> "1","942","source","bulkmarcimport","1","1","0","0"
> "2","967","source","bulkmarcimport","0","0","0","0"
> "3","852","source","bulkmarcimport","1","1","0","0"
> "4","866","source","bulkmarcimport","1","1","0","0"
> "6","003","source","bulkmarcimport","1","0","0","0"
> 
> I initially had add+append set to 0, and I think setting them to 1 like 8xx
> was one of attempts at finding the root cause. Last few attempts were done
> with this exact settings, after changing the filter back to be the same.

David, can you maybe help? Is this related to the reported bug or maybe should be moved elsewhere?
Comment 65 M 2023-11-07 11:25:38 UTC
Sorry for the fuss... cache! The problem was cache. Some kind of cache was holding the old marc overlay rules, after restarting and flushing everything the changed rules are now correctly applied.

Now everything seems to work perfectly fine. I'm not gonna lie, I share a huge hope this changeset could be merged in for 23.11 release in such case so that we can use those scripts for daily upstream updates
Comment 66 Katrin Fischer 2023-11-07 21:59:02 UTC
(In reply to M from comment #65)
> Sorry for the fuss... cache! The problem was cache. Some kind of cache was
> holding the old marc overlay rules, after restarting and flushing everything
> the changed rules are now correctly applied.
> 
> Now everything seems to work perfectly fine. I'm not gonna lie, I share a
> huge hope this changeset could be merged in for 23.11 release in such case
> so that we can use those scripts for daily upstream updates

The dependency currently requires testing, that would be great way to help moving this forward. But only bug fixes will usually be ported back - so it might be too late for 23.11 now.
Comment 67 M 2023-11-08 12:17:46 UTC
> But only bug fixes will usually be ported back - so it might be too late for 23.11 now.

Eh, I daresay it'd fall under big fixes technically, since it fixes the overlay rules application on imported records, and I understand that it was one of the main points of the script's refactor to have that working properly as a result.

It's possible that actually the dependency fixes that bug, but this patch here re-enables the overlay context (also both made by the same author, so I assume it's some kind of logical continuation)
Comment 68 Victor Grousset/tuxayo 2023-11-11 18:46:34 UTC
> Eh, I daresay it'd fall under big fixes technically

Even if Bug 29440 were to be reclassified, it still needs signoff and then QA. And given it's size, we might be too close to release date to let enough time for eventual issues to be noticed in wider testing.

After release, enhancements can still be backported at the discretion of the release maintainer of the branch. But Bug 29440 sounds like a bit too large to hope for this.
Comment 69 M 2023-12-20 18:46:48 UTC
Created attachment 160158 [details] [review]
Bug 25539: Remove AddBiblio option "defer_marc_save"
Comment 70 M 2023-12-20 18:46:51 UTC
Created attachment 160159 [details] [review]
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script
Comment 71 M 2023-12-20 18:46:56 UTC
Created attachment 160160 [details] [review]
Bug 25539: Enable bulkmarcimport overlay context
Comment 72 M 2023-12-20 18:48:02 UTC
Signed off, but status remains as BLOCKED as it still depends on Bug 29440
Comment 73 Martin Renvoize 2023-12-20 19:58:47 UTC
This has been on my radar for a long time. I'll jump on it first thing in the morning and get to through QA now it rebases and signed off again.
Comment 74 Katrin Fischer 2024-01-01 09:51:14 UTC
(In reply to Martin Renvoize from comment #73)
> This has been on my radar for a long time. I'll jump on it first thing in
> the morning and get to through QA now it rebases and signed off again.

Ping? :)
Comment 75 Martin Renvoize 2024-01-03 16:38:03 UTC
Created attachment 160489 [details] [review]
Bug 25539: Remove AddBiblio option "defer_marc_save"

Items are no longer embedded in the MARCXML and because of this the MARC data
does not need to be saved once more after changing record items data.
The "defer_marc_save" is no longer needed since bulkmarcimport.pl was the only
place this option was utilized in order to resave MARC data after possibly changing
items data. There is also a bug bulkmarcimport.pl where the record data is re-saved
without stripping items if duplicate items are found and the dedup barcodes
option is enabled that is resolved by this change.

This change enables MARC Overlay rules to be enabled for bulkmarkcimport.pl as using
the defer_marc option would previously effectively bypass any defined rules.

To test
1) Apply patch 29440 (which this depends on), but no not yet apply this
   patch
2) Remove comments around "bulkmarcimport: _("bulkmarcimport.pl"),"
   in koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc-overlay-rules.tt
   line 463
3) Enable MARC overlay rules and add a rule (with source => "*" or
   'bulkmarcimport') to protect some field.
4) Import a biblio using the bulkmarcimport script. Overlay rules will
   only be applied on updates so a match condition matching the
   record in Koha when bulkmarcimport.pl is run the next time for the same
   record needs to be supplied (for example -match "control-number,001")
5) Edit the imported biblio and change the value of the protected field
6) Run bulkmarcimport with the same parameters as in 4) and verify the
   the field that should have been protected has been overwritten
7) Revert changes for line 463 in marc-overlay-rules.tt (or will result in
   conflict applying the patch), apply the patch and repeat steps 4-5,
   verify that the field now was protected from being overwritten.

Signed-off-by: Michał Kula <148193449+mkibp@users.noreply.github.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 76 Martin Renvoize 2024-01-03 16:38:07 UTC
Created attachment 160490 [details] [review]
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script

Signed-off-by: Michał Kula <148193449+mkibp@users.noreply.github.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 77 Martin Renvoize 2024-01-03 16:38:10 UTC
Created attachment 160491 [details] [review]
Bug 25539: Enable bulkmarcimport overlay context

Signed-off-by: Michał Kula <148193449+mkibp@users.noreply.github.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 78 Martin Renvoize 2024-01-03 16:39:02 UTC
*** Bug 35075 has been marked as a duplicate of this bug. ***
Comment 79 Katrin Fischer 2024-01-05 13:28:06 UTC
Hm, a bugfix depending on an enhancement can be an issue as it makes backporting hard or impossible. Any chance we could get a bugfix for the stable versions independent of bug 29440?
Comment 80 M 2024-01-05 13:38:56 UTC
I don't think that it would be viable at all at this point... Imo they should be either backported both or neither of them. Otherwise I'm not even sure if it applies to the original unpatched scripts without breakage or side-effects. I don't think cherry-picking only parts of this for backport is worth it.

Not to mention that commit "Enable bulkmarcimport overlay context" itself also depends on fixes from the refactor made in the other bug.

If we're extra-careful, it could be even dubious if they should be backported at all, in case someone might currently depend on old slightly broken behavior of the scripts, such as having "*" overlay rules that were completely ignored despite code suggesting otherwise. I previously suggested merging it as bugfix in November, but before 23.11 was actually officially released.