Bug 32926 - Cannot expand or collapse some System preference sections after a search
Summary: Cannot expand or collapse some System preference sections after a search
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Andreas Roussos
QA Contact: Testopia
URL: admin/preferences.pl
Keywords:
Depends on:
Blocks: 34655 33634 33687
  Show dependency treegraph
 
Reported: 2023-02-09 14:17 UTC by Andreas Roussos
Modified: 2023-12-28 20:47 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.04,22.05.11,21.11.19


Attachments
Bug 32926: Assign unique ids to <h3> elements (2.46 KB, patch)
2023-02-09 16:30 UTC, Andreas Roussos
Details | Diff | Splinter Review
Bug 32926: Add Selenium tests (5.20 KB, patch)
2023-02-09 16:30 UTC, Andreas Roussos
Details | Diff | Splinter Review
Bug 32926: Add Selenium tests (5.20 KB, patch)
2023-02-09 19:13 UTC, Andreas Roussos
Details | Diff | Splinter Review
Bug 32926: Assign unique ids to <h3> elements (3.22 KB, patch)
2023-02-17 15:10 UTC, Andreas Roussos
Details | Diff | Splinter Review
Bug 32926: Add Selenium tests (5.25 KB, patch)
2023-02-20 20:46 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 32926: Assign unique ids to <h3> elements (3.27 KB, patch)
2023-02-20 20:46 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 32926: Add Selenium tests (5.33 KB, patch)
2023-02-23 09:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32926: Assign unique ids to <h3> elements (3.35 KB, patch)
2023-02-23 09:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32926: Fix selenium tests (1.44 KB, patch)
2023-02-23 09:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32926: Restore scroll to sub-tab (1.27 KB, patch)
2023-05-05 12:09 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Roussos 2023-02-09 14:17:27 UTC
If you perform a System preferences search for a common word such as
"type" or "log", you then lose the ability to expand or collapse the
sections with names that appear more than once in the results page.
All sections except the first one in each group of duplicates are
affected by this. When you click on the 2nd, 3rd, etc. occurence
of a section, the expand/collapse arrow does change, but in reality
only the section that appears first in the page is actually toggled.

I have created an animated GIF that shows this in action:
https://imgur.com/a/NSslIcV

This affects Koha 20.11.00 or later; `git bisect` tells me that
the relevant commit is 9d890c7636ef1d6ec0a1de2161906fd9f4375e97
from Bug 23410.

To reproduce:
Perform a System preferences search for a common word such as "type"
or "log". In the results page, the "Policy" section will appear more
than once. Now try to expand/collapse any "Policy" section but the
first one. It won't work, only the first "Policy" section is toggled.
Comment 1 Andreas Roussos 2023-02-09 14:20:14 UTC
This is happening because the page does not have unique ids for
the <h3> elements. To see for yourself, perform the steps to
reproduce, then open your web browser's Developer Tools (F12 key).
Now copy/paste the following code [*] in the JavaScript console
and press <ENTER>:

ids = Array.from(document.querySelectorAll('h3[id]'))
  .map(v => v.id)
  .reduce((acc, v) => { acc[v] = (acc[v] || 0) + 1; return acc }, {});
dupes = Object.entries(ids)
  .filter(([key, value]) => value > 1);
console.table(dupes);

You should get a table in the console window similar to the one below:
+---------+--------------+---+
| (index) | 0            | 1 |
+---------+--------------+---+
| 0       | 'Features'   | 3 |
| 1       | 'Policy'     | 2 |
| 2       | 'Appearance' | 2 |
+---------+--------------+---+

The table lists the <h3> ids that occur more than once in the DOM.
The 2nd column is the actual id value and the last column is the
number of occurences. Notice that the "Features" id occurs 3 times,
while "Policy" and "Appearance" occur 2 times each.

[*] Code adapted from https://stackoverflow.com/a/70373818
Comment 2 Andreas Roussos 2023-02-09 16:30:01 UTC Comment hidden (obsolete)
Comment 3 Andreas Roussos 2023-02-09 16:30:04 UTC Comment hidden (obsolete)
Comment 4 Andreas Roussos 2023-02-09 19:13:06 UTC
Created attachment 146448 [details] [review]
Bug 32926: Add Selenium tests

This adds a new Selenium test to automatically test the correct
behaviour when expanding/collapsing System preferences sections
after a System preferences search.

Test plan:

1) Apply the other patch from this bug report, then run the
   new Selenium unit test which should pass without failures:

   prove -v t/db_dependent/selenium/system_preferences_search.t
Comment 5 Andreas Roussos 2023-02-09 19:17:45 UTC
Apologies, I somehow messed up the file permissions on the Selenium
unit test file just before committing (644 instead of the correct 755).
This has now been fixed and I've obsoleted my previous patch.
Comment 6 Fridolin Somers 2023-02-17 09:22:58 UTC
I've tested, works well.

But this breaks the links "View all xxx preferences",
which link to an id (ie #Policy).
Comment 7 Andreas Roussos 2023-02-17 09:50:39 UTC
(In reply to Fridolin Somers from comment #6)
> I've tested, works well.
Thanks for the feedback, Fridolin.

> But this breaks the links "View all xxx preferences",
> which link to an id (ie #Policy).
Ah, well spotted! I will provide a follow-up patch.
Comment 8 Andreas Roussos 2023-02-17 15:10:37 UTC
Created attachment 146852 [details] [review]
Bug 32926: Assign unique ids to <h3> elements

If you perform a System preferences search for a common word
such as "type", you lose the ability to expand or collapse some
sections. In particular, you cannot expand/collapse the sections
with names that appear more than once in the results page.
All sections except the first one in each group of duplicates
are affected by this. See https://imgur.com/a/NSslIcV for a demo.

This patch fixes that by prepending the <h3> ids with the
top-level System preference section name to make them unique.

Test plan:

1) Perform a System preferences search for a common word such
   as "type" or "log". In the results page, the "Policy" section
   will appear more than once. Now try to expand/collapse any
   "Policy" section but the first one. It won't work, only the
   first "Policy" section is toggled.
2) Apply this patch and refresh the page.
3) This time you should be able to expand or collapse all of
   the available system preference sections.
4) Clicking on the "View all xxx preferences" links on the
   right hand side should take you to the correct System
   preferences tab *and* automatically scroll the appropriate
   section into view at the top of the page.
Comment 9 Andreas Roussos 2023-02-17 15:14:02 UTC
(In reply to Andreas Roussos from comment #7)
> Ah, well spotted! I will provide a follow-up patch.
Actually, since this hasn't been signed off yet, I amended
my previous patch and added an extra item to the test plan
to cater for the issue that Fridolin has spotted.
Comment 10 Fridolin Somers 2023-02-20 20:46:15 UTC
Created attachment 146973 [details] [review]
Bug 32926: Add Selenium tests

This adds a new Selenium test to automatically test the correct
behaviour when expanding/collapsing System preferences sections
after a System preferences search.

Test plan:

1) Apply the other patch from this bug report, then run the
   new Selenium unit test which should pass without failures:

   prove -v t/db_dependent/selenium/system_preferences_search.t

Signed-off-by: Fridolin <fridolin.somers@biblibre.com>
Comment 11 Fridolin Somers 2023-02-20 20:46:29 UTC
Created attachment 146974 [details] [review]
Bug 32926: Assign unique ids to <h3> elements

If you perform a System preferences search for a common word
such as "type", you lose the ability to expand or collapse some
sections. In particular, you cannot expand/collapse the sections
with names that appear more than once in the results page.
All sections except the first one in each group of duplicates
are affected by this. See https://imgur.com/a/NSslIcV for a demo.

This patch fixes that by prepending the <h3> ids with the
top-level System preference section name to make them unique.

Test plan:

1) Perform a System preferences search for a common word such
   as "type" or "log". In the results page, the "Policy" section
   will appear more than once. Now try to expand/collapse any
   "Policy" section but the first one. It won't work, only the
   first "Policy" section is toggled.
2) Apply this patch and refresh the page.
3) This time you should be able to expand or collapse all of
   the available system preference sections.
4) Clicking on the "View all xxx preferences" links on the
   right hand side should take you to the correct System
   preferences tab *and* automatically scroll the appropriate
   section into view at the top of the page.

Signed-off-by: Fridolin <fridolin.somers@biblibre.com>
Comment 12 Jonathan Druart 2023-02-23 09:06:26 UTC
Created attachment 147197 [details] [review]
Bug 32926: Add Selenium tests

This adds a new Selenium test to automatically test the correct
behaviour when expanding/collapsing System preferences sections
after a System preferences search.

Test plan:

1) Apply the other patch from this bug report, then run the
   new Selenium unit test which should pass without failures:

   prove -v t/db_dependent/selenium/system_preferences_search.t

Signed-off-by: Fridolin <fridolin.somers@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Jonathan Druart 2023-02-23 09:06:29 UTC
Created attachment 147198 [details] [review]
Bug 32926: Assign unique ids to <h3> elements

If you perform a System preferences search for a common word
such as "type", you lose the ability to expand or collapse some
sections. In particular, you cannot expand/collapse the sections
with names that appear more than once in the results page.
All sections except the first one in each group of duplicates
are affected by this. See https://imgur.com/a/NSslIcV for a demo.

This patch fixes that by prepending the <h3> ids with the
top-level System preference section name to make them unique.

Test plan:

1) Perform a System preferences search for a common word such
   as "type" or "log". In the results page, the "Policy" section
   will appear more than once. Now try to expand/collapse any
   "Policy" section but the first one. It won't work, only the
   first "Policy" section is toggled.
2) Apply this patch and refresh the page.
3) This time you should be able to expand or collapse all of
   the available system preference sections.
4) Clicking on the "View all xxx preferences" links on the
   right hand side should take you to the correct System
   preferences tab *and* automatically scroll the appropriate
   section into view at the top of the page.

Signed-off-by: Fridolin <fridolin.somers@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Jonathan Druart 2023-02-23 09:06:32 UTC
Created attachment 147199 [details] [review]
Bug 32926: Fix selenium tests

We should not rely on existing data, and we should remove data at the end.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Tomás Cohen Arazi 2023-02-24 20:38:57 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 16 Matt Blenkinsop 2023-02-28 17:08:45 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 17 Lucas Gass 2023-03-08 14:05:36 UTC
Backported to 22.05.x for upcoming 22.05.11
Comment 18 Arthur Suzuki 2023-03-29 12:52:13 UTC
applied to 21.11 for 21.11.19
Comment 19 wainuiwitikapark 2023-04-27 05:33:27 UTC
Not backported to 21.05.x
Comment 20 Pedro Amorim 2023-05-05 12:09:36 UTC
Created attachment 150739 [details] [review]
Bug 32926: Restore scroll to sub-tab

Test plan:
Go to sys prefs, click 'Enhanced Content'
Click 'OverDrive', notice it doesnt scroll there

Apply this patch:
Do the same test plan, but notice it does scroll there, like before
Comment 21 Pedro Amorim 2023-05-05 12:17:51 UTC
Made patch obsolete, opened bug 33687 instead.