Bug 21800

Summary: TransformKohaToMarc should respect non-repeatability of item subfields
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Architecture, internals, and plumbingAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: 1joynelson, ere.maijala, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20882
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24652
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21774
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Bug Depends on:    
Bug Blocks: 24715    
Attachments: Bug 21800: Add tests for repeatable subfields
Bug 21800: Make TransformKohaToMarc aware of non-repeatable subfields
Bug 21800: Add tests for repeatable subfields
Bug 21800: Make TransformKohaToMarc aware of non-repeatable subfields
Bug 21800: Check the correct framework for the repeatable tag
Bug 21800: Add tests for _check_split
Bug 21800: Add tests for _check_split
Bug 21800: Add tests for repeatable subfields
Bug 21800: Make TransformKohaToMarc aware of non-repeatable subfields
Bug 21800: Check the correct framework for the repeatable tag
Bug 21800: Add tests for _check_split
Bug 21800: Add tests for repeatable subfields
Bug 21800: Make TransformKohaToMarc aware of non-repeatable subfields
Bug 21800: Check the correct framework for the repeatable tag
Bug 21800: Add tests for _check_split
Bug 21800: (RM follow-up) Fix unit tests

Description Marcel de Rooy 2018-11-09 07:10:30 UTC
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.
Comment 1 Marcel de Rooy 2020-02-10 12:59:28 UTC
At the same time wondering if we not should allow some item subfields to be repeatable just as some under in MARC21 852 ?
Comment 2 Marcel de Rooy 2020-02-13 11:57:19 UTC
(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.
Comment 3 Marcel de Rooy 2020-02-13 12:12:01 UTC
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>
Comment 4 Marcel de Rooy 2020-02-13 12:12:05 UTC
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>
Comment 5 Marcel de Rooy 2020-02-20 13:25:25 UTC
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>
Comment 6 Marcel de Rooy 2020-02-20 13:25:29 UTC
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>
Comment 7 Marcel de Rooy 2020-02-20 13:25:34 UTC
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>
Comment 8 Marcel de Rooy 2020-02-20 13:25:37 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2020-02-20 13:29:26 UTC
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>
Comment 10 Ere Maijala 2020-02-20 15:16:18 UTC
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?
Comment 11 Ere Maijala 2020-02-21 13:05:54 UTC
Setting to Failed QA until my previous comments have been resolved.
Comment 12 Marcel de Rooy 2020-02-21 17:10:59 UTC
(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 ;)
Comment 13 Ere Maijala 2020-02-24 07:26:28 UTC
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).
Comment 14 Marcel de Rooy 2020-02-24 07:31:26 UTC
(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.
Comment 15 Marcel de Rooy 2020-02-24 07:31:54 UTC
(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.
Comment 16 Ere Maijala 2020-02-24 08:19:28 UTC
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.
Comment 17 Marcel de Rooy 2020-02-24 12:39:09 UTC
(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.
Comment 18 Ere Maijala 2020-02-24 13:37:02 UTC
(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.
Comment 19 Ere Maijala 2020-02-24 13:38:22 UTC
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>
Comment 20 Ere Maijala 2020-02-24 13:38:25 UTC
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>
Comment 21 Ere Maijala 2020-02-24 13:38:28 UTC
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>
Comment 22 Ere Maijala 2020-02-24 13:38:32 UTC
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>
Comment 23 Marcel de Rooy 2020-02-24 13:46:10 UTC
(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!
Comment 24 Katrin Fischer 2020-03-22 13:21:34 UTC
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>
Comment 25 Katrin Fischer 2020-03-22 13:21:37 UTC
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>
Comment 26 Katrin Fischer 2020-03-22 13:21:40 UTC
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>
Comment 27 Katrin Fischer 2020-03-22 13:21:44 UTC
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>
Comment 28 Martin Renvoize 2020-03-24 11:05:04 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 29 Martin Renvoize 2020-03-25 08:06:00 UTC
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>
Comment 30 Martin Renvoize 2020-03-25 08:06:19 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 31 Marcel de Rooy 2020-03-25 12:12:49 UTC
Thx for following up, Martin.
Comment 32 Joy Nelson 2020-04-03 21:39:07 UTC
enhancement not backported to 19.11.x