Bug 17817 - Repeat this Tag (cloning) not working
Summary: Repeat this Tag (cloning) not working
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All Linux
: P5 - low critical (vote)
Assignee: Patricio Marrone
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 17818 17863 (view as bug list)
Depends on: 13501
Blocks: 17725
  Show dependency treegraph
 
Reported: 2016-12-27 18:12 UTC by José Anjos
Modified: 2017-01-27 09:06 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17817: Temporarily remove select2 where is is used (2.92 KB, patch)
2016-12-28 10:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
[DO NOT PUSH] Bug 17817: This is just a try (1.58 KB, patch)
2016-12-28 10:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17817: Temporarily remove select2 where is is used (2.98 KB, patch)
2017-01-03 18:49 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 17817: Fix cloning subfields using select2 (2.59 KB, patch)
2017-01-11 14:21 UTC, Patricio Marrone
Details | Diff | Splinter Review
Bug 17817: Fix cloning subfields using select2 (3.12 KB, patch)
2017-01-11 15:49 UTC, Patricio Marrone
Details | Diff | Splinter Review
Bug 17817: (Follow up) Fix reordering subfields issues (1.28 KB, patch)
2017-01-11 15:49 UTC, Patricio Marrone
Details | Diff | Splinter Review
Bug 17817: Fix cloning subfields using select2 (3.17 KB, patch)
2017-01-11 16:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17817: (Follow up) Fix reordering subfields issues (1.35 KB, patch)
2017-01-11 16:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17817: Fix cloning subfields using select2 (4.71 KB, patch)
2017-01-11 19:50 UTC, Patricio Marrone
Details | Diff | Splinter Review
Bug 17817: (Follow up) Fix reordering subfields issues (1.23 KB, patch)
2017-01-11 19:50 UTC, Patricio Marrone
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17817: Fix cloning subfields using select2 (4.78 KB, patch)
2017-01-11 21:10 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17817: Fix cloning subfields using select2 (4.78 KB, patch)
2017-01-11 21:10 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17817: (Follow up) Fix reordering subfields issues (1.30 KB, patch)
2017-01-11 21:11 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17817: Fix cloning subfields using select2 (4.95 KB, patch)
2017-01-12 08:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17817: (Follow up) Fix reordering subfields issues (1.36 KB, patch)
2017-01-12 08:52 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description José Anjos 2016-12-27 18:12:12 UTC
Full details here:
http://koha.1045719.n5.nabble.com/Repeat-this-Tag-not-working-td5914615.html

Description:
I've setup a completely new VM:
Debian GNU/Linux 8.6 (jessie)
MariaDB  Ver 14.14 Distrib 5.5.53
Apache/2.4.10 (Debian)

Installed the defaults in fr-FR

Choose Cataloging -> New Record -> Default Framework
Filed the mandatory fields.
If I try to duplicate 701 it will jump to the top of the page
If I try to duplicate 711 I have no problem.

Conclusion is: If I try to duplicate any field that contains a Drop Menu (with authorized values) it will jump to the top of page. If there is no Drop Menu it will clone ok.
If I choose a value from the Authorized Values, than In can duplicate it.
But, when I save it, that fields are not saved.
Comment 1 Jonathan Druart 2016-12-28 10:37:24 UTC
Created attachment 58464 [details] [review]
Bug 17817:  Temporarily remove select2 where is is used

If you try to clone a field (or subfield) which contains at least a
subfield linked to authorized values, you will get a JS error and the
field will not be cloned.

I have failed trying to propose a fix in a reasonable delay so I prefer
to simply deactivate the feature.

Test plan:
Edit a bibliographic record, an item and use the batch item modification
tool.
You should see simple selects
Comment 2 Jonathan Druart 2016-12-28 10:40:28 UTC
Created attachment 58465 [details] [review]
[DO NOT PUSH] Bug 17817: This is just a try

This does not work correctly:
- width is wrongly calculated
- "Move up" raises a JS error
Comment 3 Owen Leonard 2017-01-03 18:49:55 UTC
Created attachment 58579 [details] [review]
[SIGNED-OFF] Bug 17817: Temporarily remove select2 where is is used

If you try to clone a field (or subfield) which contains at least a
subfield linked to authorized values, you will get a JS error and the
field will not be cloned.

I have failed trying to propose a fix in a reasonable delay so I prefer
to simply deactivate the feature.

Test plan:
Edit a bibliographic record, an item and use the batch item modification
tool.
You should see simple selects

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 4 Jonathan Druart 2017-01-09 09:01:04 UTC
*** Bug 17863 has been marked as a duplicate of this bug. ***
Comment 5 Patricio Marrone 2017-01-11 14:21:20 UTC
Created attachment 58817 [details] [review]
Bug 17817: Fix cloning subfields using select2

Based on Jonathan's patch (the DO NOT PUSH one), I put together this fix.
What was changed is that select2 is reinitialized only after the cloned element
has been appended to the DOM (so that select2 can correctly calculate the field's
width). Also, I changed the selectors that searched for the line divs (for reordering)
and for the subfield's input element (for erasing the field's value) to be more specific,
since select2 introduced divs that broke some assumptions about the expected HTML structure
Comment 6 Jonathan Druart 2017-01-11 14:32:08 UTC
(In reply to Patricio Marrone from comment #5)

Hi Patricio,
Can you confirm that your patch fixes the following:
 - width is wrongly calculated
 - "Move up" raises a JS error
?
Comment 7 Patricio Marrone 2017-01-11 14:50:17 UTC
(In reply to Jonathan Druart from comment #6)
> (In reply to Patricio Marrone from comment #5)
> 
> Hi Patricio,
> Can you confirm that your patch fixes the following:
>  - width is wrongly calculated
>  - "Move up" raises a JS error
> ?

Hi Jonathan! Short answer: yes. 
I checked that
- The width is the same as the one from the cloned line. This was fixed by first
  appending the cloned element to the dom and then reactivating select2, so that
  it can calculate the width.
- Moving up should work. select2 introduces some divs which broke the present
  assumptions that all divs were lines. I added more specific selectors to
  choose what's a line and what's not. This could use some regression testing to
  check that other kind of fields still work correctly (that is, textfields,
  textareas...). I did test that they worked to the limits of my not very extensive
  Koha knowledge.
Comment 8 Tomás Cohen Arazi 2017-01-11 15:03:30 UTC
There is a regression on moving subfields up to the top.
Comment 9 Patricio Marrone 2017-01-11 15:49:39 UTC
Created attachment 58823 [details] [review]
Bug 17817: Fix cloning subfields using select2

Based on Jonathan's patch (the DO NOT PUSH one), I put together this fix.
What was changed is that select2 is reinitialized only after the cloned element
has been appended to the DOM (so that select2 can correctly calculate the field's
width). Also, I changed the selectors that searched for the line divs (for reordering)
and for the subfield's input element (for erasing the field's value) to be more specific,
since select2 introduced divs that broke some assumptions about the expected HTML structure
Comment 10 Patricio Marrone 2017-01-11 15:49:51 UTC
Created attachment 58824 [details] [review]
Bug 17817: (Follow up) Fix reordering subfields issues

Authority controlled subfields have invisible divs which produced
a strange behavior when reordering (multiple clicks were needed
to push a subfield up over an authority controlled subfield)
Comment 11 Tomás Cohen Arazi 2017-01-11 16:07:05 UTC
Created attachment 58827 [details] [review]
Bug 17817: Fix cloning subfields using select2

Based on Jonathan's patch (the DO NOT PUSH one), I put together this fix.
What was changed is that select2 is reinitialized only after the cloned element
has been appended to the DOM (so that select2 can correctly calculate the field's
width). Also, I changed the selectors that searched for the line divs (for reordering)
and for the subfield's input element (for erasing the field's value) to be more specific,
since select2 introduced divs that broke some assumptions about the expected HTML structure

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2017-01-11 16:07:17 UTC
Created attachment 58828 [details] [review]
Bug 17817: (Follow up) Fix reordering subfields issues

Authority controlled subfields have invisible divs which produced
a strange behavior when reordering (multiple clicks were needed
to push a subfield up over an authority controlled subfield)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Tomás Cohen Arazi 2017-01-11 16:07:55 UTC
Works as expected. Nice catch with reordering!
Comment 14 Jonathan Druart 2017-01-11 17:59:39 UTC
Still 2 regressions:
1/ On the add item form, it does not work
2/ Try to clone a whole field containing select2: TypeError: divs[i].getAttribute(...) is null

You know, I really think that we should go first with my patch, then try to fix it on another bug report. This is critical as it impacts stable releases.
Comment 15 Patricio Marrone 2017-01-11 19:50:12 UTC
Created attachment 58844 [details] [review]
Bug 17817: Fix cloning subfields using select2

Based on Jonathan's patch (the DO NOT PUSH one), I put together this fix.
What was changed is that select2 is reinitialized only after the cloned element
has been appended to the DOM (so that select2 can correctly calculate the field's
width). Also, I changed the selectors that searched for the line divs (for reordering)
and for the subfield's input element (for erasing the field's value) to be more specific,
since select2 introduced divs that broke some assumptions about the expected HTML structure
Comment 16 Patricio Marrone 2017-01-11 19:50:32 UTC
Created attachment 58845 [details] [review]
Bug 17817: (Follow up) Fix reordering subfields issues

Authority controlled subfields have invisible divs which produced
a strange behavior when reordering (multiple clicks were needed
to push a subfield up over an authority controlled subfield)
Comment 17 Patricio Marrone 2017-01-11 19:57:50 UTC
I added fixes to the regressions discovered by Jonathan. Are there more places where these select2 fields need to be cloned?
Comment 18 Josef Moravec 2017-01-11 21:10:12 UTC
Created attachment 58847 [details] [review]
[SIGNED-OFF] Bug 17817: Fix cloning subfields using select2

Based on Jonathan's patch (the DO NOT PUSH one), I put together this fix.
What was changed is that select2 is reinitialized only after the cloned element
has been appended to the DOM (so that select2 can correctly calculate the field's
width). Also, I changed the selectors that searched for the line divs (for reordering)
and for the subfield's input element (for erasing the field's value) to be more specific,
since select2 introduced divs that broke some assumptions about the expected HTML structure

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 19 Josef Moravec 2017-01-11 21:10:51 UTC
Created attachment 58848 [details] [review]
[SIGNED-OFF] Bug 17817: Fix cloning subfields using select2

Based on Jonathan's patch (the DO NOT PUSH one), I put together this fix.
What was changed is that select2 is reinitialized only after the cloned element
has been appended to the DOM (so that select2 can correctly calculate the field's
width). Also, I changed the selectors that searched for the line divs (for reordering)
and for the subfield's input element (for erasing the field's value) to be more specific,
since select2 introduced divs that broke some assumptions about the expected HTML structure

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 20 Josef Moravec 2017-01-11 21:11:12 UTC
Created attachment 58849 [details] [review]
[SIGNED-OFF] Bug 17817: (Follow up) Fix reordering subfields issues

Authority controlled subfields have invisible divs which produced
a strange behavior when reordering (multiple clicks were needed
to push a subfield up over an authority controlled subfield)

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 21 Jonathan Druart 2017-01-12 08:52:12 UTC
Created attachment 58863 [details] [review]
Bug 17817: Fix cloning subfields using select2

Based on Jonathan's patch (the DO NOT PUSH one), I put together this fix.
What was changed is that select2 is reinitialized only after the cloned element
has been appended to the DOM (so that select2 can correctly calculate the field's
width). Also, I changed the selectors that searched for the line divs (for reordering)
and for the subfield's input element (for erasing the field's value) to be more specific,
since select2 introduced divs that broke some assumptions about the expected HTML structure

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
I confirm that these 2 patches fix the add item and mod biblio views as
well as the batch item modification tools.
Comment 22 Jonathan Druart 2017-01-12 08:52:18 UTC
Created attachment 58864 [details] [review]
Bug 17817: (Follow up) Fix reordering subfields issues

Authority controlled subfields have invisible divs which produced
a strange behavior when reordering (multiple clicks were needed
to push a subfield up over an authority controlled subfield)

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 23 Jonathan Druart 2017-01-12 08:58:28 UTC
Great job Patricio!
Do not forget to add a test plan, for you next patches :)
Comment 24 Kyle M Hall 2017-01-13 11:25:16 UTC
Pushed to master for 17.05, thanks Patricia!
Comment 25 Kyle M Hall 2017-01-13 11:43:10 UTC
Apologies, thanks Patricio!
Comment 26 Patricio Marrone 2017-01-13 11:47:05 UTC
(In reply to Jonathan Druart from comment #23)
> Great job Patricio!
> Do not forget to add a test plan, for you next patches :)

Thanks Jonathan :) . I'm glad to help. I'll try to put together the steps needed to test the changes and won't forget to add them next time.
Comment 27 Patricio Marrone 2017-01-13 11:49:06 UTC
(In reply to Kyle M Hall from comment #24)
> Pushed to master for 17.05, thanks Patricia!

(In reply to Kyle M Hall from comment #25)
> Apologies, thanks Patricio!

Thanks Kyle! No problem :)
Comment 28 Katrin Fischer 2017-01-15 16:13:40 UTC
These patches have been pushed to 16.11.x and will be in 16.11.02.
Comment 29 Mason James 2017-01-25 00:28:25 UTC
Blocked by enhancement, skipping for 16.05.x
Comment 30 Jonathan Druart 2017-01-26 12:42:24 UTC
*** Bug 17818 has been marked as a duplicate of this bug. ***
Comment 31 angelo 2017-01-26 13:56:19 UTC
In the same bug there is another problem. When we try to add an authority in field 701 702, that is, where a field with authorized value exists, it does not add the authority that exists, it creates another authority id.
Comment 32 Jonathan Druart 2017-01-27 09:06:00 UTC
(In reply to angelo from comment #31)
> In the same bug there is another problem. When we try to add an authority in
> field 701 702, that is, where a field with authorized value exists, it does
> not add the authority that exists, it creates another authority id.

Please open a new bug report, it's a completely different problem?
Comment 33 Jonathan Druart 2017-01-27 09:06:14 UTC
s/?//