Bug 17988 - Select2 prevents correct tag expand/minimize functionality
Summary: Select2 prevents correct tag expand/minimize functionality
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Oleg Vasilenko
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 13501
Blocks: 18119
  Show dependency treegraph
 
Reported: 2017-01-25 09:54 UTC by Oleg Vasilenko
Modified: 2017-06-14 22:17 UTC (History)
6 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 17988: add attribute check (2.00 KB, patch)
2017-01-25 10:06 UTC, Oleg Vasilenko
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17988 - Select2 prevents correct tag expand/minimize functionality (2.05 KB, patch)
2017-02-01 21:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17988 - Select2 prevents correct tag expand/minimize functionality (2.10 KB, patch)
2017-02-02 09:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17988: Add a comment to explain the line (1.20 KB, patch)
2017-02-02 09:29 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 Oleg Vasilenko 2017-01-25 09:54:02 UTC
Overview: 
Select2 (Bug 13501) introduced divs and inputs that broke some assumptions about the expected HTML structure.
Because of that, expanding fields to show all hidden subfields does not work properly.

Steps to Reproduce: 
1. Open some book in the editor or create new (cataloguing/addbiblio.pl)
2. Try to minimize or expand fields, that have among subfields the following:
	— Thesaurus driven subfield → subfield with Select2
	— Hidden subfield.

Actual Results: 
 — some fields become hidden, some not, and vice versa
 — in the console, you'll see «Uncaught TypeError: Cannot read property 'match' of null»

Expected Results: 
 — all subfields should minimize/maximize completely

Additional Information:
This happens because Select2 adds some divs, that do not have ID property.
Comment 1 Oleg Vasilenko 2017-01-25 10:06:36 UTC
Created attachment 59543 [details] [review]
Bug 17988: add attribute check

The following patch adds check for the needed attribute existance.
Comment 2 Josef Moravec 2017-02-01 21:59:55 UTC
Created attachment 59758 [details] [review]
[SIGNED-OFF] Bug 17988 - Select2 prevents correct tag expand/minimize functionality

Overview:
Select2 (Bug 13501) introduced divs and inputs that broke some assumptions about the expected HTML structure.
Because of that, expanding fields to show all hidden subfields does not work properly.

Steps to Reproduce:
1. Open some book in the editor or create new (cataloguing/addbiblio.pl)
2. Try to minimize or expand fields, that have among subfields the following:
	— Thesaurus driven subfield → subfield with Select2
	— Hidden subfield.

Actual Results:
 — some fields become hidden, some not, and vice versa
 — in the console, you'll see «Uncaught TypeError: Cannot read property 'match' of null»

Expected Results:
 — all subfields should minimize/maximize completely

Additional Information:
This happens because Select2 adds some divs, that do not have ID property.
The following patch adds check for the needed attribute existance.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 3 Josef Moravec 2017-02-01 22:00:41 UTC
I was able to reproduce the problem with field 942, with this patch it is OK
Comment 4 Jonathan Druart 2017-02-02 09:29:11 UTC
Created attachment 59770 [details] [review]
Bug 17988 - Select2 prevents correct tag expand/minimize functionality

Overview:
Select2 (Bug 13501) introduced divs and inputs that broke some assumptions about the expected HTML structure.
Because of that, expanding fields to show all hidden subfields does not work properly.

Steps to Reproduce:
1. Open some book in the editor or create new (cataloguing/addbiblio.pl)
2. Try to minimize or expand fields, that have among subfields the following:
	— Thesaurus driven subfield → subfield with Select2
	— Hidden subfield.

Actual Results:
 — some fields become hidden, some not, and vice versa
 — in the console, you'll see «Uncaught TypeError: Cannot read property 'match' of null»

Expected Results:
 — all subfields should minimize/maximize completely

Additional Information:
This happens because Select2 adds some divs, that do not have ID property.
The following patch adds check for the needed attribute existance.

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

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 5 Jonathan Druart 2017-02-02 09:29:15 UTC
Created attachment 59771 [details] [review]
Bug 17988: Add a comment to explain the line

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 6 Kyle M Hall 2017-02-14 13:57:15 UTC
Pushed to master for 17.05, thanks Oleg, Jonathan!
Comment 7 Katrin Fischer 2017-02-14 19:40:47 UTC
These patches have been pushed to 16.11.x and will be in 16.11.04.
Comment 8 Katrin Fischer 2017-02-14 21:23:23 UTC
Follow up breaks the collapsing/uncollapsing of the fields -  
I think # is not allowed in Javascript for comments.
Needs to be fixed on master - I will remove the second patch from my branch before pushing it up.
Comment 9 Oleg Vasilenko 2017-02-15 06:26:15 UTC
It would be great, if another bug connected with Select2 would also be pushed to master — Bug 17780
It gives us a lot of problems, because authority records either are not added at all, or added without ID (and so would not be updated).
Comment 10 Katrin Fischer 2017-02-15 06:30:13 UTC
Hi Oleg, we will need a sign-off there first - I have upped the severity a bit!
Comment 11 Jonathan Druart 2017-02-15 07:32:58 UTC
Silly me, comment in JS is not #!
Comment 12 Mason James 2017-02-22 23:14:05 UTC
(In reply to Katrin Fischer from comment #7)
> These patches have been pushed to 16.11.x and will be in 16.11.04.

Pushed to 16.05.x, for 16.05.10 release
Comment 13 Jonathan Druart 2017-02-24 07:57:18 UTC
(In reply to Mason James from comment #12)
> (In reply to Katrin Fischer from comment #7)
> > These patches have been pushed to 16.11.x and will be in 16.11.04.
> 
> Pushed to 16.05.x, for 16.05.10 release

Mason, you forgot bug 18119!
Comment 14 Mason James 2017-02-24 23:11:39 UTC
(In reply to Jonathan Druart from comment #13)
> (In reply to Mason James from comment #12)
> > (In reply to Katrin Fischer from comment #7)
> > > These patches have been pushed to 16.11.x and will be in 16.11.04.
> > 
> > Pushed to 16.05.x, for 16.05.10 release
> 
> Mason, you forgot bug 18119!

thanks!, i've pushed 18119