Bug 5572 - Improve C4::AuthoritiesMarc->merge() + syspref AuthCleanBiblios
Summary: Improve C4::AuthoritiesMarc->merge() + syspref AuthCleanBiblios
Status: CLOSED DUPLICATE of bug 17913
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: master
Hardware: All All
: P1 - high enhancement (vote)
Assignee: Frédéric Demians
QA Contact:
URL:
Keywords:
Depends on: 6094
Blocks: 17908
  Show dependency treegraph
 
Reported: 2011-01-03 15:22 UTC by Janusz Kaczmarek
Modified: 2017-01-23 08:07 UTC (History)
10 users (show)

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


Attachments
Proposed patch (1.66 KB, patch)
2011-01-03 15:22 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
A safer varsion of previous patch (1.68 KB, patch)
2011-07-24 16:19 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
A revised version of the previous patch (1.73 KB, patch)
2012-01-20 20:50 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Proposed patch - refreshed (1.62 KB, patch)
2013-05-24 13:57 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 5572 Improve C4::AuthoritiesMarc->merge() function (26.75 KB, patch)
2015-11-07 15:37 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 5572 Improve C4::AuthoritiesMarc->merge() function (27.25 KB, patch)
2015-11-09 05:48 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 5572 Improve C4::AuthoritiesMarc->merge() + syspref AuthCleanBiblios (27.22 KB, patch)
2015-12-09 07:01 UTC, Frédéric Demians
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5572: Improve C4::AuthoritiesMarc->merge() + syspref AuthCleanBiblios (27.39 KB, patch)
2015-12-18 13:48 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 5572: (follow-up) atomic update (2.43 KB, patch)
2015-12-18 13:49 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 5572: (follow-up) fix koha-qa errors (3.53 KB, patch)
2015-12-18 13:49 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Janusz Kaczmarek 2011-01-03 15:22:54 UTC Comment hidden (obsolete)
Comment 1 Marcel de Rooy 2011-01-13 13:20:02 UTC
Tested your patch: it works as described.
I see your point when you should clear a subfield at the authority side and you want to see the same at the biblio side. [How often would that happen?]
However, if the subfield was already empty at the authority side and you update the auth record, do you also want to clear at that moment any possible info in that subfield at the biblio side?
Does an empty authority subfield that is "not in the ignore tab" in the framework, really mean that that subfield should always be empty too at the biblio side? I am not sure about that; I will copy this comment to the Koha list for possible further discussion.
Comment 2 Janusz Kaczmarek 2011-01-13 16:19:10 UTC
(In reply to comment #1)
> Tested your patch: it works as described.
> I see your point when you should clear a subfield at the authority side and you
> want to see the same at the biblio side. [How often would that happen?]

Well, me myself was not convinced that it happens and that such a refinement is  is necessary--it was the library staff that told me it really happens sometimes and demanded to make Koha to behave like described in the proposal.  It does not happen often perhaps, but if it happens then the staff would have to edit even hundreds of biblio records by hand without this refinement.

> However, if the subfield was already empty at the authority side and you update
> the auth record, do you also want to clear at that moment any possible info in
> that subfield at the biblio side?

Yes, because I was told this exactly means that the set of subfields of a field in biblio is *controlled* by the authority record.  In other words, if a subfield can have a value in authority (in Koha: it is not in 'ignored' state), then you are not supposed to edit it by hand in biblio.  I do not know if it is really so--I was told that.

> Does an empty authority subfield that is "not in the ignore tab" in the
> framework, really mean that that subfield should always be empty too at the
> biblio side? I am not sure about that; I will copy this comment to the Koha
> list for possible further discussion.

At least this is what I was told.  This means of course that with my refinement you have to carefully check which subfields are active in the authority framework, in order not to delete what you do not intend to.

I will appreciate any comments.  For now this is what was needed in my library.  I thought it worth to share this with others.
Comment 3 Janusz Kaczmarek 2011-01-25 19:20:51 UTC
I am still thinking about my previous proposal.  Had some doubts, but I still think it should be OK.  Even in case of PERSO_NAME (in MARC 21) for instance.  In fact this authority comes in two flavors: author and author/uniform title.  So, if you will have empty (but active) 100$t in the author-flavored version of the authority record for instance, there is nothing bad if the procedures attempts to delete $t in some 700, since it should be empty...  I really try, but still cannot find cons for the proposal ;)
Comment 4 Stéphane Delaune 2011-04-06 13:13:37 UTC
This patch conflicts with patch bug 5943. It just changes the sub merge C4::AuthoritiesMarc::merge which is only used by function C4::AuthoritiesMarc::ModAuthority if (C4::Context->preference('MergeAuthoritiesOnUpdate') ). This syspref does not exist by default. So it makes me doubt the relevance of this patch
Comment 5 Janusz Kaczmarek 2011-04-06 13:29:22 UTC
I am wondering--and how then about the merge_authority.pl script (it makes use
of C4::AuthoritiesMarc::merge), which in turn should be executed (as cron job
for example, if I understand well) if MergeAuthoritiesOnUpdate is off (which is
the default)...?
Comment 6 Marcel de Rooy 2011-04-06 13:30:49 UTC
Commit 928905570305d8ace24e4deba03a17bbe95774c2 appears to introduce this problem:
@@ -646,7 +649,9 @@ sub ModAuthority {
 ### If a library thinks that updating all biblios is a long process and wishes
 ### they should have a system preference "dontmerge=1" otherwise by default bib
 ### the $merge flag is now depreceated and will be removed at code cleaning
-  if (C4::Context->preference('dontmerge') ){
+  if (C4::Context->preference('MergeAuthoritiesOnUpdate') ){
Comment 7 Marcel de Rooy 2011-04-06 13:33:10 UTC
I guess the new pref should be added and set based on the older dontmerge option. Should be FIXED!
Comment 8 Marcel de Rooy 2011-04-06 14:50:57 UTC
(In reply to comment #7)
> I guess the new pref should be added and set based on the older dontmerge
> option. Should be FIXED!
Opening bug 6094 for this one.
Comment 9 Marcel de Rooy 2011-04-07 08:32:04 UTC
januszop or stephane: I submitted a patch for 6094. Could you have a look please? Thanks.
Comment 10 Janusz Kaczmarek 2011-04-07 15:25:48 UTC
Marcel, because of my other commitments I wouldn't be able to jump into Koha coding thing before Monday, though I looked through your changes -- it looks fine for me.  The only concern I have -- do you think it is a good idea to put authid memo files into general /tmp subdirectory which can be purged beyond your control?
Comment 11 Marcel de Rooy 2011-04-08 17:44:54 UTC
(In reply to comment #10)
> Marcel, because of my other commitments I wouldn't be able to jump into Koha
> coding thing before Monday, though I looked through your changes -- it looks
> fine for me.  The only concern I have -- do you think it is a good idea to put
> authid memo files into general /tmp subdirectory which can be purged beyond
> your control?
Janusz, I copy this comment to 6094 and reply there.
Comment 12 Janusz Kaczmarek 2011-07-24 16:19:47 UTC Comment hidden (obsolete)
Comment 13 Stefano Bargioni 2011-09-28 15:16:52 UTC
If I'm not wrong, no words were spent about indicators. They must be propagated in most cases, but we need to observe MARC specifications.
For instance, in MARC21 the # of nonfiling characters for tag 130 auth is stored in ind2, while it is stored in 130 bib ind1. And 730 auth doesn't have the nonfiling characters, that is present in 730 bib ind1.
Comment 14 Janusz Kaczmarek 2011-09-28 15:54:12 UTC
(In reply to comment #13)
> If I'm not wrong, no words were spent about indicators. They must be propagated
> in most cases, but we need to observe MARC specifications.
> For instance, in MARC21 the # of nonfiling characters for tag 130 auth is
> stored in ind2, while it is stored in 130 bib ind1. And 730 auth doesn't have
> the nonfiling characters, that is present in 730 bib ind1.

You are perfectly right.  But this is beyond the scope of this patch.  

But, to make things clear, when updating the controlled field in biblio records the merge function preserves the old indicators from biblio record.  At this stage, it does not copy nor makes any use of the indicators stored in the authority record.  This had to be still implemented.

BTW, you could also have a look at Bug 5262.
Comment 15 Stefano Bargioni 2011-10-13 11:04:30 UTC
I tested the latest patch (attachment 4730 [details] [review]) in Koha 3.2.3 and 3.2.7 (production environment). It works.
Two little questions: I changed
$marcrecord->insert_grouped_field($field_to);
with 
$marcrecord->insert_fields_ordered($field_to);
This will avoid that a modified 700, for instance, will be placed at its correct place and not after every 7xx field, like 740.
The second one is the performance: if the change involves hundreds of biblios, it is very slow and a web server timeout can occurr.
I hope to test the patch in Koha 3.4.4 ASAP.
Comment 16 Paul Poulain 2011-10-24 11:38:10 UTC
Updating Version : This ENH will be for Koha 3.8
Comment 17 Paul Poulain 2011-10-25 15:05:43 UTC
Bug versionned for master. entries will be made against rel_3_8 once the patch has been applied (see thread about that on koha-devel yesterday)
Comment 18 Marcel de Rooy 2011-11-30 14:43:14 UTC
Applying: Changes to sub merge in C4::AuthoritiesMarc to refine authorities merging -- safer version
error: patch failed: C4/AuthoritiesMarc.pm:1315
error: C4/AuthoritiesMarc.pm: patch does not apply
fatal: sha1 information is lacking or useless (C4/AuthoritiesMarc.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Comment 19 Janusz Kaczmarek 2012-01-20 20:50:22 UTC Comment hidden (obsolete)
Comment 20 Jared Camins-Esakov 2012-03-10 16:27:42 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > Tested your patch: it works as described.
> > I see your point when you should clear a subfield at the authority side and you
> > want to see the same at the biblio side. [How often would that happen?]
> 
> Well, me myself was not convinced that it happens and that such a refinement
> is  is necessary--it was the library staff that told me it really happens
> sometimes and demanded to make Koha to behave like described in the
> proposal.  It does not happen often perhaps, but if it happens then the
> staff would have to edit even hundreds of biblio records by hand without
> this refinement.
> 
> > However, if the subfield was already empty at the authority side and you update
> > the auth record, do you also want to clear at that moment any possible info in
> > that subfield at the biblio side?
> 
> Yes, because I was told this exactly means that the set of subfields of a
> field in biblio is *controlled* by the authority record.  In other words, if
> a subfield can have a value in authority (in Koha: it is not in 'ignored'
> state), then you are not supposed to edit it by hand in biblio.  I do not
> know if it is really so--I was told that.

I do not think this is correct. Subfield 'e' is not authority-controlled, but Koha treats all non-numeric subfields in authority-controlled fields as if they are. Or am I misunderstanding what this bug does, and there is a workaround for this?

> 
> > Does an empty authority subfield that is "not in the ignore tab" in the
> > framework, really mean that that subfield should always be empty too at the
> > biblio side? I am not sure about that; I will copy this comment to the Koha
> > list for possible further discussion.
> 
> At least this is what I was told.  This means of course that with my
> refinement you have to carefully check which subfields are active in the
> authority framework, in order not to delete what you do not intend to.
> 
> I will appreciate any comments.  For now this is what was needed in my
> library.  I thought it worth to share this with others.
Comment 21 Marcel de Rooy 2012-05-03 07:44:06 UTC
Patch still applies. But I still have my doubts on clearing subfields on the biblio side for an empty authority subfield. This may theoretically be true, but could result in data loss when used without enough care.
I will retry to get more feedback from the list.
Comment 22 Janusz Kaczmarek 2013-05-24 13:49:35 UTC
(In reply to comment #20)
> I do not think this is correct. Subfield 'e' is not authority-controlled,
> but Koha treats all non-numeric subfields in authority-controlled fields as
> if they are. Or am I misunderstanding what this bug does, and there is a
> workaround for this?

OK, the action of the modified merge procedure is based on "Select to display or not" (i.e. auth_subfield_structure.hidden) and of course one has to be cautious about it.  If a subfield of the heading is hidden in authority editor == it not supposed to be controlled == will never be overwriten.  This would be the case of auth. 100 applied on biblio. 700 with $e present.  $e will stay untouched provided it is not active in the authorities framework.

Why I now think it is really important: a very common case for my libraries (participating in a cooperative cataloguing program) are personal headings with duplicated family name + surname.  So, if the librarian who creates the entry does no know the dates, he/she adds a qualifier like $c.  But then another librarian who found the dates (maybe after some months or even years) exchanges $c for $d in the authority data.  Now, without this enhancement you will have $c and $d in bibliographic records, which is what you don't want. 

Another reason is the case when wrongly entered data was propagated to several dozens of biblio recs.
Comment 23 Janusz Kaczmarek 2013-05-24 13:57:12 UTC Comment hidden (obsolete)
Comment 24 Martin Renvoize 2014-05-08 13:57:38 UTC
Can anyone comment as to whether this bug is still relevant in the upcoming 3.16 release?
Comment 25 Janusz Kaczmarek 2014-06-07 21:11:10 UTC
Yes, it is.  In ver. 3.16.00, sub merge still does not remove from header fields in biblio those controlled subfields that has been removed from associated authority record.
Comment 26 Jacek Ablewicz 2014-09-02 14:28:08 UTC
Nice patch, quite usefull for our library particular needs as well..

Would it be possible to make this feature controllable by e.g. some system preference (= disabled by default)? I have a feeling that chances for reaching any agreement whenever such change would be "universally" good, or not good, are close to nil; there are way too many different habits, approaches etc. in various libraries regarding authority editing nad maintaining. But, I believe that as long as this patch:

- does not by default introduce any changes to the existing behaviour,
- libraries willing to use it are made aware (e.g. by some kind of remark in syspref description) that they should carefully review their existing authority frameworks before enabling this feature, to prevent any possible "data loss" in authority-controlled biblio fields,

all concerns indicated in previous comments will be sufficiently (?) addressed, so it could hopefully get a promotion from "In Discussion" to "Needs Signoff" ;).
Comment 27 Frédéric Demians 2015-11-06 18:23:26 UTC
Janusz, do you still use your patch for your library? Have you seen bug 14583? IMO your patch is required in master.
Comment 28 Frédéric Demians 2015-11-07 15:37:34 UTC Comment hidden (obsolete)
Comment 29 Frédéric Demians 2015-11-09 05:48:31 UTC Comment hidden (obsolete)
Comment 30 Frédéric Demians 2015-12-09 07:01:36 UTC Comment hidden (obsolete)
Comment 31 Bernardo Gonzalez Kriegel 2015-12-18 13:48:48 UTC
Created attachment 45821 [details] [review]
[SIGNED-OFF] Bug 5572: Improve C4::AuthoritiesMarc->merge() + syspref AuthCleanBiblios

A new syspref AuthCleanBiblios controls how modified info in a modified
authority are propagated to the attached biblio records. By default, the
merge() behavior isn't changed. When AuthCleanBiblios is set to yes, the
subfields of the authority heading are copied to biblio field. If the
biblio field contains subfields that are not anymore in the authority,
they are removed (this is true only for the authority subfields that are
neither ignored nor hidden in the authority editor)

This patch does some code refactoring:

  - New function C4::AuthoritiesMarc::GetBibliosByAuthid
  - New function C4::Biblios::GetBiblionumber($record)
  - Minimizing SQL queries, and caching
  - Code cleaning + doc

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Tested with dontmerge on/off, AuthCleanBiblioson/off
Work as described
Few errors fixed in folloups
Comment 32 Bernardo Gonzalez Kriegel 2015-12-18 13:49:23 UTC
Created attachment 45822 [details] [review]
Bug 5572: (follow-up) atomic update
Comment 33 Bernardo Gonzalez Kriegel 2015-12-18 13:49:43 UTC
Created attachment 45823 [details] [review]
Bug 5572: (follow-up) fix koha-qa errors
Comment 34 Frédéric Demians 2015-12-22 09:03:37 UTC
Bugs 15380 and 15381 should be pushed to master before refactoring this one to use new authorities related classes.
Comment 35 Jonathan Druart 2015-12-22 09:18:17 UTC
(In reply to Frédéric Demians from comment #34)
> Bugs 15380 and 15381 should be pushed to master before refactoring this one
> to use new authorities related classes.

This can be pushed before the refactoring, I will rebase bug 15381 if needed.
Comment 36 Frédéric Demians 2015-12-22 10:14:41 UTC
(In reply to Jonathan Druart from comment #35)
> (In reply to Frédéric Demians from comment #34)
> > Bugs 15380 and 15381 should be pushed to master before refactoring this one
> > to use new authorities related classes.
> 
> This can be pushed before the refactoring, I will rebase bug 15381 if needed.

OK thanks. So I reset this patch to signed-off. And let see which patch will be pushed first.
Comment 37 Jonathan Druart 2016-01-12 12:32:58 UTC
This patch set does not apply anymore.
We will need tests here, the merge functionality is completely unstable at the moment and it seems dangerous to continue without good basis.
Comment 38 Frédéric Demians 2016-01-12 14:25:29 UTC
(In reply to Jonathan Druart from comment #37)
> This patch set does not apply anymore.
> We will need tests here, the merge functionality is completely unstable at
> the moment and it seems dangerous to continue without good basis.

Bug 15358 need to be fixed before doing anything here.
Comment 39 Jacek Ablewicz 2016-04-18 10:12:04 UTC
Note that - apart from the new feature, various performance enhancements, code clenups etc., - attachment in comment #30 introduces one pretty important bugfix:

    - $marcrecord->delete_field($field);
    - $marcrecord->insert_grouped_field($field_to);

    + $field->replace_with($field_to);

Thanks to that, merge() does not mess up field order in biblio records anymore. 

This ("merge() messing up field order in biblio records") is a long standing issue (since commit efc64f44241efcc8a02ee0a8549a5346cdd026b0, Sat Aug 9 10:53:14 2008 -0500, "Bug fix : use delete_field/insert_grouped_field rather than replace_with") witch may cause severe data loss in some setups. Quite a nasty one, not always easy to spot (unless everybody & their dog is using MARC preview very often in a given setup). I was under the impression that it got already fixed somewhere around 3.20, but nope, that particular insert_grouped_field() call is still there in the current master.

I guess technically it's a critical bug (if not a blocker).
Comment 40 Jonathan Druart 2016-04-18 11:01:05 UTC
So it should be moved on its own bug report, that's what you mean?
Comment 41 Jacek Ablewicz 2016-04-18 11:40:41 UTC
(In reply to Jonathan Druart from comment #40)
> So it should be moved on its own bug report, that's what you mean?

Not really, it's more like (yet another) good reason for upping priority to P1 for this one ;). It would be really nice to have this bug included in the next release IMO - preferably as a whole or at least as much of it as possible; merge() has a lot of issues in current master - some of them are (partially) dealt with in the other reports, but this implementation is most comprehensive and most beneficial so far. Splitting it to separate bugs may help a bit, but it's not necessary in may opinion. Different fixes, enhancements and improvements it provides, if done in separate patches, would vastly improve it's readability though (in particular, code indentation related changes are making the other changes harder to evaluate).
Comment 42 Marcel de Rooy 2017-01-23 08:06:40 UTC
Can be closed now: see omnibus bug 17908 and especially bug 17913

*** This bug has been marked as a duplicate of bug 17913 ***