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)
Created attachment 7257 [details] 1) Example : a repeatable field, with repeatable subfields
Created attachment 7258 [details] 2) Click to clone a subfield
Created attachment 7259 [details] 3) The subfield is not created in the selected field
Created attachment 10884 [details] [review] Proposed patch Proposed patch fixes several bugs in authority edition page. I also worked for authority edition page to be maximum similar to biblio edition page. I merged common javascript code into "cataloguing.js" file. It contains all cloning methods. Therefore, I also merged some javascript code from item edition (additem.tt, serials-edit.tt, batchMod-edit.tt). - I've add the type "text" on inputs that did not add this attribute (it is useful for js and is mandatory in XHTML). - I changed the code that tests if one is trying to remove the last field : I used JQuery instead of "fields_in_use" var. - I add cloning little images instead of "+" and "-". - Patch fixes a bug on indicators cloning in both biblio and authorities edition. Enjoy.
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.
+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?
Is something missing in my test plan (comment 5), or must be more explained ?
> 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.
No problem. Feel free to ask for details.
Created attachment 11032 [details] [review] Signed-off patch I can repeat the bug, and confirm the patch fix it. This patch aggregate common JavaScript functions coming from various templates into one place, which is valuable for maintainability.
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
Created attachment 11338 [details] [review] Proposed patch (follow-up) Asked patch. Fixes cloning problem. + 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. + fixes a missing random in id of indicator div.
(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.
Created attachment 11850 [details] Problematic bib editor layout
(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.
(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?
(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.
(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.
Created attachment 11866 [details] [review] Proposed patch (follow-up) (revised)
(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.
Created attachment 11908 [details] [review] Bug 7455: Authority subfields are cloned in the wrong field Signed-off-by: Frédéric Demians <f.demians@tamil.fr> I can repeat the bug, and confirm the patch fix it. This patch aggregate common JavaScript functions coming from various templates into one place, which is valuable for maintainability. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 11909 [details] [review] Bug 7455: Authority subfields are cloned in the wrong field (follow-up) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Tested with and without patch. Everything seems to working correctly post-patch.
Created attachment 12216 [details] [review] Bug 7455: Authority subfields are cloned in the wrong field (follow-up) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
> 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
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 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 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).
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)
Created attachment 12313 [details] [review] Proposed patch (rebased master) Main patch rebased master.
Created attachment 12314 [details] [review] Proposed patch (follow-up 1) (rebased master) First follow-up rebased master.
Created attachment 12315 [details] [review] Proposed patch (follow-up 2) After rebase of main patch, I noticed that bug 8332 has introduced a behavior I had to manage. So here is second follow-up. Note : 8332 introduces a source='auth' parameter in openAuth() js function only when called from authorities.tt. Since I moved this method into cataloguing.js to be mutualized between authorities et bilios edition, I added a source argument in this method.
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.
Created attachment 12546 [details] [review] Proposed patch (follow-up 3) follow-up 3 : A small correction made in Bug 4381 must be integrated since it concerns field cloning.
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 this with hide_marc set both ways and with advancedMARCeditor set both ways - and it works just fine. Nicole
(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.
(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 ?
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.
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.
Created attachment 12827 [details] [review] Proposed patch (follow-up 2) (rebased master) follow-up 2 rebased master
Created attachment 12828 [details] [review] Proposed patch (follow-up 3) (rebased master) follow-up 3 rebased master
Created attachment 12982 [details] [review] Bug 7455: Authority subfields are cloned in the wrong field Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 12983 [details] [review] Bug 7455: Authority subfields are cloned in the wrong field (follow-up) Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 12984 [details] [review] Bug 7455: Authority subfields are cloned in the wrong field (follow-up 2) Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 12985 [details] [review] Bug 7455: Authority subfields are cloned in the wrong field (follow-up 3) Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
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
(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
This patch has been pushed to master and is suitable for backporting to 3.10.
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)
Integrated in 3.10.0