See the discussion on bug 21774 which refers back to adjustments by 10306. But actually Koha did never check if an item subfield is repeatable or not. Just put two barcodes and a pipe in the barcode field, save and open again and the barcode field is duplicated in the editor. This is before 18.05. And also after fix 21774 on 18.11 (if pushed which should). So how do we resolve the issue while 19096 removed the frameworkcode because it was no longer needed for Koha to Marc mappings? That is what we should solve here.
At the same time wondering if we not should allow some item subfields to be repeatable just as some under in MARC21 852 ?
(In reply to Marcel de Rooy from comment #1) > At the same time wondering if we not should allow some item subfields to be > repeatable just as some under in MARC21 852 ? We should. We can specify it in the frameworks.
Created attachment 98816 [details] [review] Bug 21800: Add tests for repeatable subfields Test plan: Do not apply the second patch [note 1]. Run t/db_dependent/Biblio/TransformKohaToMarc.t Run t/db_dependent/Biblio/TransformMarcToKoha.t Apply the second patch and run them again. Both tests should pass now. Note 1: The TransformKohaToMarc test should fail with something like: # Failed test 'Check 260e' # at t/db_dependent/Biblio/TransformKohaToMarc.t line 60. # got: 'A' # expected: 'A | B' Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 98817 [details] [review] Bug 21800: Make TransformKohaToMarc aware of non-repeatable subfields If a kohafield (in Koha to MARC mappings) contains a pipe char (say A | B), we split it up into two subfields A and B in MARC. We will only do that for repeatable subfields now. If the field is not repeatable, the value will just be 'A | B'. Note: Does this impact the reverse operation in TransformMarcToKoha? No, the check on repeatable subfields is done in the interface and not in TransformMarcToKoha. This routine simply translates two instances of the same subfield, say A and B, into the value 'A | B' for a kohafield. Not allowing two instances of a non-repeatable subfield is not in the scope of this report. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 99309 [details] [review] Bug 21800: Add tests for repeatable subfields Test plan: Do not apply the second patch [note 1]. Run t/db_dependent/Biblio/TransformKohaToMarc.t Run t/db_dependent/Biblio/TransformMarcToKoha.t Apply the second patch and run them again. Both tests should pass now. Note 1: The TransformKohaToMarc test should fail with something like: # Failed test 'Check 260e' # at t/db_dependent/Biblio/TransformKohaToMarc.t line 60. # got: 'A' # expected: 'A | B' Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 99310 [details] [review] Bug 21800: Make TransformKohaToMarc aware of non-repeatable subfields If a kohafield (in Koha to MARC mappings) contains a pipe char (say A | B), we split it up into two subfields A and B in MARC. We will only do that for repeatable subfields now. If the field is not repeatable, the value will just be 'A | B'. Note 1: As bug 10306 and its friends (19096) made the Default framework authoritative, we do no longer have the frameworkcode in this routine. Formally, we should check the corresponding framework. Note 2: Does this impact the reverse operation in TransformMarcToKoha? No, the check on repeatable subfields is done in the interface and not in TransformMarcToKoha. This routine simply translates two instances of the same subfield, say A and B, into the value 'A | B' for a kohafield. Not allowing two instances of a non-repeatable subfield is not in the scope of this report. Test plan: [1] Mark an item field as repeatable in the Default framework. Edit an item. Insert A|B in this field and another not-repeatable field. Save and reopen. Verify that the repeatable field is duplicated and the other one contains the pipe character in the text box. [2] Look for a repeatable subfield in MARC like e.g. 260$c. Go to the cataloguing editor and add A|B in this field. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 99311 [details] [review] Bug 21800: Check the correct framework for the repeatable tag This depends on the framework parameter. Which should be added back to the call in C4::Items. Test plan: [1] Mark in Default framework one subfield A repeatable and B not repeatable. Go to item editor. (Work on a biblio in Default framework.) Check saving and reopening these subfields with VAL1 | VAL2. Subfield A should be cloned, B should be glued as entered. [2] Mark in another Framework A not repeatable and B repeatable. Change framework for this biblio. Go to item editor again. Reopen item. Behavior subfields in reverse? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 99312 [details] [review] Bug 21800: Add tests for _check_split This new subroutine supports checking the repeatable tag from another framework. (It will be useful for the item editor.) Test plan: Run t/db_dependent/Biblio/TransformKohaToMarc.t
Created attachment 99313 [details] [review] Bug 21800: Add tests for _check_split This new subroutine supports checking the repeatable tag from another framework. (It will be useful for the item editor.) Test plan: Run t/db_dependent/Biblio/TransformKohaToMarc.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Why isn't the default framework also authoritative for repeatability? If not, at least the framework information should be fetched e.g. with GetMarcSubfieldStructure so that it's cached to avoid a performance penalty, right?
Setting to Failed QA until my previous comments have been resolved.
(In reply to Ere Maijala from comment #10) > Why isn't the default framework also authoritative for repeatability? We only consider it authoritative for the Koha to MARC mapping. The interface allows you to set repeatability per framework. Which is good. Since we changed mappings, it does no longer look at the other frameworks. So this is the crux of the change. > If not, at least the framework information should be fetched e.g. with > GetMarcSubfieldStructure so that it's cached to avoid a performance penalty, > right? I did experiment a bit with caching but didnt submit that portion. The existing cached structures are based on entry by kohafield, not MARC tags. Makes it a bit less handy. Performance penalty is a big word here. We use this for items. The item editor needs 2 or 3 ms more to open one record (in my benchmarking). Batch item modification depends on the number of items, but is a staff action, probably not executed continuously. Rebuild zebra should not be a problem too. Considering that, I feel that caching is no requirement here. And we can always add it on its own report. Back to NSO. Hoping for a SO ;)
Ok, I'm fine with the framework stuff, but it strikes me a bit odd that we use mapping from default framework and repeatability from another. Unless I'm completely lost it provides ample opportunity for user confusion. Sorry, I'd say that caching is pretty crucial. TransformKohaToMarc is used also e.g. in the Elasticsearch index builder, OAI-PMH Provider and exporter (really, anything that calls e.g. EmbedItemsInMarcBiblio).
(In reply to Ere Maijala from comment #13) > Ok, I'm fine with the framework stuff, but it strikes me a bit odd that we > use mapping from default framework and repeatability from another. Unless > I'm completely lost it provides ample opportunity for user confusion. The interface follows this line. No frameworks in Koha to MARC mappings. No possibility to change kohafield in framework editor (as before). All other fields (like repeatability) are framework based.
(In reply to Ere Maijala from comment #13) > Sorry, I'd say that caching is pretty crucial. TransformKohaToMarc is used > also e.g. in the Elasticsearch index builder, OAI-PMH Provider and exporter > (really, anything that calls e.g. EmbedItemsInMarcBiblio). Will address that on a new report then.
If it's in a separate issue, how do we make sure bug 24715 and this one are committed at same time? My concern is that this one alone will introduce a serious performance regression, and if 24715 doesn't get committed at the same time, developers and in worst case also users suffer the consequences.
(In reply to Ere Maijala from comment #16) > If it's in a separate issue, how do we make sure bug 24715 and this one are > committed at same time? My concern is that this one alone will introduce a > serious performance regression, and if 24715 doesn't get committed at the > same time, developers and in worst case also users suffer the consequences. We did that before, Ere. And note that I do not confirm that this is a serious performance regression. If so, please make it solid.
(In reply to Marcel de Rooy from comment #17) > (In reply to Ere Maijala from comment #16) > > If it's in a separate issue, how do we make sure bug 24715 and this one are > > committed at same time? My concern is that this one alone will introduce a > > serious performance regression, and if 24715 doesn't get committed at the > > same time, developers and in worst case also users suffer the consequences. > > We did that before, Ere. And note that I do not confirm that this is a > serious performance regression. If so, please make it solid. I failed to realize that the code checks for the pipe character first. So the database call doesn't actually get executed that often. Sorry about the confusion.
Created attachment 99517 [details] [review] Bug 21800: Add tests for repeatable subfields Test plan: Do not apply the second patch [note 1]. Run t/db_dependent/Biblio/TransformKohaToMarc.t Run t/db_dependent/Biblio/TransformMarcToKoha.t Apply the second patch and run them again. Both tests should pass now. Note 1: The TransformKohaToMarc test should fail with something like: # Failed test 'Check 260e' # at t/db_dependent/Biblio/TransformKohaToMarc.t line 60. # got: 'A' # expected: 'A | B' Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Created attachment 99518 [details] [review] Bug 21800: Make TransformKohaToMarc aware of non-repeatable subfields If a kohafield (in Koha to MARC mappings) contains a pipe char (say A | B), we split it up into two subfields A and B in MARC. We will only do that for repeatable subfields now. If the field is not repeatable, the value will just be 'A | B'. Note 1: As bug 10306 and its friends (19096) made the Default framework authoritative, we do no longer have the frameworkcode in this routine. Formally, we should check the corresponding framework. Note 2: Does this impact the reverse operation in TransformMarcToKoha? No, the check on repeatable subfields is done in the interface and not in TransformMarcToKoha. This routine simply translates two instances of the same subfield, say A and B, into the value 'A | B' for a kohafield. Not allowing two instances of a non-repeatable subfield is not in the scope of this report. Test plan: [1] Mark an item field as repeatable in the Default framework. Edit an item. Insert A|B in this field and another not-repeatable field. Save and reopen. Verify that the repeatable field is duplicated and the other one contains the pipe character in the text box. [2] Look for a repeatable subfield in MARC like e.g. 260$c. Go to the cataloguing editor and add A|B in this field. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Created attachment 99519 [details] [review] Bug 21800: Check the correct framework for the repeatable tag This depends on the framework parameter. Which should be added back to the call in C4::Items. Test plan: [1] Mark in Default framework one subfield A repeatable and B not repeatable. Go to item editor. (Work on a biblio in Default framework.) Check saving and reopening these subfields with VAL1 | VAL2. Subfield A should be cloned, B should be glued as entered. [2] Mark in another Framework A not repeatable and B repeatable. Change framework for this biblio. Go to item editor again. Reopen item. Behavior subfields in reverse? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Created attachment 99520 [details] [review] Bug 21800: Add tests for _check_split This new subroutine supports checking the repeatable tag from another framework. (It will be useful for the item editor.) Test plan: Run t/db_dependent/Biblio/TransformKohaToMarc.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
(In reply to Ere Maijala from comment #18) > I failed to realize that the code checks for the pipe character first. So > the database call doesn't actually get executed that often. Sorry about the > confusion. Thanks for signing off!
Created attachment 101364 [details] [review] Bug 21800: Add tests for repeatable subfields Test plan: Do not apply the second patch [note 1]. Run t/db_dependent/Biblio/TransformKohaToMarc.t Run t/db_dependent/Biblio/TransformMarcToKoha.t Apply the second patch and run them again. Both tests should pass now. Note 1: The TransformKohaToMarc test should fail with something like: # Failed test 'Check 260e' # at t/db_dependent/Biblio/TransformKohaToMarc.t line 60. # got: 'A' # expected: 'A | B' Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101365 [details] [review] Bug 21800: Make TransformKohaToMarc aware of non-repeatable subfields If a kohafield (in Koha to MARC mappings) contains a pipe char (say A | B), we split it up into two subfields A and B in MARC. We will only do that for repeatable subfields now. If the field is not repeatable, the value will just be 'A | B'. Note 1: As bug 10306 and its friends (19096) made the Default framework authoritative, we do no longer have the frameworkcode in this routine. Formally, we should check the corresponding framework. Note 2: Does this impact the reverse operation in TransformMarcToKoha? No, the check on repeatable subfields is done in the interface and not in TransformMarcToKoha. This routine simply translates two instances of the same subfield, say A and B, into the value 'A | B' for a kohafield. Not allowing two instances of a non-repeatable subfield is not in the scope of this report. Test plan: [1] Mark an item field as repeatable in the Default framework. Edit an item. Insert A|B in this field and another not-repeatable field. Save and reopen. Verify that the repeatable field is duplicated and the other one contains the pipe character in the text box. [2] Look for a repeatable subfield in MARC like e.g. 260$c. Go to the cataloguing editor and add A|B in this field. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101366 [details] [review] Bug 21800: Check the correct framework for the repeatable tag This depends on the framework parameter. Which should be added back to the call in C4::Items. Test plan: [1] Mark in Default framework one subfield A repeatable and B not repeatable. Go to item editor. (Work on a biblio in Default framework.) Check saving and reopening these subfields with VAL1 | VAL2. Subfield A should be cloned, B should be glued as entered. [2] Mark in another Framework A not repeatable and B repeatable. Change framework for this biblio. Go to item editor again. Reopen item. Behavior subfields in reverse? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101367 [details] [review] Bug 21800: Add tests for _check_split This new subroutine supports checking the repeatable tag from another framework. (It will be useful for the item editor.) Test plan: Run t/db_dependent/Biblio/TransformKohaToMarc.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Nice work everyone! Pushed to master for 20.05
Created attachment 101667 [details] [review] Bug 21800: (RM follow-up) Fix unit tests We were not setting the repeatable flag for the default framework in the Item2Marc tests and we also needed to flush the cash to get our framework changes to be recognised. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Thx for following up, Martin.
enhancement not backported to 19.11.x