Bug 10306 - Koha to MARC mappings (Part 1): Allow multiple mappings per kohafield (for say 260/RDA 264)
Summary: Koha to MARC mappings (Part 1): Allow multiple mappings per kohafield (for sa...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact:
URL:
Keywords:
: 12756 20149 (view as bug list)
Depends on: 18951
Blocks: 10344 19096 19594 21774
  Show dependency treegraph
 
Reported: 2013-05-22 01:27 UTC by Nicole C. Engard
Modified: 2020-11-12 15:03 UTC (History)
18 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patchset adds the ability to map several MARC fields to a single Koha field. The first existing mapped field will be saved into the database. This allows for flexibility in a system using RDA and AACR2 records where some store the publication data in the 260 fields and others in the 264.
Version(s) released in:


Attachments
Bug 10306: Biblio module changes (20.07 KB, patch)
2017-08-07 13:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Allow controlfields in TransformKohaToMarc (1.28 KB, patch)
2017-08-07 13:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Add tests for module changes (22.78 KB, patch)
2017-08-07 13:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Restructure TransformMarcToKoha and update some POD lines (9.55 KB, patch)
2017-08-07 13:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Support for multiple mappings in koha2marclinks (15.48 KB, patch)
2017-08-07 13:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Adjusted online help text for koha2marclinks (3.91 KB, patch)
2017-08-07 13:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Do not allow changes to kohafield within MARC frameworks (3.38 KB, patch)
2017-08-07 13:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Core module changes for multiple mappings (20.09 KB, patch)
2017-08-14 11:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Allow controlfields in TransformKohaToMarc (1.28 KB, patch)
2017-08-14 11:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Add tests for module changes (22.78 KB, patch)
2017-08-14 11:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Restructure TransformMarcToKoha and update some POD lines (9.55 KB, patch)
2017-08-14 11:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Support for multiple mappings in koha2marclinks (16.57 KB, patch)
2017-08-14 11:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Adjusted online help text for koha2marclinks (3.67 KB, patch)
2017-08-14 11:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Support for multiple mappings in koha2marclinks (16.57 KB, patch)
2017-08-17 07:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Adjusted online help text for koha2marclinks (3.67 KB, patch)
2017-08-17 07:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Core module changes for multiple mappings (20.09 KB, patch)
2017-08-17 09:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Allow controlfields in TransformKohaToMarc (1.28 KB, patch)
2017-08-17 09:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Add tests for module changes (22.77 KB, patch)
2017-08-17 09:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Restructure TransformMarcToKoha and update some POD lines (9.55 KB, patch)
2017-08-17 09:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Support for multiple mappings in koha2marclinks (16.57 KB, patch)
2017-08-17 09:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Adjusted online help text for koha2marclinks (3.67 KB, patch)
2017-08-17 09:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10306: Core module changes for multiple mappings (20.12 KB, patch)
2017-08-22 12:51 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10306: Allow controlfields in TransformKohaToMarc (1.31 KB, patch)
2017-08-22 12:51 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10306: Add tests for module changes (22.80 KB, patch)
2017-08-22 12:52 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10306: Restructure TransformMarcToKoha and update some POD lines (9.58 KB, patch)
2017-08-22 12:52 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10306: Support for multiple mappings in koha2marclinks (16.59 KB, patch)
2017-08-22 12:52 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10306: Adjusted online help text for koha2marclinks (3.70 KB, patch)
2017-08-22 12:52 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 10306: [QA Follow-up] Correct a TestBuilder call (1.55 KB, patch)
2017-08-24 10:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Adjusted online help text for koha2marclinks (3.60 KB, patch)
2017-08-28 07:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: [QA Follow-up] Correct a TestBuilder call (1.55 KB, patch)
2017-08-28 07:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Core module changes for multiple mappings (20.22 KB, patch)
2017-10-02 09:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Allow controlfields in TransformKohaToMarc (1.34 KB, patch)
2017-10-02 09:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Add tests for module changes (22.83 KB, patch)
2017-10-02 09:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Restructure TransformMarcToKoha and update some POD lines (9.61 KB, patch)
2017-10-02 09:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Support for multiple mappings in koha2marclinks (16.62 KB, patch)
2017-10-02 09:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Adjusted online help text for koha2marclinks (3.60 KB, patch)
2017-10-02 09:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: [QA Follow-up] Correct a TestBuilder call (1.55 KB, patch)
2017-10-02 09:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: (QA follow-up) More feedback for admins in koha2marclinks (2.90 KB, patch)
2017-10-23 10:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10306: Core module changes for multiple mappings (20.23 KB, patch)
2017-10-27 16:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10306: Allow controlfields in TransformKohaToMarc (1.36 KB, patch)
2017-10-27 16:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10306: Add tests for module changes (22.84 KB, patch)
2017-10-27 16:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10306: Restructure TransformMarcToKoha and update some POD lines (9.62 KB, patch)
2017-10-27 16:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10306: Support for multiple mappings in koha2marclinks (16.64 KB, patch)
2017-10-27 16:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10306: Adjusted online help text for koha2marclinks (3.62 KB, patch)
2017-10-27 16:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10306: [QA Follow-up] Correct a TestBuilder call (1.57 KB, patch)
2017-10-27 16:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10306: (QA follow-up) More feedback for admins in koha2marclinks (2.92 KB, patch)
2017-10-27 16:50 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2013-05-22 01:27:03 UTC
The acquisitions module show publisher info from the 260, but now libraries are using RDA and need the 264 to show if there is no 260.  This is easy enough on XSLT styled pages, but acquisitions is not one of those.
Comment 1 David Cook 2015-06-09 03:09:07 UTC
Yeah, I've been wondering a lot about this sort of thing. 

"publisher" is a column in the database which maps to 260... which makes sense... but it should potentially also map to 264... but it can't currently do that.

I'm not sure whether the solution is to improve the mapping so that a person can add multiple fields/subfields to a column... or if this particular case should be hardcoded in the short term... or what.
Comment 2 Katrin Fischer 2015-07-09 01:29:37 UTC
It's really important that we fix this, because otherwise while the catalogs still contain 'mixed data' we will have lots of places not only in acq where there will no publication year, publication place or publisher be shown for part of the records.

I think changing the mapping so you can have 2 fields is probably the more complicated thing to do - also, how will the system act if both are present?
My feeling would be to prefer the newer field 264 over the older 260 in case both are present.

I am not sure how to reflect this in the GUI properly, but we need a fix soon.
Comment 3 Katrin Fischer 2015-07-09 01:30:54 UTC
To be clear, the problem stems from our Koha-to-MARC-Mappings - the problem will show up whereever we get the information from the biblio and biblioitems tables.
Comment 4 Katrin Fischer 2015-09-30 16:50:57 UTC
Well, actually we got a problem everywhere but in the XSLT files, that already have been adjustred to at show 264 in some way.

I think with more and more libraries starting to get RDA data - we can update this to be a bug.
Comment 5 Galen Charlton 2015-10-02 15:07:17 UTC
To outline a way of dealing with this:

One way of looking at the problem is that RDA and the 260/264 forces us to deal with the fact that some attributes that we might want to stick in a column in biblio or biblioitems needs to draw from multiple MARC fields.

XSLT and XPath give ways to do that sort of parsing... so we can add it to Koha something like this:

- Create a new table to store XSLT stylesheets.  This could be a combination of standard stylesheets (e.g., the MARC21 to MODS transforming) and local ones.
- Create a new table to specify mappings to columns, something like this:

  koha_attribute_map
     kohafield    # name of table and column to store the results
     xslt         # reference to the XSLT to use, if need be
     xpath        # what to use to extract the value

The process would then be, when saving a bib record, that for each attribute specified in koha_attribute_map, the XSLT (if specified) would be applied to the MARCXML, then the XPath used to extract a result, which is then placed in the column specified by kohafield.

This process could be used to supplemnt the one-subfield-to-one-column mapping that the MARC frameworks already specify, or with a bit more work, replace it.

I should note that this general idea is inspired largely by similar mechanisms in Evergreen.
Comment 6 Katrin Fischer 2015-10-03 18:05:29 UTC
*** Bug 12756 has been marked as a duplicate of this bug. ***
Comment 7 Héctor Eduardo Castro Avalos 2015-10-03 19:22:31 UTC
(In reply to Galen Charlton from comment #5)

 
> - Create a new table to store XSLT stylesheets.  This could be a combination
> of standard stylesheets (e.g., the MARC21 to MODS transforming) and local
> ones.
> - Create a new table to specify mappings to columns, something like this:
> 
>   koha_attribute_map
>      kohafield    # name of table and column to store the results
>      xslt         # reference to the XSLT to use, if need be
>      xpath        # what to use to extract the value
> 
> The process would then be, when saving a bib record, that for each attribute
> specified in koha_attribute_map, the XSLT (if specified) would be applied to
> the MARCXML, then the XPath used to extract a result, which is then placed
> in the column specified by kohafield.

It seems to be a good solution, Galen, with this you can fix the problem with UNIMARC too.

The new 264 field is unsearchable in Zebra, I filed a bug for this <http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14198>. But since there are a new development with Elastic Search I don't know the future of Zebra in Koha.

Regards.
Comment 8 Nicole C. Engard 2015-10-15 17:51:47 UTC
How do people feel about Keyword to Marc mapping as a solution here?
Comment 9 David Cook 2015-10-15 22:17:01 UTC
(In reply to Nicole C. Engard from comment #8)
> How do people feel about Keyword to Marc mapping as a solution here?

Not very positive, because Keyword to Marc mapping only allows one mapping, when you theoretically might want to use 264 "or" 260 depending on which is available per record.
Comment 10 Nicole C. Engard 2015-10-16 13:11:18 UTC
(In reply to David Cook from comment #9)
> Not very positive, because Keyword to Marc mapping only allows one mapping,
> when you theoretically might want to use 264 "or" 260 depending on which is
> available per record.

But, we could enhance it to do what we need ... couldn't we? Just spit balling ideas here.
Comment 11 David Cook 2015-10-18 22:21:59 UTC
(In reply to Nicole C. Engard from comment #10)
> (In reply to David Cook from comment #9)
> > Not very positive, because Keyword to Marc mapping only allows one mapping,
> > when you theoretically might want to use 264 "or" 260 depending on which is
> > available per record.
> 
> But, we could enhance it to do what we need ... couldn't we? Just spit
> balling ideas here.

I'd be down with that. A while back, I was hard-coding something like that (checking to see if the record had a 264 and if not falling back to the 260), but got sidetracked by more important things.
Comment 12 Katrin Fischer 2015-10-19 05:38:56 UTC
I think enhancing the Koha-to-MARC-Mappings would be better than using the Keyword-to-MARC-Mappings for some reasons:
- the keyword system gets the data from the MARCXML record for display each time, so I suspect it to be slower than the Koha-to-MARC-mappings that get the information from the db and it might slow down Koha a bit more.
- I'd like to keep the option to use a database column for things like the publication year when writing reports, as the XML functions also seem quite slow in comparison.
Comment 13 David Cook 2015-10-19 05:47:44 UTC
(In reply to Katrin Fischer from comment #12)
> I think enhancing the Koha-to-MARC-Mappings would be better than using the
> Keyword-to-MARC-Mappings for some reasons:
> - the keyword system gets the data from the MARCXML record for display each
> time, so I suspect it to be slower than the Koha-to-MARC-mappings that get
> the information from the db and it might slow down Koha a bit more.
> - I'd like to keep the option to use a database column for things like the
> publication year when writing reports, as the XML functions also seem quite
> slow in comparison.

Ahh, I misread Nicole's comment. I thought she was talking about enhancing Koha-to-MARC-Mapping.

Yeah, no, I think Koha-to-MARC-Mapping is the way to go. I don't think it would make sense to use Keyword-to-MARC-Mappings.

Especially since there are already so many places where we're using/need to use Koha-to-MARC-mapping... it makes sense to enhance the thing that we're already using.
Comment 14 Marcel de Rooy 2017-07-17 13:26:27 UTC
No moves here yet?
Comment 15 David Cook 2017-07-18 01:47:54 UTC
(In reply to Marcel de Rooy from comment #14)
> No moves here yet?

Not that I know of. It's been on my to do list for a long time, but there have been other priorities. Still an issue though I believe.
Comment 16 Katrin Fischer 2017-07-19 05:51:41 UTC
As most records represent a mix of RDA and non-RDA records atm, this would still mean a big improvement.
Comment 17 Marcel de Rooy 2017-07-19 10:17:46 UTC
(In reply to Katrin Fischer from comment #16)
> As most records represent a mix of RDA and non-RDA records atm, this would
> still mean a big improvement.

Will give it a try..
Comment 18 Marcel de Rooy 2017-07-19 12:23:14 UTC
Putting this on top of 18951. That report moves TransformKohaToMarc.t to db_dependent and creates its own data for it. Building further here.
Comment 19 Marcel de Rooy 2017-07-27 13:45:38 UTC
Changed the title from duplicate to multiple. Might be a better fit.
Comment 20 Marcel de Rooy 2017-08-07 13:07:56 UTC
Created attachment 65539 [details] [review]
Bug 10306: Biblio module changes

In order to allow multiple Koha to MARC mappings (for one kohafield), we
need to adjust a few key routines in C4/Biblio.pm. This results in a few
changes in dependent modules too.

Note: Multiple mappings also include 'alternating' mappings. Such as the
case of MARC21 260 and 264: only one of both fields will be used. Sub
TransformMarcToKoha will handle that just fine; the opposite transformation
is harder, since we do no longer know which field was the source. In that
case TransformKohaToMarc will fill both fields. We only use that operation
in Koha for items (in Acquisition and Cataloging).

Sub GetMarcSubfieldStructure
This sub used a selectall_hashref, which is fine as long as we have only
one mapping for each kohafield. But as DBI states it: If a row has the same
key as an earlier row then it replaces the earlier row. In other words,
we lose the first mapping if we have two.
This patch uses selectall_arrayref with Slice and rearranges the output so
that the returned hash returns an arrayref of hashrefs for each kohafield.
In order to improve consistency, we add an order clause to the SQL
statement used too.

Sub GetMarcFromKohaField
This sub just returned one tag and subfield, but in case of multiple
mappings we should return them all now.
Note: Many calls still expect just one result and will work just fine:
    my ($tag, $sub) = GetMarcFromKohaField(...)
A possible second mapping would be silently ignored. Often the sub is
called for biblionumber or itemnumber. I would not recommend the use of
multiple mappings for such fields btw.
In case the sub is called in scalar context, it will return only the first
tag (instead of the number of tags and subfields).

Sub GetMarcSubfieldStructureFromKohaField
This sub previously returned the hash for one kohafield.
In scalar context it will behave like before: it returns the first hashref
in the arrayref that comes from GetMarcSubfieldStructure.
In list context, it returns an array of all hashrefs (incl. multiple
mappings).
The sub is not used in C4::Ris. Removed the use statement.

Sub TransformKohaToMarc
This sub got a second parameter: frameworkcode.
Historically, Koha more or less assumes kohafields to be defined across all
frameworks (see Koha to MARC mappings). Therefore it falls back to Default
when it is not passed.
When going thru all mappings in building a MARC record, it also supports
multiple mappings. Note that Koha uses this routine in Acquisition and in
Cataloging for items. Normally the MARC record is leading however and the
Koha fields are derivatives for optimization and reporting.

The added third parameter allows for passing a new option no_split => 1.
We use this option in C4::Items::Item2Marc; if two item fields are mapped to
one kohafield but would have different values (which would be very unusual),
these values are glued together. When transforming to MARC again, we do not
want to duplicate the item subfields, but we keep the glued value in both
subfields. This operation only affects items, since we are not doing this
reverse operation for biblio and biblioitem fields.

Sub _get_inverted_marc_field_map
This sub is a helper routine of TransformMarcToKoha, the opposite
transformation. When saving a MARC record, all kohafields are extracted
including multiple mappings.
Suppose that you had both 260c and 264c in your record (which you won't),
than both values get saved initially into copyrightdate like A | B. The
additional code for copyrightdate will extract the first year from this
string.
A small fix in TransformMarcToKoha makes that it only saves a value in a
kohafield if it is defined and not empty. (Same for concatenation.)

Sub TransformMarcToKohaOneField
This sub now just calls TransformMarcToKoha and extracts the requested
field. Note that since we are caching the structure, this does not result
in additional database access and is therefore performance-wise
insignificant. We simplify code and maintenance.
Instead of modifying the passed hashref, it simply returns a value. A call
in C4::Breeding is adjusted accordingly. The routine getKohaField in
Koha::MetadataRecord is redirected to TransformMarcToKohaOneField.
NOTE: The fourth patch restructures/optimizes TransformMarcToKoha[OneField].

Sub get_koha_field_from_marc
This sub can be removed. A call is replaced by TransformMarcToKohaOneField
in C4::XISBN.

Note: The commented lines for sub ModZebrafiles are removed (directly under
TransformMarcToKohaOneField).

Test plan:
For unit tests and interface tests, please see follow-ups.
Run qa tools in order to verify that the modules still compile well.
Read the code changes and verify that they make sense.
Comment 21 Marcel de Rooy 2017-08-07 13:08:00 UTC
Created attachment 65540 [details] [review]
Bug 10306: Allow controlfields in TransformKohaToMarc

Since the interface allows you to connect a kohafield to a MARC
controlfield, this routine should be able to handle that. Unfortunately
it did not.

Test plan:
Change will be tested in Biblio/TransformKohaToMarc.t in the next patch.
Comment 22 Marcel de Rooy 2017-08-07 13:08:05 UTC
Created attachment 65541 [details] [review]
Bug 10306: Add tests for module changes

GetMarcSubfieldStructure: In Biblio.t we are adding a subtest that checks
the structure returned by this routine. Is it a hashref pointing to
arrayrefs of hashrefs? ;)
In Search.t this routine was mocked. The change in the returned structure
is now applied to this mock too (moving the marc tag hashes into
arrayrefs).

GetMarcFromKohaField: In Biblio.t we add a subtest for it. We are checking
if it returns multiple mappings (per kohafield) and calling it in scalar
context too.

GetMarcSubfieldStructureFromKohaField: The existing subtest in Biblio.t is
expanded to also test the call in list context.

TransformKohaToMarc: This sub has its own test script. We are adding a
subtest in TransformKohaToMarc.t for a test with multiple mappings,
and for mapping to a control field in another framework. This also tests
the additional framework parameter.
Additionally, we add a test for the new no_split option used for items.

TransformMarcToKoha: This implicitly tests its helper sub _get_inverted.
This patch adds a new test script for this routine.

TransformMarcToKohaOneField: A few tests are added to the previous new
test script.

Test plan:
[1] Run t/db_dependent/Biblio.t
[2] Run t/db_dependent/Biblio/TransformKohaToMarc.t
[3] Run t/db_dependent/Biblio/TransformMarcToKoha.t
[4] Run t/db_dependent/Search.t
Comment 23 Marcel de Rooy 2017-08-07 13:08:09 UTC
Created attachment 65542 [details] [review]
Bug 10306: Restructure TransformMarcToKoha and update some POD lines

This patch makes the following changes:

[1] Added POD for CountItemsIssued, GetBiblioItemData
[2] Moved TransformMarcToKohaOneField closer to TransformMarcToKoha (before
    sub CountItemsIssued)
[3] Restructured TransformMarcToKoha by extracting individual kohafields via
    TransformMarcToKohaOneField. The latter does no longer call the former.
    This small optimization avoids traversing the whole MARC record again
    and again.
[4] Moved adjusting copyrightdate/publicationyear to separate helper routine
    _adjust_pubyear
[5] Removed obsolete sub _get_inverted_marc_field_map.

Test plan:
Run t/db_dependent/Biblio/TransformMarcToKoha.t
Comment 24 Marcel de Rooy 2017-08-07 13:08:14 UTC
Created attachment 65543 [details] [review]
Bug 10306: Support for multiple mappings in koha2marclinks

This actually refactors koha2marclinks.pl in order to support multiple
mappings per kohafield.

Instead of three separate mapping pages for biblio, biblioitems and items,
the script now lists them together. This gives a complete overview of all
mappings rightaway. Changes are applied immediately across all frameworks.

Note: This report builds on the assumption that we do not keep mappings per
framework, but consider the Default framework as authoritative. A change
to Default is applied to the other frameworks though to keep them in sync.

On each line two buttons are provided, Add and Remove, in order to add or
remove an individual mapping. We do no longer provide a separate form with
the names of MARC tags. Since this form is targeted for administrators,
it should be enough to ask for a field tag and subfield code.

Note: The mappings for biblionumber, biblioitemnumber and itemnumber are
so vital that this form marks them as readonly. It is not recommended to
change them.

Test plan:
[1] Add a mapping. Verify via Frameworks or mysql command line that the
    kohafield is saved to the other frameworks too.
[2] Remove the mapping again. Check Frameworks or mysql cl again.
[3] Test adding a second mapping. For instance map copyrightdate to 260c
    and 264c. Check Frameworks or mysql cl again.
Comment 25 Marcel de Rooy 2017-08-07 13:08:19 UTC
Created attachment 65544 [details] [review]
Bug 10306: Adjusted online help text for koha2marclinks

Test plan:
Click on Help. Read the text.
Comment 26 Marcel de Rooy 2017-08-07 13:08:23 UTC
Created attachment 65545 [details] [review]
Bug 10306: Do not allow changes to kohafield within MARC frameworks

This patch makes two changes 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.

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 27 Marcel de Rooy 2017-08-07 13:09:41 UTC
This is not ready yet. Needs some feedback about a decision to make on viewing Default as authoritative for these mappings. See mail on dev list.
Comment 28 Marcel de Rooy 2017-08-14 11:12:16 UTC
Created attachment 65948 [details] [review]
Bug 10306: Core module changes for multiple mappings

In order to allow multiple Koha to MARC mappings (for one kohafield), we
need to adjust a few key routines in C4/Biblio.pm. This results in a few
changes in dependent modules too.

Note: Multiple mappings also include 'alternating' mappings. Such as the
case of MARC21 260 and 264: only one of both fields will be used. Sub
TransformMarcToKoha will handle that just fine; the opposite transformation
is harder, since we do no longer know which field was the source. In that
case TransformKohaToMarc will fill both fields. We only use that operation
in Koha for items (in Acquisition and Cataloging).

Sub GetMarcSubfieldStructure
This sub used a selectall_hashref, which is fine as long as we have only
one mapping for each kohafield. But as DBI states it: If a row has the same
key as an earlier row then it replaces the earlier row. In other words,
we lose the first mapping if we have two.
This patch uses selectall_arrayref with Slice and rearranges the output so
that the returned hash returns an arrayref of hashrefs for each kohafield.
In order to improve consistency, we add an order clause to the SQL
statement used too.

Sub GetMarcFromKohaField
This sub just returned one tag and subfield, but in case of multiple
mappings we should return them all now.
Note: Many calls still expect just one result and will work just fine:
    my ($tag, $sub) = GetMarcFromKohaField(...)
A possible second mapping would be silently ignored. Often the sub is
called for biblionumber or itemnumber. I would not recommend the use of
multiple mappings for such fields btw.
In case the sub is called in scalar context, it will return only the first
tag (instead of the number of tags and subfields).

Sub GetMarcSubfieldStructureFromKohaField
This sub previously returned the hash for one kohafield.
In scalar context it will behave like before: it returns the first hashref
in the arrayref that comes from GetMarcSubfieldStructure.
In list context, it returns an array of all hashrefs (incl. multiple
mappings).
The sub is not used in C4::Ris. Removed the use statement.

Sub TransformKohaToMarc
This sub got a second parameter: frameworkcode.
Historically, Koha more or less assumes kohafields to be defined across all
frameworks (see Koha to MARC mappings). Therefore it falls back to Default
when it is not passed.
When going thru all mappings in building a MARC record, it also supports
multiple mappings. Note that Koha uses this routine in Acquisition and in
Cataloging for items. Normally the MARC record is leading however and the
Koha fields are derivatives for optimization and reporting.

The added third parameter allows for passing a new option no_split => 1.
We use this option in C4::Items::Item2Marc; if two item fields are mapped to
one kohafield but would have different values (which would be very unusual),
these values are glued together. When transforming to MARC again, we do not
want to duplicate the item subfields, but we keep the glued value in both
subfields. This operation only affects items, since we are not doing this
reverse operation for biblio and biblioitem fields.

Sub _get_inverted_marc_field_map
This sub is a helper routine of TransformMarcToKoha, the opposite
transformation. When saving a MARC record, all kohafields are extracted
including multiple mappings.
Suppose that you had both 260c and 264c in your record (which you won't),
than both values get saved initially into copyrightdate like A | B. The
additional code for copyrightdate will extract the first year from this
string.
A small fix in TransformMarcToKoha makes that it only saves a value in a
kohafield if it is defined and not empty. (Same for concatenation.)

Sub TransformMarcToKohaOneField
This sub now just calls TransformMarcToKoha and extracts the requested
field. Note that since we are caching the structure, this does not result
in additional database access and is therefore performance-wise
insignificant. We simplify code and maintenance.
Instead of modifying the passed hashref, it simply returns a value. A call
in C4::Breeding is adjusted accordingly. The routine getKohaField in
Koha::MetadataRecord is redirected to TransformMarcToKohaOneField.
NOTE: The fourth patch restructures/optimizes TransformMarcToKoha[OneField].

Sub get_koha_field_from_marc
This sub can be removed. A call is replaced by TransformMarcToKohaOneField
in C4::XISBN.

Note: The commented lines for sub ModZebrafiles are removed (directly under
TransformMarcToKohaOneField).

Test plan:
For unit tests and interface tests, please see follow-ups.
Run qa tools in order to verify that the modules still compile well.
Read the code changes and verify that they make sense.
Comment 29 Marcel de Rooy 2017-08-14 11:12:22 UTC
Created attachment 65949 [details] [review]
Bug 10306: Allow controlfields in TransformKohaToMarc

Since the interface allows you to connect a kohafield to a MARC
controlfield, this routine should be able to handle that. Unfortunately
it did not.

Test plan:
Change will be tested in Biblio/TransformKohaToMarc.t in the next patch.
Comment 30 Marcel de Rooy 2017-08-14 11:12:27 UTC
Created attachment 65950 [details] [review]
Bug 10306: Add tests for module changes

GetMarcSubfieldStructure: In Biblio.t we are adding a subtest that checks
the structure returned by this routine. Is it a hashref pointing to
arrayrefs of hashrefs? ;)
In Search.t this routine was mocked. The change in the returned structure
is now applied to this mock too (moving the marc tag hashes into
arrayrefs).

GetMarcFromKohaField: In Biblio.t we add a subtest for it. We are checking
if it returns multiple mappings (per kohafield) and calling it in scalar
context too.

GetMarcSubfieldStructureFromKohaField: The existing subtest in Biblio.t is
expanded to also test the call in list context.

TransformKohaToMarc: This sub has its own test script. We are adding a
subtest in TransformKohaToMarc.t for a test with multiple mappings,
and for mapping to a control field in another framework. This also tests
the additional framework parameter.
Additionally, we add a test for the new no_split option used for items.

TransformMarcToKoha: This implicitly tests its helper sub _get_inverted.
This patch adds a new test script for this routine.

TransformMarcToKohaOneField: A few tests are added to the previous new
test script.

Test plan:
[1] Run t/db_dependent/Biblio.t
[2] Run t/db_dependent/Biblio/TransformKohaToMarc.t
[3] Run t/db_dependent/Biblio/TransformMarcToKoha.t
[4] Run t/db_dependent/Search.t
Comment 31 Marcel de Rooy 2017-08-14 11:12:32 UTC
Created attachment 65951 [details] [review]
Bug 10306: Restructure TransformMarcToKoha and update some POD lines

This patch makes the following changes:

[1] Added POD for CountItemsIssued, GetBiblioItemData
[2] Moved TransformMarcToKohaOneField closer to TransformMarcToKoha (before
    sub CountItemsIssued)
[3] Restructured TransformMarcToKoha by extracting individual kohafields via
    TransformMarcToKohaOneField. The latter does no longer call the former.
    This small optimization avoids traversing the whole MARC record again
    and again.
[4] Moved adjusting copyrightdate/publicationyear to separate helper routine
    _adjust_pubyear
[5] Removed obsolete sub _get_inverted_marc_field_map.

Test plan:
Run t/db_dependent/Biblio/TransformMarcToKoha.t
Comment 32 Marcel de Rooy 2017-08-14 11:12:37 UTC Comment hidden (obsolete)
Comment 33 Marcel de Rooy 2017-08-14 11:12:42 UTC Comment hidden (obsolete)
Comment 34 Marcel de Rooy 2017-08-14 11:20:40 UTC
Note for testers/QA:
This patch set adds the multiple mappings, but does not completely resolve the ambiguity within Koha on which framework should be consulted for these mappings. koha2marclinks reflects another opinion than marc_subfields_structure and C4::Biblio::GetMarcFromKohaField. Note that the latter routine is sometimes called with individual frameworkcode, but also with Default.

Although this patch set could go on its own (it just leaves the current inconsistencies), I would rather combine it with the Part 2 patches (Bug 19096). That patch set will make Default the authoritative framework for these mappings.
Comment 35 Marcel de Rooy 2017-08-17 07:09:54 UTC
Created attachment 66134 [details] [review]
Bug 10306: Support for multiple mappings in koha2marclinks

This actually refactors koha2marclinks.pl in order to support multiple
mappings per kohafield.

Instead of three separate mapping pages for biblio, biblioitems and items,
the script now lists them together. This gives a complete overview of all
mappings rightaway. Changes are applied immediately across all frameworks.

Note: This report handles the Default mappings just like it did before.
In this script Koha already considered them as authoritative, although
other parts of Koha did not. Follow-up report 19096 makes Default
mappings authoritative throughout all Koha.

On each line two buttons are provided, Add and Remove, in order to add or
remove an individual mapping. We do no longer provide a separate form with
the names of MARC tags. Since this form is targeted for administrators,
it should be enough to ask for a field tag and subfield code.

Note: The mappings for biblionumber, biblioitemnumber and itemnumber are
so vital that this form marks them as readonly. It is not recommended to
change them.

Test plan:
[1] Add a mapping. Verify via Frameworks or mysql command line that the
    kohafield is saved to the other frameworks too.
[2] Remove the mapping again. Check Frameworks or mysql cl again.
[3] Test adding a second mapping. Map copyrightdate to 260c and 264c.
    And map biblioitems.place to 260a and 264a.
[4] Edit biblio record 1: Put 1980 in 260c. Do not include 264c.
    Edit biblio record 2: Put 1990 in 264c. Do not include 260c.
    Edit biblio record 3: Put 2000 in both 260c and 264c. Put CityA in 260a
    and in 264a.
    Edit biblio record 4: Put 2010 in 260c, and 2015 in 264c (which you
    should refuse normally). Put CityA in 260a, and CityB in 264a.
[5] Create a report that shows biblioitems.place and biblio.copyrightdate
    for those biblio records.
    Record 4 should have 2010 in copyrightdate (since TransformMarcToKoha
    picks the first year for copyrightdate).
    Record 3 should have place CityA; record 4 should have CityA | CityB.
    Note: The CityA | CityB example illustrates that we should add some
    additional handling in TransformMarcToKoha for multiple 264s.
[6] Add these four biblio records to a new list. Sort by Year.
    With OPACXSLTListsDisplay==default, check if the order = 1,2,3,4.
    (The order is based on biblio.copyrightdate.)
    Note that (RDA) record 2 would be on top without this patch set, since
    copyrightdate would have been null.
Comment 36 Marcel de Rooy 2017-08-17 07:10:00 UTC
Created attachment 66135 [details] [review]
Bug 10306: Adjusted online help text for koha2marclinks

Test plan:
Click on Help. Read the text.
Comment 37 Marcel de Rooy 2017-08-17 07:11:21 UTC
Resolved a "Odd number of elements in anonymous hash" warning in koha2marclinks.
Comment 38 Marcel de Rooy 2017-08-17 09:53:42 UTC
Created attachment 66137 [details] [review]
Bug 10306: Core module changes for multiple mappings

In order to allow multiple Koha to MARC mappings (for one kohafield), we
need to adjust a few key routines in C4/Biblio.pm. This results in a few
changes in dependent modules too.

Note: Multiple mappings also include 'alternating' mappings. Such as the
case of MARC21 260 and 264: only one of both fields will be used. Sub
TransformMarcToKoha will handle that just fine; the opposite transformation
is harder, since we do no longer know which field was the source. In that
case TransformKohaToMarc will fill both fields. We only use that operation
in Koha for items (in Acquisition and Cataloging).

Sub GetMarcSubfieldStructure
This sub used a selectall_hashref, which is fine as long as we have only
one mapping for each kohafield. But as DBI states it: If a row has the same
key as an earlier row then it replaces the earlier row. In other words,
we lose the first mapping if we have two.
This patch uses selectall_arrayref with Slice and rearranges the output so
that the returned hash returns an arrayref of hashrefs for each kohafield.
In order to improve consistency, we add an order clause to the SQL
statement used too.

Sub GetMarcFromKohaField
This sub just returned one tag and subfield, but in case of multiple
mappings we should return them all now.
Note: Many calls still expect just one result and will work just fine:
    my ($tag, $sub) = GetMarcFromKohaField(...)
A possible second mapping would be silently ignored. Often the sub is
called for biblionumber or itemnumber. I would not recommend the use of
multiple mappings for such fields btw.
In case the sub is called in scalar context, it will return only the first
tag (instead of the number of tags and subfields).

Sub GetMarcSubfieldStructureFromKohaField
This sub previously returned the hash for one kohafield.
In scalar context it will behave like before: it returns the first hashref
in the arrayref that comes from GetMarcSubfieldStructure.
In list context, it returns an array of all hashrefs (incl. multiple
mappings).
The sub is not used in C4::Ris. Removed the use statement.

Sub TransformKohaToMarc
This sub got a second parameter: frameworkcode.
Historically, Koha more or less assumes kohafields to be defined across all
frameworks (see Koha to MARC mappings). Therefore it falls back to Default
when it is not passed.
When going thru all mappings in building a MARC record, it also supports
multiple mappings. Note that Koha uses this routine in Acquisition and in
Cataloging for items. Normally the MARC record is leading however and the
Koha fields are derivatives for optimization and reporting.

The added third parameter allows for passing a new option no_split => 1.
We use this option in C4::Items::Item2Marc; if two item fields are mapped to
one kohafield but would have different values (which would be very unusual),
these values are glued together. When transforming to MARC again, we do not
want to duplicate the item subfields, but we keep the glued value in both
subfields. This operation only affects items, since we are not doing this
reverse operation for biblio and biblioitem fields.

Sub _get_inverted_marc_field_map
This sub is a helper routine of TransformMarcToKoha, the opposite
transformation. When saving a MARC record, all kohafields are extracted
including multiple mappings.
Suppose that you had both 260c and 264c in your record (which you won't),
than both values get saved initially into copyrightdate like A | B. The
additional code for copyrightdate will extract the first year from this
string.
A small fix in TransformMarcToKoha makes that it only saves a value in a
kohafield if it is defined and not empty. (Same for concatenation.)

Sub TransformMarcToKohaOneField
This sub now just calls TransformMarcToKoha and extracts the requested
field. Note that since we are caching the structure, this does not result
in additional database access and is therefore performance-wise
insignificant. We simplify code and maintenance.
Instead of modifying the passed hashref, it simply returns a value. A call
in C4::Breeding is adjusted accordingly. The routine getKohaField in
Koha::MetadataRecord is redirected to TransformMarcToKohaOneField.
NOTE: The fourth patch restructures/optimizes TransformMarcToKoha[OneField].

Sub get_koha_field_from_marc
This sub can be removed. A call is replaced by TransformMarcToKohaOneField
in C4::XISBN.

Note: The commented lines for sub ModZebrafiles are removed (directly under
TransformMarcToKohaOneField).

Test plan:
For unit tests and interface tests, please see follow-ups.
Run qa tools in order to verify that the modules still compile well.
Read the code changes and verify that they make sense.
Comment 39 Marcel de Rooy 2017-08-17 09:53:48 UTC
Created attachment 66138 [details] [review]
Bug 10306: Allow controlfields in TransformKohaToMarc

Since the interface allows you to connect a kohafield to a MARC
controlfield, this routine should be able to handle that. Unfortunately
it did not.

Test plan:
Change will be tested in Biblio/TransformKohaToMarc.t in the next patch.
Comment 40 Marcel de Rooy 2017-08-17 09:53:53 UTC
Created attachment 66139 [details] [review]
Bug 10306: Add tests for module changes

GetMarcSubfieldStructure: In Biblio.t we are adding a subtest that checks
the structure returned by this routine. Is it a hashref pointing to
arrayrefs of hashrefs? ;)
In Search.t this routine was mocked. The change in the returned structure
is now applied to this mock too (moving the marc tag hashes into
arrayrefs).

GetMarcFromKohaField: In Biblio.t we add a subtest for it. We are checking
if it returns multiple mappings (per kohafield) and calling it in scalar
context too.

GetMarcSubfieldStructureFromKohaField: The existing subtest in Biblio.t is
expanded to also test the call in list context.

TransformKohaToMarc: This sub has its own test script. We are adding a
subtest in TransformKohaToMarc.t for a test with multiple mappings,
and for mapping to a control field in another framework. This also tests
the additional framework parameter.
Additionally, we add a test for the new no_split option used for items.

TransformMarcToKoha: This implicitly tests its helper sub _get_inverted.
This patch adds a new test script for this routine.

TransformMarcToKohaOneField: A few tests are added to the previous new
test script.

Test plan:
[1] Run t/db_dependent/Biblio.t
[2] Run t/db_dependent/Biblio/TransformKohaToMarc.t
[3] Run t/db_dependent/Biblio/TransformMarcToKoha.t
[4] Run t/db_dependent/Search.t
Comment 41 Marcel de Rooy 2017-08-17 09:53:58 UTC
Created attachment 66140 [details] [review]
Bug 10306: Restructure TransformMarcToKoha and update some POD lines

This patch makes the following changes:

[1] Added POD for CountItemsIssued, GetBiblioItemData
[2] Moved TransformMarcToKohaOneField closer to TransformMarcToKoha (before
    sub CountItemsIssued)
[3] Restructured TransformMarcToKoha by extracting individual kohafields via
    TransformMarcToKohaOneField. The latter does no longer call the former.
    This small optimization avoids traversing the whole MARC record again
    and again.
[4] Moved adjusting copyrightdate/publicationyear to separate helper routine
    _adjust_pubyear
[5] Removed obsolete sub _get_inverted_marc_field_map.

Test plan:
Run t/db_dependent/Biblio/TransformMarcToKoha.t
Comment 42 Marcel de Rooy 2017-08-17 09:54:03 UTC
Created attachment 66141 [details] [review]
Bug 10306: Support for multiple mappings in koha2marclinks

This actually refactors koha2marclinks.pl in order to support multiple
mappings per kohafield.

Instead of three separate mapping pages for biblio, biblioitems and items,
the script now lists them together. This gives a complete overview of all
mappings rightaway. Changes are applied immediately across all frameworks.

Note: This report handles the Default mappings just like it did before.
In this script Koha already considered them as authoritative, although
other parts of Koha did not. Follow-up report 19096 makes Default
mappings authoritative throughout all Koha.

On each line two buttons are provided, Add and Remove, in order to add or
remove an individual mapping. We do no longer provide a separate form with
the names of MARC tags. Since this form is targeted for administrators,
it should be enough to ask for a field tag and subfield code.

Note: The mappings for biblionumber, biblioitemnumber and itemnumber are
so vital that this form marks them as readonly. It is not recommended to
change them.

Test plan:
[1] Add a mapping. Verify via Frameworks or mysql command line that the
    kohafield is saved to the other frameworks too.
[2] Remove the mapping again. Check Frameworks or mysql cl again.
[3] Test adding a second mapping. Map copyrightdate to 260c and 264c.
    And map biblioitems.place to 260a and 264a.
[4] Edit biblio record 1: Put 1980 in 260c. Do not include 264c.
    Edit biblio record 2: Put 1990 in 264c. Do not include 260c.
    Edit biblio record 3: Put 2000 in both 260c and 264c. Put CityA in 260a
    and in 264a.
    Edit biblio record 4: Put 2010 in 260c, and 2015 in 264c (which you
    should refuse normally). Put CityA in 260a, and CityB in 264a.
[5] Create a report that shows biblioitems.place and biblio.copyrightdate
    for those biblio records.
    Record 4 should have 2010 in copyrightdate (since TransformMarcToKoha
    picks the first year for copyrightdate).
    Record 3 should have place CityA; record 4 should have CityA | CityB.
    Note: The CityA | CityB example illustrates that we should add some
    additional handling in TransformMarcToKoha for multiple 264s.
[6] Add these four biblio records to a new list. Sort by Year.
    With OPACXSLTListsDisplay==default, check if the order = 1,2,3,4.
    (The order is based on biblio.copyrightdate.)
    Note that (RDA) record 2 would be on top without this patch set, since
    copyrightdate would have been null.
Comment 43 Marcel de Rooy 2017-08-17 09:54:08 UTC
Created attachment 66142 [details] [review]
Bug 10306: Adjusted online help text for koha2marclinks

Test plan:
Click on Help. Read the text.
Comment 44 Marcel de Rooy 2017-08-17 09:55:19 UTC
Trivial rebase on current master.
Comment 45 Marcel de Rooy 2017-08-22 12:35:58 UTC
Additional note for testers:
On the follow-up report 19096 I have added multiple mappings for items in the test plan. It is not that useful in a practical sense, but ensures us that the functionality is not disturbed.
Comment 46 Josef Moravec 2017-08-22 12:51:53 UTC
Created attachment 66327 [details] [review]
[SIGNED-OFF] Bug 10306: Core module changes for multiple mappings

In order to allow multiple Koha to MARC mappings (for one kohafield), we
need to adjust a few key routines in C4/Biblio.pm. This results in a few
changes in dependent modules too.

Note: Multiple mappings also include 'alternating' mappings. Such as the
case of MARC21 260 and 264: only one of both fields will be used. Sub
TransformMarcToKoha will handle that just fine; the opposite transformation
is harder, since we do no longer know which field was the source. In that
case TransformKohaToMarc will fill both fields. We only use that operation
in Koha for items (in Acquisition and Cataloging).

Sub GetMarcSubfieldStructure
This sub used a selectall_hashref, which is fine as long as we have only
one mapping for each kohafield. But as DBI states it: If a row has the same
key as an earlier row then it replaces the earlier row. In other words,
we lose the first mapping if we have two.
This patch uses selectall_arrayref with Slice and rearranges the output so
that the returned hash returns an arrayref of hashrefs for each kohafield.
In order to improve consistency, we add an order clause to the SQL
statement used too.

Sub GetMarcFromKohaField
This sub just returned one tag and subfield, but in case of multiple
mappings we should return them all now.
Note: Many calls still expect just one result and will work just fine:
    my ($tag, $sub) = GetMarcFromKohaField(...)
A possible second mapping would be silently ignored. Often the sub is
called for biblionumber or itemnumber. I would not recommend the use of
multiple mappings for such fields btw.
In case the sub is called in scalar context, it will return only the first
tag (instead of the number of tags and subfields).

Sub GetMarcSubfieldStructureFromKohaField
This sub previously returned the hash for one kohafield.
In scalar context it will behave like before: it returns the first hashref
in the arrayref that comes from GetMarcSubfieldStructure.
In list context, it returns an array of all hashrefs (incl. multiple
mappings).
The sub is not used in C4::Ris. Removed the use statement.

Sub TransformKohaToMarc
This sub got a second parameter: frameworkcode.
Historically, Koha more or less assumes kohafields to be defined across all
frameworks (see Koha to MARC mappings). Therefore it falls back to Default
when it is not passed.
When going thru all mappings in building a MARC record, it also supports
multiple mappings. Note that Koha uses this routine in Acquisition and in
Cataloging for items. Normally the MARC record is leading however and the
Koha fields are derivatives for optimization and reporting.

The added third parameter allows for passing a new option no_split => 1.
We use this option in C4::Items::Item2Marc; if two item fields are mapped to
one kohafield but would have different values (which would be very unusual),
these values are glued together. When transforming to MARC again, we do not
want to duplicate the item subfields, but we keep the glued value in both
subfields. This operation only affects items, since we are not doing this
reverse operation for biblio and biblioitem fields.

Sub _get_inverted_marc_field_map
This sub is a helper routine of TransformMarcToKoha, the opposite
transformation. When saving a MARC record, all kohafields are extracted
including multiple mappings.
Suppose that you had both 260c and 264c in your record (which you won't),
than both values get saved initially into copyrightdate like A | B. The
additional code for copyrightdate will extract the first year from this
string.
A small fix in TransformMarcToKoha makes that it only saves a value in a
kohafield if it is defined and not empty. (Same for concatenation.)

Sub TransformMarcToKohaOneField
This sub now just calls TransformMarcToKoha and extracts the requested
field. Note that since we are caching the structure, this does not result
in additional database access and is therefore performance-wise
insignificant. We simplify code and maintenance.
Instead of modifying the passed hashref, it simply returns a value. A call
in C4::Breeding is adjusted accordingly. The routine getKohaField in
Koha::MetadataRecord is redirected to TransformMarcToKohaOneField.
NOTE: The fourth patch restructures/optimizes TransformMarcToKoha[OneField].

Sub get_koha_field_from_marc
This sub can be removed. A call is replaced by TransformMarcToKohaOneField
in C4::XISBN.

Note: The commented lines for sub ModZebrafiles are removed (directly under
TransformMarcToKohaOneField).

Test plan:
For unit tests and interface tests, please see follow-ups.
Run qa tools in order to verify that the modules still compile well.
Read the code changes and verify that they make sense.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 47 Josef Moravec 2017-08-22 12:51:57 UTC
Created attachment 66328 [details] [review]
[SIGNED-OFF] Bug 10306: Allow controlfields in TransformKohaToMarc

Since the interface allows you to connect a kohafield to a MARC
controlfield, this routine should be able to handle that. Unfortunately
it did not.

Test plan:
Change will be tested in Biblio/TransformKohaToMarc.t in the next patch.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 48 Josef Moravec 2017-08-22 12:52:03 UTC
Created attachment 66329 [details] [review]
[SIGNED-OFF] Bug 10306: Add tests for module changes

GetMarcSubfieldStructure: In Biblio.t we are adding a subtest that checks
the structure returned by this routine. Is it a hashref pointing to
arrayrefs of hashrefs? ;)
In Search.t this routine was mocked. The change in the returned structure
is now applied to this mock too (moving the marc tag hashes into
arrayrefs).

GetMarcFromKohaField: In Biblio.t we add a subtest for it. We are checking
if it returns multiple mappings (per kohafield) and calling it in scalar
context too.

GetMarcSubfieldStructureFromKohaField: The existing subtest in Biblio.t is
expanded to also test the call in list context.

TransformKohaToMarc: This sub has its own test script. We are adding a
subtest in TransformKohaToMarc.t for a test with multiple mappings,
and for mapping to a control field in another framework. This also tests
the additional framework parameter.
Additionally, we add a test for the new no_split option used for items.

TransformMarcToKoha: This implicitly tests its helper sub _get_inverted.
This patch adds a new test script for this routine.

TransformMarcToKohaOneField: A few tests are added to the previous new
test script.

Test plan:
[1] Run t/db_dependent/Biblio.t
[2] Run t/db_dependent/Biblio/TransformKohaToMarc.t
[3] Run t/db_dependent/Biblio/TransformMarcToKoha.t
[4] Run t/db_dependent/Search.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 49 Josef Moravec 2017-08-22 12:52:07 UTC
Created attachment 66330 [details] [review]
[SIGNED-OFF] Bug 10306: Restructure TransformMarcToKoha and update some POD lines

This patch makes the following changes:

[1] Added POD for CountItemsIssued, GetBiblioItemData
[2] Moved TransformMarcToKohaOneField closer to TransformMarcToKoha (before
    sub CountItemsIssued)
[3] Restructured TransformMarcToKoha by extracting individual kohafields via
    TransformMarcToKohaOneField. The latter does no longer call the former.
    This small optimization avoids traversing the whole MARC record again
    and again.
[4] Moved adjusting copyrightdate/publicationyear to separate helper routine
    _adjust_pubyear
[5] Removed obsolete sub _get_inverted_marc_field_map.

Test plan:
Run t/db_dependent/Biblio/TransformMarcToKoha.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 50 Josef Moravec 2017-08-22 12:52:11 UTC
Created attachment 66331 [details] [review]
[SIGNED-OFF] Bug 10306: Support for multiple mappings in koha2marclinks

This actually refactors koha2marclinks.pl in order to support multiple
mappings per kohafield.

Instead of three separate mapping pages for biblio, biblioitems and items,
the script now lists them together. This gives a complete overview of all
mappings rightaway. Changes are applied immediately across all frameworks.

Note: This report handles the Default mappings just like it did before.
In this script Koha already considered them as authoritative, although
other parts of Koha did not. Follow-up report 19096 makes Default
mappings authoritative throughout all Koha.

On each line two buttons are provided, Add and Remove, in order to add or
remove an individual mapping. We do no longer provide a separate form with
the names of MARC tags. Since this form is targeted for administrators,
it should be enough to ask for a field tag and subfield code.

Note: The mappings for biblionumber, biblioitemnumber and itemnumber are
so vital that this form marks them as readonly. It is not recommended to
change them.

Test plan:
[1] Add a mapping. Verify via Frameworks or mysql command line that the
    kohafield is saved to the other frameworks too.
[2] Remove the mapping again. Check Frameworks or mysql cl again.
[3] Test adding a second mapping. Map copyrightdate to 260c and 264c.
    And map biblioitems.place to 260a and 264a.
[4] Edit biblio record 1: Put 1980 in 260c. Do not include 264c.
    Edit biblio record 2: Put 1990 in 264c. Do not include 260c.
    Edit biblio record 3: Put 2000 in both 260c and 264c. Put CityA in 260a
    and in 264a.
    Edit biblio record 4: Put 2010 in 260c, and 2015 in 264c (which you
    should refuse normally). Put CityA in 260a, and CityB in 264a.
[5] Create a report that shows biblioitems.place and biblio.copyrightdate
    for those biblio records.
    Record 4 should have 2010 in copyrightdate (since TransformMarcToKoha
    picks the first year for copyrightdate).
    Record 3 should have place CityA; record 4 should have CityA | CityB.
    Note: The CityA | CityB example illustrates that we should add some
    additional handling in TransformMarcToKoha for multiple 264s.
[6] Add these four biblio records to a new list. Sort by Year.
    With OPACXSLTListsDisplay==default, check if the order = 1,2,3,4.
    (The order is based on biblio.copyrightdate.)
    Note that (RDA) record 2 would be on top without this patch set, since
    copyrightdate would have been null.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 51 Josef Moravec 2017-08-22 12:52:16 UTC
Created attachment 66332 [details] [review]
[SIGNED-OFF] Bug 10306: Adjusted online help text for koha2marclinks

Test plan:
Click on Help. Read the text.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 52 David Cook 2017-08-24 00:49:24 UTC
Just wanted to cheer for this one some more... looking forward to its addition to Koha!
Comment 53 Marcel de Rooy 2017-08-24 10:26:01 UTC
Created attachment 66405 [details] [review]
Bug 10306: [QA Follow-up] Correct a TestBuilder call

In order to create a new framework, MarcSubfieldStructure was used as
source. This works since that table contains a FK for frameworkcode.
But obviously, we should use BiblioFramework as source.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 54 Marcel de Rooy 2017-08-28 07:47:56 UTC
Created attachment 66547 [details] [review]
Bug 10306: Adjusted online help text for koha2marclinks

Test plan:
Click on Help. Read the text.
Comment 55 Marcel de Rooy 2017-08-28 07:48:03 UTC
Created attachment 66548 [details] [review]
Bug 10306: [QA Follow-up] Correct a TestBuilder call

In order to create a new framework, MarcSubfieldStructure was used as
source. This works since that table contains a FK for frameworkcode.
But obviously, we should use BiblioFramework as source.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 56 Marcel de Rooy 2017-10-02 09:50:05 UTC
Created attachment 67501 [details] [review]
Bug 10306: Core module changes for multiple mappings

In order to allow multiple Koha to MARC mappings (for one kohafield), we
need to adjust a few key routines in C4/Biblio.pm. This results in a few
changes in dependent modules too.

Note: Multiple mappings also include 'alternating' mappings. Such as the
case of MARC21 260 and 264: only one of both fields will be used. Sub
TransformMarcToKoha will handle that just fine; the opposite transformation
is harder, since we do no longer know which field was the source. In that
case TransformKohaToMarc will fill both fields. We only use that operation
in Koha for items (in Acquisition and Cataloging).

Sub GetMarcSubfieldStructure
This sub used a selectall_hashref, which is fine as long as we have only
one mapping for each kohafield. But as DBI states it: If a row has the same
key as an earlier row then it replaces the earlier row. In other words,
we lose the first mapping if we have two.
This patch uses selectall_arrayref with Slice and rearranges the output so
that the returned hash returns an arrayref of hashrefs for each kohafield.
In order to improve consistency, we add an order clause to the SQL
statement used too.

Sub GetMarcFromKohaField
This sub just returned one tag and subfield, but in case of multiple
mappings we should return them all now.
Note: Many calls still expect just one result and will work just fine:
    my ($tag, $sub) = GetMarcFromKohaField(...)
A possible second mapping would be silently ignored. Often the sub is
called for biblionumber or itemnumber. I would not recommend the use of
multiple mappings for such fields btw.
In case the sub is called in scalar context, it will return only the first
tag (instead of the number of tags and subfields).

Sub GetMarcSubfieldStructureFromKohaField
This sub previously returned the hash for one kohafield.
In scalar context it will behave like before: it returns the first hashref
in the arrayref that comes from GetMarcSubfieldStructure.
In list context, it returns an array of all hashrefs (incl. multiple
mappings).
The sub is not used in C4::Ris. Removed the use statement.

Sub TransformKohaToMarc
This sub got a second parameter: frameworkcode.
Historically, Koha more or less assumes kohafields to be defined across all
frameworks (see Koha to MARC mappings). Therefore it falls back to Default
when it is not passed.
When going thru all mappings in building a MARC record, it also supports
multiple mappings. Note that Koha uses this routine in Acquisition and in
Cataloging for items. Normally the MARC record is leading however and the
Koha fields are derivatives for optimization and reporting.

The added third parameter allows for passing a new option no_split => 1.
We use this option in C4::Items::Item2Marc; if two item fields are mapped to
one kohafield but would have different values (which would be very unusual),
these values are glued together. When transforming to MARC again, we do not
want to duplicate the item subfields, but we keep the glued value in both
subfields. This operation only affects items, since we are not doing this
reverse operation for biblio and biblioitem fields.

Sub _get_inverted_marc_field_map
This sub is a helper routine of TransformMarcToKoha, the opposite
transformation. When saving a MARC record, all kohafields are extracted
including multiple mappings.
Suppose that you had both 260c and 264c in your record (which you won't),
than both values get saved initially into copyrightdate like A | B. The
additional code for copyrightdate will extract the first year from this
string.
A small fix in TransformMarcToKoha makes that it only saves a value in a
kohafield if it is defined and not empty. (Same for concatenation.)

Sub TransformMarcToKohaOneField
This sub now just calls TransformMarcToKoha and extracts the requested
field. Note that since we are caching the structure, this does not result
in additional database access and is therefore performance-wise
insignificant. We simplify code and maintenance.
Instead of modifying the passed hashref, it simply returns a value. A call
in C4::Breeding is adjusted accordingly. The routine getKohaField in
Koha::MetadataRecord is redirected to TransformMarcToKohaOneField.
NOTE: The fourth patch restructures/optimizes TransformMarcToKoha[OneField].

Sub get_koha_field_from_marc
This sub can be removed. A call is replaced by TransformMarcToKohaOneField
in C4::XISBN.

Note: The commented lines for sub ModZebrafiles are removed (directly under
TransformMarcToKohaOneField).

Test plan:
For unit tests and interface tests, please see follow-ups.
Run qa tools in order to verify that the modules still compile well.
Read the code changes and verify that they make sense.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 57 Marcel de Rooy 2017-10-02 09:50:11 UTC
Created attachment 67502 [details] [review]
Bug 10306: Allow controlfields in TransformKohaToMarc

Since the interface allows you to connect a kohafield to a MARC
controlfield, this routine should be able to handle that. Unfortunately
it did not.

Test plan:
Change will be tested in Biblio/TransformKohaToMarc.t in the next patch.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 58 Marcel de Rooy 2017-10-02 09:50:16 UTC
Created attachment 67503 [details] [review]
Bug 10306: Add tests for module changes

GetMarcSubfieldStructure: In Biblio.t we are adding a subtest that checks
the structure returned by this routine. Is it a hashref pointing to
arrayrefs of hashrefs? ;)
In Search.t this routine was mocked. The change in the returned structure
is now applied to this mock too (moving the marc tag hashes into
arrayrefs).

GetMarcFromKohaField: In Biblio.t we add a subtest for it. We are checking
if it returns multiple mappings (per kohafield) and calling it in scalar
context too.

GetMarcSubfieldStructureFromKohaField: The existing subtest in Biblio.t is
expanded to also test the call in list context.

TransformKohaToMarc: This sub has its own test script. We are adding a
subtest in TransformKohaToMarc.t for a test with multiple mappings,
and for mapping to a control field in another framework. This also tests
the additional framework parameter.
Additionally, we add a test for the new no_split option used for items.

TransformMarcToKoha: This implicitly tests its helper sub _get_inverted.
This patch adds a new test script for this routine.

TransformMarcToKohaOneField: A few tests are added to the previous new
test script.

Test plan:
[1] Run t/db_dependent/Biblio.t
[2] Run t/db_dependent/Biblio/TransformKohaToMarc.t
[3] Run t/db_dependent/Biblio/TransformMarcToKoha.t
[4] Run t/db_dependent/Search.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 59 Marcel de Rooy 2017-10-02 09:50:21 UTC
Created attachment 67504 [details] [review]
Bug 10306: Restructure TransformMarcToKoha and update some POD lines

This patch makes the following changes:

[1] Added POD for CountItemsIssued, GetBiblioItemData
[2] Moved TransformMarcToKohaOneField closer to TransformMarcToKoha (before
    sub CountItemsIssued)
[3] Restructured TransformMarcToKoha by extracting individual kohafields via
    TransformMarcToKohaOneField. The latter does no longer call the former.
    This small optimization avoids traversing the whole MARC record again
    and again.
[4] Moved adjusting copyrightdate/publicationyear to separate helper routine
    _adjust_pubyear
[5] Removed obsolete sub _get_inverted_marc_field_map.

Test plan:
Run t/db_dependent/Biblio/TransformMarcToKoha.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 60 Marcel de Rooy 2017-10-02 09:50:26 UTC
Created attachment 67505 [details] [review]
Bug 10306: Support for multiple mappings in koha2marclinks

This actually refactors koha2marclinks.pl in order to support multiple
mappings per kohafield.

Instead of three separate mapping pages for biblio, biblioitems and items,
the script now lists them together. This gives a complete overview of all
mappings rightaway. Changes are applied immediately across all frameworks.

Note: This report handles the Default mappings just like it did before.
In this script Koha already considered them as authoritative, although
other parts of Koha did not. Follow-up report 19096 makes Default
mappings authoritative throughout all Koha.

On each line two buttons are provided, Add and Remove, in order to add or
remove an individual mapping. We do no longer provide a separate form with
the names of MARC tags. Since this form is targeted for administrators,
it should be enough to ask for a field tag and subfield code.

Note: The mappings for biblionumber, biblioitemnumber and itemnumber are
so vital that this form marks them as readonly. It is not recommended to
change them.

Test plan:
[1] Add a mapping. Verify via Frameworks or mysql command line that the
    kohafield is saved to the other frameworks too.
[2] Remove the mapping again. Check Frameworks or mysql cl again.
[3] Test adding a second mapping. Map copyrightdate to 260c and 264c.
    And map biblioitems.place to 260a and 264a.
[4] Edit biblio record 1: Put 1980 in 260c. Do not include 264c.
    Edit biblio record 2: Put 1990 in 264c. Do not include 260c.
    Edit biblio record 3: Put 2000 in both 260c and 264c. Put CityA in 260a
    and in 264a.
    Edit biblio record 4: Put 2010 in 260c, and 2015 in 264c (which you
    should refuse normally). Put CityA in 260a, and CityB in 264a.
[5] Create a report that shows biblioitems.place and biblio.copyrightdate
    for those biblio records.
    Record 4 should have 2010 in copyrightdate (since TransformMarcToKoha
    picks the first year for copyrightdate).
    Record 3 should have place CityA; record 4 should have CityA | CityB.
    Note: The CityA | CityB example illustrates that we should add some
    additional handling in TransformMarcToKoha for multiple 264s.
[6] Add these four biblio records to a new list. Sort by Year.
    With OPACXSLTListsDisplay==default, check if the order = 1,2,3,4.
    (The order is based on biblio.copyrightdate.)
    Note that (RDA) record 2 would be on top without this patch set, since
    copyrightdate would have been null.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 61 Marcel de Rooy 2017-10-02 09:50:32 UTC
Created attachment 67506 [details] [review]
Bug 10306: Adjusted online help text for koha2marclinks

Test plan:
Click on Help. Read the text.
Comment 62 Marcel de Rooy 2017-10-02 09:50:37 UTC
Created attachment 67507 [details] [review]
Bug 10306: [QA Follow-up] Correct a TestBuilder call

In order to create a new framework, MarcSubfieldStructure was used as
source. This works since that table contains a FK for frameworkcode.
But obviously, we should use BiblioFramework as source.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 63 Marcel de Rooy 2017-10-02 09:51:09 UTC
Simple rebase
Comment 64 Tomás Cohen Arazi 2017-10-20 20:17:25 UTC
Marcel, I've looked at this patchset, along with bug 19096. Everything looks correct so far. I'm only concerned about the UI changes, the 'comma' separator for subfields, and no feedback on wrong input.

Maybe we can talk about it on monday, and maybe a quick fix might allow passing QA on this one :-D
Comment 65 Marcel de Rooy 2017-10-23 09:42:45 UTC
(In reply to Tomás Cohen Arazi from comment #64)
> Marcel, I've looked at this patchset, along with bug 19096. Everything looks
> correct so far. I'm only concerned about the UI changes, the 'comma'
> separator for subfields, and no feedback on wrong input.

About the UI changes of koha2marclinks.pl: You mentioned "It seems to be no option to edit a mapping. Only an 'Add' button.". 
We have a few possibilities: 1) There are no buttons for fields like biblionumber, itemnumber 2) There is only one Add button, when the field is not yet used in a mapping. 3) There are two buttons: Add and Remove. You can remove the current mapping or add a new one. Why is there no Edit button? This is by design. You cant change a mapping but you can remove it. Or add a new one.

The comma is used when adding a new field in the js form. Do you have another suggestion? When you type something silly, there is no feedback. This was somewhat lazy indeed, thinking that we have an admin here; if you are not knowing what you are doing, this is a very dangerous form..
Comment 66 Marcel de Rooy 2017-10-23 10:35:57 UTC
Created attachment 68382 [details] [review]
Bug 10306: (QA follow-up) More feedback for admins in koha2marclinks

As requested by Tomas, this patch does:

[1] Add a js alert when you did not type field,subfield
[2] Print a yellow alert div when the field,subfield is not found.
[3] Bonus: Make it little bit more secure by demanding a POST. (Leaving
    CSRF etc. for another report.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 67 Kyle M Hall 2017-10-27 16:50:04 UTC
Created attachment 68766 [details] [review]
Bug 10306: Core module changes for multiple mappings

In order to allow multiple Koha to MARC mappings (for one kohafield), we
need to adjust a few key routines in C4/Biblio.pm. This results in a few
changes in dependent modules too.

Note: Multiple mappings also include 'alternating' mappings. Such as the
case of MARC21 260 and 264: only one of both fields will be used. Sub
TransformMarcToKoha will handle that just fine; the opposite transformation
is harder, since we do no longer know which field was the source. In that
case TransformKohaToMarc will fill both fields. We only use that operation
in Koha for items (in Acquisition and Cataloging).

Sub GetMarcSubfieldStructure
This sub used a selectall_hashref, which is fine as long as we have only
one mapping for each kohafield. But as DBI states it: If a row has the same
key as an earlier row then it replaces the earlier row. In other words,
we lose the first mapping if we have two.
This patch uses selectall_arrayref with Slice and rearranges the output so
that the returned hash returns an arrayref of hashrefs for each kohafield.
In order to improve consistency, we add an order clause to the SQL
statement used too.

Sub GetMarcFromKohaField
This sub just returned one tag and subfield, but in case of multiple
mappings we should return them all now.
Note: Many calls still expect just one result and will work just fine:
    my ($tag, $sub) = GetMarcFromKohaField(...)
A possible second mapping would be silently ignored. Often the sub is
called for biblionumber or itemnumber. I would not recommend the use of
multiple mappings for such fields btw.
In case the sub is called in scalar context, it will return only the first
tag (instead of the number of tags and subfields).

Sub GetMarcSubfieldStructureFromKohaField
This sub previously returned the hash for one kohafield.
In scalar context it will behave like before: it returns the first hashref
in the arrayref that comes from GetMarcSubfieldStructure.
In list context, it returns an array of all hashrefs (incl. multiple
mappings).
The sub is not used in C4::Ris. Removed the use statement.

Sub TransformKohaToMarc
This sub got a second parameter: frameworkcode.
Historically, Koha more or less assumes kohafields to be defined across all
frameworks (see Koha to MARC mappings). Therefore it falls back to Default
when it is not passed.
When going thru all mappings in building a MARC record, it also supports
multiple mappings. Note that Koha uses this routine in Acquisition and in
Cataloging for items. Normally the MARC record is leading however and the
Koha fields are derivatives for optimization and reporting.

The added third parameter allows for passing a new option no_split => 1.
We use this option in C4::Items::Item2Marc; if two item fields are mapped to
one kohafield but would have different values (which would be very unusual),
these values are glued together. When transforming to MARC again, we do not
want to duplicate the item subfields, but we keep the glued value in both
subfields. This operation only affects items, since we are not doing this
reverse operation for biblio and biblioitem fields.

Sub _get_inverted_marc_field_map
This sub is a helper routine of TransformMarcToKoha, the opposite
transformation. When saving a MARC record, all kohafields are extracted
including multiple mappings.
Suppose that you had both 260c and 264c in your record (which you won't),
than both values get saved initially into copyrightdate like A | B. The
additional code for copyrightdate will extract the first year from this
string.
A small fix in TransformMarcToKoha makes that it only saves a value in a
kohafield if it is defined and not empty. (Same for concatenation.)

Sub TransformMarcToKohaOneField
This sub now just calls TransformMarcToKoha and extracts the requested
field. Note that since we are caching the structure, this does not result
in additional database access and is therefore performance-wise
insignificant. We simplify code and maintenance.
Instead of modifying the passed hashref, it simply returns a value. A call
in C4::Breeding is adjusted accordingly. The routine getKohaField in
Koha::MetadataRecord is redirected to TransformMarcToKohaOneField.
NOTE: The fourth patch restructures/optimizes TransformMarcToKoha[OneField].

Sub get_koha_field_from_marc
This sub can be removed. A call is replaced by TransformMarcToKohaOneField
in C4::XISBN.

Note: The commented lines for sub ModZebrafiles are removed (directly under
TransformMarcToKohaOneField).

Test plan:
For unit tests and interface tests, please see follow-ups.
Run qa tools in order to verify that the modules still compile well.
Read the code changes and verify that they make sense.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 68 Kyle M Hall 2017-10-27 16:50:13 UTC
Created attachment 68767 [details] [review]
Bug 10306: Allow controlfields in TransformKohaToMarc

Since the interface allows you to connect a kohafield to a MARC
controlfield, this routine should be able to handle that. Unfortunately
it did not.

Test plan:
Change will be tested in Biblio/TransformKohaToMarc.t in the next patch.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 69 Kyle M Hall 2017-10-27 16:50:17 UTC
Created attachment 68768 [details] [review]
Bug 10306: Add tests for module changes

GetMarcSubfieldStructure: In Biblio.t we are adding a subtest that checks
the structure returned by this routine. Is it a hashref pointing to
arrayrefs of hashrefs? ;)
In Search.t this routine was mocked. The change in the returned structure
is now applied to this mock too (moving the marc tag hashes into
arrayrefs).

GetMarcFromKohaField: In Biblio.t we add a subtest for it. We are checking
if it returns multiple mappings (per kohafield) and calling it in scalar
context too.

GetMarcSubfieldStructureFromKohaField: The existing subtest in Biblio.t is
expanded to also test the call in list context.

TransformKohaToMarc: This sub has its own test script. We are adding a
subtest in TransformKohaToMarc.t for a test with multiple mappings,
and for mapping to a control field in another framework. This also tests
the additional framework parameter.
Additionally, we add a test for the new no_split option used for items.

TransformMarcToKoha: This implicitly tests its helper sub _get_inverted.
This patch adds a new test script for this routine.

TransformMarcToKohaOneField: A few tests are added to the previous new
test script.

Test plan:
[1] Run t/db_dependent/Biblio.t
[2] Run t/db_dependent/Biblio/TransformKohaToMarc.t
[3] Run t/db_dependent/Biblio/TransformMarcToKoha.t
[4] Run t/db_dependent/Search.t

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 70 Kyle M Hall 2017-10-27 16:50:21 UTC
Created attachment 68769 [details] [review]
Bug 10306: Restructure TransformMarcToKoha and update some POD lines

This patch makes the following changes:

[1] Added POD for CountItemsIssued, GetBiblioItemData
[2] Moved TransformMarcToKohaOneField closer to TransformMarcToKoha (before
    sub CountItemsIssued)
[3] Restructured TransformMarcToKoha by extracting individual kohafields via
    TransformMarcToKohaOneField. The latter does no longer call the former.
    This small optimization avoids traversing the whole MARC record again
    and again.
[4] Moved adjusting copyrightdate/publicationyear to separate helper routine
    _adjust_pubyear
[5] Removed obsolete sub _get_inverted_marc_field_map.

Test plan:
Run t/db_dependent/Biblio/TransformMarcToKoha.t

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 71 Kyle M Hall 2017-10-27 16:50:26 UTC
Created attachment 68770 [details] [review]
Bug 10306: Support for multiple mappings in koha2marclinks

This actually refactors koha2marclinks.pl in order to support multiple
mappings per kohafield.

Instead of three separate mapping pages for biblio, biblioitems and items,
the script now lists them together. This gives a complete overview of all
mappings rightaway. Changes are applied immediately across all frameworks.

Note: This report handles the Default mappings just like it did before.
In this script Koha already considered them as authoritative, although
other parts of Koha did not. Follow-up report 19096 makes Default
mappings authoritative throughout all Koha.

On each line two buttons are provided, Add and Remove, in order to add or
remove an individual mapping. We do no longer provide a separate form with
the names of MARC tags. Since this form is targeted for administrators,
it should be enough to ask for a field tag and subfield code.

Note: The mappings for biblionumber, biblioitemnumber and itemnumber are
so vital that this form marks them as readonly. It is not recommended to
change them.

Test plan:
[1] Add a mapping. Verify via Frameworks or mysql command line that the
    kohafield is saved to the other frameworks too.
[2] Remove the mapping again. Check Frameworks or mysql cl again.
[3] Test adding a second mapping. Map copyrightdate to 260c and 264c.
    And map biblioitems.place to 260a and 264a.
[4] Edit biblio record 1: Put 1980 in 260c. Do not include 264c.
    Edit biblio record 2: Put 1990 in 264c. Do not include 260c.
    Edit biblio record 3: Put 2000 in both 260c and 264c. Put CityA in 260a
    and in 264a.
    Edit biblio record 4: Put 2010 in 260c, and 2015 in 264c (which you
    should refuse normally). Put CityA in 260a, and CityB in 264a.
[5] Create a report that shows biblioitems.place and biblio.copyrightdate
    for those biblio records.
    Record 4 should have 2010 in copyrightdate (since TransformMarcToKoha
    picks the first year for copyrightdate).
    Record 3 should have place CityA; record 4 should have CityA | CityB.
    Note: The CityA | CityB example illustrates that we should add some
    additional handling in TransformMarcToKoha for multiple 264s.
[6] Add these four biblio records to a new list. Sort by Year.
    With OPACXSLTListsDisplay==default, check if the order = 1,2,3,4.
    (The order is based on biblio.copyrightdate.)
    Note that (RDA) record 2 would be on top without this patch set, since
    copyrightdate would have been null.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 72 Kyle M Hall 2017-10-27 16:50:30 UTC
Created attachment 68771 [details] [review]
Bug 10306: Adjusted online help text for koha2marclinks

Test plan:
Click on Help. Read the text.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 73 Kyle M Hall 2017-10-27 16:50:34 UTC
Created attachment 68772 [details] [review]
Bug 10306: [QA Follow-up] Correct a TestBuilder call

In order to create a new framework, MarcSubfieldStructure was used as
source. This works since that table contains a FK for frameworkcode.
But obviously, we should use BiblioFramework as source.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 74 Kyle M Hall 2017-10-27 16:50:38 UTC
Created attachment 68773 [details] [review]
Bug 10306: (QA follow-up) More feedback for admins in koha2marclinks

As requested by Tomas, this patch does:

[1] Add a js alert when you did not type field,subfield
[2] Print a yellow alert div when the field,subfield is not found.
[3] Bonus: Make it little bit more secure by demanding a POST. (Leaving
    CSRF etc. for another report.)

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 75 Jonathan Druart 2017-12-11 14:35:04 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 76 Tomás Cohen Arazi 2017-12-11 14:42:30 UTC
(In reply to Jonathan Druart from comment #75)
> Pushed to master for 18.05, thanks to everybody involved!

YAY!
Comment 77 Nick Clemens 2017-12-14 16:45:53 UTC
Awesome work all!

Enhancement, will not be backported
Comment 78 Aleisha Amohia 2017-12-19 03:08:28 UTC
This commit 01fbe2be9925f8586b9a2cb4dc908fc856b672a8 (Bug 10306: Core module changes for multiple mappings) is breaking a bunch of things for me. A few examples:

/cgi-bin/koha/opac-user.pl - when trying to see OPAC user summary, I get
internal server error: Not an ARRAY reference at /home/vagrant/kohaclone/C4/Biblio.pm line 1124
/cgi-bin/koha/acqui/neworderbiblio.pl - when trying to add an order to a basket from an existing record, I get
internal server error: Not an ARRAY reference at /home/vagrant/kohaclone/C4/Biblio.pm line 2667
Comment 79 Marcel de Rooy 2017-12-19 13:15:00 UTC
(In reply to Aleisha Amohia from comment #78)
> This commit 01fbe2be9925f8586b9a2cb4dc908fc856b672a8 (Bug 10306: Core module
> changes for multiple mappings) is breaking a bunch of things for me. A few
> examples:
> 
> /cgi-bin/koha/opac-user.pl - when trying to see OPAC user summary, I get
> internal server error: Not an ARRAY reference at
> /home/vagrant/kohaclone/C4/Biblio.pm line 1124
> /cgi-bin/koha/acqui/neworderbiblio.pl - when trying to add an order to a
> basket from an existing record, I get
> internal server error: Not an ARRAY reference at
> /home/vagrant/kohaclone/C4/Biblio.pm line 2667

Are you sure?
Please restart Plack and - in this case important - flush the cache. The cache might contain some constructs that have been outdated in view of this patch. A regular upgrade should include restarting Plack and flashing the cache too.

Note that the line number 1124 does refer to a POD line in the current module. This may be an indication of the above.

Please report back if this resolved the problem. Thanks.
Comment 80 Marcel de Rooy 2017-12-19 13:19:06 UTC
(In reply to Aleisha Amohia from comment #78)
> /cgi-bin/koha/opac-user.pl - when trying to see OPAC user summary, I get
> internal server error: Not an ARRAY reference at
> /home/vagrant/kohaclone/C4/Biblio.pm line 1124

Cannot reproduce btw.

> /cgi-bin/koha/acqui/neworderbiblio.pl - when trying to add an order to a
> basket from an existing record, I get
> internal server error: Not an ARRAY reference at
> /home/vagrant/kohaclone/C4/Biblio.pm line 2667

Works for me.
Comment 81 Katrin Fischer 2018-02-07 18:32:24 UTC
*** Bug 20149 has been marked as a duplicate of this bug. ***
Comment 82 David Cook 2018-05-02 03:00:05 UTC
Something that we might not have thought about with this bug...

The 264 isn't always the publisher. The second indicator lets you specify a producer, publisher, distributor, manufacturer... so there could be times when the stuff in the 264 really shouldn't go into publisher. 

https://www.loc.gov/marc/bibliographic/bd264.html
Comment 83 Marcel de Rooy 2018-05-02 11:11:58 UTC
(In reply to David Cook from comment #82)
> Something that we might not have thought about with this bug...
> 
> The 264 isn't always the publisher. The second indicator lets you specify a
> producer, publisher, distributor, manufacturer... so there could be times
> when the stuff in the 264 really shouldn't go into publisher. 
> 
> https://www.loc.gov/marc/bibliographic/bd264.html

Please look at the see also bugs.
I opened bug 19098 for additional logic around such things, but we still need some patches there.
Comment 84 Agnes Rivers-Moore 2018-05-02 19:19:19 UTC
Librarian input - cheering for all of you!. We really want this, thanks for your work on it.
We deal with books 95% of the time. For those records we would really like the same data in 260 and 264.
For the other 5% we can manually fix them while other approaches are explored. It's a lot less work than copy/paste for every single record.


I would like to request that the method for setting this up is documented in some detail for the end user. Many smaller libraries stay well away from mapping & frameworks for fear of breaking something so we are not familiar with making adjustments. If the frameworks can be adjusted/new ones set up set up during upgrade that would be nice.
Thanks again!
Comment 85 Marcel de Rooy 2018-05-14 08:18:16 UTC
(In reply to Agnes Rivers-Moore from comment #84)
> I would like to request that the method for setting this up is documented in
> some detail for the end user. Many smaller libraries stay well away from
> mapping & frameworks for fear of breaking something so we are not familiar
> with making adjustments. If the frameworks can be adjusted/new ones set up
> set up during upgrade that would be nice.

This is a very tiny and easy adjustment:
Go to Administration/Koha to MARC mapping.
For MARC21, find the table row for biblio.copyrightdate (260c), and click on Add.
Type 264,c in the popup. Click OK.
You're done.

Note: At this point new or edited records with 264c, will adjust copyrightdate. Unchanged records will not.
A sysadmin may run the script misc/maintenance/touch_all_biblios --where "copyrightdate is null" for you. This would update all biblios without 260c but having a 264c. (Note: maybe IFNULL(copyrightdate,0)=0 is even better as where condition.)

In view of comment83 this setting is not yet included in frameworks etc. This will be done on the follow-up reports.