|
Description
Marcel de Rooy
2017-08-14 07:44:57 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. 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. 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. 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. 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. 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 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). 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..
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. 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. 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. 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. 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 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). Trivial rebase on top of correction in 10306 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. 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. 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. 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. 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 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). Simple rebase 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. 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> 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> 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> 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> 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> 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> (In reply to Josef Moravec from comment #29) > Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Thanks, Josef ! (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. Rebase forthcoming 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> 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> 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> 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> 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> 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> Rebased (Items.t) 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> 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> 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> 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> 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> 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> 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. (In reply to Kyle M Hall from comment #45) > > Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Thanks, Kyle (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. (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. 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? 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> 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.
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. Created attachment 69647 [details] [review] Bug 19096: Move the sql queries to the update DB entry 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... (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. (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. (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. 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. 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. (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. Pushed to master for 18.05, thanks to everybody involved! Awesome work all! Will not be backported to 17.11, enhancement+dependencies |