Bug 16428 - The framework is not checked to know if a field is mapped
Summary: The framework is not checked to know if a field is mapped
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 12874
Blocks: 13074
  Show dependency treegraph
 
Reported: 2016-05-03 10:03 UTC by Jonathan Druart
Modified: 2017-12-07 22:16 UTC (History)
7 users (show)

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


Attachments
Bug 16428: Use the biblio framework to know if a field is mapped (1.41 KB, patch)
2016-05-03 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16428: (regression tests) (3.89 KB, patch)
2016-05-05 13:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16428: Use the biblio framework to know if a field is mapped (1.46 KB, patch)
2016-05-09 14:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16428: Use the biblio framework to know if a field is mapped (1.57 KB, patch)
2016-05-11 13:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16428: [QA Follow-up] Useless call to GetMarcStructure in _build_default_values_for_mod_marc (1.05 KB, patch)
2016-05-11 13:54 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 Jonathan Druart 2016-05-03 10:03:22 UTC

    
Comment 1 Jonathan Druart 2016-05-03 10:08:16 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2016-05-05 13:16:36 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2016-05-05 13:42:29 UTC
Comment on attachment 51230 [details] [review]
Bug 16428: (regression tests)

The same tests are introduced on 13074 by Jonathan. I'm picking his tests.
Comment 4 Tomás Cohen Arazi 2016-05-09 14:09:00 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2016-05-11 13:54:21 UTC
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>
Comment 6 Marcel de Rooy 2016-05-11 13:54:26 UTC
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>
Comment 7 Jesse Weaver 2016-05-12 22:08:51 UTC
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
Comment 8 Jonathan Druart 2016-05-13 14:17:30 UTC
(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 :)
Comment 9 Marcel de Rooy 2016-06-01 09:23:18 UTC
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?
Comment 10 Jonathan Druart 2016-06-07 18:11:09 UTC
Sent back to the RM queue
Comment 11 Kyle M Hall 2016-06-10 16:44:41 UTC
Pushed to master for 16.11, thanks Jonathan, Marcel!
Comment 12 Frédéric Demians 2016-06-15 07:41:41 UTC
Pushed in 16.05. Will be in 16.05.01.
Comment 13 Julian Maurice 2016-06-16 08:42:41 UTC
Patches pushed to 3.22.x, will be in 3.22.8