Bug 6210 - Choose framework on Merge
Summary: Choose framework on Merge
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Srdjan Jankovic
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-20 00:26 UTC by Nicole C. Engard
Modified: 2013-12-05 20:04 UTC (History)
6 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
patch (8.71 KB, patch)
2011-10-06 06:58 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] bug_6210: Select framework if merging two records with different frameworks (8.78 KB, patch)
2011-10-06 12:49 UTC, Nicole C. Engard
Details | Diff | Splinter Review
patch (9.56 KB, patch)
2011-10-07 05:40 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (9.54 KB, patch)
2011-11-07 02:39 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (12.33 KB, patch)
2011-11-14 06:30 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (12.65 KB, patch)
2011-12-19 06:25 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] bug_6210: Select framework if merging two records with different frameworks (13.30 KB, patch)
2011-12-20 15:40 UTC, Nicole C. Engard
Details | Diff | Splinter Review
screenshot (18.62 KB, image/png)
2012-01-24 11:15 UTC, Paul Poulain
Details
Follow-up fix (6.66 KB, patch)
2012-01-24 15:47 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6210 - Follow up, Choose framework on merge (6.70 KB, patch)
2012-01-24 22:06 UTC, Nicole C. Engard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2011-04-20 00:26:36 UTC
Right now when you merge two bibs they have to be the same framework, I'd like to see the librarian be able to choose which framework to apply to the newly merged record instead of just stopping them from merging two records with different frameworks.
Comment 1 Srdjan Jankovic 2011-09-21 03:02:42 UTC
We can go three ways with this:
* Take reference record framework
* Pick from two records frameworks
* Pick from the full list
Can you please pick the most appropriate one
Comment 2 Katrin Fischer 2011-09-21 07:44:36 UTC
Could we use the full list and preselect the reference record's framework?
Comment 3 Srdjan Jankovic 2011-09-21 23:58:55 UTC
That is certainly possible. Do we need a confirmation on this, or shall I proceed?
Comment 4 Nicole C. Engard 2011-09-28 14:59:37 UTC
I confirm that this is the way it should work. Go ahead with Katrin's idea.

Nicole
Comment 5 Srdjan Jankovic 2011-10-06 06:58:46 UTC Comment hidden (obsolete)
Comment 6 Nicole C. Engard 2011-10-06 12:49:24 UTC Comment hidden (obsolete)
Comment 7 Nicole C. Engard 2011-10-06 17:05:34 UTC
New test plan found that this doesn't pass QA:

    Test 1:  Merge two records with the same framework
    Desired result:  shouldn't get any prompting to pick a framework, and the same framework should be used

**PASSED Test 1**

    Test 2:  2 records, different frameworks, into the kept record's framework
    Desired result:  merge with kept records framework used

**PASSED Test 2**

    Test 3:  2 records, different frameworks, into the discarded record's framework
    Desired result:  merge with used records framework used

**FAILED Test 3**

    Test 4:  2 records, different frameworks, into a third framework
    Desired result:  merge with third framework used

**FAILED Test 4**
Comment 8 Srdjan Jankovic 2011-10-07 05:40:54 UTC Comment hidden (obsolete)
Comment 9 Srdjan Jankovic 2011-11-07 02:39:32 UTC Comment hidden (obsolete)
Comment 10 Nicole C. Engard 2011-11-11 14:56:28 UTC
When repeating test #2 I get the following when trying to complete the merge:


Software error:

Tag "" is not a valid tag. at /home/nengard/kohaclone/C4/Biblio.pm line 3219

For help, please send mail to the webmaster (webmaster@debian), giving this error message and the time and date of the error.
Comment 11 Srdjan Jankovic 2011-11-14 06:30:25 UTC Comment hidden (obsolete)
Comment 12 Nicole C. Engard 2011-12-12 15:44:08 UTC
    Test 1:  Merge two records with the same framework
    Desired result:  shouldn't get any prompting to pick a framework, and the
same framework should be used

**PASSED Test 1**

    Test 2:  2 records, different frameworks, into the kept record's framework
    Desired result:  merge with kept records framework used

**PASSED Test 2**

    Test 3:  2 records, different frameworks, into the discarded record's
framework
    Desired result:  merge with used records framework used

**PASSED Test 3**

    Test 4:  2 records, different frameworks, into a third framework
    Desired result:  merge with third framework used

** FAILED Test 4 **

After clicking 'merge' I get this - I tried to merge a books framework title and a fast add framework title in to the default framework.

Software error:

No biblionumber tag for framework "Default" at /home/nengard/kohaclone/C4/Biblio.pm line 3186.

For help, please send mail to the webmaster (webmaster@debian), giving this error message and the time and date of the error.
Comment 13 Srdjan Jankovic 2011-12-19 06:25:31 UTC Comment hidden (obsolete)
Comment 14 Nicole C. Engard 2011-12-20 15:39:48 UTC
Test 1:  Merge two records with the same framework
    Desired result:  shouldn't get any prompting to pick a framework, and the
same framework should be used

**PASSED Test 1**

    Test 2:  2 records, different frameworks, into the kept record's framework
    Desired result:  merge with kept records framework used

**PASSED Test 2**

    Test 3:  2 records, different frameworks, into the discarded record's
framework
    Desired result:  merge with used records framework used

**PASSED Test 3**

    Test 4:  2 records, different frameworks, into a third framework
    Desired result:  merge with third framework used

** PASSED Test 4 **
Comment 15 Nicole C. Engard 2011-12-20 15:40:46 UTC
Created attachment 6889 [details] [review]
[SIGNED-OFF] bug_6210: Select framework if merging two records with different frameworks

ModBiblio() - set framework to "" if "Default"

Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>

All 4 tests passed:

Test 1:  Merge two records with the same framework
    Desired result:  shouldn't get any prompting to pick a framework, and the
same framework should be used

    Test 2:  2 records, different frameworks, into the kept record's framework
    Desired result:  merge with kept records framework used

    Test 3:  2 records, different frameworks, into the discarded record's
framework
    Desired result:  merge with used records framework used

    Test 4:  2 records, different frameworks, into a third framework
    Desired result:  merge with third framework used
Comment 16 Paul Poulain 2012-01-24 11:13:42 UTC
QA comments:
1- there is an unconditionnal warn: warn qq{No marc tags for framework "$frameworkcode" field $kohafield};
2- strengthen/clean the code on some subs by:
   - testing input parameters (_koha_marc_update_bib_ids)
   - switching a if !cond {A} else {B} to a if cond {B} else {A}
(this was not announced, it's OK, but would have been better if announced
3- cataloguing/merge.pl add a 
+use C4::Koha;
=> where is this C4::Koha used ? (I can't see, as there are also a lot of reindenting)
4- my tests show a look I think strange, was it intended ? (screenshot attached)

Failed QA for 1, waiting for an answer to 3 and 4, (2 is OK)
Comment 17 Paul Poulain 2012-01-24 11:15:46 UTC
Created attachment 7311 [details]
screenshot

the framework list is on the right, is it intended ? looks a little bit strange to me
Comment 18 Nicole C. Engard 2012-01-24 14:24:46 UTC
It is intended - it's there so you can choose what the framework will be when you finish merging.

Nicole
Comment 19 Paul Poulain 2012-01-24 14:43:03 UTC
(In reply to comment #18)
> It is intended - it's there so you can choose what the framework will be when
> you finish merging.
Nicole, I understand what it's made for. My surprise is his position on the screen.
Comment 20 Owen Leonard 2012-01-24 15:47:36 UTC Comment hidden (obsolete)
Comment 21 Nicole C. Engard 2012-01-24 16:48:57 UTC
Owen,

Is your patch a follow up? or an edited patch? Should I apply both? Cause when I tried to apply both I got this:


nengard@kohavm:~/kohaclone$ git bz apply 6210
Bug 6210 - Choose framework on Merge

[SIGNED-OFF] bug_6210: Select framework if merging two records with different frameworks
Apply? [yn] y

Applying: bug_6210: Select framework if merging two records with different frameworks
/home/nengard/kohaclone/.git/rebase-apply/patch:195: trailing whitespace.
                $framework = $frameworkcode1;
/home/nengard/kohaclone/.git/rebase-apply/patch:221: trailing whitespace.
                choosereference => 1,
/home/nengard/kohaclone/.git/rebase-apply/patch:299: trailing whitespace.
                                 [% frameworkcodeloo.frameworktext %]                              
warning: 3 lines add whitespace errors.
Follow-up fix
Apply? [yn] y

Applying: Bug 6210 - Follow up, Choose framework on merge
error: patch failed: koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt:198
error: koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt: patch does not apply
Patch failed at 0001 Bug 6210 - Follow up, Choose framework on merge
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Patch left in /tmp/Follow-up-fix-z8BjoC.patch
nengard@kohavm:~/kohaclone$
Comment 22 Owen Leonard 2012-01-24 21:08:11 UTC
My patch is a follow-up. I just tried using git bz to apply both patches on a fresh branch and had no problems. I wonder if it's because I have this git configuration tweak?

http://blog.bigballofwax.co.nz/2011/12/15/fixing-whitespace-when-apply-patches-with-git/
Comment 23 Nicole C. Engard 2012-01-24 22:06:46 UTC
Created attachment 7327 [details] [review]
[SIGNED-OFF] Bug 6210 - Follow up, Choose framework on merge

- Adding subtitle to the display of titles to be merged
- Adding a link to preview the MARC record of titles to be merged
- Fixing up markup of form to improve appearance
- Correcting breadcrumbs

Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>
Comment 24 Nicole C. Engard 2012-01-24 22:07:39 UTC
That was it - the follow up was signed off on.
Comment 25 Srdjan Jankovic 2012-01-25 07:02:22 UTC
Issues from comment 16:

1. warn is not unconditional, sub returns before warn if all is ok

3. C4::Koha::getframeworks() is used

4. Other layouts are posiible. Fields can be moved around easily without affecting functionality.
Comment 26 Paul Poulain 2012-02-01 16:44:17 UTC
thx owen for the new display, thanks srdjan for your answers, passed QA
Comment 27 Marcel de Rooy 2012-02-02 13:57:17 UTC
(In reply to comment #26)
> thx owen for the new display, thanks srdjan for your answers, passed QA

Probably, these new warnings (like below) come from this first patch, making changes in GetMarcFromKohaField:

[Thu Feb 02 14:46:00 2012] [error] [client 192.87.126.61] [Thu Feb  2 14:46:00 2012] opac-search.pl: No marc tags for framework "" field items.biblionumber at /usr/share/koha/testclone/C4/Biblio.pm line 1051., referer: http://libdevelop.rijksmuseum.nl:8008/cgi-bin/koha/opac-search.pl?q=vermeer

Note that it repeats for multiple fields!!

Just a side note: When writing/testing your patches, please check your error log too.
Comment 28 Paul Poulain 2012-02-02 14:34:53 UTC
In Comment #16 I wrote:
> QA comments:
> 1- there is an unconditionnal warn: warn qq{No marc tags for framework
> "$frameworkcode" field $kohafield};
Srdjan answered in comment #25:
> Issues from comment 16:
> 1. warn is not unconditional, sub returns before warn if all is ok

Why didn't I checked, I don't know. But the result is the verbose logs !
In UNIMARC, there are many fields that are in items table and not connected to a MARC field. And that should not trouble anyone.
So we should not issue any warning.

I've pushed a follow-up removing the warning, the logs are silenced now
Comment 29 Marcel de Rooy 2012-02-03 07:41:38 UTC
Confirming that follow up fixed noise in the log. Thx!
Comment 30 Jared Camins-Esakov 2012-12-31 00:33:33 UTC
There have been no further reports of problems so I am marking this bug resolved.