Bug 19096 - Koha to MARC mappings (Part 2): Make Default authoritative
Summary: Koha to MARC mappings (Part 2): Make Default authoritative
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on: 10306
Blocks: 19097 11046 22721
  Show dependency treegraph
 
Reported: 2017-08-14 07:44 UTC by Marcel de Rooy
Modified: 2020-11-12 15:02 UTC (History)
5 users (show)

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


Attachments
Bug 19096: Make Default authoritative in core modules (19.00 KB, patch)
2017-08-22 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Adjusts unit tests (26.07 KB, patch)
2017-08-22 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Do not allow changes to kohafield within MARC frameworks (5.36 KB, patch)
2017-08-22 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Online help changes (3.27 KB, patch)
2017-08-22 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Restructure MarcSubfieldStructures.t first (3.25 KB, patch)
2017-08-22 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Add dbrev to sync kohafield in all frameworks (10.64 KB, patch)
2017-08-22 12:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Make Default authoritative in core modules (19.00 KB, patch)
2017-08-24 10:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Adjusts unit tests (26.06 KB, patch)
2017-08-24 10:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Do not allow changes to kohafield within MARC frameworks (5.36 KB, patch)
2017-08-24 10:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Online help changes (3.27 KB, patch)
2017-08-24 10:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Restructure MarcSubfieldStructures.t first (3.25 KB, patch)
2017-08-24 10:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Add dbrev to sync kohafield in all frameworks (10.64 KB, patch)
2017-08-24 10:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Make Default authoritative in core modules (19.04 KB, patch)
2017-10-02 09:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Adjusts unit tests (26.06 KB, patch)
2017-10-02 09:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Do not allow changes to kohafield within MARC frameworks (5.36 KB, patch)
2017-10-02 09:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Online help changes (3.27 KB, patch)
2017-10-02 09:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Restructure MarcSubfieldStructures.t first (3.25 KB, patch)
2017-10-02 09:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Add dbrev to sync kohafield in all frameworks (10.64 KB, patch)
2017-10-02 09:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Make Default authoritative in core modules (19.05 KB, patch)
2017-10-04 07:10 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19096: Adjusts unit tests (26.08 KB, patch)
2017-10-04 07:10 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19096: Do not allow changes to kohafield within MARC frameworks (5.37 KB, patch)
2017-10-04 07:10 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19096: Online help changes (3.28 KB, patch)
2017-10-04 07:10 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19096: Restructure MarcSubfieldStructures.t first (3.27 KB, patch)
2017-10-04 07:10 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19096: Add dbrev to sync kohafield in all frameworks (10.66 KB, patch)
2017-10-04 07:10 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19096: Make Default authoritative in core modules (19.09 KB, patch)
2017-10-17 13:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Adjusts unit tests (26.12 KB, patch)
2017-10-17 13:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Do not allow changes to kohafield within MARC frameworks (5.41 KB, patch)
2017-10-17 13:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Online help changes (3.32 KB, patch)
2017-10-17 13:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Restructure MarcSubfieldStructures.t first (3.31 KB, patch)
2017-10-17 13:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Add dbrev to sync kohafield in all frameworks (10.70 KB, patch)
2017-10-17 13:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Make Default authoritative in core modules (19.11 KB, patch)
2017-10-27 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19096: Adjusts unit tests (26.13 KB, patch)
2017-10-27 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19096: Do not allow changes to kohafield within MARC frameworks (5.43 KB, patch)
2017-10-27 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19096: Online help changes (3.34 KB, patch)
2017-10-27 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19096: Restructure MarcSubfieldStructures.t first (3.33 KB, patch)
2017-10-27 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19096: Add dbrev to sync kohafield in all frameworks (10.71 KB, patch)
2017-10-27 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19096: (QA follow-up) Move two routines out of Koha::MSS (12.87 KB, patch)
2017-12-08 07:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19096: Use raw SQL queries instead for the update DB entry (5.01 KB, patch)
2017-12-08 14:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19096: Move the sql queries to the update DB entry (13.42 KB, patch)
2017-12-08 14:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19096: Add dbrev to sync kohafield in all frameworks (9.13 KB, patch)
2017-12-11 10:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2017-08-14 07:44:57 UTC

    
Comment 1 Marcel de Rooy 2017-08-14 11:33:47 UTC
After community feedback on the dev mailing list (see the thread on "MARC frameworks in Koha" in August 2017), it seems appropriate to propose making the Default framework authoritative for Koha to MARC mappings. This patch set will enforce that, building on top of the multiple mappings per kohafield in bug 10306.

In other words: Routines like GetMarcFromKohaField will no longer look at the frameworkcode, but always consult Default. And it will no longer be allowed to change a kohafield via MARC Frameworks (marc_subfields_structure). You need to go through "Koha to MARC mapping" (koha2marclinks).

A follow-up report (bug 19097) will do some housekeeping tasks like removing all framework parameters from various calls, etc.
Comment 2 Marcel de Rooy 2017-08-22 12:52:44 UTC
Created attachment 66333 [details] [review]
Bug 19096: Make Default authoritative in core modules

After feedback from the dev mailing list, it seems appropriate here to
propose making the Default framework authoritative for Koha to MARC
mappings. This implies checking only the Default framework in the
routines:

[1] GetMarcFromKohaField: The parameter frameworkcode is removed. A
    follow-up report (19097) will update the calls not adjusted here.
    This is safe since the parameter is silently ignored.
[2] GetMarcSubfieldStructureFromKohaField: Framework parameter is removed
    and calls are adjusted. Includes acquisitions_stats.pl.
[3] TransformKohaToMarc: The parameter is removed; all calls are verified
    or adjusted.
[4] TransformMarcToKoha: The parameter is no longer used and will be
    removed in a follow-up report (19097). It always goes to Default now.
[5] TransformMarcToKohaOneField: The parameter is removed and all calls
    are adjusted. Including: Breeding, XISBN and MetadataRecord modules.
[6] C4::Koha::IsKohaFieldLinked: This routine was called only once (in
    C4::Items::_build_default_values_for_mod_marc. It can be replaced by
    calling GetMarcFromKohaField. If there is no kohafield linked, undef
    is returned. (Corresponding unit test is removed here.)
[7] C4::Items::ModItemFromMarc: The helper routine
    _build_default_values_for_mod_marc does no longer have a framework
    parameter. The cache key default_value_for_mod_marc- is no longer
    combined with a frameworkcode. Three admin scripts are adjusted
    accordingly; some tests will be corrected in the next patch.

Test plan:
See next patch. That patch adjusts all tests involved.
Comment 3 Marcel de Rooy 2017-08-22 12:52:48 UTC
Created attachment 66334 [details] [review]
Bug 19096: Adjusts unit tests

The subroutines listed in the former patch are extensively tested in
Biblio.t, TransformKohaToMarc.t and TransformMarcToKoha.t.
These tests do no longer use new frameworks to add mappings.

In Biblio.t and TransformMarcToKoha.t we also test passing an individual
framework code (for an empty framework) to see if Default is still used.
Note that this parameter will be removed later on.

In Items.t the cache keys MarcStructure-0- and MarcStructure-1- are not
touched, so they do not need to be cleared. The cache key
default_value_for_mod_marc- should be used without framework now.
Similar adjustments in Items/AutomaticItemModificationByAge.t and
Reserves.t.

Furthermore the subtest for _build_default_values_for_mod_marc in Items.t
is adjusted since framework is no longer relevant. The biblio record with
items is created in a new framework, but the mappings from Default are
consulted.

Test plan:
Run all adjusted unit tests.
Comment 4 Marcel de Rooy 2017-08-22 12:52:52 UTC
Created attachment 66335 [details] [review]
Bug 19096: Do not allow changes to kohafield within MARC frameworks

This patch makes two changes in the script for managing the MARC
frameworks in order to support making the Default authoritative
as for Koha to MARC mappings:

[1] Disable the kohafield select combo. Add a hidden input to save value.
[2] When a new subfield is added, the kohafield is still empty and should
    be overwritten with its Default counterpart.
    Note: Although we could leave the field empty, since Koha looks at the
    Default framework only, it does not cost much to keep these fields in
    sync with Default and perhaps catch a bug when someone somewhere looks
    in the wrong framework.

Note: The description of Koha to MARC mapping on admin-home has been
adjusted accordingly (removing last two lines).

Test plan:
[1] Add a new tag and subfield in Default.
[2] Map it to a kohafield in koha2marclinks.pl
[3] Add the same tag and subfield in another framework.
    Verify that the kohafield is updated after you saved the subfield.
Comment 5 Marcel de Rooy 2017-08-22 12:52:56 UTC
Created attachment 66336 [details] [review]
Bug 19096: Online help changes

On the page admin/marc_subfields_structure we now tell that Koha link
has been disabled and refer to the help for koha2marclinks.
On the page admin/koha2marclinks we mention that the Default mapping is
considered authoritative now.

Test plan:
Read these online help pages and verify that the changes make sense.
Comment 6 Marcel de Rooy 2017-08-22 12:52:59 UTC
Created attachment 66337 [details] [review]
Bug 19096: Restructure MarcSubfieldStructures.t first

Before adding more tests, move all current tests in one subtest.

Test plan:
Run t/db_dependent/Koha/MarcSubfieldStructures.t
Comment 7 Marcel de Rooy 2017-08-22 12:53:03 UTC
Created attachment 66338 [details] [review]
Bug 19096: Add dbrev to sync kohafield in all frameworks

The dbrev will use two new routines in MarcSubfieldStructures:
[1] get_kohafield_exceptions is used to report deviating kohafields in
    the additional frameworks,
[2] sync_kohafield is used to reset kohafield in the other frameworks to
    the mapping in Default.

Test plan:

Unit test and database revision:
[1] Run t/db_dependent/Koha/MarcSubfieldStructures.t
[2] Verify that your Default 100a is mapped to biblio.author. Go to another
    framework and clear the mapping via mysql command line:
    UPDATE marc_subfield_structure SET kohafield=NULL WHERE frameworkcode=[your_framework] AND tagfield='100' AND tagsubfield='a';
[3] Run the db revision. It should report that 100a was adjusted.
[4] Check in admin/marc_subfield_structure that your 100a is mapped to
    biblio.author again in that framework.

Additional interface testing (ensuring that the changes on this report do
not interfere with multiple mappings):
[5] Make two mappings for copyrightdate: 260c and 264a. And make two
    mappings for biblioitems.pages: 300a and say 300g. Toggle with some
    field values in those fields in the cataloging editor and verify the
    contents of biblio.copyrightdate and biblioitems.pages. The former
    should contain one year (due to additional logic) and the latter should
    contain A | B if both fields are filled.
    Remove the mapping for 300g.
[6] Set AcqCreateItem to ordering or placing. Verify that you can still
    add or receive an order as usual.
[7] Add a mapping for itemcallnumber to 952f (this should remove the one
    for coded_location_qualifier). This is very unusual but serves well in
    testing multiple mappings for items.
    Add or receive an order (fill 952f and 952o) with same and/or different
    values. Verify the contents of items.callnumber. (Check with regular
    item editor; see note.)
    Do a similar edit in the regular item editor.
    Note: You should expect to see A | B in both 952f and 925o if both
    fields are filled with a different value.
    Set items.coded_location_qualifier back to 952f in koha2marclinks.

    Note: When AcqCreateItem==ordering, you will not see A|B in the callno
    field when adding an item on neworderempty.pl. But when you submit
    the main form, addorder.pl is called. At that time an item is created
    and you will see that A|B is in both fields (952f and 952o).
Comment 8 Marcel de Rooy 2017-08-22 13:00:39 UTC
Note for QA:
 FAIL   C4/Koha.pm
   OK     critic
   OK     forbidden patterns
   OK     git manipulation
   OK     pod
   FAIL   pod coverage
                POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666

This is a false negative. I stumbled over it already before. If you remove one routine from a module without full coverage, the coverage will drop a little bit. Qa tools wrongly blames me for that now..
Comment 9 Marcel de Rooy 2017-08-24 10:30:24 UTC
Created attachment 66406 [details] [review]
Bug 19096: Make Default authoritative in core modules

After feedback from the dev mailing list, it seems appropriate here to
propose making the Default framework authoritative for Koha to MARC
mappings. This implies checking only the Default framework in the
routines:

[1] GetMarcFromKohaField: The parameter frameworkcode is removed. A
    follow-up report (19097) will update the calls not adjusted here.
    This is safe since the parameter is silently ignored.
[2] GetMarcSubfieldStructureFromKohaField: Framework parameter is removed
    and calls are adjusted. Includes acquisitions_stats.pl.
[3] TransformKohaToMarc: The parameter is removed; all calls are verified
    or adjusted.
[4] TransformMarcToKoha: The parameter is no longer used and will be
    removed in a follow-up report (19097). It always goes to Default now.
[5] TransformMarcToKohaOneField: The parameter is removed and all calls
    are adjusted. Including: Breeding, XISBN and MetadataRecord modules.
[6] C4::Koha::IsKohaFieldLinked: This routine was called only once (in
    C4::Items::_build_default_values_for_mod_marc. It can be replaced by
    calling GetMarcFromKohaField. If there is no kohafield linked, undef
    is returned. (Corresponding unit test is removed here.)
[7] C4::Items::ModItemFromMarc: The helper routine
    _build_default_values_for_mod_marc does no longer have a framework
    parameter. The cache key default_value_for_mod_marc- is no longer
    combined with a frameworkcode. Three admin scripts are adjusted
    accordingly; some tests will be corrected in the next patch.

Test plan:
See next patch. That patch adjusts all tests involved.
Comment 10 Marcel de Rooy 2017-08-24 10:30:28 UTC
Created attachment 66407 [details] [review]
Bug 19096: Adjusts unit tests

The subroutines listed in the former patch are extensively tested in
Biblio.t, TransformKohaToMarc.t and TransformMarcToKoha.t.
These tests do no longer use new frameworks to add mappings.

In Biblio.t and TransformMarcToKoha.t we also test passing an individual
framework code (for an empty framework) to see if Default is still used.
Note that this parameter will be removed later on.

In Items.t the cache keys MarcStructure-0- and MarcStructure-1- are not
touched, so they do not need to be cleared. The cache key
default_value_for_mod_marc- should be used without framework now.
Similar adjustments in Items/AutomaticItemModificationByAge.t and
Reserves.t.

Furthermore the subtest for _build_default_values_for_mod_marc in Items.t
is adjusted since framework is no longer relevant. The biblio record with
items is created in a new framework, but the mappings from Default are
consulted.

Test plan:
Run all adjusted unit tests.
Comment 11 Marcel de Rooy 2017-08-24 10:30:32 UTC
Created attachment 66408 [details] [review]
Bug 19096: Do not allow changes to kohafield within MARC frameworks

This patch makes two changes in the script for managing the MARC
frameworks in order to support making the Default authoritative
as for Koha to MARC mappings:

[1] Disable the kohafield select combo. Add a hidden input to save value.
[2] When a new subfield is added, the kohafield is still empty and should
    be overwritten with its Default counterpart.
    Note: Although we could leave the field empty, since Koha looks at the
    Default framework only, it does not cost much to keep these fields in
    sync with Default and perhaps catch a bug when someone somewhere looks
    in the wrong framework.

Note: The description of Koha to MARC mapping on admin-home has been
adjusted accordingly (removing last two lines).

Test plan:
[1] Add a new tag and subfield in Default.
[2] Map it to a kohafield in koha2marclinks.pl
[3] Add the same tag and subfield in another framework.
    Verify that the kohafield is updated after you saved the subfield.
Comment 12 Marcel de Rooy 2017-08-24 10:30:36 UTC
Created attachment 66409 [details] [review]
Bug 19096: Online help changes

On the page admin/marc_subfields_structure we now tell that Koha link
has been disabled and refer to the help for koha2marclinks.
On the page admin/koha2marclinks we mention that the Default mapping is
considered authoritative now.

Test plan:
Read these online help pages and verify that the changes make sense.
Comment 13 Marcel de Rooy 2017-08-24 10:30:40 UTC
Created attachment 66410 [details] [review]
Bug 19096: Restructure MarcSubfieldStructures.t first

Before adding more tests, move all current tests in one subtest.

Test plan:
Run t/db_dependent/Koha/MarcSubfieldStructures.t
Comment 14 Marcel de Rooy 2017-08-24 10:30:44 UTC
Created attachment 66411 [details] [review]
Bug 19096: Add dbrev to sync kohafield in all frameworks

The dbrev will use two new routines in MarcSubfieldStructures:
[1] get_kohafield_exceptions is used to report deviating kohafields in
    the additional frameworks,
[2] sync_kohafield is used to reset kohafield in the other frameworks to
    the mapping in Default.

Test plan:

Unit test and database revision:
[1] Run t/db_dependent/Koha/MarcSubfieldStructures.t
[2] Verify that your Default 100a is mapped to biblio.author. Go to another
    framework and clear the mapping via mysql command line:
    UPDATE marc_subfield_structure SET kohafield=NULL WHERE frameworkcode=[your_framework] AND tagfield='100' AND tagsubfield='a';
[3] Run the db revision. It should report that 100a was adjusted.
[4] Check in admin/marc_subfield_structure that your 100a is mapped to
    biblio.author again in that framework.

Additional interface testing (ensuring that the changes on this report do
not interfere with multiple mappings):
[5] Make two mappings for copyrightdate: 260c and 264a. And make two
    mappings for biblioitems.pages: 300a and say 300g. Toggle with some
    field values in those fields in the cataloging editor and verify the
    contents of biblio.copyrightdate and biblioitems.pages. The former
    should contain one year (due to additional logic) and the latter should
    contain A | B if both fields are filled.
    Remove the mapping for 300g.
[6] Set AcqCreateItem to ordering or placing. Verify that you can still
    add or receive an order as usual.
[7] Add a mapping for itemcallnumber to 952f (this should remove the one
    for coded_location_qualifier). This is very unusual but serves well in
    testing multiple mappings for items.
    Add or receive an order (fill 952f and 952o) with same and/or different
    values. Verify the contents of items.callnumber. (Check with regular
    item editor; see note.)
    Do a similar edit in the regular item editor.
    Note: You should expect to see A | B in both 952f and 925o if both
    fields are filled with a different value.
    Set items.coded_location_qualifier back to 952f in koha2marclinks.

    Note: When AcqCreateItem==ordering, you will not see A|B in the callno
    field when adding an item on neworderempty.pl. But when you submit
    the main form, addorder.pl is called. At that time an item is created
    and you will see that A|B is in both fields (952f and 952o).
Comment 15 Marcel de Rooy 2017-08-24 10:31:15 UTC
Trivial rebase on top of correction in 10306
Comment 16 Marcel de Rooy 2017-10-02 09:55:25 UTC
Created attachment 67508 [details] [review]
Bug 19096: Make Default authoritative in core modules

After feedback from the dev mailing list, it seems appropriate here to
propose making the Default framework authoritative for Koha to MARC
mappings. This implies checking only the Default framework in the
routines:

[1] GetMarcFromKohaField: The parameter frameworkcode is removed. A
    follow-up report (19097) will update the calls not adjusted here.
    This is safe since the parameter is silently ignored.
[2] GetMarcSubfieldStructureFromKohaField: Framework parameter is removed
    and calls are adjusted. Includes acquisitions_stats.pl.
[3] TransformKohaToMarc: The parameter is removed; all calls are verified
    or adjusted.
[4] TransformMarcToKoha: The parameter is no longer used and will be
    removed in a follow-up report (19097). It always goes to Default now.
[5] TransformMarcToKohaOneField: The parameter is removed and all calls
    are adjusted. Including: Breeding, XISBN and MetadataRecord modules.
[6] C4::Koha::IsKohaFieldLinked: This routine was called only once (in
    C4::Items::_build_default_values_for_mod_marc. It can be replaced by
    calling GetMarcFromKohaField. If there is no kohafield linked, undef
    is returned. (Corresponding unit test is removed here.)
[7] C4::Items::ModItemFromMarc: The helper routine
    _build_default_values_for_mod_marc does no longer have a framework
    parameter. The cache key default_value_for_mod_marc- is no longer
    combined with a frameworkcode. Three admin scripts are adjusted
    accordingly; some tests will be corrected in the next patch.

Test plan:
See next patch. That patch adjusts all tests involved.
Comment 17 Marcel de Rooy 2017-10-02 09:55:29 UTC
Created attachment 67509 [details] [review]
Bug 19096: Adjusts unit tests

The subroutines listed in the former patch are extensively tested in
Biblio.t, TransformKohaToMarc.t and TransformMarcToKoha.t.
These tests do no longer use new frameworks to add mappings.

In Biblio.t and TransformMarcToKoha.t we also test passing an individual
framework code (for an empty framework) to see if Default is still used.
Note that this parameter will be removed later on.

In Items.t the cache keys MarcStructure-0- and MarcStructure-1- are not
touched, so they do not need to be cleared. The cache key
default_value_for_mod_marc- should be used without framework now.
Similar adjustments in Items/AutomaticItemModificationByAge.t and
Reserves.t.

Furthermore the subtest for _build_default_values_for_mod_marc in Items.t
is adjusted since framework is no longer relevant. The biblio record with
items is created in a new framework, but the mappings from Default are
consulted.

Test plan:
Run all adjusted unit tests.
Comment 18 Marcel de Rooy 2017-10-02 09:55:32 UTC
Created attachment 67510 [details] [review]
Bug 19096: Do not allow changes to kohafield within MARC frameworks

This patch makes two changes in the script for managing the MARC
frameworks in order to support making the Default authoritative
as for Koha to MARC mappings:

[1] Disable the kohafield select combo. Add a hidden input to save value.
[2] When a new subfield is added, the kohafield is still empty and should
    be overwritten with its Default counterpart.
    Note: Although we could leave the field empty, since Koha looks at the
    Default framework only, it does not cost much to keep these fields in
    sync with Default and perhaps catch a bug when someone somewhere looks
    in the wrong framework.

Note: The description of Koha to MARC mapping on admin-home has been
adjusted accordingly (removing last two lines).

Test plan:
[1] Add a new tag and subfield in Default.
[2] Map it to a kohafield in koha2marclinks.pl
[3] Add the same tag and subfield in another framework.
    Verify that the kohafield is updated after you saved the subfield.
Comment 19 Marcel de Rooy 2017-10-02 09:55:36 UTC
Created attachment 67511 [details] [review]
Bug 19096: Online help changes

On the page admin/marc_subfields_structure we now tell that Koha link
has been disabled and refer to the help for koha2marclinks.
On the page admin/koha2marclinks we mention that the Default mapping is
considered authoritative now.

Test plan:
Read these online help pages and verify that the changes make sense.
Comment 20 Marcel de Rooy 2017-10-02 09:55:40 UTC
Created attachment 67512 [details] [review]
Bug 19096: Restructure MarcSubfieldStructures.t first

Before adding more tests, move all current tests in one subtest.

Test plan:
Run t/db_dependent/Koha/MarcSubfieldStructures.t
Comment 21 Marcel de Rooy 2017-10-02 09:55:44 UTC
Created attachment 67513 [details] [review]
Bug 19096: Add dbrev to sync kohafield in all frameworks

The dbrev will use two new routines in MarcSubfieldStructures:
[1] get_kohafield_exceptions is used to report deviating kohafields in
    the additional frameworks,
[2] sync_kohafield is used to reset kohafield in the other frameworks to
    the mapping in Default.

Test plan:

Unit test and database revision:
[1] Run t/db_dependent/Koha/MarcSubfieldStructures.t
[2] Verify that your Default 100a is mapped to biblio.author. Go to another
    framework and clear the mapping via mysql command line:
    UPDATE marc_subfield_structure SET kohafield=NULL WHERE frameworkcode=[your_framework] AND tagfield='100' AND tagsubfield='a';
[3] Run the db revision. It should report that 100a was adjusted.
[4] Check in admin/marc_subfield_structure that your 100a is mapped to
    biblio.author again in that framework.

Additional interface testing (ensuring that the changes on this report do
not interfere with multiple mappings):
[5] Make two mappings for copyrightdate: 260c and 264a. And make two
    mappings for biblioitems.pages: 300a and say 300g. Toggle with some
    field values in those fields in the cataloging editor and verify the
    contents of biblio.copyrightdate and biblioitems.pages. The former
    should contain one year (due to additional logic) and the latter should
    contain A | B if both fields are filled.
    Remove the mapping for 300g.
[6] Set AcqCreateItem to ordering or placing. Verify that you can still
    add or receive an order as usual.
[7] Add a mapping for itemcallnumber to 952f (this should remove the one
    for coded_location_qualifier). This is very unusual but serves well in
    testing multiple mappings for items.
    Add or receive an order (fill 952f and 952o) with same and/or different
    values. Verify the contents of items.callnumber. (Check with regular
    item editor; see note.)
    Do a similar edit in the regular item editor.
    Note: You should expect to see A | B in both 952f and 925o if both
    fields are filled with a different value.
    Set items.coded_location_qualifier back to 952f in koha2marclinks.

    Note: When AcqCreateItem==ordering, you will not see A|B in the callno
    field when adding an item on neworderempty.pl. But when you submit
    the main form, addorder.pl is called. At that time an item is created
    and you will see that A|B is in both fields (952f and 952o).
Comment 22 Marcel de Rooy 2017-10-02 09:56:19 UTC
Simple rebase
Comment 23 David Cook 2017-10-03 23:53:46 UTC
It would be great to have a single test plan in a comment. I'm not entirely sure what I need to test here... 

I'll do my best to find all the relevant test plans but we'll see.
Comment 24 Josef Moravec 2017-10-04 07:10:31 UTC
Created attachment 67590 [details] [review]
Bug 19096: Make Default authoritative in core modules

After feedback from the dev mailing list, it seems appropriate here to
propose making the Default framework authoritative for Koha to MARC
mappings. This implies checking only the Default framework in the
routines:

[1] GetMarcFromKohaField: The parameter frameworkcode is removed. A
    follow-up report (19097) will update the calls not adjusted here.
    This is safe since the parameter is silently ignored.
[2] GetMarcSubfieldStructureFromKohaField: Framework parameter is removed
    and calls are adjusted. Includes acquisitions_stats.pl.
[3] TransformKohaToMarc: The parameter is removed; all calls are verified
    or adjusted.
[4] TransformMarcToKoha: The parameter is no longer used and will be
    removed in a follow-up report (19097). It always goes to Default now.
[5] TransformMarcToKohaOneField: The parameter is removed and all calls
    are adjusted. Including: Breeding, XISBN and MetadataRecord modules.
[6] C4::Koha::IsKohaFieldLinked: This routine was called only once (in
    C4::Items::_build_default_values_for_mod_marc. It can be replaced by
    calling GetMarcFromKohaField. If there is no kohafield linked, undef
    is returned. (Corresponding unit test is removed here.)
[7] C4::Items::ModItemFromMarc: The helper routine
    _build_default_values_for_mod_marc does no longer have a framework
    parameter. The cache key default_value_for_mod_marc- is no longer
    combined with a frameworkcode. Three admin scripts are adjusted
    accordingly; some tests will be corrected in the next patch.

Test plan:
See next patch. That patch adjusts all tests involved.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 25 Josef Moravec 2017-10-04 07:10:35 UTC
Created attachment 67591 [details] [review]
Bug 19096: Adjusts unit tests

The subroutines listed in the former patch are extensively tested in
Biblio.t, TransformKohaToMarc.t and TransformMarcToKoha.t.
These tests do no longer use new frameworks to add mappings.

In Biblio.t and TransformMarcToKoha.t we also test passing an individual
framework code (for an empty framework) to see if Default is still used.
Note that this parameter will be removed later on.

In Items.t the cache keys MarcStructure-0- and MarcStructure-1- are not
touched, so they do not need to be cleared. The cache key
default_value_for_mod_marc- should be used without framework now.
Similar adjustments in Items/AutomaticItemModificationByAge.t and
Reserves.t.

Furthermore the subtest for _build_default_values_for_mod_marc in Items.t
is adjusted since framework is no longer relevant. The biblio record with
items is created in a new framework, but the mappings from Default are
consulted.

Test plan:
Run all adjusted unit tests.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 26 Josef Moravec 2017-10-04 07:10:38 UTC
Created attachment 67592 [details] [review]
Bug 19096: Do not allow changes to kohafield within MARC frameworks

This patch makes two changes in the script for managing the MARC
frameworks in order to support making the Default authoritative
as for Koha to MARC mappings:

[1] Disable the kohafield select combo. Add a hidden input to save value.
[2] When a new subfield is added, the kohafield is still empty and should
    be overwritten with its Default counterpart.
    Note: Although we could leave the field empty, since Koha looks at the
    Default framework only, it does not cost much to keep these fields in
    sync with Default and perhaps catch a bug when someone somewhere looks
    in the wrong framework.

Note: The description of Koha to MARC mapping on admin-home has been
adjusted accordingly (removing last two lines).

Test plan:
[1] Add a new tag and subfield in Default.
[2] Map it to a kohafield in koha2marclinks.pl
[3] Add the same tag and subfield in another framework.
    Verify that the kohafield is updated after you saved the subfield.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 27 Josef Moravec 2017-10-04 07:10:42 UTC
Created attachment 67593 [details] [review]
Bug 19096: Online help changes

On the page admin/marc_subfields_structure we now tell that Koha link
has been disabled and refer to the help for koha2marclinks.
On the page admin/koha2marclinks we mention that the Default mapping is
considered authoritative now.

Test plan:
Read these online help pages and verify that the changes make sense.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 28 Josef Moravec 2017-10-04 07:10:45 UTC
Created attachment 67594 [details] [review]
Bug 19096: Restructure MarcSubfieldStructures.t first

Before adding more tests, move all current tests in one subtest.

Test plan:
Run t/db_dependent/Koha/MarcSubfieldStructures.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 29 Josef Moravec 2017-10-04 07:10:48 UTC
Created attachment 67595 [details] [review]
Bug 19096: Add dbrev to sync kohafield in all frameworks

The dbrev will use two new routines in MarcSubfieldStructures:
[1] get_kohafield_exceptions is used to report deviating kohafields in
    the additional frameworks,
[2] sync_kohafield is used to reset kohafield in the other frameworks to
    the mapping in Default.

Test plan:

Unit test and database revision:
[1] Run t/db_dependent/Koha/MarcSubfieldStructures.t
[2] Verify that your Default 100a is mapped to biblio.author. Go to another
    framework and clear the mapping via mysql command line:
    UPDATE marc_subfield_structure SET kohafield=NULL WHERE frameworkcode=[your_framework] AND tagfield='100' AND tagsubfield='a';
[3] Run the db revision. It should report that 100a was adjusted.
[4] Check in admin/marc_subfield_structure that your 100a is mapped to
    biblio.author again in that framework.

Additional interface testing (ensuring that the changes on this report do
not interfere with multiple mappings):
[5] Make two mappings for copyrightdate: 260c and 264a. And make two
    mappings for biblioitems.pages: 300a and say 300g. Toggle with some
    field values in those fields in the cataloging editor and verify the
    contents of biblio.copyrightdate and biblioitems.pages. The former
    should contain one year (due to additional logic) and the latter should
    contain A | B if both fields are filled.
    Remove the mapping for 300g.
[6] Set AcqCreateItem to ordering or placing. Verify that you can still
    add or receive an order as usual.
[7] Add a mapping for itemcallnumber to 952f (this should remove the one
    for coded_location_qualifier). This is very unusual but serves well in
    testing multiple mappings for items.
    Add or receive an order (fill 952f and 952o) with same and/or different
    values. Verify the contents of items.callnumber. (Check with regular
    item editor; see note.)
    Do a similar edit in the regular item editor.
    Note: You should expect to see A | B in both 952f and 925o if both
    fields are filled with a different value.
    Set items.coded_location_qualifier back to 952f in koha2marclinks.

    Note: When AcqCreateItem==ordering, you will not see A|B in the callno
    field when adding an item on neworderempty.pl. But when you submit
    the main form, addorder.pl is called. At that time an item is created
    and you will see that A|B is in both fields (952f and 952o).

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 30 Marcel de Rooy 2017-10-04 11:48:19 UTC
(In reply to Josef Moravec from comment #29)
> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Thanks, Josef !
Comment 31 Marcel de Rooy 2017-10-04 11:50:26 UTC
(In reply to David Cook from comment #23)
> It would be great to have a single test plan in a comment. I'm not entirely
> sure what I need to test here... 
> 
> I'll do my best to find all the relevant test plans but we'll see.

Sorry that you could not find what you needed. But thx for trying.
Each patch (except the first one) had a test plan in the commit message and also visible in the individual Bugzilla comments.
Comment 32 Marcel de Rooy 2017-10-17 12:47:29 UTC
Rebase forthcoming
Comment 33 Marcel de Rooy 2017-10-17 13:00:14 UTC
Created attachment 68205 [details] [review]
Bug 19096: Make Default authoritative in core modules

After feedback from the dev mailing list, it seems appropriate here to
propose making the Default framework authoritative for Koha to MARC
mappings. This implies checking only the Default framework in the
routines:

[1] GetMarcFromKohaField: The parameter frameworkcode is removed. A
    follow-up report (19097) will update the calls not adjusted here.
    This is safe since the parameter is silently ignored.
[2] GetMarcSubfieldStructureFromKohaField: Framework parameter is removed
    and calls are adjusted. Includes acquisitions_stats.pl.
[3] TransformKohaToMarc: The parameter is removed; all calls are verified
    or adjusted.
[4] TransformMarcToKoha: The parameter is no longer used and will be
    removed in a follow-up report (19097). It always goes to Default now.
[5] TransformMarcToKohaOneField: The parameter is removed and all calls
    are adjusted. Including: Breeding, XISBN and MetadataRecord modules.
[6] C4::Koha::IsKohaFieldLinked: This routine was called only once (in
    C4::Items::_build_default_values_for_mod_marc. It can be replaced by
    calling GetMarcFromKohaField. If there is no kohafield linked, undef
    is returned. (Corresponding unit test is removed here.)
[7] C4::Items::ModItemFromMarc: The helper routine
    _build_default_values_for_mod_marc does no longer have a framework
    parameter. The cache key default_value_for_mod_marc- is no longer
    combined with a frameworkcode. Three admin scripts are adjusted
    accordingly; some tests will be corrected in the next patch.

Test plan:
See next patch. That patch adjusts all tests involved.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 34 Marcel de Rooy 2017-10-17 13:00:18 UTC
Created attachment 68206 [details] [review]
Bug 19096: Adjusts unit tests

The subroutines listed in the former patch are extensively tested in
Biblio.t, TransformKohaToMarc.t and TransformMarcToKoha.t.
These tests do no longer use new frameworks to add mappings.

In Biblio.t and TransformMarcToKoha.t we also test passing an individual
framework code (for an empty framework) to see if Default is still used.
Note that this parameter will be removed later on.

In Items.t the cache keys MarcStructure-0- and MarcStructure-1- are not
touched, so they do not need to be cleared. The cache key
default_value_for_mod_marc- should be used without framework now.
Similar adjustments in Items/AutomaticItemModificationByAge.t and
Reserves.t.

Furthermore the subtest for _build_default_values_for_mod_marc in Items.t
is adjusted since framework is no longer relevant. The biblio record with
items is created in a new framework, but the mappings from Default are
consulted.

Test plan:
Run all adjusted unit tests.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 35 Marcel de Rooy 2017-10-17 13:00:23 UTC
Created attachment 68207 [details] [review]
Bug 19096: Do not allow changes to kohafield within MARC frameworks

This patch makes two changes in the script for managing the MARC
frameworks in order to support making the Default authoritative
as for Koha to MARC mappings:

[1] Disable the kohafield select combo. Add a hidden input to save value.
[2] When a new subfield is added, the kohafield is still empty and should
    be overwritten with its Default counterpart.
    Note: Although we could leave the field empty, since Koha looks at the
    Default framework only, it does not cost much to keep these fields in
    sync with Default and perhaps catch a bug when someone somewhere looks
    in the wrong framework.

Note: The description of Koha to MARC mapping on admin-home has been
adjusted accordingly (removing last two lines).

Test plan:
[1] Add a new tag and subfield in Default.
[2] Map it to a kohafield in koha2marclinks.pl
[3] Add the same tag and subfield in another framework.
    Verify that the kohafield is updated after you saved the subfield.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 36 Marcel de Rooy 2017-10-17 13:00:27 UTC
Created attachment 68208 [details] [review]
Bug 19096: Online help changes

On the page admin/marc_subfields_structure we now tell that Koha link
has been disabled and refer to the help for koha2marclinks.
On the page admin/koha2marclinks we mention that the Default mapping is
considered authoritative now.

Test plan:
Read these online help pages and verify that the changes make sense.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 37 Marcel de Rooy 2017-10-17 13:00:31 UTC
Created attachment 68209 [details] [review]
Bug 19096: Restructure MarcSubfieldStructures.t first

Before adding more tests, move all current tests in one subtest.

Test plan:
Run t/db_dependent/Koha/MarcSubfieldStructures.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 38 Marcel de Rooy 2017-10-17 13:00:37 UTC
Created attachment 68210 [details] [review]
Bug 19096: Add dbrev to sync kohafield in all frameworks

The dbrev will use two new routines in MarcSubfieldStructures:
[1] get_kohafield_exceptions is used to report deviating kohafields in
    the additional frameworks,
[2] sync_kohafield is used to reset kohafield in the other frameworks to
    the mapping in Default.

Test plan:

Unit test and database revision:
[1] Run t/db_dependent/Koha/MarcSubfieldStructures.t
[2] Verify that your Default 100a is mapped to biblio.author. Go to another
    framework and clear the mapping via mysql command line:
    UPDATE marc_subfield_structure SET kohafield=NULL WHERE frameworkcode=[your_framework] AND tagfield='100' AND tagsubfield='a';
[3] Run the db revision. It should report that 100a was adjusted.
[4] Check in admin/marc_subfield_structure that your 100a is mapped to
    biblio.author again in that framework.

Additional interface testing (ensuring that the changes on this report do
not interfere with multiple mappings):
[5] Make two mappings for copyrightdate: 260c and 264a. And make two
    mappings for biblioitems.pages: 300a and say 300g. Toggle with some
    field values in those fields in the cataloging editor and verify the
    contents of biblio.copyrightdate and biblioitems.pages. The former
    should contain one year (due to additional logic) and the latter should
    contain A | B if both fields are filled.
    Remove the mapping for 300g.
[6] Set AcqCreateItem to ordering or placing. Verify that you can still
    add or receive an order as usual.
[7] Add a mapping for itemcallnumber to 952f (this should remove the one
    for coded_location_qualifier). This is very unusual but serves well in
    testing multiple mappings for items.
    Add or receive an order (fill 952f and 952o) with same and/or different
    values. Verify the contents of items.callnumber. (Check with regular
    item editor; see note.)
    Do a similar edit in the regular item editor.
    Note: You should expect to see A | B in both 952f and 925o if both
    fields are filled with a different value.
    Set items.coded_location_qualifier back to 952f in koha2marclinks.

    Note: When AcqCreateItem==ordering, you will not see A|B in the callno
    field when adding an item on neworderempty.pl. But when you submit
    the main form, addorder.pl is called. At that time an item is created
    and you will see that A|B is in both fields (952f and 952o).

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 39 Marcel de Rooy 2017-10-17 13:01:28 UTC
Rebased (Items.t)
Comment 40 Kyle M Hall 2017-10-27 16:53:34 UTC
Created attachment 68774 [details] [review]
Bug 19096: Make Default authoritative in core modules

After feedback from the dev mailing list, it seems appropriate here to
propose making the Default framework authoritative for Koha to MARC
mappings. This implies checking only the Default framework in the
routines:

[1] GetMarcFromKohaField: The parameter frameworkcode is removed. A
    follow-up report (19097) will update the calls not adjusted here.
    This is safe since the parameter is silently ignored.
[2] GetMarcSubfieldStructureFromKohaField: Framework parameter is removed
    and calls are adjusted. Includes acquisitions_stats.pl.
[3] TransformKohaToMarc: The parameter is removed; all calls are verified
    or adjusted.
[4] TransformMarcToKoha: The parameter is no longer used and will be
    removed in a follow-up report (19097). It always goes to Default now.
[5] TransformMarcToKohaOneField: The parameter is removed and all calls
    are adjusted. Including: Breeding, XISBN and MetadataRecord modules.
[6] C4::Koha::IsKohaFieldLinked: This routine was called only once (in
    C4::Items::_build_default_values_for_mod_marc. It can be replaced by
    calling GetMarcFromKohaField. If there is no kohafield linked, undef
    is returned. (Corresponding unit test is removed here.)
[7] C4::Items::ModItemFromMarc: The helper routine
    _build_default_values_for_mod_marc does no longer have a framework
    parameter. The cache key default_value_for_mod_marc- is no longer
    combined with a frameworkcode. Three admin scripts are adjusted
    accordingly; some tests will be corrected in the next patch.

Test plan:
See next patch. That patch adjusts all tests involved.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 41 Kyle M Hall 2017-10-27 16:53:41 UTC
Created attachment 68775 [details] [review]
Bug 19096: Adjusts unit tests

The subroutines listed in the former patch are extensively tested in
Biblio.t, TransformKohaToMarc.t and TransformMarcToKoha.t.
These tests do no longer use new frameworks to add mappings.

In Biblio.t and TransformMarcToKoha.t we also test passing an individual
framework code (for an empty framework) to see if Default is still used.
Note that this parameter will be removed later on.

In Items.t the cache keys MarcStructure-0- and MarcStructure-1- are not
touched, so they do not need to be cleared. The cache key
default_value_for_mod_marc- should be used without framework now.
Similar adjustments in Items/AutomaticItemModificationByAge.t and
Reserves.t.

Furthermore the subtest for _build_default_values_for_mod_marc in Items.t
is adjusted since framework is no longer relevant. The biblio record with
items is created in a new framework, but the mappings from Default are
consulted.

Test plan:
Run all adjusted unit tests.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 42 Kyle M Hall 2017-10-27 16:53:44 UTC
Created attachment 68776 [details] [review]
Bug 19096: Do not allow changes to kohafield within MARC frameworks

This patch makes two changes in the script for managing the MARC
frameworks in order to support making the Default authoritative
as for Koha to MARC mappings:

[1] Disable the kohafield select combo. Add a hidden input to save value.
[2] When a new subfield is added, the kohafield is still empty and should
    be overwritten with its Default counterpart.
    Note: Although we could leave the field empty, since Koha looks at the
    Default framework only, it does not cost much to keep these fields in
    sync with Default and perhaps catch a bug when someone somewhere looks
    in the wrong framework.

Note: The description of Koha to MARC mapping on admin-home has been
adjusted accordingly (removing last two lines).

Test plan:
[1] Add a new tag and subfield in Default.
[2] Map it to a kohafield in koha2marclinks.pl
[3] Add the same tag and subfield in another framework.
    Verify that the kohafield is updated after you saved the subfield.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 43 Kyle M Hall 2017-10-27 16:53:47 UTC
Created attachment 68777 [details] [review]
Bug 19096: Online help changes

On the page admin/marc_subfields_structure we now tell that Koha link
has been disabled and refer to the help for koha2marclinks.
On the page admin/koha2marclinks we mention that the Default mapping is
considered authoritative now.

Test plan:
Read these online help pages and verify that the changes make sense.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 44 Kyle M Hall 2017-10-27 16:53:50 UTC
Created attachment 68778 [details] [review]
Bug 19096: Restructure MarcSubfieldStructures.t first

Before adding more tests, move all current tests in one subtest.

Test plan:
Run t/db_dependent/Koha/MarcSubfieldStructures.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 45 Kyle M Hall 2017-10-27 16:53:53 UTC
Created attachment 68779 [details] [review]
Bug 19096: Add dbrev to sync kohafield in all frameworks

The dbrev will use two new routines in MarcSubfieldStructures:
[1] get_kohafield_exceptions is used to report deviating kohafields in
    the additional frameworks,
[2] sync_kohafield is used to reset kohafield in the other frameworks to
    the mapping in Default.

Test plan:

Unit test and database revision:
[1] Run t/db_dependent/Koha/MarcSubfieldStructures.t
[2] Verify that your Default 100a is mapped to biblio.author. Go to another
    framework and clear the mapping via mysql command line:
    UPDATE marc_subfield_structure SET kohafield=NULL WHERE frameworkcode=[your_framework] AND tagfield='100' AND tagsubfield='a';
[3] Run the db revision. It should report that 100a was adjusted.
[4] Check in admin/marc_subfield_structure that your 100a is mapped to
    biblio.author again in that framework.

Additional interface testing (ensuring that the changes on this report do
not interfere with multiple mappings):
[5] Make two mappings for copyrightdate: 260c and 264a. And make two
    mappings for biblioitems.pages: 300a and say 300g. Toggle with some
    field values in those fields in the cataloging editor and verify the
    contents of biblio.copyrightdate and biblioitems.pages. The former
    should contain one year (due to additional logic) and the latter should
    contain A | B if both fields are filled.
    Remove the mapping for 300g.
[6] Set AcqCreateItem to ordering or placing. Verify that you can still
    add or receive an order as usual.
[7] Add a mapping for itemcallnumber to 952f (this should remove the one
    for coded_location_qualifier). This is very unusual but serves well in
    testing multiple mappings for items.
    Add or receive an order (fill 952f and 952o) with same and/or different
    values. Verify the contents of items.callnumber. (Check with regular
    item editor; see note.)
    Do a similar edit in the regular item editor.
    Note: You should expect to see A | B in both 952f and 925o if both
    fields are filled with a different value.
    Set items.coded_location_qualifier back to 952f in koha2marclinks.

    Note: When AcqCreateItem==ordering, you will not see A|B in the callno
    field when adding an item on neworderempty.pl. But when you submit
    the main form, addorder.pl is called. At that time an item is created
    and you will see that A|B is in both fields (952f and 952o).

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 46 Jonathan Druart 2017-10-27 18:59:59 UTC
Reviewed on top of bug 10306

1/ I always though we would need to fix the mapping at some point, and make it use the correct template. Here we are moving the other way around.
Does that mean we should move the mapping out of the framework tables?

2/ return wantarray ? @retval : ( @retval ? $retval[0] : undef );
This is not necessary, do not do that please.
Caller can do
my ( $v ) = the_sub();
Not blocker.
Comment 47 Marcel de Rooy 2017-10-30 08:22:16 UTC
(In reply to Kyle M Hall from comment #45)
> 
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Thanks, Kyle
Comment 48 Marcel de Rooy 2017-10-30 09:10:12 UTC
(In reply to Jonathan Druart from comment #46)
> Reviewed on top of bug 10306
> 
> 1/ I always though we would need to fix the mapping at some point, and make
> it use the correct template. Here we are moving the other way around.
> Does that mean we should move the mapping out of the framework tables?

Implicitly, yes. They could in be another table. The code now looks only at Default, but keeps the other frameworks in sync.

Note that I only did this after consulting the mailings lists in order to know if there are relevant objections for doing so. I did receive responses of approval, but no objections.
Comment 49 Marcel de Rooy 2017-10-30 11:02:46 UTC
(In reply to Jonathan Druart from comment #46)
> 2/ return wantarray ? @retval : ( @retval ? $retval[0] : undef );
> This is not necessary, do not do that please.
> Caller can do
> my ( $v ) = the_sub();
> Not blocker.

This is about GetMarcFromKohaField.
Bug 19097 will adjust all calls. This seems imo now the best opportunity to address this point and verify that we should miss a call in scalar context. I will also need to remove the associated test. Added a comment on that bug.
If it is no blocker, leave it now as-is here.
Comment 50 Jonathan Druart 2017-12-04 15:19:07 UTC
Marcel,
It is good to see test for the 2 subroutines you add to Koha::MSS, but I am not sure we should add them to the module. As we need them to be executed only once during the update DB process, we should have these changes moved to the updatedatabase.pl script (or to a separate script). Do not you think?
Comment 51 Marcel de Rooy 2017-12-08 07:42:22 UTC
Created attachment 69629 [details] [review]
Bug 19096: (QA follow-up) Move two routines out of Koha::MSS

As requested by RM, this patch moves sync_kohafield and
get_kohafield_exceptions from Koha/MarcSubfieldStructures.pm.

At this moment they are only used in a database revision; it is not clear
if they may be of use later on.

In order to keep them in a module and not remove the unit tests, this
patch adds a Koha::Util module Dbrev.pm. It is now required in the atomic
update, but could be added in a use statement in updatedatabase.pl.

Test plan:
[1] Run updatedatabase.pl
[2] Run t/db_dependent/Koha/MarcSubfieldStructures.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 52 Marcel de Rooy 2017-12-08 07:42:49 UTC
FAIL   Koha/MarcSubfieldStructures.pm
   FAIL   pod coverage
                POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666
=> As explained before on other reports, this is a false warning. It can be ignored.
Comment 53 Jonathan Druart 2017-12-08 14:29:48 UTC
Created attachment 69646 [details] [review]
Bug 19096: Use raw SQL queries instead for the update DB entry

I strongly thing we must not use C4 or Koha subroutine in the update DB
process, only sql queries.
Especially not DBIC Schema files as they will change.
For instance the update DB 17.12.00.00X is using a specific Koha::Schema::RS::MSS
representing the current schema, but in X months/years, this schema will change
and the ->search we used may failed.
Comment 54 Jonathan Druart 2017-12-08 14:29:53 UTC
Created attachment 69647 [details] [review]
Bug 19096: Move the sql queries to the update DB entry
Comment 55 Jonathan Druart 2017-12-08 14:30:48 UTC
Marcel, will you agree with these 2 patches? I do not think it's a good idea to use the DBIC schema files during the update DB upgrade.
I thought we already agreed on that, but cannot find logs...
Comment 56 Marcel de Rooy 2017-12-08 14:59:53 UTC
(In reply to Jonathan Druart from comment #55)
> Marcel, will you agree with these 2 patches? I do not think it's a good idea
> to use the DBIC schema files during the update DB upgrade.
> I thought we already agreed on that, but cannot find logs...

Might have been a good idea to give some more direction in this regard. This was a waste of time.
Will still have a look in detail.
Comment 57 Marcel de Rooy 2017-12-11 06:28:19 UTC
(In reply to Marcel de Rooy from comment #56)
> (In reply to Jonathan Druart from comment #55)
> > Marcel, will you agree with these 2 patches? I do not think it's a good idea
> > to use the DBIC schema files during the update DB upgrade.
> > I thought we already agreed on that, but cannot find logs...
> 
> Might have been a good idea to give some more direction in this regard. This
> was a waste of time.
> Will still have a look in detail.

Sorry for being a bit harsh here. You got me thinking about this thing in general; and it sure is rather serious.. Will test a few things too.
Thx for adjusting the SQL queries. Need to run the dbrev again now.
Comment 58 Marcel de Rooy 2017-12-11 07:38:56 UTC
(In reply to Jonathan Druart from comment #55)
> I do not think it's a good idea
> to use the DBIC schema files during the update DB upgrade.

Added a new report (bug 19789) to pinpoint a few calls in updatedatabase that could lead to future problems.
Comment 59 Marcel de Rooy 2017-12-11 09:43:10 UTC
Hmm. Also added another new report (bug 19790) to clear additionalauthors.author from kohafield and the installer files. Note that this table was removed before 3.0 !
Somehow I changed my 700$a in an earlier test and now I noticed it when running your dbrev.
Comment 60 Marcel de Rooy 2017-12-11 10:35:31 UTC
Created attachment 69681 [details] [review]
Bug 19096: Add dbrev to sync kohafield in all frameworks

Squashed the last four follow-ups into one patch. Instead of adding
two routines to Koha::MarcSubfieldStructures, and instead of moving
them to Koha::Util::Dbrev, we finally ended up with converting the
code to raw SQL queries (thx Jonathan). No need to 'pollute' the git
history with all this moving around.

As Jonathan pointed out, there is a risk in using DBIx calls (with Koha
objects) while running database revisions. See also bug 17292 and
bug 19789.

I tested the resulting db revision by adding a few fields to the Default
framework and adding deviating kohafields in other frameworks. And confirm
that it works as expected.

Please read the remainder of this commit message in the light of the above:

===

The dbrev will use two new routines in MarcSubfieldStructures:
[1] get_kohafield_exceptions is used to report deviating kohafields in
    the additional frameworks,
[2] sync_kohafield is used to reset kohafield in the other frameworks to
    the mapping in Default.

Test plan:

Unit test and database revision:
[1] Run t/db_dependent/Koha/MarcSubfieldStructures.t
[2] Verify that your Default 100a is mapped to biblio.author. Go to another
    framework and clear the mapping via mysql command line:
    UPDATE marc_subfield_structure SET kohafield=NULL WHERE frameworkcode=[your_framework] AND tagfield='100' AND tagsubfield='a';
[3] Run the db revision. It should report that 100a was adjusted.
[4] Check in admin/marc_subfield_structure that your 100a is mapped to
    biblio.author again in that framework.

Additional interface testing (ensuring that the changes on this report do
not interfere with multiple mappings):
[5] Make two mappings for copyrightdate: 260c and 264a. And make two
    mappings for biblioitems.pages: 300a and say 300g. Toggle with some
    field values in those fields in the cataloging editor and verify the
    contents of biblio.copyrightdate and biblioitems.pages. The former
    should contain one year (due to additional logic) and the latter should
    contain A | B if both fields are filled.
    Remove the mapping for 300g.
[6] Set AcqCreateItem to ordering or placing. Verify that you can still
    add or receive an order as usual.
[7] Add a mapping for itemcallnumber to 952f (this should remove the one
    for coded_location_qualifier). This is very unusual but serves well in
    testing multiple mappings for items.
    Add or receive an order (fill 952f and 952o) with same and/or different
    values. Verify the contents of items.callnumber. (Check with regular
    item editor; see note.)
    Do a similar edit in the regular item editor.
    Note: You should expect to see A | B in both 952f and 925o if both
    fields are filled with a different value.
    Set items.coded_location_qualifier back to 952f in koha2marclinks.

    Note: When AcqCreateItem==ordering, you will not see A|B in the callno
    field when adding an item on neworderempty.pl. But when you submit
    the main form, addorder.pl is called. At that time an item is created
    and you will see that A|B is in both fields (952f and 952o).

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 19096: (QA follow-up) Move two routines out of Koha::MSS

As requested by RM, this patch moves sync_kohafield and
get_kohafield_exceptions from Koha/MarcSubfieldStructures.pm.

At this moment they are only used in a database revision; it is not clear
if they may be of use later on.

In order to keep them in a module and not remove the unit tests, this
patch adds a Koha::Util module Dbrev.pm. It is now required in the atomic
update, but could be added in a use statement in updatedatabase.pl.

Test plan:
[1] Run updatedatabase.pl
[2] Run t/db_dependent/Koha/MarcSubfieldStructures.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Bug 19096: Use raw SQL queries instead for the update DB entry

I strongly thing we must not use C4 or Koha subroutine in the update DB
process, only sql queries.
Especially not DBIC Schema files as they will change.
For instance the update DB 17.12.00.00X is using a specific Koha::Schema::RS::MSS
representing the current schema, but in X months/years, this schema will change
and the ->search we used may failed.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Bug 19096: Move the sql queries to the update DB entry

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Print "Field 700$a" instead of "Field 700.a" in the dbrev.
Comment 61 Marcel de Rooy 2017-12-11 10:38:46 UTC
(In reply to Jonathan Druart from comment #55)
> Marcel, will you agree with these 2 patches? I do not think it's a good idea
> to use the DBIC schema files during the update DB upgrade.

So yes, I finally did :) Thx again.
Comment 62 Jonathan Druart 2017-12-11 14:35:24 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 63 Nick Clemens 2017-12-14 17:01:05 UTC
Awesome work all!

Will not be backported to 17.11, enhancement+dependencies