Bug 10648 - In records merge greatest field can not be added
Summary: In records merge greatest field can not be added
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Fridolin Somers
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 10650
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-26 12:55 UTC by Fridolin Somers
Modified: 2015-06-04 23:33 UTC (History)
8 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
Bug 10648 - In records merge greatest field can not be added (3.47 KB, patch)
2013-07-26 15:16 UTC, Fridolin Somers
Details | Diff | Splinter Review
[3.12.x] Bug 10648 - In records merge greatest field can not be added (3.32 KB, patch)
2013-07-26 15:23 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 10648 - In records merge greatest field can not be added (3.47 KB, patch)
2013-09-18 11:52 UTC, Fridolin Somers
Details | Diff | Splinter Review
[3.12.x] Bug 10648 - In records merge greatest field can not be added (3.32 KB, patch)
2013-09-18 11:52 UTC, Fridolin Somers
Details | Diff | Splinter Review
[PATCH][SIGNED OFF] Bug 10648 - In records merge greatest field can not be added (3.54 KB, patch)
2013-09-18 12:56 UTC, Mathieu Saby
Details | Diff | Splinter Review
[PASSED QA][3.12.x] Bug 10648 - In records merge greatest field can not be added (3.56 KB, patch)
2013-10-06 21:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10648 - In records merge greatest field can not be added (3.62 KB, patch)
2013-11-04 12:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10648 - In records merge greatest field can not be added (2.97 KB, patch)
2013-12-31 11:10 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 10648 - In records merge greatest field can not be added (3.03 KB, patch)
2014-07-09 13:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 10648 - In records merge greatest field can not be added (3.10 KB, patch)
2014-07-18 13:49 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description 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.