|
Description
Julian Maurice
2012-05-10 08:53:17 UTC
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. Do not sign off this patch right now, i'm still working on it 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) Does this take into account the (already signed-off) refactoring I did on bug 9755? Sponsored by Rennes 2 M. Saby Absolutely not... I think Julian did not know this report exist... 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 I am working on a rebase of Biblibre on top of 9755. Mathieu I mark it in discussion, as 9755 will be soon QAed and pushed... M. Saby 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 Please apply all patches from Bug 9755 before testing this one. 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
Created attachment 18923 [details] [review] [SIGNED OFF] Bug 8064: Merge several biblios This does not appear to be working correctly. The field merging seems to be the problem. Video: http://screencast.com/t/9T9SMPut7wjT 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?
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 Patch rebased on Bug 9755 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 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. 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? I wanted to test it again, but there is a conflict, the main patch does not apply anymore. Mathieu 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) Patch rebased on master Patch applied cleanly, go forth and signoff 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. 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 I as worked with MARC21 en. 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 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. Created attachment 23708 [details] [review] Bug 8064: Merge several biblios Rebased on master Created attachment 23709 [details] [review] Bug 8064: Change the way target record is built. Rebased on master 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. Created attachment 24078 [details] [review] Bug 8064: Merge several biblios Rebased on master Created attachment 24079 [details] [review] Bug 8064: Change the way target record is built. Rebased on master Created attachment 24080 [details] [review] Bug 8064: Fix issues with control fields and non-repeatable check Rebased on master 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. nobody to test that on MARC21? Mathieu 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.
Created attachment 26055 [details] [review] Bug 8064: Merge several biblios Created attachment 26056 [details] [review] Bug 8064: Change the way target record is built. Created attachment 26057 [details] [review] Bug 8064: Fix issues with control fields and non-repeatable check Created attachment 26058 [details] [review] 8064-Little fix for 003 005 008 in MARC21 This patch adds one line to the check_mandatory function Stefano signed off for MARC21 records. So now the patch is working for Unimarc and MARC21 (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. 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. 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 Created attachment 28726 [details] [review] Bug 8064: Fix unit tests for createMergeHash Unit tests were wrong. 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. Created attachment 28829 [details] [review] Bug 8064: Merge several biblios Rebased on master Created attachment 28830 [details] [review] Bug 8064: Change the way target record is built. Rebased on master Created attachment 28831 [details] [review] Bug 8064: Fix issues with control fields and non-repeatable check Rebased on master Created attachment 28832 [details] [review] Bug 8064: Little fix for 003, 005, 008 in MARC21 Rebased on master Created attachment 28833 [details] [review] Bug 8064: Fix unit tests for createMergeHash Rebased on master 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 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> 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> 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> Created attachment 29328 [details] [review] [SIGNED-OFF] Bug 8064: Fix unit tests for createMergeHash Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> 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 Created attachment 29921 [details] [review] [SIGNED-OFF] Bug 8064: Merge several biblios Rebased on master Created attachment 29922 [details] [review] [SIGNED-OFF] Bug 8064: Change the way target record is built. Rebased on master Created attachment 29923 [details] [review] [SIGNED-OFF] Bug 8064: Fix issues with control fields and non-repeatable check Rebased on master Created attachment 29924 [details] [review] [SIGNED-OFF] Bug 8064: Little fix for 003, 005, 008 in MARC21 Rebased on master Created attachment 29925 [details] [review] [SIGNED-OFF] Bug 8064: Fix unit tests for createMergeHash Rebased on master 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. Created attachment 35738 [details] [review] [SIGNED-OFF] Bug 8064: Merge several biblios Rebased on master Created attachment 35739 [details] [review] [SIGNED-OFF] Bug 8064: Change the way target record is built. Rebased on master Created attachment 35740 [details] [review] [SIGNED-OFF] Bug 8064: Fix issues with control fields and non-repeatable check Rebased on master Created attachment 35741 [details] [review] [SIGNED-OFF] Bug 8064: Little fix for 003, 005, 008 in MARC21 Rebased on master Created attachment 35742 [details] [review] [SIGNED-OFF] Bug 8064: Fix unit tests for createMergeHash Rebased on master 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> 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> 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> 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> 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> Created attachment 35905 [details] [review] [PASSED QA] Bug 8064 [QA Followup] - Remove trailing whitespace Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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. Please rebase against master. Better put the updatedatabase.pl entry on an atomicupdate file. Created attachment 44633 [details] [review] [PASSED QA] Bug 8064: Merge several biblios Rebased on master Created attachment 44634 [details] [review] [PASSED QA] Bug 8064: Change the way target record is built. Rebased on master Created attachment 44635 [details] [review] [PASSED QA] Bug 8064: Fix issues with control fields and non-repeatable check Rebased on master Created attachment 44636 [details] [review] [PASSED QA] Bug 8064: Little fix for 003, 005, 008 in MARC21 Rebased on master Created attachment 44637 [details] [review] [PASSED QA] Bug 8064: Fix unit tests for createMergeHash Rebased on master Created attachment 44638 [details] [review] [PASSED QA] Bug 8064 [QA Followup] - Remove trailing whitespace Rebased on master updatedatabase.pl entry moved into atomicupdate/ Patches pushed to master. Thanks Julian! These patches seems to break authority merging. I have opened a new Bug report in Bug 15358 regarding this. *** Bug 4283 has been marked as a duplicate of this bug. *** |