8064 – Merge several biblio records

Bug 8064 - Merge several biblio records

Summary: Merge several biblio records

Status:	CLOSED FIXED

Alias:	None

Product:	Koha
Classification:	Unclassified
Component:	Cataloging (show other bugs)
Version:	Main
Hardware:	All All

Importance:	P5 - low enhancement
Assignee:	Julian Maurice
QA Contact:

URL:
Keywords:

Duplicates (1):	4283 (view as bug list)
Depends on:	9755
Blocks:	8149 15358 15682
	Show dependency tree / graph

Reported:	2012-05-10 08:53 UTC by Julian Maurice
Modified:	2022-08-22 05:41 UTC (History)
CC List:	18 users (show)

See Also:	4283 10551 8411
Change sponsored?:	---
Patch complexity:	Large patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:

Attachments
Merge several biblios (41.27 KB, patch) 2012-05-10 08:55 UTC, Julian Maurice	Details \| Diff \| Splinter Review
Bug 8064: Merge several biblios (56.08 KB, patch) 2013-05-28 14:34 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 8064: Merge several biblios (61.62 KB, patch) 2013-06-11 14:01 UTC, Julian Maurice	Details \| Diff \| Splinter Review
[SIGNED OFF] Bug 8064: Merge several biblios (61.69 KB, patch) 2013-06-13 10:55 UTC, Mathieu Saby	Details \| Diff \| Splinter Review
Screenshot of merge form (70.90 KB, image/png) 2013-07-05 13:36 UTC, Julian Maurice	Details
Bug 8064: Merge several biblios (54.76 KB, patch) 2013-07-05 14:03 UTC, Julian Maurice	Details \| Diff \| Splinter Review
Bug 8064: Merge several biblios (44.58 KB, patch) 2013-09-23 09:59 UTC, Julian Maurice	Details \| Diff \| Splinter Review
Bug 8064: Change the way target record is built. (14.49 KB, patch) 2013-11-06 11:37 UTC, Julian Maurice	Details \| Diff \| Splinter Review
Bug 8064: Merge several biblios (44.41 KB, patch) 2013-12-20 09:11 UTC, Julian Maurice	Details \| Diff \| Splinter Review
Bug 8064: Change the way target record is built. (14.49 KB, patch) 2013-12-20 09:11 UTC, Julian Maurice	Details \| Diff \| Splinter Review
Bug 8064: Fix issues with control fields and non-repeatable check (3.20 KB, patch) 2013-12-20 10:09 UTC, Julian Maurice	Details \| Diff \| Splinter Review
Bug 8064: Merge several biblios (44.41 KB, patch) 2014-01-09 13:53 UTC, Julian Maurice	Details \| Diff \| Splinter Review
Bug 8064: Change the way target record is built. (14.49 KB, patch) 2014-01-09 13:53 UTC, Julian Maurice	Details \| Diff \| Splinter Review
Bug 8064: Fix issues with control fields and non-repeatable check (3.20 KB, patch) 2014-01-09 13:53 UTC, Julian Maurice	Details \| Diff \| Splinter Review
Bug 8064: Merge several biblios (44.45 KB, patch) 2014-03-11 09:33 UTC, Stefano Bargioni	Details \| Diff \| Splinter Review
Bug 8064: Change the way target record is built. (14.50 KB, patch) 2014-03-11 09:35 UTC, Stefano Bargioni	Details \| Diff \| Splinter Review
Bug 8064: Fix issues with control fields and non-repeatable check (3.21 KB, patch) 2014-03-11 09:35 UTC, Stefano Bargioni	Details \| Diff \| Splinter Review
8064-Little fix for 003 005 008 in MARC21 (1.38 KB, patch) 2014-03-11 09:37 UTC, Stefano Bargioni	Details \| Diff \| Splinter Review
Bug 8064: Fix unit tests for createMergeHash (5.09 KB, patch) 2014-06-09 07:56 UTC, Julian Maurice	Details \| Diff \| Splinter Review
Bug 8064: Merge several biblios (44.49 KB, patch) 2014-06-13 09:33 UTC, Julian Maurice	Details \| Diff \| Splinter Review
Bug 8064: Change the way target record is built. (14.49 KB, patch) 2014-06-13 09:34 UTC, Julian Maurice	Details \| Diff \| Splinter Review
Bug 8064: Fix issues with control fields and non-repeatable check (3.20 KB, patch) 2014-06-13 09:34 UTC, Julian Maurice	Details \| Diff \| Splinter Review
Bug 8064: Little fix for 003, 005, 008 in MARC21 (1.38 KB, patch) 2014-06-13 09:34 UTC, Julian Maurice	Details \| Diff \| Splinter Review
Bug 8064: Fix unit tests for createMergeHash (5.09 KB, patch) 2014-06-13 09:34 UTC, Julian Maurice	Details \| Diff \| Splinter Review
[SIGNED-OFF] Bug 8064: Merge several biblios (44.77 KB, patch) 2014-06-27 14:03 UTC, Bernardo Gonzalez Kriegel	Details \| Diff \| Splinter Review
[SIGNED-OFF] Bug 8064: Change the way target record is built. (14.57 KB, patch) 2014-06-27 14:04 UTC, Bernardo Gonzalez Kriegel	Details \| Diff \| Splinter Review
[SIGNED-OFF] Bug 8064: Fix issues with control fields and non-repeatable check (3.28 KB, patch) 2014-06-27 14:04 UTC, Bernardo Gonzalez Kriegel	Details \| Diff \| Splinter Review
[SIGNED-OFF] Bug 8064: Little fix for 003, 005, 008 in MARC21 (1.44 KB, patch) 2014-06-27 14:04 UTC, Bernardo Gonzalez Kriegel	Details \| Diff \| Splinter Review
[SIGNED-OFF] Bug 8064: Fix unit tests for createMergeHash (5.17 KB, patch) 2014-06-27 14:04 UTC, Bernardo Gonzalez Kriegel	Details \| Diff \| Splinter Review
[SIGNED-OFF] Bug 8064: Merge several biblios (44.21 KB, patch) 2014-07-21 11:33 UTC, Julian Maurice	Details \| Diff \| Splinter Review
[SIGNED-OFF] Bug 8064: Change the way target record is built. (14.41 KB, patch) 2014-07-21 11:34 UTC, Julian Maurice	Details \| Diff \| Splinter Review
[SIGNED-OFF] Bug 8064: Fix issues with control fields and non-repeatable check (3.20 KB, patch) 2014-07-21 11:34 UTC, Julian Maurice	Details \| Diff \| Splinter Review
[SIGNED-OFF] Bug 8064: Little fix for 003, 005, 008 in MARC21 (1.38 KB, patch) 2014-07-21 11:35 UTC, Julian Maurice	Details \| Diff \| Splinter Review
[SIGNED-OFF] Bug 8064: Fix unit tests for createMergeHash (5.09 KB, patch) 2014-07-21 11:35 UTC, Julian Maurice	Details \| Diff \| Splinter Review
[SIGNED-OFF] Bug 8064: Merge several biblios (44.29 KB, patch) 2015-02-09 08:47 UTC, Julian Maurice	Details \| Diff \| Splinter Review
[SIGNED-OFF] Bug 8064: Change the way target record is built. (15.17 KB, patch) 2015-02-09 08:47 UTC, Julian Maurice	Details \| Diff \| Splinter Review
[SIGNED-OFF] Bug 8064: Fix issues with control fields and non-repeatable check (3.26 KB, patch) 2015-02-09 08:47 UTC, Julian Maurice	Details \| Diff \| Splinter Review
[SIGNED-OFF] Bug 8064: Little fix for 003, 005, 008 in MARC21 (1.43 KB, patch) 2015-02-09 08:48 UTC, Julian Maurice	Details \| Diff \| Splinter Review
[SIGNED-OFF] Bug 8064: Fix unit tests for createMergeHash (5.15 KB, patch) 2015-02-09 08:48 UTC, Julian Maurice	Details \| Diff \| Splinter Review
[PASSED QA] Bug 8064: Merge several biblios (44.37 KB, patch) 2015-02-13 14:19 UTC, Kyle M Hall (khall)	Details \| Diff \| Splinter Review
[PASSED QA] Bug 8064: Change the way target record is built. (15.25 KB, patch) 2015-02-13 14:20 UTC, Kyle M Hall (khall)	Details \| Diff \| Splinter Review
[PASSED QA] Bug 8064: Fix issues with control fields and non-repeatable check (3.33 KB, patch) 2015-02-13 14:20 UTC, Kyle M Hall (khall)	Details \| Diff \| Splinter Review
[PASSED QA] Bug 8064: Little fix for 003, 005, 008 in MARC21 (1.50 KB, patch) 2015-02-13 14:20 UTC, Kyle M Hall (khall)	Details \| Diff \| Splinter Review
[PASSED QA] Bug 8064: Fix unit tests for createMergeHash (5.22 KB, patch) 2015-02-13 14:20 UTC, Kyle M Hall (khall)	Details \| Diff \| Splinter Review
[PASSED QA] Bug 8064 [QA Followup] - Remove trailing whitespace (1.12 KB, patch) 2015-02-13 14:20 UTC, Kyle M Hall (khall)	Details \| Diff \| Splinter Review
[PASSED QA] Bug 8064: Merge several biblios (44.18 KB, patch) 2015-11-09 10:48 UTC, Julian Maurice	Details \| Diff \| Splinter Review
[PASSED QA] Bug 8064: Change the way target record is built. (15.23 KB, patch) 2015-11-09 10:48 UTC, Julian Maurice	Details \| Diff \| Splinter Review
[PASSED QA] Bug 8064: Fix issues with control fields and non-repeatable check (3.32 KB, patch) 2015-11-09 10:49 UTC, Julian Maurice	Details \| Diff \| Splinter Review
[PASSED QA] Bug 8064: Little fix for 003, 005, 008 in MARC21 (1.49 KB, patch) 2015-11-09 10:49 UTC, Julian Maurice	Details \| Diff \| Splinter Review
[PASSED QA] Bug 8064: Fix unit tests for createMergeHash (5.21 KB, patch) 2015-11-09 10:49 UTC, Julian Maurice	Details \| Diff \| Splinter Review
[PASSED QA] Bug 8064 [QA Followup] - Remove trailing whitespace (1.12 KB, patch) 2015-11-09 10:50 UTC, Julian Maurice	Details \| Diff \| Splinter Review
Show Obsolete (44) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Julian Maurice 2012-05-10 08:53:17 UTC

Add the possibility to merge more than 2 biblio record.
Patch coming.

Comment 1 Julian Maurice 2012-05-10 08:55:00 UTC Comment hidden (obsolete)

Created attachment 9498 [details] [review]
Merge several biblios

This patch improve the existing merging tool by adding possibility to
merge more than 2 biblios.
There is no functional changes:
  - Add some biblios to a list
  - In the list check some biblios and click on 'Merge selected records'
  - Choose the biblio which will be kept, all others will be deleted
  - On the next page you have all biblios you chose in tabs (left side
    of the screen) and the preview of result (right side)
  - Pick some fields or subfields from records that will be deleted or
    delete some fields from reference record.
  - Click on 'Merge', if there is no errors you are redirected to the
    biblio view.

Comment 2 Julian Maurice 2012-05-10 09:45:58 UTC

Do not sign off this patch right now, i'm still working on it

Comment 3 Jonathan Druart 2013-05-28 14:34:00 UTC Comment hidden (obsolete)

Created attachment 18433 [details] [review]
Bug 8064: Merge several biblios

This patch improves the existing merging tool by adding possibility to
merge more than 2 biblios.
There is no functional changes:
  - Add some biblios to a list
  - In the list check some biblios and click on 'Merge selected records'
  - Choose the biblio which will be kept, all others will be deleted
  - On the next page you have all biblios you chose in tabs (left side
    of the screen) and the preview of result (right side)
  - Pick some fields or subfields from records that will be deleted or
    delete some fields from reference record.
  - Click on 'Merge', if there is no errors you are redirected to the
    biblio view.

Added checks for non-repeatable subfields
Added checks for mandatory fields and subfields before submitting the
form.
Added a final report which display deleted records (see syspref
MergeReportFields)

Comment 4 Jared Camins-Esakov 2013-05-28 14:49:00 UTC

Does this take into account the (already signed-off) refactoring I did on bug 9755?

Comment 5 Mathieu Saby 2013-05-28 15:08:50 UTC

Sponsored by Rennes 2

M. Saby

Comment 6 Jonathan Druart 2013-05-28 15:11:37 UTC

Absolutely not...
I think Julian did not know this report exist...

Comment 7 Mathieu Saby 2013-05-28 15:15:07 UTC

Jared, I tested your patch in march, so I hopped it would be in 3.12. But it was not QAed, and I forgot it...

Do you think it will cause major conflict with this bug?

M. Saby

Comment 8 Mathieu Saby 2013-05-28 19:34:39 UTC

I am working on a rebase of Biblibre on top of 9755.

Mathieu

Comment 9 Mathieu Saby 2013-05-30 07:21:27 UTC

I mark it in discussion, as 9755 will be soon QAed and pushed...

M. Saby

Comment 10 Julian Maurice 2013-06-11 14:01:57 UTC Comment hidden (obsolete)

Created attachment 18840 [details] [review]
Bug 8064: Merge several biblios

This patch improves the existing merging tool by adding possibility to
merge more than 2 biblios.
There is no functional changes:
  - Add some biblios to a list
  - In the list check some biblios and click on 'Merge selected records'
  - Choose the biblio which will be kept, all others will be deleted
  - On the next page you have all biblios you chose in tabs (left side
    of the screen) and the preview of result (right side)
  - Pick some fields or subfields from records that will be deleted or
    delete some fields from reference record.
  - Click on 'Merge', if there is no errors you are redirected to the
    biblio view.

Added checks for non-repeatable subfields
Added checks for mandatory fields and subfields before submitting the
form.
Added a final report which display deleted records (see syspref
MergeReportFields)

This patch is built on top of Bug 9755

Comment 11 Julian Maurice 2013-06-11 14:02:47 UTC

Please apply all patches from Bug 9755 before testing this one.

Comment 12 Mathieu Saby 2013-06-13 10:52:47 UTC

I applied 9755, then 8064.
I launched "perl updatedatabase.pl" for creating the new syspref.
I put 20 records in a list and merge them.
Everything works fine : 
- all items are moved,
- holds and checkouts are not lost,
- I could move some fields of merged records in destination record
- I could remove some fields of reference record from destination record
- the report table is nice
Ex : with Syspref MergeReportFields "200af,210a,600" the result is : 
Biblionumber 	200$a 	200$f 	210$a 	606$2 	606$3 	606$9 	606$a 	606$x 	606$y 	606$z

I have only one comment, regarding the syspref and its description : 
"example: '001,245ab,600' displays:
    value of 001
    subfields a and b of fields 245
    all subfields of fields 600"

Maybe the mention of 001 should be removed, as your report table always contains a first column with biblionumber.
(I assume here 001 always contains biblionumber. Maybe it is not the case everywhere?)
 
But this is a detail, so I sign off.

M. Saby

Comment 13 Mathieu Saby 2013-06-13 10:55:16 UTC Comment hidden (obsolete)

Created attachment 18923 [details] [review]
[SIGNED OFF] Bug 8064: Merge several biblios

Comment 14 Kyle M Hall (khall) 2013-07-05 12:33:05 UTC

This does not appear to be working correctly. The field merging seems to be the problem. Video: http://screencast.com/t/9T9SMPut7wjT

Comment 15 Julian Maurice 2013-07-05 13:36:58 UTC

Created attachment 19411 [details]
Screenshot of merge form

Hi Kyle,

I think you did not apply the patch correctly.
On the merge page you can normally see "(ref)" in one tab and a text box at the bottom of the page (see attached screenshot).
Could you confirm, please?

Comment 16 Julian Maurice 2013-07-05 14:03:24 UTC Comment hidden (obsolete)

Created attachment 19413 [details] [review]
Bug 8064: Merge several biblios

This patch improves the existing merging tool by adding possibility to
merge more than 2 biblios.
There is no functional changes:
  - Add some biblios to a list
  - In the list check some biblios and click on 'Merge selected records'
  - Choose the biblio which will be kept, all others will be deleted
  - On the next page you have all biblios you chose in tabs (left side
    of the screen) and the preview of result (right side)
  - Pick some fields or subfields from records that will be deleted or
    delete some fields from reference record.
  - Click on 'Merge', if there is no errors you are redirected to the
    biblio view.

Added checks for non-repeatable subfields
Added checks for mandatory fields and subfields before submitting the
form.
Added a final report which display deleted records (see syspref
MergeReportFields)

This patch is built on top of Bug 9755

Comment 17 Julian Maurice 2013-07-05 14:04:13 UTC

Patch rebased on Bug 9755

Comment 18 Mathieu Saby 2013-08-20 19:29:15 UTC

Kyle, there is a problem in master when you try to copy the LAST field of the record you destroy to the destination record (942 field in your example).
The problem occur even withou this patch.

M. Saby

Comment 19 Jared Camins-Esakov 2013-08-22 13:43:28 UTC

I just tested the most recent version of this patch (which has not been signed off), and I'm not sure it works. I get the following message when I click the Merge button:
Following required subfields are missing:
	- 003$@
	- 008$@
	- 005$@

However, all three fields are in my target record.

Comment 20 Katrin Fischer 2013-09-14 11:40:12 UTC

Hi Julian and Mathieu,
bit confused here - there is no sign-off on the latest bug report and also it looks like Jared found a problem while testing - I am setting this to Failed QA. Julian - please check if you can reproduce Jared's problem?

Comment 21 Mathieu Saby 2013-09-14 12:00:38 UTC

I wanted to test it again, but there is a conflict, the main patch does not apply anymore.

Mathieu

Comment 22 Julian Maurice 2013-09-23 09:59:00 UTC Comment hidden (obsolete)

Created attachment 21351 [details] [review]
Bug 8064: Merge several biblios

This patch improves the existing merging tool by adding possibility to
merge more than 2 biblios.
There is no functional changes:
  - Add some biblios to a list
  - In the list check some biblios and click on 'Merge selected records'
  - Choose the biblio which will be kept, all others will be deleted
  - On the next page you have all biblios you chose in tabs (left side
    of the screen) and the preview of result (right side)
  - Pick some fields or subfields from records that will be deleted or
    delete some fields from reference record.
  - Click on 'Merge', if there is no errors you are redirected to the
    biblio view.

Added checks for non-repeatable subfields
Added checks for mandatory fields and subfields before submitting the
form.
Added a final report which display deleted records (see syspref
MergeReportFields)

Comment 23 Julian Maurice 2013-09-23 10:00:53 UTC

Patch rebased on master

Comment 24 I'm just a bot 2013-09-23 22:02:57 UTC

Patch applied cleanly, go forth and signoff

Comment 25 Paola Rossi 2013-10-14 12:00:40 UTC

As it was written in comment 19, I still get the following message when I click the Merge button:
Following required subfields are missing:
	- 003$@
	- 008$@
	- 005$@
As it was written in comment 18, there is a problem when you try to copy the LAST field of the record you destroy to the destination record.
So I don't sign off, and pass the patch to Failed QA.

Comment 26 Mathieu Saby 2013-10-14 12:03:37 UTC

Hi
I don't remember having the fist issue you mentionned when I tested with UNIMARC records.
Jonathan, is it possible that the patch does not work well with MARC21 records?

Mathieu

Comment 27 Paola Rossi 2013-10-14 12:07:54 UTC

I as worked with MARC21 en.

Comment 28 Julian Maurice 2013-11-06 11:37:58 UTC Comment hidden (obsolete)

Created attachment 22749 [details] [review]
Bug 8064: Change the way target record is built.

Instead of copying/removing a piece of DOM in target record each time a
checkbox is checked/unchecked, the target record is *entirely* rebuilt
each time a checkbox is checked/unchecked.
This is slower but allow for a more consistent and less error-prone
behaviour.

This patch also fix the mandatory check for subfields

Comment 29 Paola Rossi 2013-11-11 16:09:48 UTC

Against master 3.13.00.042, updatedatabase.pl needs to be rebased.
So I pass the patch to "Failed QA" status.
If I install marc21, then 001/003/005/008 are always required by koha; The four fields are displayed and are unchecked, but they are not selectable in any way. So I can't merge the records in any way.
If I install unimarc, I can merge.
If a mandatory field of a merging record is unchecked by user, and it isn't a repeatable field, the user can't add it in any way: koha says "The field is non-repeatable and already exists in the destination record. Therefore, you cannot add it.".
Now I can copy the LAST field of a merging record to the destination record.

Comment 30 Julian Maurice 2013-12-20 09:11:35 UTC Comment hidden (obsolete)

Created attachment 23708 [details] [review]
Bug 8064: Merge several biblios

Rebased on master

Comment 31 Julian Maurice 2013-12-20 09:11:53 UTC Comment hidden (obsolete)

Created attachment 23709 [details] [review]
Bug 8064: Change the way target record is built.

Rebased on master

Comment 32 Julian Maurice 2013-12-20 10:09:28 UTC Comment hidden (obsolete)

Created attachment 23710 [details] [review]
Bug 8064: Fix issues with control fields and non-repeatable check

Fields < 010 were systematically unchecked during target rebuild because
they have no subfields (unchecking manually all subfields of a field
automatically uncheck the field)
When user check a field or a subfield, we have to uncheck it before
calling (sub)field_can_be_added. Otherwise, field is treated as if it
was already in target record, and this is a problem for non-repeatable
fields.

Comment 33 Julian Maurice 2014-01-09 13:53:20 UTC Comment hidden (obsolete)

Created attachment 24078 [details] [review]
Bug 8064: Merge several biblios

Rebased on master

Comment 34 Julian Maurice 2014-01-09 13:53:32 UTC Comment hidden (obsolete)

Created attachment 24079 [details] [review]
Bug 8064: Change the way target record is built.

Rebased on master

Comment 35 Julian Maurice 2014-01-09 13:53:42 UTC Comment hidden (obsolete)

Created attachment 24080 [details] [review]
Bug 8064: Fix issues with control fields and non-repeatable check

Rebased on master

Comment 36 Paola Rossi 2014-01-17 14:13:59 UTC

I test against master 3.15.00.012 , unimarc, icu, italian version.

All the items of all the merged records are kept in the output record (OK).
In 005 time is the time of the merge (OK).
The use of pref "MergeReportFields" is OK too. Report shows all the required columns (except the ones whose values are empty for all the records involved in the merge, OK).

So my test is OK for unimarc, italian version.

I let you complete the test on marc21. I shall not work on bugzilla in the next weeks.

I keep the "Needs Signoff" status.

Comment 37 Mathieu Saby 2014-02-18 11:51:32 UTC

nobody to test that on MARC21?

Mathieu

Comment 38 Stefano Bargioni 2014-03-11 09:31:14 UTC

I signed off the patch, with a little extra fix.
Function check_mandatory() -in file koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt- states that subfield '@' of tags 003, 005, 008 are missing.
I changed
if (subfieldcode == 'lib' || subfieldcode == 'mandatory'
                || subfieldcode == 'repeatable' || subfieldcode == 'tab') {
                    continue;
                }
to 
if (subfieldcode == 'lib' || subfieldcode == 'mandatory'
                || subfieldcode == 'repeatable' || subfieldcode == 'tab'
                || subfieldcode == '@') {
                    continue;
                }
This fix solved the problem.

Comment 39 Stefano Bargioni 2014-03-11 09:33:33 UTC Comment hidden (obsolete)

Created attachment 26055 [details] [review]
Bug 8064: Merge several biblios

Comment 40 Stefano Bargioni 2014-03-11 09:35:07 UTC Comment hidden (obsolete)

Created attachment 26056 [details] [review]
Bug 8064: Change the way target record is built.

Comment 41 Stefano Bargioni 2014-03-11 09:35:46 UTC Comment hidden (obsolete)

Created attachment 26057 [details] [review]
Bug 8064: Fix issues with control fields and non-repeatable check

Comment 42 Stefano Bargioni 2014-03-11 09:37:26 UTC Comment hidden (obsolete)

Created attachment 26058 [details] [review]
8064-Little fix for 003 005 008 in MARC21

This patch adds one line to the check_mandatory function

Comment 43 Zeno Tajoli 2014-03-11 10:51:28 UTC

Stefano signed off for MARC21 records.
So now the patch is working for Unimarc and MARC21

Comment 44 Magnus Enger 2014-03-11 11:23:03 UTC

(In reply to Zeno Tajoli from comment #43)
> Stefano signed off for MARC21 records.
> So now the patch is working for Unimarc and MARC21

I think MARC21 and NORMARC are similar enough that testing with NORMARC is not necessary.

Comment 45 Julian Maurice 2014-03-11 12:47:54 UTC

This patch (attachment 26058 [details] [review]) bypass mandatory check for fields < 010. But at least the merge is not blocked anymore. So I think it could be pushed.

Comment 46 Katrin Fischer 2014-04-27 13:36:33 UTC

With these patches some unit tests fail, they are ok on master:

t/Koha_MetadataRecord.t           (Wstat: 512 Tests: 4 Failed: 2)
  Failed tests:  3-4
  Non-zero exit status: 2
t/Koha_Util_MARC.t                (Wstat: 512 Tests: 4 Failed: 2)
  Failed tests:  2-3
  Non-zero exit status: 2

Comment 47 Julian Maurice 2014-06-09 07:56:25 UTC Comment hidden (obsolete)

Created attachment 28726 [details] [review]
Bug 8064: Fix unit tests for createMergeHash

Comment 48 Julian Maurice 2014-06-09 07:57:17 UTC

Unit tests were wrong.

Comment 49 Paola Rossi 2014-06-09 13:46:01 UTC

I try to apply against master 3.17.00.007

Applying: Bug 8064: Merge several biblios
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql
Failed to merge in the changes.
Patch failed at 0001 Bug 8064: Merge several biblios

I pass the patch to "Patch doesn't apply" status.

Comment 50 Julian Maurice 2014-06-13 09:33:56 UTC Comment hidden (obsolete)

Created attachment 28829 [details] [review]
Bug 8064: Merge several biblios

Rebased on master

Comment 51 Julian Maurice 2014-06-13 09:34:09 UTC Comment hidden (obsolete)

Created attachment 28830 [details] [review]
Bug 8064: Change the way target record is built.

Rebased on master

Comment 52 Julian Maurice 2014-06-13 09:34:22 UTC Comment hidden (obsolete)

Created attachment 28831 [details] [review]
Bug 8064: Fix issues with control fields and non-repeatable check

Rebased on master

Comment 53 Julian Maurice 2014-06-13 09:34:36 UTC Comment hidden (obsolete)

Created attachment 28832 [details] [review]
Bug 8064: Little fix for 003, 005, 008 in MARC21

Rebased on master

Comment 54 Julian Maurice 2014-06-13 09:34:44 UTC Comment hidden (obsolete)

Created attachment 28833 [details] [review]
Bug 8064: Fix unit tests for createMergeHash

Rebased on master

Comment 55 Bernardo Gonzalez Kriegel 2014-06-27 14:03:48 UTC Comment hidden (obsolete)

Created attachment 29324 [details] [review]
[SIGNED-OFF] Bug 8064: Merge several biblios

This patch improves the existing merging tool by adding possibility to
merge more than 2 biblios.
There is no functional changes:
  - Add some biblios to a list
  - In the list check some biblios and click on 'Merge selected records'
  - Choose the biblio which will be kept, all others will be deleted
  - On the next page you have all biblios you chose in tabs (left side
    of the screen) and the preview of result (right side)
  - Pick some fields or subfields from records that will be deleted or
    delete some fields from reference record.
  - Click on 'Merge', if there is no errors you are redirected to the
    biblio view.

Added checks for non-repeatable subfields
Added checks for mandatory fields and subfields before submitting the
form.
Added a final report which display deleted records (see syspref
MergeReportFields)

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Tested on MARC21, works as decribed.
Mergind 3 records, uncheking some field on target, adding fields from
deleted records.
Add bug number to updatedatabase message.

All test pass. No koha-qa errors

Comment 56 Bernardo Gonzalez Kriegel 2014-06-27 14:04:01 UTC Comment hidden (obsolete)

Created attachment 29325 [details] [review]
[SIGNED-OFF] Bug 8064: Change the way target record is built.

Instead of copying/removing a piece of DOM in target record each time a
checkbox is checked/unchecked, the target record is *entirely* rebuilt
each time a checkbox is checked/unchecked.
This is slower but allow for a more consistent and less error-prone
behaviour.

This patch also fix the mandatory check for subfields

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Comment 57 Bernardo Gonzalez Kriegel 2014-06-27 14:04:12 UTC Comment hidden (obsolete)

Created attachment 29326 [details] [review]
[SIGNED-OFF] Bug 8064: Fix issues with control fields and non-repeatable check

Fields < 010 were systematically unchecked during target rebuild because
they have no subfields (unchecking manually all subfields of a field
automatically uncheck the field)
When user check a field or a subfield, we have to uncheck it before
calling (sub)field_can_be_added. Otherwise, field is treated as if it
was already in target record, and this is a problem for non-repeatable
fields.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Comment 58 Bernardo Gonzalez Kriegel 2014-06-27 14:04:23 UTC Comment hidden (obsolete)

Created attachment 29327 [details] [review]
[SIGNED-OFF] Bug 8064: Little fix for 003, 005, 008 in MARC21

I found a little bug in the patch. Subfield '@' of tags 003, 005, 008 in
MARC21 were not correctly handled by function check_mandatory() in
merge.tt. I fixed the problem. In my opinion, the patch can be signed
off.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Comment 59 Bernardo Gonzalez Kriegel 2014-06-27 14:04:36 UTC Comment hidden (obsolete)

Created attachment 29328 [details] [review]
[SIGNED-OFF] Bug 8064: Fix unit tests for createMergeHash

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Comment 60 Katrin Fischer 2014-07-06 21:29:44 UTC

Patch doesn't apply currently:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 8064: Merge several biblios
Applying: Bug 8064: Change the way target record is built.
Using index info to reconstruct a base tree...
M	koha-tmpl/intranet-tmpl/prog/en/includes/merge-record-strings.inc
M	koha-tmpl/intranet-tmpl/prog/en/js/merge-record.js
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/js/merge-record.js
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/js/merge-record.js
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/merge-record-strings.inc
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/includes/merge-record-strings.inc
Failed to merge in the changes.
Patch failed at 0001 Bug 8064: Change the way target record is built.
The copy of the patch that failed is found in:
   /home/katrin/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/SIGNED-OFF-Bug-8064-Change-the-way-target-record-i-ze4drL.patch

Comment 61 Julian Maurice 2014-07-21 11:33:44 UTC Comment hidden (obsolete)

Created attachment 29921 [details] [review]
[SIGNED-OFF] Bug 8064: Merge several biblios

Rebased on master

Comment 62 Julian Maurice 2014-07-21 11:34:13 UTC Comment hidden (obsolete)

Created attachment 29922 [details] [review]
[SIGNED-OFF] Bug 8064: Change the way target record is built.

Rebased on master

Comment 63 Julian Maurice 2014-07-21 11:34:33 UTC Comment hidden (obsolete)

Created attachment 29923 [details] [review]
[SIGNED-OFF] Bug 8064: Fix issues with control fields and non-repeatable check

Rebased on master

Comment 64 Julian Maurice 2014-07-21 11:35:00 UTC Comment hidden (obsolete)

Created attachment 29924 [details] [review]
[SIGNED-OFF] Bug 8064: Little fix for 003, 005, 008 in MARC21

Rebased on master

Comment 65 Julian Maurice 2014-07-21 11:35:20 UTC Comment hidden (obsolete)

Created attachment 29925 [details] [review]
[SIGNED-OFF] Bug 8064: Fix unit tests for createMergeHash

Rebased on master

Comment 66 Olli-Antti Kivilahti 2014-12-01 13:42:36 UTC

Sorry, couldn't apply this feature and the reject files didn't make much sense to me. I worked on Bug 4283 and tried to sign this one off as a target of opportunity.

Comment 67 Julian Maurice 2015-02-09 08:47:22 UTC Comment hidden (obsolete)

Created attachment 35738 [details] [review]
[SIGNED-OFF] Bug 8064: Merge several biblios

Rebased on master

Comment 68 Julian Maurice 2015-02-09 08:47:40 UTC Comment hidden (obsolete)

Created attachment 35739 [details] [review]
[SIGNED-OFF] Bug 8064: Change the way target record is built.

Rebased on master

Comment 69 Julian Maurice 2015-02-09 08:47:57 UTC Comment hidden (obsolete)

Created attachment 35740 [details] [review]
[SIGNED-OFF] Bug 8064: Fix issues with control fields and non-repeatable check

Rebased on master

Comment 70 Julian Maurice 2015-02-09 08:48:16 UTC Comment hidden (obsolete)

Created attachment 35741 [details] [review]
[SIGNED-OFF] Bug 8064: Little fix for 003, 005, 008 in MARC21

Rebased on master

Comment 71 Julian Maurice 2015-02-09 08:48:35 UTC Comment hidden (obsolete)

Created attachment 35742 [details] [review]
[SIGNED-OFF] Bug 8064: Fix unit tests for createMergeHash

Rebased on master

Comment 72 Kyle M Hall (khall) 2015-02-13 14:19:58 UTC Comment hidden (obsolete)

Created attachment 35900 [details] [review]
[PASSED QA] Bug 8064: Merge several biblios

This patch improves the existing merging tool by adding possibility to
merge more than 2 biblios.
There is no functional changes:
  - Add some biblios to a list
  - In the list check some biblios and click on 'Merge selected records'
  - Choose the biblio which will be kept, all others will be deleted
  - On the next page you have all biblios you chose in tabs (left side
    of the screen) and the preview of result (right side)
  - Pick some fields or subfields from records that will be deleted or
    delete some fields from reference record.
  - Click on 'Merge', if there is no errors you are redirected to the
    biblio view.

Added checks for non-repeatable subfields
Added checks for mandatory fields and subfields before submitting the
form.
Added a final report which display deleted records (see syspref
MergeReportFields)

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Comment 73 Kyle M Hall (khall) 2015-02-13 14:20:30 UTC Comment hidden (obsolete)

Created attachment 35901 [details] [review]
[PASSED QA] Bug 8064: Change the way target record is built.

Instead of copying/removing a piece of DOM in target record each time a
checkbox is checked/unchecked, the target record is *entirely* rebuilt
each time a checkbox is checked/unchecked.
This is slower but allow for a more consistent and less error-prone
behaviour.

This patch also fix the mandatory check for subfields

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Comment 74 Kyle M Hall (khall) 2015-02-13 14:20:35 UTC Comment hidden (obsolete)

Created attachment 35902 [details] [review]
[PASSED QA] Bug 8064: Fix issues with control fields and non-repeatable check

Fields < 010 were systematically unchecked during target rebuild because
they have no subfields (unchecking manually all subfields of a field
automatically uncheck the field)
When user check a field or a subfield, we have to uncheck it before
calling (sub)field_can_be_added. Otherwise, field is treated as if it
was already in target record, and this is a problem for non-repeatable
fields.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Comment 75 Kyle M Hall (khall) 2015-02-13 14:20:39 UTC Comment hidden (obsolete)

Created attachment 35903 [details] [review]
[PASSED QA] Bug 8064: Little fix for 003, 005, 008 in MARC21

I found a little bug in the patch. Subfield '@' of tags 003, 005, 008 in
MARC21 were not correctly handled by function check_mandatory() in
merge.tt. I fixed the problem. In my opinion, the patch can be signed
off.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Comment 76 Kyle M Hall (khall) 2015-02-13 14:20:43 UTC Comment hidden (obsolete)

Created attachment 35904 [details] [review]
[PASSED QA] Bug 8064: Fix unit tests for createMergeHash

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Comment 77 Kyle M Hall (khall) 2015-02-13 14:20:47 UTC Comment hidden (obsolete)

Created attachment 35905 [details] [review]
[PASSED QA] Bug 8064 [QA Followup] - Remove trailing whitespace

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Comment 78 Kyle M Hall (khall) 2015-02-13 14:26:12 UTC

QA Note: holds are merged in a very odd, un-good way, but the behavior exists on master, so the issue predates this patch set.

Comment 79 Tomás Cohen Arazi (tcohen) 2015-11-02 15:31:12 UTC

Please rebase against master. Better put the updatedatabase.pl entry on an atomicupdate file.

Comment 81 Julian Maurice 2015-11-09 10:48:36 UTC

Created attachment 44633 [details] [review]
[PASSED QA] Bug 8064: Merge several biblios

Rebased on master

Comment 82 Julian Maurice 2015-11-09 10:48:58 UTC

Created attachment 44634 [details] [review]
[PASSED QA] Bug 8064: Change the way target record is built.

Rebased on master

Comment 83 Julian Maurice 2015-11-09 10:49:14 UTC

Created attachment 44635 [details] [review]
[PASSED QA] Bug 8064: Fix issues with control fields and non-repeatable check

Rebased on master

Comment 84 Julian Maurice 2015-11-09 10:49:30 UTC

Created attachment 44636 [details] [review]
[PASSED QA] Bug 8064: Little fix for 003, 005, 008 in MARC21

Rebased on master

Comment 85 Julian Maurice 2015-11-09 10:49:45 UTC

Created attachment 44637 [details] [review]
[PASSED QA] Bug 8064: Fix unit tests for createMergeHash

Rebased on master

Comment 86 Julian Maurice 2015-11-09 10:50:02 UTC

Created attachment 44638 [details] [review]
[PASSED QA] Bug 8064 [QA Followup] - Remove trailing whitespace

Rebased on master

Comment 87 Julian Maurice 2015-11-09 10:51:08 UTC

updatedatabase.pl entry moved into atomicupdate/

Comment 88 Tomás Cohen Arazi (tcohen) 2015-11-09 18:43:33 UTC

Patches pushed to master.

Thanks Julian!

Comment 89 Ian Palko 2015-12-14 21:15:06 UTC

These patches seems to break authority merging.

I have opened a new Bug report in Bug 15358 regarding this.

Comment 90 David Cook 2022-08-22 05:41:48 UTC

*** Bug 4283 has been marked as a duplicate of this bug. ***