Created attachment 51125 [details] [review] Bug 16428: Use the biblio framework to know if a field is mapped The subroutine _build_default_values_for_mod_marc takes the frameworkcode in parameter, but ModItemFromMarc did not pass it. It uses it to know if a field is mapped or not to a Koha field (C4::Koha::IsKohaFieldLinked). Consequently the default framework ("") was always used. This bug has been found working on bug 13074 and has been put on a separate bug report to ease the backport. Test plan: Without this change, the tests added by bug 16428 won't pass
Created attachment 51230 [details] [review] Bug 16428: (regression tests) This patch adds tests to t/db_dependent/Items.t to verify the kohafield mapping is not always taken from the default framework, but from the correct one. To test: - Apply the patch - Run: $ prove t/db_dependent/Items.t => FAIL: Mapping is not done on the right framework Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment on attachment 51230 [details] [review] Bug 16428: (regression tests) The same tests are introduced on 13074 by Jonathan. I'm picking his tests.
Created attachment 51327 [details] [review] Bug 16428: Use the biblio framework to know if a field is mapped The subroutine _build_default_values_for_mod_marc takes the frameworkcode in parameter, but ModItemFromMarc did not pass it. It uses it to know if a field is mapped or not to a Koha field (C4::Koha::IsKohaFieldLinked). Consequently the default framework ("") was always used. This bug has been found working on bug 13074 and has been put on a separate bug report to ease the backport. Test plan: Without this change, the tests added by bug 16428 won't pass Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 51430 [details] [review] Bug 16428: Use the biblio framework to know if a field is mapped The subroutine _build_default_values_for_mod_marc takes the frameworkcode in parameter, but ModItemFromMarc did not pass it. It uses it to know if a field is mapped or not to a Koha field (C4::Koha::IsKohaFieldLinked). Consequently the default framework ("") was always used. This bug has been found working on bug 13074 and has been put on a separate bug report to ease the backport. Test plan: Without this change, the tests added by bug 16428 won't pass Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 51431 [details] [review] Bug 16428: [QA Follow-up] Useless call to GetMarcStructure in _build_default_values_for_mod_marc The routine calls GetMarcStructure and does not use its return value after all. Test plan: Run t/db_dependent/Items.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
I have some qualms about pushing this... there are a very significant number of places in Koha that use the default framework's mappings (enough to make it a de-facto standard). See the results of a $ git grep TransformMarcToKoha
(In reply to Jesse Weaver from comment #7) > I have some qualms about pushing this... there are a very significant number > of places in Koha that use the default framework's mappings (enough to make > it a de-facto standard). See the results of a $ git grep TransformMarcToKoha It seems that we are using it when adding/modifying a record/item. git grep TransformMarcToKoha|grep -v frameworkcode does not return a lot of relevant results :)
Just adding to the discussion, this is a specific context. We are looking at items. And only looking at default values to use when an item field would be empty. And maybe adding somewhat more weight, the default framework by standard does not even contain any other default value than NULL.. Since this patch theoretically does the right thing and its change of behavior may probably be quite limited (only impacting local defaults for items), I still would support to push it to master. We c/should add a dbrev with a warning about this change. Or even copy (in this dbrev) all item defaults from the current default framework to other frameworks (not overwriting not-null values), more or less ensuring unchanged behavior? Note that not-null items defaults currently are not used in this context, but it seems better not to overwrite them without consent. What do you think?
Sent back to the RM queue
Pushed to master for 16.11, thanks Jonathan, Marcel!
Pushed in 16.05. Will be in 16.05.01.
Patches pushed to 3.22.x, will be in 3.22.8