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

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.

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.

(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.

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.

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

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.

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.

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.

Er.. not sure what's being 'Discussed' here at the moment.. is this ready for testing again?

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?

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.

> Removing bug 14957 as a dependant

Actually it was re-added ^^

(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 ;)

(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

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.

(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.

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.

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.

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 :)

(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.

Now rebased upon bug 29440

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

Created attachment 127981 [details] [review]
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script

(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?

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...

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.

Created attachment 133031 [details] [review]
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script

Created attachment 133032 [details] [review]
Bug 25539: Enable bulkmarcimport overlay context

@(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.

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.

Created attachment 133035 [details] [review]
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script

Created attachment 133036 [details] [review]
Bug 25539: Enable bulkmarcimport overlay context

Also forgot to mention that also added a commit enabling bulkmarcimport context in the UI as part of this patch.

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.

Created attachment 140993 [details] [review]
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script

Created attachment 140994 [details] [review]
Bug 25539: Enable bulkmarcimport overlay context

Rebased against master (including bug_29440)

Depends on Bug 29440, which currently does not apply.

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.

Created attachment 156225 [details] [review]
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script

Created attachment 156226 [details] [review]
Bug 25539: Enable bulkmarcimport overlay context

Rebased against master.

> 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)

(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?

> 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...

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...

(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.

"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.

(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?

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

(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.

> 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)

> 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.

Created attachment 160158 [details] [review]
Bug 25539: Remove AddBiblio option "defer_marc_save"

Created attachment 160159 [details] [review]
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script

Created attachment 160160 [details] [review]
Bug 25539: Enable bulkmarcimport overlay context

Signed off, but status remains as BLOCKED as it still depends on Bug 29440

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.

(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? :)

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>

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>

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>

*** Bug 35075 has been marked as a duplicate of this bug. ***

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?

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.

Pushed for 24.05!

Well done everyone, thank you!

Depends on Bug 29440 not in 23.11.x

Note there is an error in commit to master :
b374358e84 Bug 35539: (QA follow-up) Remove defer_marc_save from Elasticsearch.t

Maybe need to fix manually in release notes.

(In reply to Fridolin Somers from comment #83)
> Note there is an error in commit to master :
> b374358e84 Bug 35539: (QA follow-up) Remove defer_marc_save from
> Elasticsearch.t
> 
> Maybe need to fix manually in release notes.

Thanks Frido - I think it was bound to happen.