Hi Julian,

this statement 'Biblio field indicators should be the same indicators as the linked authority's main field' is not correct, at least in MARC21.
From LOC Manual:
Bib tag 600 is connect with Auth tag 100
Bib ind 1 == Auth ind 1
Bib ind 2 is different from Auth ind 2

Bib tag 240 is connect with Auth tag 130
Bib ind 1 is different from Auth ind 1
Bib ind 2 == Auth ind 2

Bib tag 630 is connect with Auth tag 130
Bib ind 1 == Auth ind 2
Bib ind 2 is different from Auth ind 1

If fact it is true also for Unimarc.
From IFLA Manual:
Bib tag 500 is connect with Auth tag 230
Bib ind 1 is different from Auth ind 1
Bib ind 2 is different from Auth ind 2

I think that the idea is good but need to manage those cases with specific routines for MARC21 and Unimarc. For NORMARC we need to ask to Magnus Enger, we can start with a copy of MARC21 routine

Hi Zeno,

It seems to be true for all UNIMARC 7XX, and I assumed (hoped?) it was true for all authority links...
Thanks.

Good observation from Zeno. When you merge within the same authority type, we can just rely on the indicators of the original biblio field.
But we do have a problem when we merge from type say PERSO_NAME to type CORPO_NAME. This is exceptional, but possible. The first indicator does no longer have the same meaning.
In that case we should have a fallback if only we had default indicators in the framework. Adding a dependency on bug 9701 therefore.

No need to keep this in FQA. We should wait for progress on 9701 first. Setting back to Assigned.

From Zeno on the dev ml:
===
in fact we partialy trust the indicators from the authority record

 tag 110 'A' + tag 111 'B' = tag 100 'CC'
 an tag 100 'CC'  is connect with biblios 100s, 700s, 600s, etc.

 I hope that before to definitily save I can fix indicators in auth.
 I think is mandatory to have 'change-by-hand' in merge.
 After I save; now modifications start on biblios records and tags.

 What we want ?
 Knowing that auth tag = 100, I know that I can copy 1° auth indicator 
 into 1° bib tag indicator.
 And for the second bib tag indicator ?
 Two option:
 -1) default of bib tag framewok [your solution]
 -2) mantain what is still present [I prefer this option].

 I think that the selection between the two option could by done by a 
 system preference.
===

(In reply to Zeno Tajoli from comment #6)

This makes me think about a solution with a table like below.
You check the table with the new biblio field number.
If there is no entry in the table, pick a default behavior (could be the current behavior: oldbiblio).

Table Where_should_i_get_the_indicators_when_changing_authtype  :)
column field
column marcflavour
column ind1
column ind2
ind1 and ind2 should have the options: auth|oldbiblio|bibframework

Here auth should mean: trust the new authority here, oldbiblio means pick from old biblio field and bibframework means use the default indicator for this biblio field. (As long as the new development is not ready, hardcode a simple hash somewhere.) 

Note that number of auth fields is limited. Could be extendend by users for custom purposes.

Would that address all possibilities?

(In reply to Marcel de Rooy from comment #7)
> This makes me think about a solution with a table like below.
...
> Would that address all possibilities?

I think that a table like this is a good tool, I will write defaults for this table for Unimarc an MARC21.
I think the problem are the classic problems to setup confing in Koha:
-- SQL table with web interface ? Or only command line?
-- YAML / XML FILE ? Web interface ? Or only command line ?

This would still be great to have..

Hello to all.  To not open a new bug I would like to submit here a patch that should fix this painful and far-too-long-still-not-resolved issue.  The proposed approach relies on authority records (which should be correct--that's exactly why they are called 'authority' records).  Especially MARC 21 case, as stated by Zeno, is more complex, specially for 6XX (2nd ind., $2 subfield etc.)

To test -- have some authority recs. with different tags (eg. 100, 130,
150), use them in biblio, especially in 6XX, 7XX (setting 2nd ind).
Have running indexer and proper value of AuthorityMergeLimit, i.e. not 0.
Then play with indicators in authorities. For MARC 21 auth used in 6XX
play also with 008/11 and 040 $f (if 008/11 set to 'z').  Have a look
at the results in biblio (see subfield $2 in MARC 21 6XX if 008/11 = z
and 040 $f defined).

(In reply to Janusz Kaczmarek from comment #10)
> Hello to all.  To not open a new bug I would like to submit here a patch
> that should fix this painful and far-too-long-still-not-resolved issue.  The
> proposed approach relies on authority records (which should be
> correct--that's exactly why they are called 'authority' records). 
> Especially MARC 21 case, as stated by Zeno, is more complex, specially for
> 6XX (2nd ind., $2 subfield etc.)
> 
> To test -- have some authority recs. with different tags (eg. 100, 130,
> 150), use them in biblio, especially in 6XX, 7XX (setting 2nd ind).
> Have running indexer and proper value of AuthorityMergeLimit, i.e. not 0.
> Then play with indicators in authorities. For MARC 21 auth used in 6XX
> play also with 008/11 and 040 $f (if 008/11 set to 'z').  Have a look
> at the results in biblio (see subfield $2 in MARC 21 6XX if 008/11 = z
> and 040 $f defined).

Hi Janusz
Thx for submitting this proposal. It really looks interesting. Will need more time to have a closer look, but I am glad that we take a next step.
Quick question: The patch is MARC21 oriented; what about the UNIMARC side ?

> Thx for submitting this proposal. It really looks interesting. Will need
> more time to have a closer look, but I am glad that we take a next step.
> Quick question: The patch is MARC21 oriented; what about the UNIMARC side ?

Hi Marcel,

Thank you for your comments.  To answer your question, UNIMARC is covered here in the same way as it is covered in the enhancement introduced with Bug 5262 four years ago (see the discussion there)--at that time I worked mainly in UNIMARC environment, so I hope it shouldn't be very wrong also now.  (BTW, see the initial message of Julian.)  UNIMARC is rather straightforward here.  It is MARC 21 that makes things more complicated.

But really, I should probably reconsider operations on $2 for UNIMARC subject headings fields in respect to 152 $b in UNIMARC authority record. 

In 2013, also, Magnus reported: "NORMARC does not support authorities at all at the moment" (still from Bug 5262 discussion).

I could rebase this one as-is, but I'd rather separate the $2 issue from filling the indicators in general.
I opened up report 20030 for the $2 issue and would like to submit them separately.
Janusz: Please let me know if you see any issues.

Well, it is kind of hard to separate. Maybe apart from this:

+sub _bib_indicator2 {
+    my ( $code ) = @_;
+    my %thes_mapping = ( a => 0, b => 1, c => 2, d => 3, k => 5, n => 4, r => 7, s => 7, v => 6, z => 7, '|' => 4 );
+    return $thes_mapping{ $code } // $code // '4';
+}
+
+sub _bib_subfield2 {
+    my ( $indicator2, $subject_headings, $conventions ) = @_;
+    return if $indicator2 ne '7';
+    return 'aat' if $subject_headings eq 'r';
+    return 'sears' if $subject_headings eq 's';
+    return $conventions if $subject_headings eq 'z';
+}

I noted that some fields still contain more exceptions. Like:
653, 654 ind2, 655 ind1, 656 and 657 only 7 in ind2, 658, 662, 69X

I still feel more for an approach I originally had in mind by adding a conf file containing essentially a hash that tells you what to do like:
600, ind1 => authind1 | authind2 | bibind1 | bibind2 | [fixed_value] | thesaurus
where thesaurus could trigger a call to bib_indicator2.
The uniform title stuff would move to that config file.
It would give people more flexibility to make changes.

Still work in progress. I will still merge the code of Janusz into BiblioIndicators.pm and partially into the config file that it uses.

Created attachment 71339 [details] [review]
Bug 14769: Introduce Koha::Authority::ControlledIndicators

This module reads its settings from pref AuthorityControlledIndicators.
The get method allows you to retrieve indicator values for specific
biblio fields.

Test plan:
Run t/Koha/Authority/ControlledIndicators.t

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

Created attachment 71340 [details] [review]
Bug 14769: Biblio indicators based on authority's thesaurus code

Original patch from Janusz Kaczmarek on November 24, 2017.
Amended by Marcel de Rooy on February 6, 2018.
Code moved from AuthoritiesMarc.pm to ControlledIndicators.pm.

Special attention has been paid to the proper application of 008/11
while controlling 6XX in MARC 21, specially if 008/11 =~ /[rsz]/
(and if it is 'z' and 040 $f is defined).

Test plan:
See next patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
The construction $thes_mapping{ $code } // $code // '4' will still get
some attention on a follow-up.

Created attachment 71341 [details] [review]
Bug 14769: Add tests for _thesaurus_info in ControlledIndicators.t

Apart from adding tests for this sub, we make the following change
too and support it with a test.

The fallback to $code in the construction
    $thes_mapping{ $code } // $code // '4'
is removed by this patch. When $code is not in $thes_mapping, we cannot
assume that it is a valid code for a biblio indicator. In this case it is
more safe to mark it as 4 (Source not specified).

Test plan:
Run t/Koha/Authority/ControlledIndicators.t

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

Created attachment 71342 [details] [review]
Bug 14769: Add preference AuthorityControlledIndicators

Including:

[1] Atomic update to add pref for existing installs
[2] Add new pref to sysprefs.sql for new installs
[3] Add description to authorities preference file.

Test plan:
[1] Run new install or upgrade to add pref.
[2] Check value of pref via System preferences in staff interface.

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

Created attachment 71343 [details] [review]
Bug 14769: Put ControlledIndicators to work

This patch does:
[1] Adds Koha::Authority->controlled_indicators (with a test).
[2] Adds a call to controlled_indicators in AuthoritiesMarc::merge.
    Unit test Merge.t is extended too.
[3] Simplifies the code in authorities/blinddetail-biblio-search.pl by
    calling controlled_indicators.

Test plan:
[1] Run t/db_dependent/Koha/Authorities.t
[2] Run t/db_dependent/Authority/Merge.t
[3] Steps 3 to 7 for MARC21:
    Create a PERSO_NAME authority with 008/11=r and ind1=3
[4] Edit a biblio and add a 600 linked to the new authority.
[5] Verify that the biblio has ind1==3 and ind2==7 and $2==aat.
    (If $2 is not visible, check the metadata in biblio_metadata.)
[6] Edit the PERSO_NAME authority and change 008/11 to '|' (bar).
[7] Verify that merge updated your biblio record: $ind2==4 and $2 gone.
[8] UNIMARC: Follow the pattern from steps 3 to 7.
    Create authority, link it in a biblio, check indicators (they should
    be copied both). Edit authority, change indicators and verify the
    merge results in the biblio record.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested PERSO_NAME and UNIF_TITLE.
For UNIF_TITLE the second authority indicator is copied to ind1 or ind2,
depending on the biblio tag involved.

Ready for signoff please.

Further refinements for the default preference value for the UNIMARC side should go to another report. (This should do it in most cases.)

Another observation for a new report:
# 240 is not connected to UNIF_TITLE in installer script
# 751 is not connected to GEOGR_NAME in installer script

Hoping for feedback..

Created attachment 71351 [details] [review]
Bug 14769: Introduce Koha::Authority::ControlledIndicators

This module reads its settings from pref AuthorityControlledIndicators.
The get method allows you to retrieve indicator values for specific
biblio fields.

Test plan:
Run t/Koha/Authority/ControlledIndicators.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 71352 [details] [review]
Bug 14769: Biblio indicators based on authority's thesaurus code

Original patch from Janusz Kaczmarek on November 24, 2017.
Amended by Marcel de Rooy on February 6, 2018.
Code moved from AuthoritiesMarc.pm to ControlledIndicators.pm.

Special attention has been paid to the proper application of 008/11
while controlling 6XX in MARC 21, specially if 008/11 =~ /[rsz]/
(and if it is 'z' and 040 $f is defined).

Test plan:
See next patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
The construction $thes_mapping{ $code } // $code // '4' will still get
some attention on a follow-up.
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 71353 [details] [review]
Bug 14769: Add tests for _thesaurus_info in ControlledIndicators.t

Apart from adding tests for this sub, we make the following change
too and support it with a test.

The fallback to $code in the construction
    $thes_mapping{ $code } // $code // '4'
is removed by this patch. When $code is not in $thes_mapping, we cannot
assume that it is a valid code for a biblio indicator. In this case it is
more safe to mark it as 4 (Source not specified).

Test plan:
Run t/Koha/Authority/ControlledIndicators.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 71354 [details] [review]
Bug 14769: Add preference AuthorityControlledIndicators

Including:

[1] Atomic update to add pref for existing installs
[2] Add new pref to sysprefs.sql for new installs
[3] Add description to authorities preference file.

Test plan:
[1] Run new install or upgrade to add pref.
[2] Check value of pref via System preferences in staff interface.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 71355 [details] [review]
Bug 14769: Put ControlledIndicators to work

This patch does:
[1] Adds Koha::Authority->controlled_indicators (with a test).
[2] Adds a call to controlled_indicators in AuthoritiesMarc::merge.
    Unit test Merge.t is extended too.
[3] Simplifies the code in authorities/blinddetail-biblio-search.pl by
    calling controlled_indicators.

Test plan:
[1] Run t/db_dependent/Koha/Authorities.t
[2] Run t/db_dependent/Authority/Merge.t
[3] Steps 3 to 7 for MARC21:
    Create a PERSO_NAME authority with 008/11=r and ind1=3
[4] Edit a biblio and add a 600 linked to the new authority.
[5] Verify that the biblio has ind1==3 and ind2==7 and $2==aat.
    (If $2 is not visible, check the metadata in biblio_metadata.)
[6] Edit the PERSO_NAME authority and change 008/11 to '|' (bar).
[7] Verify that merge updated your biblio record: $ind2==4 and $2 gone.
[8] UNIMARC: Follow the pattern from steps 3 to 7.
    Create authority, link it in a biblio, check indicators (they should
    be copied both). Edit authority, change indicators and verify the
    merge results in the biblio record.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested PERSO_NAME and UNIF_TITLE.
For UNIF_TITLE the second authority indicator is copied to ind1 or ind2,
depending on the biblio tag involved.
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Patches work as expected.
I signed off the patches but I think another sign-off from a MARC21 expert would be great.

A few comments on the code:
- I think that the subroutine controlled_indicators doesn't belong in Koha::Authority. The FIXME added in C4::AuthoritiesMarc strengthens that opinion
- In Koha::Authority::ControlledIndicators, there is a package variable $cached_indicators. Maybe this module should use Koha::Cache instead ?

Also, I'm not sure if this is a problem, but it seems that there is no way to tell Koha to change only one of the two indicators.

(In reply to Julian Maurice from comment #33)

Thanks Julian!

> A few comments on the code:
> - I think that the subroutine controlled_indicators doesn't belong in
> Koha::Authority. The FIXME added in C4::AuthoritiesMarc strengthens that
> opinion

I removed the FIXME and optimized it a bit in the last follow-up.
I thought about where to put this code when developing, and it is somewhat harder. This translated in putting the essential code in a separate object. (Parsing the pref and applying its rules on a MARC record should imo not be in Koha::Authority.) But since the indicators really are determined by the specific values of a given authority record, I decided to add the main controlled_indicators method to Koha::Authority.

> - In Koha::Authority::ControlledIndicators, there is a package variable
> $cached_indicators. Maybe this module should use Koha::Cache instead ?

I do not really see the advantage. We do not win time imo by saving the structure to the L2 cache. We could argue about putting it in L1 only (and check if we would gain time), but that seems not possible now. Since the internal structure is kept only during the lifetime of the object, the name could be misleading. If required, I could rename it.

> Also, I'm not sure if this is a problem, but it seems that there is no way
> to tell Koha to change only one of the two indicators.

I am not sure if I understand this correctly. The current approach with the pref allows for great flexibility.
If you say e.g. MARC21,100,ind1:auth1 you only copy the first indicator. The second indicator will be empty when creating a new field or will be taken from the biblio record when updating/merging.
But if you say e.g. MARC21,830,,ind2:auth2 you only copy the second indicator. One of the unit tests contains an example where indicators are swapped.
So for UNIMARC you can also make all changes you want by adding specific rules. In comment27 I proposed to open a new report if we need further refinements for UNIMARC specifically.

Hope this helps.

(In reply to Marcel de Rooy from comment #35)
> > - In Koha::Authority::ControlledIndicators, there is a package variable
> > $cached_indicators. Maybe this module should use Koha::Cache instead ?
> 
> I do not really see the advantage. We do not win time imo by saving the
> structure to the L2 cache. We could argue about putting it in L1 only (and
> check if we would gain time), but that seems not possible now. Since the
> internal structure is kept only during the lifetime of the object, the name
> could be misleading. If required, I could rename it.
I'm just saying that maybe we should use Koha::Cache for consistency (I can totally be wrong :))
You're saying that the structure is kept only during the lifetime of the object, but with Plack, isn't the object's lifetime infinite ?

> 
> > Also, I'm not sure if this is a problem, but it seems that there is no way
> > to tell Koha to change only one of the two indicators.
> 
> I am not sure if I understand this correctly. The current approach with the
> pref allows for great flexibility.
> If you say e.g. MARC21,100,ind1:auth1 you only copy the first indicator. The
> second indicator will be empty when creating a new field or will be taken
> from the biblio record when updating/merging.
> But if you say e.g. MARC21,830,,ind2:auth2 you only copy the second
> indicator. One of the unit tests contains an example where indicators are
> swapped.
With "MARC21,100,ind1:auth1" for instance, if I manually put something in the 2nd indicator, when I choose an authority it erases the content of the 2nd indicator.

(In reply to Julian Maurice from comment #36)
> You're saying that the structure is kept only during the lifetime of the
> object, but with Plack, isn't the object's lifetime infinite ?

No. The code is cached for some time, but we create a new Controller object for each new Authority object. And in new we clear the saved structure. (We could also save it in $self alternatively.)

(In reply to Marcel de Rooy from comment #37)
> (In reply to Julian Maurice from comment #36)
> > You're saying that the structure is kept only during the lifetime of the
> > object, but with Plack, isn't the object's lifetime infinite ?
> 
> No. The code is cached for some time, but we create a new Controller object
> for each new Authority object. And in new we clear the saved structure. (We
> could also save it in $self alternatively.)

Ah sorry, I missed that.
I don't know what is best, there is nothing about this in coding guidelines, so I guess it's okay to keep it like that.

(In reply to Julian Maurice from comment #36)
> With "MARC21,100,ind1:auth1" for instance, if I manually put something in
> the 2nd indicator, when I choose an authority it erases the content of the
> 2nd indicator.

OK. When you first add an indicator and then run the authority plugin.
Will add a fix..

Created attachment 71375 [details] [review]
Bug 14769: Introduce Koha::Authority::ControlledIndicators

This module reads its settings from pref AuthorityControlledIndicators.
The get method allows you to retrieve indicator values for specific
biblio fields.

Test plan:
Run t/Koha/Authority/ControlledIndicators.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Added a test to show that "ind1:" returns empty string.

Created attachment 71376 [details] [review]
Bug 14769: Biblio indicators based on authority's thesaurus code

Original patch from Janusz Kaczmarek on November 24, 2017.
Amended by Marcel de Rooy on February 6, 2018.
Code moved from AuthoritiesMarc.pm to ControlledIndicators.pm.

Special attention has been paid to the proper application of 008/11
while controlling 6XX in MARC 21, specially if 008/11 =~ /[rsz]/
(and if it is 'z' and 040 $f is defined).

Test plan:
See next patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
The construction $thes_mapping{ $code } // $code // '4' will still get
some attention on a follow-up.
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 71377 [details] [review]
Bug 14769: Add tests for _thesaurus_info in ControlledIndicators.t

Apart from adding tests for this sub, we make the following change
too and support it with a test.

The fallback to $code in the construction
    $thes_mapping{ $code } // $code // '4'
is removed by this patch. When $code is not in $thes_mapping, we cannot
assume that it is a valid code for a biblio indicator. In this case it is
more safe to mark it as 4 (Source not specified).

Test plan:
Run t/Koha/Authority/ControlledIndicators.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 71378 [details] [review]
Bug 14769: Add preference AuthorityControlledIndicators

Including:

[1] Atomic update to add pref for existing installs
[2] Add new pref to sysprefs.sql for new installs
[3] Add description to authorities preference file.

Test plan:
[1] Run new install or upgrade to add pref.
[2] Check value of pref via System preferences in staff interface.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 71379 [details] [review]
Bug 14769: Put ControlledIndicators to work

This patch does:
[1] Adds Koha::Authority->controlled_indicators (with a test).
[2] Adds a call to controlled_indicators in AuthoritiesMarc::merge.
    Unit test Merge.t is extended too.
[3] Simplifies the code in authorities/blinddetail-biblio-search.pl by
    calling controlled_indicators.

Test plan:
[1] Run t/db_dependent/Koha/Authorities.t
[2] Run t/db_dependent/Authority/Merge.t
[3] Steps 3 to 7 for MARC21:
    Create a PERSO_NAME authority with 008/11=r and ind1=3
[4] Edit a biblio and add a 600 linked to the new authority.
[5] Verify that the biblio has ind1==3 and ind2==7 and $2==aat.
    (If $2 is not visible, check the metadata in biblio_metadata.)
[6] Edit the PERSO_NAME authority and change 008/11 to '|' (bar).
[7] Verify that merge updated your biblio record: $ind2==4 and $2 gone.
[8] UNIMARC: Follow the pattern from steps 3 to 7.
    Create authority, link it in a biblio, check indicators (they should
    be copied both). Edit authority, change indicators and verify the
    merge results in the biblio record.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested PERSO_NAME and UNIF_TITLE.
For UNIF_TITLE the second authority indicator is copied to ind1 or ind2,
depending on the biblio tag involved.
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 71380 [details] [review]
Bug 14769: (Follow-up) Reduce number of preference parses

In sub merge the Koha::Authority object was already created, so it was kind
of silly to recreate it each time with new.
In Koha::Authority, the ControlledIndicators object can be saved in the
object too for reuse during its life time. Similarly, we prevent repeated
lookups for the reporting tag (via authority type).

Test plan:
[1] Run t/db_dependent/Authority/Merge.t
[2] Run t/db_dependent/Koha/Authorities.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Limited benchmarking shows me a reduction of 11% on Merge.t and a
reduction of 4% on Authorities.t when comparing with and without this
last patch. The larger impact on Merge.t could be expected.

Created attachment 71381 [details] [review]
Bug 14769: (Follow-up) Let blinddetail look at indicator values

If the indicators are not controlled, blinddetail should not overwrite
what the user already entered. Very strictly seen, we could say that it
is outside the scope. But it is strongly related.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested with 100 and 600 in the cataloging editor.
For 100 the second indicator should not be overwritten. For 600 it should.
Also tested "ind2:" which should blank ind2.

Created attachment 72398 [details] [review]
Bug 14769: Introduce Koha::Authority::ControlledIndicators

This module reads its settings from pref AuthorityControlledIndicators.
The get method allows you to retrieve indicator values for specific
biblio fields.

Test plan:
Run t/Koha/Authority/ControlledIndicators.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Added a test to show that "ind1:" returns empty string.

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

Created attachment 72399 [details] [review]
Bug 14769: Biblio indicators based on authority's thesaurus code

Original patch from Janusz Kaczmarek on November 24, 2017.
Amended by Marcel de Rooy on February 6, 2018.
Code moved from AuthoritiesMarc.pm to ControlledIndicators.pm.

Special attention has been paid to the proper application of 008/11
while controlling 6XX in MARC 21, specially if 008/11 =~ /[rsz]/
(and if it is 'z' and 040 $f is defined).

Test plan:
See next patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
The construction $thes_mapping{ $code } // $code // '4' will still get
some attention on a follow-up.
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

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

Created attachment 72400 [details] [review]
Bug 14769: Add tests for _thesaurus_info in ControlledIndicators.t

Apart from adding tests for this sub, we make the following change
too and support it with a test.

The fallback to $code in the construction
    $thes_mapping{ $code } // $code // '4'
is removed by this patch. When $code is not in $thes_mapping, we cannot
assume that it is a valid code for a biblio indicator. In this case it is
more safe to mark it as 4 (Source not specified).

Test plan:
Run t/Koha/Authority/ControlledIndicators.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

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

Created attachment 72401 [details] [review]
Bug 14769: Add preference AuthorityControlledIndicators

Including:

[1] Atomic update to add pref for existing installs
[2] Add new pref to sysprefs.sql for new installs
[3] Add description to authorities preference file.

Test plan:
[1] Run new install or upgrade to add pref.
[2] Check value of pref via System preferences in staff interface.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

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

Created attachment 72402 [details] [review]
Bug 14769: Put ControlledIndicators to work

This patch does:
[1] Adds Koha::Authority->controlled_indicators (with a test).
[2] Adds a call to controlled_indicators in AuthoritiesMarc::merge.
    Unit test Merge.t is extended too.
[3] Simplifies the code in authorities/blinddetail-biblio-search.pl by
    calling controlled_indicators.

Test plan:
[1] Run t/db_dependent/Koha/Authorities.t
[2] Run t/db_dependent/Authority/Merge.t
[3] Steps 3 to 7 for MARC21:
    Create a PERSO_NAME authority with 008/11=r and ind1=3
[4] Edit a biblio and add a 600 linked to the new authority.
[5] Verify that the biblio has ind1==3 and ind2==7 and $2==aat.
    (If $2 is not visible, check the metadata in biblio_metadata.)
[6] Edit the PERSO_NAME authority and change 008/11 to '|' (bar).
[7] Verify that merge updated your biblio record: $ind2==4 and $2 gone.
[8] UNIMARC: Follow the pattern from steps 3 to 7.
    Create authority, link it in a biblio, check indicators (they should
    be copied both). Edit authority, change indicators and verify the
    merge results in the biblio record.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested PERSO_NAME and UNIF_TITLE.
For UNIF_TITLE the second authority indicator is copied to ind1 or ind2,
depending on the biblio tag involved.
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

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

Created attachment 72403 [details] [review]
Bug 14769: (Follow-up) Reduce number of preference parses

In sub merge the Koha::Authority object was already created, so it was kind
of silly to recreate it each time with new.
In Koha::Authority, the ControlledIndicators object can be saved in the
object too for reuse during its life time. Similarly, we prevent repeated
lookups for the reporting tag (via authority type).

Test plan:
[1] Run t/db_dependent/Authority/Merge.t
[2] Run t/db_dependent/Koha/Authorities.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Limited benchmarking shows me a reduction of 11% on Merge.t and a
reduction of 4% on Authorities.t when comparing with and without this
last patch. The larger impact on Merge.t could be expected.

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

Created attachment 72404 [details] [review]
Bug 14769: (Follow-up) Let blinddetail look at indicator values

If the indicators are not controlled, blinddetail should not overwrite
what the user already entered. Very strictly seen, we could say that it
is outside the scope. But it is strongly related.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested with 100 and 600 in the cataloging editor.
For 100 the second indicator should not be overwritten. For 600 it should.
Also tested "ind2:" which should blank ind2.

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

(In reply to Josef Moravec from comment #54)
> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Great. Thx Josef

Created attachment 73389 [details] [review]
Bug 14769: (Follow-up) Let blinddetail look at indicator values

If the indicators are not controlled, blinddetail should not overwrite
what the user already entered. Very strictly seen, we could say that it
is outside the scope. But it is strongly related.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested with 100 and 600 in the cataloging editor.
For 100 the second indicator should not be overwritten. For 600 it should.
Also tested "ind2:" which should blank ind2.

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

Rebased (last patch only)

Created attachment 73773 [details] [review]
Bug 14769: Introduce Koha::Authority::ControlledIndicators

This module reads its settings from pref AuthorityControlledIndicators.
The get method allows you to retrieve indicator values for specific
biblio fields.

Test plan:
Run t/Koha/Authority/ControlledIndicators.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Added a test to show that "ind1:" returns empty string.

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 73774 [details] [review]
Bug 14769: Biblio indicators based on authority's thesaurus code

Original patch from Janusz Kaczmarek on November 24, 2017.
Amended by Marcel de Rooy on February 6, 2018.
Code moved from AuthoritiesMarc.pm to ControlledIndicators.pm.

Special attention has been paid to the proper application of 008/11
while controlling 6XX in MARC 21, specially if 008/11 =~ /[rsz]/
(and if it is 'z' and 040 $f is defined).

Test plan:
See next patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
The construction $thes_mapping{ $code } // $code // '4' will still get
some attention on a follow-up.
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 73775 [details] [review]
Bug 14769: Add tests for _thesaurus_info in ControlledIndicators.t

Apart from adding tests for this sub, we make the following change
too and support it with a test.

The fallback to $code in the construction
    $thes_mapping{ $code } // $code // '4'
is removed by this patch. When $code is not in $thes_mapping, we cannot
assume that it is a valid code for a biblio indicator. In this case it is
more safe to mark it as 4 (Source not specified).

Test plan:
Run t/Koha/Authority/ControlledIndicators.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 73776 [details] [review]
Bug 14769: Add preference AuthorityControlledIndicators

Including:

[1] Atomic update to add pref for existing installs
[2] Add new pref to sysprefs.sql for new installs
[3] Add description to authorities preference file.

Test plan:
[1] Run new install or upgrade to add pref.
[2] Check value of pref via System preferences in staff interface.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 73777 [details] [review]
Bug 14769: Put ControlledIndicators to work

This patch does:
[1] Adds Koha::Authority->controlled_indicators (with a test).
[2] Adds a call to controlled_indicators in AuthoritiesMarc::merge.
    Unit test Merge.t is extended too.
[3] Simplifies the code in authorities/blinddetail-biblio-search.pl by
    calling controlled_indicators.

Test plan:
[1] Run t/db_dependent/Koha/Authorities.t
[2] Run t/db_dependent/Authority/Merge.t
[3] Steps 3 to 7 for MARC21:
    Create a PERSO_NAME authority with 008/11=r and ind1=3
[4] Edit a biblio and add a 600 linked to the new authority.
[5] Verify that the biblio has ind1==3 and ind2==7 and $2==aat.
    (If $2 is not visible, check the metadata in biblio_metadata.)
[6] Edit the PERSO_NAME authority and change 008/11 to '|' (bar).
[7] Verify that merge updated your biblio record: $ind2==4 and $2 gone.
[8] UNIMARC: Follow the pattern from steps 3 to 7.
    Create authority, link it in a biblio, check indicators (they should
    be copied both). Edit authority, change indicators and verify the
    merge results in the biblio record.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested PERSO_NAME and UNIF_TITLE.
For UNIF_TITLE the second authority indicator is copied to ind1 or ind2,
depending on the biblio tag involved.
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 73778 [details] [review]
Bug 14769: (Follow-up) Reduce number of preference parses

In sub merge the Koha::Authority object was already created, so it was kind
of silly to recreate it each time with new.
In Koha::Authority, the ControlledIndicators object can be saved in the
object too for reuse during its life time. Similarly, we prevent repeated
lookups for the reporting tag (via authority type).

Test plan:
[1] Run t/db_dependent/Authority/Merge.t
[2] Run t/db_dependent/Koha/Authorities.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Limited benchmarking shows me a reduction of 11% on Merge.t and a
reduction of 4% on Authorities.t when comparing with and without this
last patch. The larger impact on Merge.t could be expected.

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 73779 [details] [review]
Bug 14769: (Follow-up) Let blinddetail look at indicator values

If the indicators are not controlled, blinddetail should not overwrite
what the user already entered. Very strictly seen, we could say that it
is outside the scope. But it is strongly related.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested with 100 and 600 in the cataloging editor.
For 100 the second indicator should not be overwritten. For 600 it should.
Also tested "ind2:" which should blank ind2.

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Everything seesm to work as expected - I would consider putting a big warning on AuthorityControlledIndicators to specify you shouldn't touch it if you don't know what you are doing

Blocked by bug 9701

(In reply to Nick Clemens from comment #65)
> Everything seesm to work as expected - I would consider putting a big
> warning on AuthorityControlledIndicators to specify you shouldn't touch it
> if you don't know what you are doing

Thx for QAing, Nick.
If you do not know what you are doing, you should better stay out of preferences all together ?

(In reply to Jonathan Druart from comment #66)
> Blocked by bug 9701

In the history of this report we had a dependency, but the current solution is not dependent. Note that 9701 still needs an authority counterpart which 14769 is not. I remember posting a remark about that on Bugzilla before, but could not find it here.

(In reply to Marcel de Rooy from comment #67)
> (In reply to Nick Clemens from comment #65)
> > Everything seesm to work as expected - I would consider putting a big
> > warning on AuthorityControlledIndicators to specify you shouldn't touch it
> > if you don't know what you are doing
> 
> Thx for QAing, Nick.
> If you do not know what you are doing, you should better stay out of
> preferences all together ?

I think making sure consequences are clear from the description is the important bit.

(In reply to Katrin Fischer from comment #69)
> I think making sure consequences are clear from the description is the
> important bit.

Pref description, you mean, or at upgrade time, release ? Any suggestions?

Pref description if the behaviour remains unchanged after update. Also "Text to go in the release notes" should be used, especially if there is a change in behaviour.

I do not have time to test but I am pretty sure the way you cache $cached_indicators in Koha::Authority::ControlledIndicators is buggy.
You should use Koha::Cache::Memory::Lite instead.

(In reply to Jonathan Druart from comment #72)
> I do not have time to test but I am pretty sure the way you cache
> $cached_indicators in Koha::Authority::ControlledIndicators is buggy.
> You should use Koha::Cache::Memory::Lite instead.

Hmm. Must say that I have been thinking about using Koha::Cache for it, but it would not be a real benefit in time. Problem with Koha::Cache::Memory::Lite is the same as handling it in the module; the L1 cache and also the cached_indicators are kept per Plack worker for some time. So would still be buggy.

But note that this is strictly hypothetical. The pref is not expected to change on a regular basis. I decided not to add a clear statement at pref change event but to add it in the new method. This effectively makes it a minimal issue.

(In reply to Marcel de Rooy from comment #73)
> Hmm. Must say that I have been thinking about using Koha::Cache for it, but
> it would not be a real benefit in time. Problem with
> Koha::Cache::Memory::Lite is the same as handling it in the module; the L1
> cache and also the cached_indicators are kept per Plack worker for some
> time. So would still be buggy.

Nope, there is the following statement in psgi file:
 47         Koha::Caches->flush_L1_caches();
 48         Koha::Cache::Memory::Lite->flush();

(In reply to Jonathan Druart from comment #74)
> (In reply to Marcel de Rooy from comment #73)
> > Hmm. Must say that I have been thinking about using Koha::Cache for it, but
> > it would not be a real benefit in time. Problem with
> > Koha::Cache::Memory::Lite is the same as handling it in the module; the L1
> > cache and also the cached_indicators are kept per Plack worker for some
> > time. So would still be buggy.
> 
> Nope, there is the following statement in psgi file:
>  47         Koha::Caches->flush_L1_caches();
>  48         Koha::Cache::Memory::Lite->flush();

In view of my comment73 I personally do not see the need for moving to Lite here immediately. I am happy to do it on a new report. What do you think? Is it a requirement for 18.05 ?

I cannot imagine pushing something with caching issues, even if the issue will be hard to show up.

(In reply to Jonathan Druart from comment #76)
> I cannot imagine pushing something with caching issues, even if the issue
> will be hard to show up.

Yeah, no bugs in Koha?

Created attachment 74052 [details] [review]
Bug 14769: (QA follow-up) Remove global var $cached_indicators

As requested by RM, this patch replaces using the global $cached_indicators
by saving state temporarily during the (limited) lifetime of the object.

Essentially this affects two places in code:
[1] blinddetail-biblio-search.pl (loading auth record in editor)
[2] AuthoritiesMarc::merge (merging authority into biblios)

Concurrent runs of [1] and/or [2] together with a simultaneous pref change
just in between could cause slight (hypothetical) side-effects.
The current approach of keeping state in the object makes that a series of
controlled_indicators calls during an immediate merge of one specific
authority is not affected by a simultaneous pref change. So the same rules
are applied to the set of attached biblio record for that authority.
Note also that the cron job ignores a simultaneous pref change, since
it reads from the unchanged L1 cache (yes, also hypothetical).

Test plan:
[1] Run t/Koha/Authority/ControlledIndicators.t
[2] Run t/db_dependent/Authority/Merge.t
[3] Run t/db_dependent/Koha/Authorities.t

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

Pushed to master for 18.05, thanks to everybody involved!

Enhancement, not backported for 17.11.x