Bug 14769

Summary: Authorities merge: Set correct indicators in biblio field
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: MARC Authority data supportAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Nick Clemens <nick>
Severity: enhancement    
Priority: P5 - low CC: abl, januszop, jonathan.druart, katrin.fischer, m.de.rooy, marjorie.barry-vila, nick, ztajoli
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9701
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This report adds pref AuthorityControlledIndicators. It controls how the indicators of linked authority records affect the corresponding biblio indicators. Currently, the default pref value is finetuned for MARC21, and copies the authority indicators for UNIMARC. An example to illustrate: A MARC21 field 100 in a biblio record should pick its first indicator from the linked authority record. The second indicator is not controlled by the authority. This report supports such MARC conventions.
Version(s) released in:
Bug Depends on:    
Bug Blocks: 17908, 18904, 30154    
Attachments: Bug 14769: Authorities merge: Set correct indicators in biblio field
14769: Authorities merge: Set correct indicators in biblio field
Bug 14769: Introduce Koha::Authority::BiblioIndicators
Bug 14769: Add unit tests for Koha::Authority::BiblioIndicators
Bug 14769: Add Koha::Authority::controlled_biblio_indicators
Bug 14769: Call controlled_biblio_indicators in authority merge
Bug 14769: Put ControlledIndicators to work
Bug 14769: Introduce Koha::Authority::ControlledIndicators
Bug 14769: Biblio indicators based on authority's thesaurus code
Bug 14769: Add tests for _thesaurus_info in ControlledIndicators.t
Bug 14769: Add preference AuthorityControlledIndicators
Bug 14769: Put ControlledIndicators to work
Bug 14769: Introduce Koha::Authority::ControlledIndicators
Bug 14769: Biblio indicators based on authority's thesaurus code
Bug 14769: Add tests for _thesaurus_info in ControlledIndicators.t
Bug 14769: Add preference AuthorityControlledIndicators
Bug 14769: Put ControlledIndicators to work
Bug 14769: (Follow-up) Reduce number of preference parses
Bug 14769: (Follow-up) Let blinddetail look at indicator values
Bug 14769: Introduce Koha::Authority::ControlledIndicators
Bug 14769: Biblio indicators based on authority's thesaurus code
Bug 14769: Add tests for _thesaurus_info in ControlledIndicators.t
Bug 14769: Add preference AuthorityControlledIndicators
Bug 14769: Put ControlledIndicators to work
Bug 14769: (Follow-up) Reduce number of preference parses
Bug 14769: (Follow-up) Let blinddetail look at indicator values
Bug 14769: Introduce Koha::Authority::ControlledIndicators
Bug 14769: Biblio indicators based on authority's thesaurus code
Bug 14769: Add tests for _thesaurus_info in ControlledIndicators.t
Bug 14769: Add preference AuthorityControlledIndicators
Bug 14769: Put ControlledIndicators to work
Bug 14769: (Follow-up) Reduce number of preference parses
Bug 14769: (Follow-up) Let blinddetail look at indicator values
Bug 14769: (Follow-up) Let blinddetail look at indicator values
Bug 14769: Introduce Koha::Authority::ControlledIndicators
Bug 14769: Biblio indicators based on authority's thesaurus code
Bug 14769: Add tests for _thesaurus_info in ControlledIndicators.t
Bug 14769: Add preference AuthorityControlledIndicators
Bug 14769: Put ControlledIndicators to work
Bug 14769: (Follow-up) Reduce number of preference parses
Bug 14769: (Follow-up) Let blinddetail look at indicator values
Bug 14769: (QA follow-up) Remove global var $cached_indicators

Description Julian Maurice 2015-09-02 11:46:13 UTC
Biblio field indicators should be the same indicators as the linked authority's main field
Comment 1 Julian Maurice 2015-09-02 11:46:45 UTC Comment hidden (obsolete)
Comment 2 Zeno Tajoli 2015-09-02 13:07:54 UTC
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
Comment 3 Julian Maurice 2015-09-02 13:42:30 UTC
Hi Zeno,

It seems to be true for all UNIMARC 7XX, and I assumed (hoped?) it was true for all authority links...
Thanks.
Comment 4 Marcel de Rooy 2017-01-17 08:03:13 UTC
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.
Comment 5 Marcel de Rooy 2017-01-17 08:04:33 UTC
No need to keep this in FQA. We should wait for progress on 9701 first. Setting back to Assigned.
Comment 6 Marcel de Rooy 2017-01-18 12:39:18 UTC
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.
===
Comment 7 Marcel de Rooy 2017-01-18 12:46:12 UTC
(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?
Comment 8 Zeno Tajoli 2017-01-21 08:55:29 UTC
(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 ?
Comment 9 Marcel de Rooy 2017-11-07 13:55:33 UTC
This would still be great to have..
Comment 10 Janusz Kaczmarek 2017-11-24 22:28:31 UTC
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).
Comment 11 Janusz Kaczmarek 2017-11-24 22:30:51 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2017-11-27 09:40:16 UTC
(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 ?
Comment 13 Janusz Kaczmarek 2017-11-27 16:35:21 UTC
> 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).
Comment 14 Marcel de Rooy 2018-01-18 14:21:48 UTC
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.
Comment 15 Marcel de Rooy 2018-01-19 11:25:15 UTC
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.
Comment 16 Marcel de Rooy 2018-01-26 11:17:26 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2018-01-26 11:17:30 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2018-01-26 11:17:34 UTC Comment hidden (obsolete)
Comment 19 Marcel de Rooy 2018-01-26 11:17:39 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2018-01-26 11:18:45 UTC
Still work in progress. I will still merge the code of Janusz into BiblioIndicators.pm and partially into the config file that it uses.
Comment 21 Marcel de Rooy 2018-02-08 13:43:24 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2018-02-08 13:43:44 UTC
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>
Comment 23 Marcel de Rooy 2018-02-08 13:43:48 UTC
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.
Comment 24 Marcel de Rooy 2018-02-08 13:43:53 UTC
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>
Comment 25 Marcel de Rooy 2018-02-08 13:43:57 UTC
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>
Comment 26 Marcel de Rooy 2018-02-08 13:44:02 UTC
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.
Comment 27 Marcel de Rooy 2018-02-08 13:49:49 UTC
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..
Comment 28 Julian Maurice 2018-02-08 15:24:47 UTC
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>
Comment 29 Julian Maurice 2018-02-08 15:24:51 UTC
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>
Comment 30 Julian Maurice 2018-02-08 15:24:55 UTC
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>
Comment 31 Julian Maurice 2018-02-08 15:24:59 UTC
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>
Comment 32 Julian Maurice 2018-02-08 15:25:03 UTC
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>
Comment 33 Julian Maurice 2018-02-08 15:26:22 UTC
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.
Comment 34 Marcel de Rooy 2018-02-09 09:15:34 UTC Comment hidden (obsolete)
Comment 35 Marcel de Rooy 2018-02-09 09:35:56 UTC
(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.
Comment 36 Julian Maurice 2018-02-09 10:38:16 UTC
(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.
Comment 37 Marcel de Rooy 2018-02-09 10:42:23 UTC
(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.)
Comment 38 Julian Maurice 2018-02-09 10:49:49 UTC
(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.
Comment 39 Marcel de Rooy 2018-02-09 10:51:06 UTC
(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..
Comment 40 Marcel de Rooy 2018-02-09 12:17:05 UTC Comment hidden (obsolete)
Comment 41 Marcel de Rooy 2018-02-09 12:23:55 UTC
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.
Comment 42 Marcel de Rooy 2018-02-09 12:24:00 UTC
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>
Comment 43 Marcel de Rooy 2018-02-09 12:24:04 UTC
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>
Comment 44 Marcel de Rooy 2018-02-09 12:24:09 UTC
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>
Comment 45 Marcel de Rooy 2018-02-09 12:24:13 UTC
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>
Comment 46 Marcel de Rooy 2018-02-09 12:24:18 UTC
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.
Comment 47 Marcel de Rooy 2018-02-09 12:24:23 UTC
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.
Comment 48 Josef Moravec 2018-03-05 07:00:58 UTC
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>
Comment 49 Josef Moravec 2018-03-05 07:01:03 UTC
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>
Comment 50 Josef Moravec 2018-03-05 07:01:07 UTC
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>
Comment 51 Josef Moravec 2018-03-05 07:01:12 UTC
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>
Comment 52 Josef Moravec 2018-03-05 07:01:16 UTC
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>
Comment 53 Josef Moravec 2018-03-05 07:01:21 UTC
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>
Comment 54 Josef Moravec 2018-03-05 07:01:26 UTC
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>
Comment 55 Marcel de Rooy 2018-03-05 07:28:02 UTC
(In reply to Josef Moravec from comment #54)
> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Great. Thx Josef
Comment 56 Marcel de Rooy 2018-03-28 13:43:42 UTC
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>
Comment 57 Marcel de Rooy 2018-03-28 13:44:07 UTC
Rebased (last patch only)
Comment 58 Nick Clemens 2018-04-06 13:05:46 UTC
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>
Comment 59 Nick Clemens 2018-04-06 13:05:51 UTC
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>
Comment 60 Nick Clemens 2018-04-06 13:05:54 UTC
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>
Comment 61 Nick Clemens 2018-04-06 13:05:58 UTC
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>
Comment 62 Nick Clemens 2018-04-06 13:06:02 UTC
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>
Comment 63 Nick Clemens 2018-04-06 13:06:06 UTC
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>
Comment 64 Nick Clemens 2018-04-06 13:06:10 UTC
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>
Comment 65 Nick Clemens 2018-04-06 13:08:08 UTC
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
Comment 66 Jonathan Druart 2018-04-06 17:48:26 UTC
Blocked by bug 9701
Comment 67 Marcel de Rooy 2018-04-09 08:07:46 UTC
(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 ?
Comment 68 Marcel de Rooy 2018-04-09 08:10:29 UTC
(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.
Comment 69 Katrin Fischer 2018-04-09 08:11:28 UTC
(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.
Comment 70 Marcel de Rooy 2018-04-09 08:12:58 UTC
(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?
Comment 71 Katrin Fischer 2018-04-09 08:14:28 UTC
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.
Comment 72 Jonathan Druart 2018-04-10 18:50:54 UTC
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.
Comment 73 Marcel de Rooy 2018-04-11 11:26:53 UTC
(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.
Comment 74 Jonathan Druart 2018-04-11 13:45:55 UTC
(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();
Comment 75 Marcel de Rooy 2018-04-11 14:20:06 UTC
(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 ?
Comment 76 Jonathan Druart 2018-04-11 15:03:40 UTC
I cannot imagine pushing something with caching issues, even if the issue will be hard to show up.
Comment 77 Marcel de Rooy 2018-04-11 19:37:08 UTC
(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?
Comment 78 Marcel de Rooy 2018-04-12 07:38:06 UTC
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>
Comment 79 Jonathan Druart 2018-04-12 15:05:47 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 80 Nick Clemens 2018-05-14 19:35:10 UTC
Enhancement, not backported for 17.11.x