Bug 38632 - All columns shown in holdings table when displaying the filters
Summary: All columns shown in holdings table when displaying the filters
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Jonathan Druart
QA Contact: Matt Blenkinsop
URL:
Keywords: rel_24_11_candidate
Depends on: 38485
Blocks:
  Show dependency treegraph
 
Reported: 2024-12-05 13:06 UTC by Jonathan Druart
Modified: 2025-02-04 02:08 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes the holdings table - clicking "Show filters" was incorrectly displaying all columns.
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 38632: Do not show all columns when displaying the filters (2.14 KB, patch)
2024-12-09 11:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38632: Add tests (6.22 KB, patch)
2024-12-09 11:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38632: Do not show all columns when displaying the filters (2.19 KB, patch)
2024-12-11 20:01 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 38632: Add tests (6.26 KB, patch)
2024-12-11 20:01 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 38632: Do not show all columns when displaying the filters (2.27 KB, patch)
2024-12-30 10:40 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 38632: Add tests (6.35 KB, patch)
2024-12-30 10:40 UTC, Matt Blenkinsop
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2024-12-05 13:06:03 UTC
If we click on "Show filters" all the columns are shown.
Comment 1 Jonathan Druart 2024-12-09 11:26:42 UTC
Created attachment 175296 [details] [review]
Bug 38632: Do not show all columns when displaying the filters

Yet another visibility bug fix for this specific table.
When the filters are displayed the table is destroyed and recreated.
We should not save the visibility changes made by DT and consider them
changes made by the user.
A temporary variable is used to store the user settings duringt this
action, then we restore them once it's done.

Test plan:
Go to the detail page of bibliographic record with items
Click on "show filters"
=> Only columns displayed should still be displayed
Retry but show/hide some columns before you display the filters
=> Only columns displayed should still be displayed
Comment 2 Jonathan Druart 2024-12-09 11:26:44 UTC
Created attachment 175297 [details] [review]
Bug 38632: Add tests

You need bug 38461 to run successfully this test.
Comment 3 Jonathan Druart 2024-12-09 11:27:17 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2024-12-09 11:27:59 UTC
Not adding the dependency on bug 38461 because the first patch should be backported (with or without the tests).
Comment 5 Brendan Lawlor 2024-12-11 20:01:12 UTC
Created attachment 175380 [details] [review]
Bug 38632: Do not show all columns when displaying the filters

Yet another visibility bug fix for this specific table.
When the filters are displayed the table is destroyed and recreated.
We should not save the visibility changes made by DT and consider them
changes made by the user.
A temporary variable is used to store the user settings duringt this
action, then we restore them once it's done.

Test plan:
Go to the detail page of bibliographic record with items
Click on "show filters"
=> Only columns displayed should still be displayed
Retry but show/hide some columns before you display the filters
=> Only columns displayed should still be displayed

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 6 Brendan Lawlor 2024-12-11 20:01:14 UTC
Created attachment 175381 [details] [review]
Bug 38632: Add tests

You need bug 38461 to run successfully this test.

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 7 Brendan Lawlor 2024-12-11 20:13:34 UTC
This works as described.

During testing I noticed small related bug in current main. On the bibliographic record details page if you click to configure columns and you add or remove a column it automatically shows the column filters too, but it does not toggle the filter link to 'Hide filters` so you have to click twice to hide them.

In 24.05 configuring the columns does not automatically show the column filters so I think it's a regression in 24.11

I tried to run the cypress tests but couldn't get it to work:
kohadev-koha@kohadevbox:koha(main)$ cypress run --spec t/cypress/integration/KohaTable_spec.ts 
[18586:1211/194337.239929:ERROR:gpu_memory_buffer_support_x11.cc(44)] dri3 extension not supported.

DevTools listening on ws://127.0.0.1:46069/devtools/browser/82072b5e-0295-4314-b0dd-1937534ca2f2
Missing baseUrl in compilerOptions. tsconfig-paths will be skipped
Your configFile threw an error from: /kohadevbox/koha/cypress.config.ts
Comment 8 Martin Renvoize (ashimema) 2024-12-17 07:08:43 UTC
sha1 issues on attempting to apply I'm afraid
Comment 9 Jonathan Druart 2024-12-17 09:02:31 UTC
(In reply to Martin Renvoize (ashimema) from comment #8)
> sha1 issues on attempting to apply I'm afraid

"You need bug 38461 to run successfully this test."

I have not added the dependency to not confuse RMaint as the first patch needs to be backported.
Comment 10 Jonathan Druart 2024-12-17 09:12:15 UTC
(In reply to Brendan Lawlor from comment #7)
> During testing I noticed small related bug in current main. On the
> bibliographic record details page if you click to configure columns and you
> add or remove a column it automatically shows the column filters too, but it
> does not toggle the filter link to 'Hide filters` so you have to click twice
> to hide them.

See bug 38724.
Comment 11 Matt Blenkinsop 2024-12-30 10:40:06 UTC
Created attachment 175984 [details] [review]
Bug 38632: Do not show all columns when displaying the filters

Yet another visibility bug fix for this specific table.
When the filters are displayed the table is destroyed and recreated.
We should not save the visibility changes made by DT and consider them
changes made by the user.
A temporary variable is used to store the user settings duringt this
action, then we restore them once it's done.

Test plan:
Go to the detail page of bibliographic record with items
Click on "show filters"
=> Only columns displayed should still be displayed
Retry but show/hide some columns before you display the filters
=> Only columns displayed should still be displayed

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Comment 12 Matt Blenkinsop 2024-12-30 10:40:09 UTC
Created attachment 175985 [details] [review]
Bug 38632: Add tests

You need bug 38461 to run successfully this test.

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Comment 13 Katrin Fischer 2024-12-30 11:01:44 UTC
I have trouble applying these:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 38632: Do not show all columns when displaying the filters
Applying: Bug 38632: Add tests
error: sha1 information is lacking or useless (t/cypress/integration/KohaTable_spec.ts).
error: could not build fake ancestor
Patch failed at 0001 Bug 38632: Add tests
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-38632-Add-tests-nmpntz93.patch

I have some patches on my branch. I'll push these now. Could you try and see if you can rebase/reattach the patches maybe Matt?
Comment 14 Matt Blenkinsop 2024-12-30 11:05:08 UTC
You need bug 38461 - its signed off so I'll QA that now and then it should apply
Comment 15 Matt Blenkinsop 2024-12-30 11:07:20 UTC
(In reply to Matt Blenkinsop from comment #14)
> You need bug 38461 - its signed off so I'll QA that now and then it should
> apply

On a closer look there are other bugs in the tree that also need QA
Comment 16 Katrin Fischer 2024-12-30 11:15:48 UTC
(In reply to Matt Blenkinsop from comment #15)
> (In reply to Matt Blenkinsop from comment #14)
> > You need bug 38461 - its signed off so I'll QA that now and then it should
> > apply
> 
> On a closer look there are other bugs in the tree that also need QA

How do you tell? There is no depdendeny on this bug but for the RESOLVED bug 38485?
Comment 17 Matt Blenkinsop 2024-12-30 11:29:31 UTC
See comment 4 - the tests patch needs the dependency but the first one doesn't
Comment 18 Katrin Fischer 2024-12-30 13:33:43 UTC
(In reply to Matt Blenkinsop from comment #17)
> See comment 4 - the tests patch needs the dependency but the first one
> doesn't

OK, waiting for the dependency bug 38461 to pass QA. We should have the tests in main.
Comment 19 Katrin Fischer 2025-01-02 10:05:20 UTC
I have bug 38632 applied now, but still doesn't apply:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 38632: Do not show all columns when displaying the filters
Applying: Bug 38632: Add tests
error: sha1 information is lacking or useless (t/cypress/integration/KohaTable_spec.ts).
error: could not build fake ancestor
Patch failed at 0001 Bug 38632: Add tests
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-38632-Add-tests-e47wbro9.patch
Comment 20 Katrin Fischer 2025-01-02 10:05:54 UTC
Ah, wrong bug... still waiting for the depdency one.
Comment 21 Katrin Fischer 2025-01-16 13:15:52 UTC
RMAINTS please note:

(In reply to Jonathan Druart from comment #4)
> Not adding the dependency on bug 38461 because the first patch should be
> backported (with or without the tests).
Comment 22 Katrin Fischer 2025-01-16 14:28:34 UTC
Pushed for 25.05!

Well done everyone, thank you!