Bug 7455 - Authority subfields are cloned in the wrong field
Summary: Authority subfields are cloned in the wrong field
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: 3.10
Hardware: All All
: P4 normal (vote)
Assignee: Fridolin SOMERS
QA Contact: Mason James
URL:
Keywords:
Depends on:
Blocks: 4381
  Show dependency treegraph
 
Reported: 2012-01-20 15:36 UTC by Adrien SAURAT
Modified: 2014-05-26 21:04 UTC (History)
6 users (show)

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


Attachments
1) Example : a repeatable field, with repeatable subfields (42.92 KB, image/png)
2012-01-20 15:37 UTC, Adrien SAURAT
Details
2) Click to clone a subfield (59.38 KB, image/png)
2012-01-20 15:38 UTC, Adrien SAURAT
Details
3) The subfield is not created in the selected field (50.44 KB, image/png)
2012-01-20 15:39 UTC, Adrien SAURAT
Details
Proposed patch (108.16 KB, patch)
2012-07-17 09:08 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Signed-off patch (108.52 KB, patch)
2012-07-20 07:42 UTC, Frédéric Demians
Details | Diff | Splinter Review
Proposed patch (follow-up) (5.43 KB, patch)
2012-08-03 15:37 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Problematic bib editor layout (168.95 KB, image/png)
2012-08-27 17:37 UTC, Jared Camins-Esakov
Details
Proposed patch (follow-up) (revised) (2.94 KB, patch)
2012-08-28 12:58 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 7455: Authority subfields are cloned in the wrong field (108.28 KB, patch)
2012-08-31 12:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7455: Authority subfields are cloned in the wrong field (follow-up) (2.98 KB, patch)
2012-08-31 12:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7455: Authority subfields are cloned in the wrong field (follow-up) (3.00 KB, patch)
2012-09-14 12:06 UTC, Mason James
Details | Diff | Splinter Review
Proposed patch (rebased master) (106.23 KB, patch)
2012-09-18 15:38 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Proposed patch (follow-up 1) (rebased master) (2.94 KB, patch)
2012-09-18 15:39 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Proposed patch (follow-up 2) (8.01 KB, patch)
2012-09-18 15:44 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Proposed patch (follow-up 3) (1.40 KB, patch)
2012-09-27 08:37 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Proposed patch (follow-up 2) (rebased master) (8.02 KB, patch)
2012-10-15 17:12 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Proposed patch (follow-up 3) (rebased master) (1.40 KB, patch)
2012-10-15 17:13 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 7455: Authority subfields are cloned in the wrong field (105.94 KB, patch)
2012-10-21 14:44 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7455: Authority subfields are cloned in the wrong field (follow-up) (2.95 KB, patch)
2012-10-21 14:44 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7455: Authority subfields are cloned in the wrong field (follow-up 2) (7.79 KB, patch)
2012-10-21 14:44 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7455: Authority subfields are cloned in the wrong field (follow-up 3) (1.44 KB, patch)
2012-10-21 14:44 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Adrien SAURAT 2012-01-20 15:36:40 UTC
Plan to reproduce :

1) If we have a repeatable field (containing repeatable subfields) in an authority, let's repeat it at least once.

2) The first occurence will behave ok. But the following will function badly.
If we clone a subfield from the second repeated field for example, the new subfield won't be created in the second field, but in the first one!

In fact, any subfield cloned within any occurence of this field will be created in the first field on the list.

(I'll attach screenshots)
Comment 1 Adrien SAURAT 2012-01-20 15:37:50 UTC
Created attachment 7257 [details]
1) Example : a repeatable field, with repeatable subfields
Comment 2 Adrien SAURAT 2012-01-20 15:38:35 UTC
Created attachment 7258 [details]
2) Click to clone a subfield
Comment 3 Adrien SAURAT 2012-01-20 15:39:06 UTC
Created attachment 7259 [details]
3) The subfield is not created in the selected field
Comment 4 Fridolin SOMERS 2012-07-17 09:08:33 UTC Comment hidden (obsolete)
Comment 5 Fridolin SOMERS 2012-07-17 09:45:29 UTC
Here is a test plan : 

Same for Biblio and Authorities edition :
-----------------------------------------

Create a new record.
Go to a repeatable field with a repeatable subfield. I'll take as example 200$a.

Cloning :
Click on cloning image of subfield $a => subfield $a must be cloned.
Click on cloning image of field 200 => field 200 must be cloned.
In second field 200, click on cloning image of last subfield $a => subfield $a of the second field 200 must be cloned.

Deleting :
In second field 200, click on delete image of last subfield $a => last subfield $a of the second field 200 must be deleted.
In second field 200, click on delete image of all subfields $a => the last one must not be deleted but empty.
Click on delete image of first field 200 => the first field 200 is deleted.
Click on delete image of field 200 => field 200 is not deleted but all subfields are empty.

Saving : 
Recreate two fields 200, each one with two subfields $a.
Edit indicators and subfields values with different texts.
Save the record.
Check if indicators and fields 200 are ok.

Same for Item edition, serials edition and batch item modification : 
--------------------------------------------------------------------

Define an item subfield has repeatable.
Go to edition page.
Click on cloning image of subfield => subfield must be cloned.
(There is no delete function).
Edit some subfields with texts.
Save and check if all values are saved.
Comment 6 Frédéric Demians 2012-07-20 07:11:23 UTC
+1 for aggregating common JavaScript functions coming from various
templates into one place.

Could you say exactly where to check that nothing is broken (apart from
authority editing)? I would say: adding a biblio record, an item, and
batch modification?
Comment 7 Fridolin SOMERS 2012-07-20 07:19:55 UTC
Is something missing in my test plan (comment 5), or must be more explained ?
Comment 8 Frédéric Demians 2012-07-20 07:33:51 UTC
> Is something missing in my test plan (comment 5), or must be more
> explained ?

My fault! I haven't read your test plan until the end.
Comment 9 Fridolin SOMERS 2012-07-20 07:39:47 UTC
No problem.
Feel free to ask for details.
Comment 10 Frédéric Demians 2012-07-20 07:42:00 UTC Comment hidden (obsolete)
Comment 11 Paul Poulain 2012-08-02 16:18:26 UTC
QA comments:
 * patch factorize many things, his size is mostly due to moving things
 * passes koha-qa.pl
 * very good test plan

Worth a gold "passed QA" !

Hélas ! my tests show there's a remaining problem:
* create an authority with a subfield repeated containing AA, BB, CC, DD
* save it, everything is OK
* modify it
 * click on the clone image on DD... you get
 AA, AA, BB, CC, DD (AA is duplicated, it should be DD)

(Checked for biblio, the problem is not here)

Please provide a follow-up to earn your gold passed QA
Comment 12 Fridolin SOMERS 2012-08-03 15:37:14 UTC Comment hidden (obsolete)
Comment 13 Jared Camins-Esakov 2012-08-27 17:34:43 UTC
(In reply to comment #12)
> + uses subfield '@' replacing by '00' behavior like in biblios.
> + fixedfield variable was missing in subfields, in both authities and
> biblios, => test it with a field < 010.

Is the change to the display in the bibliographic editor intentional? Personally, I preferred the old display because it felt more consistent, and I can't see that the new display gains us anything. I'll post a screenshot in a moment.
Comment 14 Jared Camins-Esakov 2012-08-27 17:37:15 UTC Comment hidden (obsolete)
Comment 15 Fridolin SOMERS 2012-08-28 08:59:07 UTC
(In reply to comment #13)
> Is the change to the display in the bibliographic editor intentional?
> Personally, I preferred the old display because it felt more consistent, and
> I can't see that the new display gains us anything. I'll post a screenshot
> in a moment.

I think authorities MARC editor should be as similar as possible that biblio MARC editor. That is why I made this changes.
Comment 16 Jared Camins-Esakov 2012-08-28 11:39:22 UTC
(In reply to comment #15)
> (In reply to comment #13)
> > Is the change to the display in the bibliographic editor intentional?
> > Personally, I preferred the old display because it felt more consistent, and
> > I can't see that the new display gains us anything. I'll post a screenshot
> > in a moment.
> 
> I think authorities MARC editor should be as similar as possible that biblio
> MARC editor. That is why I made this changes.

Why don't you change the authorities MARC editor to match the bibliographic editor, rather than changing both of them, then?
Comment 17 Fridolin SOMERS 2012-08-28 12:24:55 UTC
(In reply to comment #16) 
> Why don't you change the authorities MARC editor to match the bibliographic
> editor, rather than changing both of them, then?
I think it is a bug that template is using a param (subfield_loo.fixedfield) that does not exist.
Comment 18 Jared Camins-Esakov 2012-08-28 12:26:21 UTC
(In reply to comment #17)
> (In reply to comment #16) 
> > Why don't you change the authorities MARC editor to match the bibliographic
> > editor, rather than changing both of them, then?
> I think it is a bug that template is using a param (subfield_loo.fixedfield)
> that does not exist.

I see. I would sign off on the patch if it did not change the bib editor.
Comment 19 Fridolin SOMERS 2012-08-28 12:58:59 UTC Comment hidden (obsolete)
Comment 20 Fridolin SOMERS 2012-08-28 13:00:44 UTC
(In reply to comment #18)
> I see. I would sign off on the patch if it did not change the bib editor.
Your right, bib editor is not the point of this bug.
I removed from follow-up patch the part : 
  + fixedfield variable was missing in subfields, in both authities and biblios, => test it with a field < 010.
Comment 21 Kyle M Hall 2012-08-31 12:28:19 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2012-08-31 12:28:36 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2012-08-31 12:29:22 UTC
Tested with and without patch. Everything seems to working correctly post-patch.
Comment 24 Mason James 2012-09-14 12:06:12 UTC Comment hidden (obsolete)
Comment 25 Mason James 2012-09-14 12:13:38 UTC
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

fyi: i rebased the 1st patch - it had some conflicts, and a bad UTF header

now both(2) patches apply clean, passing QA...
awesome work everyone!

$ koha-qa.pl  -c 2

testing 2 commit(s) (applied to commit 0acebb8)
 * b3778c4 Bug 7455: Authority subfields are cloned in the wrong field (foll
      authorities/authorities.pl
      koha-tmpl/intranet-tmpl/prog/en/js/cataloging.js

 * 71f0ba4 Bug 7455: Authority subfields are cloned in the wrong field
      cataloguing/additem.pl
      koha-tmpl/intranet-tmpl/prog/en/css/addbiblio.css
      koha-tmpl/intranet-tmpl/prog/en/js/cataloging.js
      koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tt
      koha-tmpl/intranet-tmpl/prog/en/modules/authorities/blinddetail-biblio-search.tt
      koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
      koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt
      koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt
      koha-tmpl/intranet-tmpl/prog/en/modules/tools/batchMod-edit.tt
      tools/batchMod.pl

* authorities/authorities.pl                                               OK
* cataloguing/additem.pl                                                   OK
* tools/batchMod.pl                                                        OK
* koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tt       OK
* koha-tmpl/intranet-tmpl/prog/en/modules/authorities/blinddetail-biblio-seOK
* koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt         OK
* koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt           OK
* koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt          OK
* koha-tmpl/intranet-tmpl/prog/en/modules/tools/batchMod-edit.tt           OK
Comment 26 Paul Poulain 2012-09-14 15:28:56 UTC
mmm... Mason (& others) :
am I right if I say that http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11908 should not have been obsoleted, while one of the 2 follow-up should be ?

atm, the 2 patches attached:
 * are too small to be relevant
 * are the same
 * does not apply

so...
Comment 27 Fridolin SOMERS 2012-09-14 15:45:06 UTC
Comment on attachment 11908 [details] [review]
Bug 7455: Authority subfields are cloned in the wrong field

Main patch is not obsolete.
It is signed-off and passed QA.
Comment 28 Fridolin SOMERS 2012-09-14 15:48:58 UTC
Comment on attachment 11909 [details] [review]
Bug 7455: Authority subfields are cloned in the wrong field (follow-up)

Mason James, you forgot to put the existing follow-up patch on obsolete (u mixed with main patch I guess).
Comment 29 Paul Poulain 2012-09-17 17:08:43 UTC
The patch does not apply anymore, sorry:

CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tt


Please rebase & resubmit (and switch back to passed QA)
Comment 30 Fridolin SOMERS 2012-09-18 15:38:28 UTC Comment hidden (obsolete)
Comment 31 Fridolin SOMERS 2012-09-18 15:39:42 UTC Comment hidden (obsolete)
Comment 32 Fridolin SOMERS 2012-09-18 15:44:13 UTC Comment hidden (obsolete)
Comment 33 Fridolin SOMERS 2012-09-18 15:51:36 UTC
Please signoff follow-up 2 patch.

Simply check that the thesaurus popup contains a relationship section only when editing an authority, not when editing a biblio.
See Bug 8332.
Comment 34 Fridolin SOMERS 2012-09-27 08:37:09 UTC Comment hidden (obsolete)
Comment 35 Nicole C. Engard 2012-10-12 16:24:22 UTC
I was just about to test and sign off on this, but am not experiencing this behavior.  I tried in two different frameworks and repeated fields and subfields multiple times and everything stayed in the right place.  Can you confirm that this is still a patch that needs to be signed off on? Maybe another patch fixed it?

Nicole
Comment 36 Nicole C. Engard 2012-10-12 16:28:22 UTC
I tried this with hide_marc set both ways and with advancedMARCeditor set both ways - and it works just fine.

Nicole
Comment 37 Fridolin SOMERS 2012-10-15 08:53:30 UTC
(In reply to comment #35)
> I was just about to test and sign off on this, but am not experiencing this
> behavior.  I tried in two different frameworks and repeated fields and
> subfields multiple times and everything stayed in the right place.  Can you
> confirm that this is still a patch that needs to be signed off on? Maybe
> another patch fixed it?
> 
> Nicole

I tried with actual master 3.09.00.061, problem is easy to reproduce using test plan in first comment.
This problem occurs in authorities edition, not biblios.

Mainly, follow-up 2 and 3 need signoff.
Comment 38 Fridolin SOMERS 2012-10-15 09:00:59 UTC
(In reply to comment #36)
> I tried this with hide_marc set both ways and with advancedMARCeditor set
> both ways - and it works just fine.
> 
> Nicole
Can you signoff Bug 4381 and followup 3 then ?
Comment 39 Nicole C. Engard 2012-10-15 12:24:29 UTC
Is it possible that this is a UNIMARC only problem? Not MARC21? I did of course follow the first comment and looked at the screenshots, but do not have this problem at all.
Comment 40 Jared Camins-Esakov 2012-10-15 13:05:49 UTC
Could you please rebase and reattach follow-up 2? I get the following error when trying to apply it:
Applying: Bug 7455: Authority subfields are cloned in the wrong field (follow-up 2)
fatal: sha1 information is lacking or useless (authorities/authorities.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Comment 41 Fridolin SOMERS 2012-10-15 17:12:39 UTC Comment hidden (obsolete)
Comment 42 Fridolin SOMERS 2012-10-15 17:13:38 UTC Comment hidden (obsolete)
Comment 43 Jared Camins-Esakov 2012-10-21 14:44:09 UTC Comment hidden (obsolete)
Comment 44 Jared Camins-Esakov 2012-10-21 14:44:15 UTC Comment hidden (obsolete)
Comment 45 Jared Camins-Esakov 2012-10-21 14:44:18 UTC Comment hidden (obsolete)
Comment 46 Jared Camins-Esakov 2012-10-21 14:44:23 UTC Comment hidden (obsolete)
Comment 47 Paul Poulain 2012-10-24 15:28:19 UTC
This patch can be passed QA I think, but as I've string freezed for the 3.10 release, I fear pushing it because the translator tool could be confused by string moves
So I won't do anything for 3.10
If someone want to argue, feel free
Comment 48 Mason James 2012-11-01 09:47:11 UTC
(In reply to comment #47)
> This patch can be passed QA I think, but as I've string freezed for the 3.10
> release, I fear pushing it because the translator tool could be confused by
> string moves
> So I won't do anything for 3.10
> If someone want to argue, feel free

passing QA...  

i've done some good tests of this, everything worked OK
very impressive patch, well done!



$ koha-qa.pl -c 4

testing 4 commit(s) (applied to commit 00539c2)
 * 10a8d0a Bug 7455: Authority subfields are cloned in the wrong field (follow-up 3)
      koha-tmpl/intranet-tmpl/prog/en/js/cataloging.js

 * 3429d1c Bug 7455: Authority subfields are cloned in the wrong field (follow-up 2)
      authorities/authorities.pl
      cataloguing/addbiblio.pl
      koha-tmpl/intranet-tmpl/prog/en/js/cataloging.js
      koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tt

 * cd1ba90 Bug 7455: Authority subfields are cloned in the wrong field (follow-up)
      authorities/authorities.pl
      koha-tmpl/intranet-tmpl/prog/en/js/cataloging.js

 * 57a15f0 Bug 7455: Authority subfields are cloned in the wrong field
      cataloguing/additem.pl
      koha-tmpl/intranet-tmpl/prog/en/css/addbiblio.css
      koha-tmpl/intranet-tmpl/prog/en/js/cataloging.js
      koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tt
      koha-tmpl/intranet-tmpl/prog/en/modules/authorities/blinddetail-biblio-search.tt
      koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
      koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt
      koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt
      koha-tmpl/intranet-tmpl/prog/en/modules/tools/batchMod-edit.tt
      tools/batchMod.pl

* authorities/authorities.pl                                               OK
* cataloguing/addbiblio.pl                                                 OK
* cataloguing/additem.pl                                                   OK
* tools/batchMod.pl                                                        OK
* koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tt       OK
* koha-tmpl/intranet-tmpl/prog/en/modules/authorities/blinddetail-biblio-seOK
* koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt         OK
* koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt           OK
* koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt          OK
* koha-tmpl/intranet-tmpl/prog/en/modules/tools/batchMod-edit.tt           OK
Comment 49 Jared Camins-Esakov 2012-11-01 14:36:41 UTC
This patch has been pushed to master and is suitable for backporting to 3.10.
Comment 50 Paul Poulain 2012-11-01 21:47:51 UTC
Patch pushed to branch 3.10.x
I merged the branch, translated to french, and everything seems translated (at least I couldn't find any string in english)
Comment 51 Fridolin SOMERS 2013-04-05 13:05:07 UTC
Integrated in 3.10.0