Bug 3264

Summary: Uncloning a dropdown list in MARC authorities/biblio editor may clear all subfields (see comment 17)
Product: Koha Reporter: Chris Cormack <chris>
Component: CatalogingAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Ian Walls <koha.sekjal>
Severity: major    
Priority: P2 CC: dpavlin, eb, jcamins, m.de.rooy, nengard, paul.poulain
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5206
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Proposed patch
Patch
Bug 3264 UnCloneField() / minus button in MARC editor can clear all subfields (authorities AND biblio)

Description Chris Cormack 2010-05-21 01:08:27 UTC


---- Reported by rch@liblime.com 2009-05-27 23:06:30 ----

When EDITING and existing authority record:

UnCloneField(), aka 'the minus button' should clear contents from a subfield, and, if that subfield was not in the original record, it should remove the input for that subfield from the DOM.  However, when editing an existing record, the clear function incorrectly clears all subfields with data from the record, forcing the user to reload the authority record.

Steps to reproduce:
  * load an authority record into the editor.
  * Click on the '-' button in any subfield that has saved data in it.
  * See all the data in your record disappear.

This does not happen in the biblio editor, despite being copy-paste code.
I'm also seeing some bad behavior when adding and removing subfields from the DOM in the authorities editor that does not happen in the bibliographic editor (again, same code).

Second issue to reproduce:
  * pick an empty subfield in your record.
  * add some text to the input.
  * Clone the subfield three consecutive times.
  * Change the text in each of your four repeatable subfields so you can tell which one gets removed when you try to UnClone one.
  * UnClone the second subfield, and watch the first subfield disappear.

Ultimately, these functions should move to a library and not be duplicated in the two MARC editors.



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:08 UTC  ---

This bug was previously known as _bug_ 3264 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=3264

Actual time not defined. Setting to 0.0
The original reporter of this bug does not have
   an account here. Reassigning to the person who moved
   it here: chris@bigballofwax.co.nz.
   Previous reporter was rch@liblime.com.

Comment 1 Fridolin Somers 2010-07-20 15:53:36 UTC Comment hidden (obsolete)
Comment 2 Fridolin Somers 2010-07-20 15:59:22 UTC
Comment on attachment 2394 [details] [review]
Proposed patch

>Index: koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tmpl
>===================================================================
>--- koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tmpl
>+++ koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tmpl
>@@ -31,7 +31,7 @@
> 	 });
> 
> /**
>- * check if mandatory subfields are writed
>+ * check if mandatory subfields are written
>  */
> function AreMandatoriesNotOk(){
>     var mandatories = new Array();
>@@ -75,7 +75,6 @@
> 	    	for(var j=0,len2=eleminputs.length; j<len2; j++){
> 	
> 	    		if(eleminputs[j].name.match(varegexp) && eleminputs[j].value){
>-					/* tag_801_subfield_c_841304_545657 */
> 					inputregexp = new RegExp("^tag_" + arr[0] + "_subfield_" + eleminputs[j].value + "_" + arr[2]);
> 					
> 					for( var k=0; k<len2; k++){
>@@ -156,7 +155,7 @@
> 	newin=window.open("../authorities/auth_finder.pl?authtypecode="+  authtype+ "&index="+tagsubfieldid+"&value_mainstr="+encodeURI(mainmainstring)+"&value_main="+encodeURI(mainstring), "_blank",'width=700,height=550,toolbar=false,scrollbars=yes');
> }
> 
>-function AddField(field,cntrepeatfield) {
>+function AddField(field,cntrepeatfield) { // TODO unused
>     document.forms['f'].op.value = "addfield";
>     document.forms['f'].addfield_field.value=field;
>     document.forms['f'].repeat_field.value=cntrepeatfield;
>@@ -179,7 +178,7 @@
> }
> 
> /**
>- * To clone a field or a subfield by clickink on '+' button
>+ * To clone a field or a subfield by clicking on '+' button
>  */ 
> function CloneField(index) {
>     var original = document.getElementById(index); //original <div>
>@@ -261,7 +260,7 @@
>             // button ...
>             var spans=0;
>             try {
>-                spans = divs[i].getElementsByTagName('span');
>+                spans = divs[i].getElementsByTagName('a');
>             } catch(e) {
>                 // no spans
>             }
>@@ -317,7 +316,7 @@
>                 
>                 var CloneButtonPlus;
>                 try {
>-                    var anchors = divs[i].getElementsByTagName('span');
>+                    var anchors = divs[i].getElementsByTagName('a');
>                     for (var j = 0; j < anchors.length; j++) {
>                         if (anchors[j].getAttribute('class') == 'buttonPlus') {
>                             anchors[j].setAttribute('onclick',"CloneField('" + new_id + "')");
>@@ -537,7 +536,7 @@
> <!-- TMPL_ELSE -->
> <h1>Adding authority <!-- TMPL_VAR name="authtypetext" --></h1>
> <!-- /TMPL_IF -->  
>-<form method="post" name="f" action="/cgi-bin/koha/authorities/authorities.pl">
>+<form method="post" name="f" id="f" action="/cgi-bin/koha/authorities/authorities.pl">
>     <input type="hidden" name="op" value="add" />
>     <input type="hidden" name="addfield_field" value="" />
>     <input type="hidden" name="repeat_field" value="" />
>@@ -658,14 +657,12 @@
>                 <!-- /TMPL_UNLESS -->
>     
>                 <!-- TMPL_UNLESS NAME="advancedMARCEditor" -->
>-                    <a onclick="ExpandField('tag_<!-- TMPL_VAR name="tag"-->_<!-- TMPL_VAR NAME='index'--><!-- TMPL_VAR name="random" -->')"><!-- TMPL_VAR NAME="tag_lib" --></a>
>+	            	<a href="#" tabindex="1" class="expandfield" onclick="ExpandField('tag_<!-- TMPL_VAR name="tag"-->_<!-- TMPL_VAR NAME='index'--><!-- TMPL_VAR name="random" -->'); return false;" title="Click to Expand this Tag"><!-- TMPL_VAR NAME="tag_lib" --></a>
>                 <!-- /TMPL_UNLESS -->
>                 <!-- TMPL_IF name="repeatable" -->
>-                    <span class="buttonPlus" onclick="CloneField('tag_<!-- TMPL_VAR name="tag"-->_<!-- TMPL_VAR NAME='index'--><!-- TMPL_VAR name="random" -->')">+</span>
>+                	<a href="#" tabindex="1" class="buttonPlus" onclick="CloneField('tag_<!-- TMPL_VAR name="tag"-->_<!-- TMPL_VAR NAME='index'--><!-- TMPL_VAR name="random" -->'); return false;" title="Repeat this Tag">+</a>
>+                	<a href="#" tabindex="1" class="buttonMinus" onclick="UnCloneField('tag_<!-- TMPL_VAR name="tag"-->_<!-- TMPL_VAR NAME='index'--><!-- TMPL_VAR name="random" -->'); return false;" title="Delete this Tag">&#8722;</a>
>                 <!-- /TMPL_IF -->
>-                <!-- TMPL_UNLESS NAME="mandatory" -->
>-                    <span class="buttonMinus" onclick="UnCloneField('tag_<!-- TMPL_VAR name="tag"-->_<!-- TMPL_VAR NAME='index'--><!-- TMPL_VAR name="random" -->')">&#8722;</span>
>-                <!-- /TMPL_UNLESS -->
>                 
>             </div>
>     
>@@ -679,7 +676,7 @@
>                     <!-- /TMPL_UNLESS --> 
>                     
>                     <!-- TMPL_UNLESS name="hide_marc" -->
>-                    <!-- TMPL_IF NAME="fixedfield" -->
>+                    <span class="subfieldcode"><!-- TMPL_IF NAME="fixedfield" -->
>                         <img class="buttonUp" style="display:none;" src="<!-- TMPL_VAR NAME="themelang" -->/../img/up.png" onclick="upSubfield('subfield<!-- TMPL_VAR NAME='tag' --><!-- TMPL_VAR NAME='subfield' -->')" alt="Move Up" title="Move Up" />
>                     <!-- TMPL_ELSE -->
>                         <img class="buttonUp"  src="<!-- TMPL_VAR NAME="themelang" -->/../img/up.png" onclick="upSubfield('subfield<!-- TMPL_VAR NAME='tag' --><!-- TMPL_VAR NAME='subfield' -->')" alt="Move Up" title="Move Up" />
>@@ -692,27 +689,28 @@
>                                 maxlength="1"
>                                 class="flat"
>                                 tabindex="0" />
>+                    </span>
>                     <!-- TMPL_ELSE -->
>                         <input type="hidden"
>                             name="tag_<!-- TMPL_VAR NAME='tag'-->_code_<!--  TMPL_VAR NAME='subfield'-->_<!-- TMPL_VAR NAME='index'-->_<!-- TMPL_VAR NAME='index_subfield'-->"
>                             value="<!-- TMPL_VAR NAME="subfield" -->"/>
>+                    </span>
>                     <!-- /TMPL_UNLESS -->
>                 
>                     <!-- TMPL_UNLESS NAME="advancedMARCEditor" -->
>-                        <!-- TMPL_IF name="mandatory" --><span class="subfield_mandatory"><!-- /TMPL_IF -->
>-                            <!-- TMPL_VAR NAME="marc_lib" -->
>-                        <!-- TMPL_IF name="mandatory" --></span><!-- /TMPL_IF -->
>+                        <!-- TMPL_IF name="mandatory" --><span class="subfield subfield_mandatory"><!-- TMPL_ELSE --><span class="subfield"><!-- /TMPL_IF -->
>+                        <!-- TMPL_VAR NAME="marc_lib_plain" -->
>+                        <!-- TMPL_IF NAME="mandatory" --><span class="mandatory_marker" title="This field is mandatory">*</span><!-- /TMPL_IF -->
>+                        </span>
>                         </label>
>                     <!-- /TMPL_UNLESS -->
>                     
>                     <!-- TMPL_VAR NAME="marc_value" -->
>                     
>                     <!-- TMPL_IF NAME="repeatable" -->
>-                        <span class="buttonPlus" onclick="CloneSubfield('subfield<!-- TMPL_VAR NAME='tag' --><!-- TMPL_VAR NAME='subfield' -->')">+</span>
>-                    <!-- /TMPL_IF -->
>-                    <!-- TMPL_UNLESS NAME="mandatory" -->
>+                        <span class="buttonPlus" onclick="CloneSubfield('subfield<!-- TMPL_VAR NAME='tag' --><!-- TMPL_VAR NAME='subfield' --><!-- TMPL_VAR name="random" -->')">+</span>
>                         <span class="buttonMinus" onclick="UnCloneField('subfield<!-- TMPL_VAR NAME='tag' --><!-- TMPL_VAR NAME='subfield' --><!-- TMPL_VAR name="random" -->')">&#8722;</span>
>-                    <!-- /TMPL_UNLESS -->
>+                    <!-- /TMPL_IF -->
>                     
>                 </div>
>                 <!-- End of the line -->
>@@ -749,7 +747,7 @@
>     <input type="hidden" name="subfield" value="<!-- TMPL_VAR NAME="oldauthtypetagsubfield" -->" />
>     <input type="hidden" name="field_value" value="<!-- TMPL_VAR NAME="authtypecode" -->" />
> <!-- /TMPL_IF -->
>-<fieldset class="action">
>+<fieldset class="action" id="action" >
>         <input type="button" id="addauth2" value="Save" onclick="Check(this.form)" accesskey="w" />
> </fieldset>
> </form>
Comment 3 Owen Leonard 2010-08-10 19:15:13 UTC
I was able to duplicate the second issue listed above, but not the first.

I tried to test the patch attached here but it would not apply.

If anyone decides to do a low-level rewrite of these clone and unclone functions, better care should be taken with regard to performance. Testing with Firefox I find that one of these operations can take up to four seconds.
Comment 4 Nicole C. Engard 2010-09-08 15:02:25 UTC
I can confirm that this is still an issue in 3.01.00.145
Comment 5 Marcel de Rooy 2012-01-19 09:33:23 UTC
*** Bug 5707 has been marked as a duplicate of this bug. ***
Comment 6 Marcel de Rooy 2012-01-19 09:35:30 UTC
Patch forthcoming..
Comment 7 Marcel de Rooy 2012-01-19 09:56:48 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2012-01-19 10:02:12 UTC
TEST PLAN:

First, check without this patch in biblio editor or authority editor:
Edit a field say 700.
Make sure two subfields are filled.
Empty/unclone the first subfield. Leave the second subfield below.
Save the record.
The subfields below the emptied subfield are GONE!

Now, with this patch do the same.
Verify if the subfields below the deleted one are still there.

Further testing can be done by adding multiple subfields, saving and removing several subfields while checking your marc record.
Comment 9 Jared Camins-Esakov 2012-01-22 15:02:17 UTC
I cannot duplicate this bug. Can you confirm that this is still a problem on master? And if it is possibly attach a video showing the problem, so that I can see what I'm doing wrong?
Comment 10 Marcel de Rooy 2012-01-23 07:36:12 UTC
Reproduce this problem in current master with for example:

1) Goto MARC biblio editor. Open a record and add a new field say 700.
[Note that the problem also occurs for existing fields.]
a) Fill 700-4 with a relator, 700-a with a name (if attached to authority, then 700-9 is filled too) and 700-d with a year.
b) Save.

2) Edit this record again. Goto the newly added 700.
a) Unclone the 700-4 subfield that is listed first. 
[The position of the subfield is crucial !! ]
b) Verify that you still have 700-a (evt. 700-9) and 700-d. 
c) Save.

3) Edit again. Check the 700 field.
Note that the subfields 700-4, 700-a (pos. 700-9) and 700-d are GONE too. The whole field 700 is erased insteaf of only 700-4. This is resolved by the submitted patch.
Comment 11 Jared Camins-Esakov 2012-01-28 13:49:11 UTC
I still cannot duplicate this problem on Master. What web browser are you using? I tried in both Firefox and Chrome on Mac.
Comment 12 Marcel de Rooy 2012-01-30 08:50:43 UTC
(In reply to comment #11)
> I still cannot duplicate this problem on Master. What web browser are you
> using? I tried in both Firefox and Chrome on Mac.
Reproduced it again under current master, with Firefox (9.0.1) Followed the steps under comment 10.
Comment 13 Marcel de Rooy 2012-01-30 09:55:06 UTC
Jared:
Chris also tested under Chromium and could not reproduce it either.
The problem will only occur if the browser "decides" not to send empty params (like for the remaining empty $4 subfield in my example). If the expected order of params is influenced like that, the TransformHtmlToMarc function stumbles over it. Your browser apparently sends the empty param. In that case no problem (with or without the patch).

Which Firefox version did you test, and on which platform?
I used Firefox 9 under Windows 7. And if i remember correctly, the combination Firefox/Vista also reproduced the error.
Comment 14 Jared Camins-Esakov 2012-01-30 13:01:16 UTC
I have tested on the following web browsers:

Mac OS X 10.6.8:
Chrome 16.0.912.77
Firefox 9.0.1

Windows 7:
Firefox 3.6.3
Firefox 9.0.1
IE 8.0.7600.16385

Ubuntu 11.10
Firefox 8.0
Chromium 15.0.874.106 (Developer Build 107270 Linux)

On none of them could I duplicate this problem. I hereby turn the testing baton over to someone else. If I can't duplicate the problem, I can't confirm the fix.
Comment 15 Marcel de Rooy 2012-01-30 13:05:50 UTC
Great testing! But strange, that you could not duplicate on FF 9. Can we make sure that we tested the same thing?
Comment 16 Jared Camins-Esakov 2012-01-30 13:16:23 UTC
I just took a screen capture of my process. You can see it at http://screencast.com/t/AzqLbFL3s6m
Comment 17 Marcel de Rooy 2012-01-30 14:00:50 UTC
(In reply to comment #16)
> I just took a screen capture of my process. You can see it at
> http://screencast.com/t/AzqLbFL3s6m

Looking again, I discovered that we had a small but apparently (very) important difference: my $4 is controlled by an authorized value (combo box). 
Looking further, it appears to me now that only the empty SELECTs are not passed to the server. Explaining too why this problem has been there for such a long time.
Big sorry for letting you test without that piece of information! Could you please e.g. add an authorized value to the relator code and unclone?
Please test this on Firefox/Windows(?); it may still be browser specific. Could not find much about that..
Comment 18 Jared Camins-Esakov 2012-01-30 16:35:07 UTC
Even with the authorized value, I could not replicate with Windows 7 and Firefox 9.
Comment 19 Marcel de Rooy 2012-02-06 11:11:21 UTC
I debugged the code again to show what happens:

[Mon Feb 06 11:42:05 2012] [error] [client 192.87.126.61] [Mon Feb  6 11:42:05 2012] addbiblio.pl: XX3: tag_700_code_b_218275_335975 tag_700_subfield_b_218275_335975 at /usr/share/koha/testclone/C4/Biblio.pm line 2186., referer: http://libdevelop.rijksmuseum.nl:8009/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=144200&amp;amp;op=
[Mon Feb 06 11:42:05 2012] [error] [client 192.87.126.61] [Mon Feb  6 11:42:05 2012] addbiblio.pl: XX3: tag_700_code_d_218275_950598 tag_700_subfield_d_218275_950598 at /usr/share/koha/testclone/C4/Biblio.pm line 2186., referer: http://libdevelop.rijksmuseum.nl:8009/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=144200&amp;amp;op=
[Mon Feb 06 11:42:05 2012] [error] [client 192.87.126.61] [Mon Feb  6 11:42:05 2012] addbiblio.pl: XX3: tag_700_code_4_218275_815299 tag_700_code_9_218275_680813 at /usr/share/koha/testclone/C4/Biblio.pm line 2186., referer: http://libdevelop.rijksmuseum.nl:8009/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=144200&amp;amp;op=
[Mon Feb 06 11:42:05 2012] [error] [client 192.87.126.61] [Mon Feb  6 11:42:05 2012] addbiblio.pl: XX4: Leaving while at /usr/share/koha/testclone/C4/Biblio.pm line 2199., referer: http://libdevelop.rijksmuseum.nl:8009/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=144200&amp;amp;op=

As you can see, code b is followed by subfield b. That's fine. Code d fine too. But code 4 is immediately followed by code 9! The uncloned subfield 4 is not in the params array!! [[After UncloneField set the selectedIndex to -1, the corresponding parameter does not reach TransformHtmlToMarc; the browser does not pass it or CGI does ignore it.]] After code 4 the script exits the while loop, skipping remaining subfields.
Comment 20 Paul Poulain 2012-02-15 16:55:25 UTC
(In reply to comment #14)
> On none of them could I duplicate this problem. I hereby turn the testing baton
> over to someone else. If I can't duplicate the problem, I can't confirm the
> fix.

Well, my position here would be to say: if this fix breaks nothing for the rest for others, and fixes the problem for Marcel, then we can apply it.

(In reply to comment #17)
> Looking again, I discovered that we had a small but apparently (very) important
> difference: my $4 is controlled by an authorized value (combo box). 
> Looking further, it appears to me now that only the empty SELECTs are not
> passed to the server. Explaining too why this problem has been there for such a
> long time.
> Big sorry for letting you test without that piece of information! Could you
> please e.g. add an authorized value to the relator code and unclone?
> Please test this on Firefox/Windows(?); it may still be browser specific. Could
> not find much about that..

I confuselly remember we had the same kind of problem many months ago with something that worked perfectly on chrome, and was not working on Firefox. It was for AixMarseille university. And it was related to an empty select too iirc. If I remember more I'll tell you here.
Comment 21 Jared Camins-Esakov 2012-02-15 17:05:04 UTC
Created attachment 7661 [details] [review]
Bug 3264 UnCloneField() / minus button in MARC editor can clear all subfields (authorities AND biblio)

All subfields following the removed subfield were not saved.
Problem is in C4/Biblio routine TransformHtmlToMarc.
If the field is emptied, the param list contains a code param but no subfield
param. The while loop handling the subfields could not handle that. Also added
a FIXME because the whole routine depends on an assumption about the order of
cgi parameters that is not strictly guaranteed.

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
I was unable to replicate the problem, but can confirm that the patch does
not break anything under any of the following platforms/browsers:
Mac OS X 10.6.8:
Chrome 16.0.912.77
Firefox 9.0.1

Windows 7:
Firefox 3.6.3
Firefox 9.0.1
IE 8.0.7600.16385

Ubuntu 11.10
Firefox 8.0
Chromium 15.0.874.106 (Developer Build 107270 Linux)
Comment 22 Ian Walls 2012-02-16 16:44:11 UTC
This patch changes loop handling in a single C4::Biblio subroutine, which does keep it's scope narrow.  Testing shows that this has not broken anything, even though the problem could not be replicated by the signer-off.  Adds numerous comments about what's going on in this while loop, which will make any subsequent revisions easier.

Marking as Passed QA.
Comment 23 Paul Poulain 2012-02-16 18:00:16 UTC
OK, now I remember: it was when AixMarseille univ tested bulk item modifications. We discovered that, for firefox, no value selected in a SELECT was ignored in the array returned if the select was an array, while chrome was returning a blank.

Example:
choose <select name="sel" values="a list including empty|option1|option2"><input name="val" value="value1">
choose <select name="sel" values="a list including empty|option1|option2"><input name="val" value="value2">
choose <select name="sel" values="a list including empty|option1|option2"><input name="val" value="value3">

If the user select empty/option1/option1 in the select, the arrays in perl, if you call the page from chrome was:
@sel = ('','option1','option1')
@val = ('value1','value2','value3')
so $sel[x] is related to $val[x]

but if you run from FF, you get
@sel = ('option1','option1')
@val = ('value1','value2','value3')
so trying to match $sel[x] with $val[x] result in an epic fail !!!

This patch address another case of this strange behaviour