Bug 10648

Summary: In records merge greatest field can not be added
Product: Koha Reporter: Fridolin Somers <fridolin.somers>
Component: CatalogingAssignee: Fridolin Somers <fridolin.somers>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: minor    
Priority: P5 - low CC: gitbot, gmcharlt, jonathan.druart, katrin.fischer, m.de.rooy, mathsabypro, sandboxes, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 10650    
Bug Blocks:    
Attachments: Bug 10648 - In records merge greatest field can not be added
[3.12.x] Bug 10648 - In records merge greatest field can not be added
Bug 10648 - In records merge greatest field can not be added
[3.12.x] Bug 10648 - In records merge greatest field can not be added
[PATCH][SIGNED OFF] Bug 10648 - In records merge greatest field can not be added
[PASSED QA][3.12.x] Bug 10648 - In records merge greatest field can not be added
Bug 10648 - In records merge greatest field can not be added
Bug 10648 - In records merge greatest field can not be added
Bug 10648 - In records merge greatest field can not be added
[PASSED QA] Bug 10648 - In records merge greatest field can not be added

Description Fridolin Somers 2013-07-26 12:55:44 UTC
When merging 2 records (/cgi-bin/koha/cataloguing/merge.pl), the destination record is build using the fields and subfields checked in source records.
When a field is checked, the javascript code searches in destination record a field with a greater tag number to insert new field before.
When the new field tag number is greater than all existing field tag numbers, the field is not added.
Comment 1 Fridolin Somers 2013-07-26 15:16:16 UTC Comment hidden (obsolete)
Comment 2 Fridolin Somers 2013-07-26 15:17:45 UTC
(In reply to Fridolyn SOMERS from comment #1)
> Created attachment 19963 [details] [review] [review]
> Bug 10648 - In records merge greatest field can not be added
This patch is based on master + Bug 10650 because this bug moves impacted code.
Comment 3 Fridolin Somers 2013-07-26 15:23:13 UTC Comment hidden (obsolete)
Comment 4 I'm just a bot 2013-09-18 10:02:42 UTC
Applying: Bug 10648 - In records merge greatest field can not be added
Using index info to reconstruct a base tree...
M	cataloguing/merge.pl
M	koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt
Auto-merging cataloguing/merge.pl
CONFLICT (content): Merge conflict in cataloguing/merge.pl
Patch failed at 0001 Bug 10648 - In records merge greatest field can not be added
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 5 Fridolin Somers 2013-09-18 11:52:22 UTC Comment hidden (obsolete)
Comment 6 Fridolin Somers 2013-09-18 11:52:59 UTC Comment hidden (obsolete)
Comment 7 Fridolin Somers 2013-09-18 11:53:45 UTC
I wonder if "I'm just a bot" has also tested patch for 3.12.x on master ?
Comment 8 Mathieu Saby 2013-09-18 12:50:18 UTC
Main patch applies on master ;-) I'm testing.
Mathieu
Comment 9 Mathieu Saby 2013-09-18 12:56:47 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2013-09-23 06:31:09 UTC
Both patches apply to their versions, but running out of time to continue here now.
Comment 11 Katrin Fischer 2013-10-06 21:57:22 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2013-10-06 21:57:59 UTC
Patch for master still needs QA, will do that later.
Comment 13 Marcel de Rooy 2013-11-04 12:29:29 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2013-11-04 12:31:20 UTC
QA Comment: 
Good catch. No string changes. Looks good to me. No complaints from qa tools.

Passed QA
Comment 15 Galen Charlton 2013-11-15 16:15:54 UTC
Comment on attachment 22697 [details] [review]
Bug 10648 - In records merge greatest field can not be added

Review of attachment 22697 [details] [review]:
-----------------------------------------------------------------

::: Koha/Util/MARC.pm
@@ +40,4 @@
>      my @array;
>      my @fields = $record->fields();
>  
> +    foreach my $field ( sort { $a->tag() <=> $b->tag() } @fields) {

This is actually a problem: in MARC21, at least, fields are not always meant to be sorted in numeric tag order.

For example, for the 6XX fields for subject headings, you could have a 651 (geographic term) followed by a 650 (topical term).  That would signify that the book was more about the geographic area expressed in the 651 than about the topic expressed in the 650.

As another example, 5XX are not always meant to be sorted in numeric order.

For the purpose of the merge interface, it's sufficient that new tags be inserted after the first field with a numerically lesser tag, or at the end of the record as a fallback -- but it shouldn't reorder existing fields in the target record.
Comment 16 Galen Charlton 2013-11-15 16:30:32 UTC
Because of the sorting issue mentioned in my previous comment, my inclination is to push the JavaScript part of the patch, but not the change to Koha/Util/MARC.pm. That way, the bug gets fixed, but there's no unexpected reordering of fields in the destination record.

Fridolin: comments on this approach?
Comment 17 Mathieu Saby 2013-11-15 16:39:46 UTC
(Note : idem in unimarc. see 210 http://multimedia.bnf.fr/unimarcb_trad/B210-6-2012.pdf : the first occurence of the 210 field have a special meaning)
Mathieu
Comment 18 Mathieu Saby 2013-11-15 16:40:48 UTC
(in fact it is not exactly the same case, but resorting all fields, even in Unimarc, may not be a good idea)
Comment 19 Fridolin Somers 2013-12-19 17:17:24 UTC
(In reply to Galen Charlton from comment #16)
> Because of the sorting issue mentioned in my previous comment, my
> inclination is to push the JavaScript part of the patch, but not the change
> to Koha/Util/MARC.pm. That way, the bug gets fixed, but there's no
> unexpected reordering of fields in the destination record.
> 
> Fridolin: comments on this approach?
I agree. It would be perfect.
Do you wan me to create a new patch ?
Comment 20 Galen Charlton 2013-12-20 04:37:09 UTC
(In reply to Fridolin SOMERS from comment #19)
> (In reply to Galen Charlton from comment #16)
> > Because of the sorting issue mentioned in my previous comment, my
> > inclination is to push the JavaScript part of the patch, but not the change
> > to Koha/Util/MARC.pm. That way, the bug gets fixed, but there's no
> > unexpected reordering of fields in the destination record.
> > 
> > Fridolin: comments on this approach?
> I agree. It would be perfect.
> Do you wan me to create a new patch ?

Yes, please.
Comment 21 Fridolin Somers 2013-12-31 11:10:47 UTC Comment hidden (obsolete)
Comment 22 Fridolin Somers 2013-12-31 11:15:27 UTC
Here is new patch.
Please re-signoff.
Comment 23 Biblibre Sandboxes 2014-05-30 20:43:50 UTC
Patch tested with a sandbox, by Nick Clemens <nick@quecheelibrary.org>
Comment 24 Marcel de Rooy 2014-07-09 12:55:15 UTC
Interesting: Status is Signed off, but all patches are obsolete.
Changing status to Failed QA.
Author or signer: Please correct.
Comment 25 Jonathan Druart 2014-07-09 13:06:07 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2014-07-09 13:07:17 UTC
(In reply to M. de Rooy from comment #24)
> Interesting: Status is Signed off, but all patches are obsolete.
> Changing status to Failed QA.
> Author or signer: Please correct.

The git-bz on sandboxes were not up-to-date, they were not able to sent patches.
I added the signoff line on the last patch.
Comment 27 Kyle M Hall 2014-07-18 13:49:59 UTC
Created attachment 29843 [details] [review]
[PASSED QA] Bug 10648 - In records merge greatest field can not be added

When merging 2 records (/cgi-bin/koha/cataloguing/merge.pl), the destination record is build using the fields and subfields checked in source records.
When a field is checked, the javascript code searches in destination record a field with a greater tag number to insert new field before.
When the new field tag number is greater than all existing field tag numbers, the field is not added.

This patch corrects this by adding at end if no greater field tag number exists. Also adds a sort of fields by tag number because all mergo code is based on this.

Test plan :
- Add to a framework a repeatable field with the greater non existing tag number. For example 998.
- Edit 2 records with this framework and add them a value in this tag.
- Put those records is a list
- Go to this list and check the two records
- Click on "Merge selected"
- Click on next
- Go to second source record
- Click on the greatest tag number. for example 998.
=> The field is added at the end of destination record

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Tomás Cohen Arazi 2014-08-05 23:28:24 UTC
Patch pushed to master.

Thanks Fridolin!
Comment 29 Galen Charlton 2014-08-26 22:48:14 UTC
Pushed to 3.16.x for inclusion in 3.16.3.