Bug 38485 - Update column visibility on holdings table correctly
Summary: Update column visibility on holdings table correctly
Status: RESOLVED FIXED
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: Testopia
URL:
Keywords:
Depends on: 38118 38436 38482
Blocks: 38433 38632 38724
  Show dependency treegraph
 
Reported: 2024-11-19 14:35 UTC by Jonathan Druart
Modified: 2024-12-17 09:11 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00
Circulation function:


Attachments
Bug 38485: Take user's previous change into consideration for holdings tables col vis (4.30 KB, patch)
2024-11-19 16:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38485: Take user's previous change into consideration for holdings tables col vis (4.25 KB, patch)
2024-11-20 12:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38485: Take user's previous change into consideration for holdings tables col vis (4.30 KB, patch)
2024-11-20 13:18 UTC, Philip Orr
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-11-19 14:35:22 UTC
Fixed on bug 38118 but broken again with bug 38436. We need to fix this behaviour for 24.11.00
Comment 1 Jonathan Druart 2024-11-19 16:39:00 UTC
Created attachment 174797 [details] [review]
Bug 38485: Take user's previous change into consideration for holdings tables col vis

A lot happened, hard to summarize everything.

Basically at this point, we have (among other things):
* fixed bug 38118 (but reintroduced it with bug 36640)
* fixed bug 38436 (the original report was that the is_hidden setting
  was no longer taken into effect on the holdings tables)

Here we are trying to deal with making user's previous changes
persistent across the pagination and col vis changes.

Test plan:
Confirm that hiding checkouts in settings hide the columns
Confirm that showing checkouts then change page (*pagination*, so you need
more than 20 items) and confirm that the columns are still the same
Add a callnumber for an item from page 2 and confirm that the column is
hidden on page 1 and shown on page 2
Hide callnumber from the settings and confirm that the column is hidden
on pages 1 and 2
Show callnumber from the holdings page and confirm that the column is
displayed on page 2, go page 1: hidden, go back page 2: displayed.

And certainly way more other things to test, just confirm that the
behaviours with this patch are consistent with what a end user would
expect.

Note for QA/self: this is not very nice as column-visibility.dt event
can change in datatables.js and not impact this one.
Also the following nice sounds good and should be added to datatables.js
+            if (recalc === false) return;
Comment 2 Jonathan Druart 2024-11-19 16:40:37 UTC
Note that the "shareable link" won't restore the column that you would have displayed if they are hidden in the settings. Let's say it's a limitation for this table.
Comment 3 Jonathan Druart 2024-11-20 12:38:58 UTC
Created attachment 174837 [details] [review]
Bug 38485: Take user's previous change into consideration for holdings tables col vis

A lot happened, hard to summarize everything.

Basically at this point, we have (among other things):
* fixed bug 38118 (but reintroduced it with bug 36640)
* fixed bug 38436 (the original report was that the is_hidden setting
  was no longer taken into effect on the holdings tables)

Here we are trying to deal with making user's previous changes
persistent across the pagination and col vis changes.

Test plan:
Confirm that hiding checkouts in settings hide the columns
Confirm that showing checkouts then change page (*pagination*, so you need
more than 20 items) and confirm that the columns are still the same
Add a callnumber for an item from page 2 and confirm that the column is
hidden on page 1 and shown on page 2
Hide callnumber from the settings and confirm that the column is hidden
on pages 1 and 2
Show callnumber from the holdings page and confirm that the column is
displayed on page 2, go page 1: hidden, go back page 2: displayed.

And certainly way more other things to test, just confirm that the
behaviours with this patch are consistent with what a end user would
expect.

Note for QA/self: this is not very nice as column-visibility.dt event
can change in datatables.js and not impact this one.
Also the following nice sounds good and should be added to datatables.js
+            if (recalc === false) return;
Comment 4 Katrin Fischer 2024-11-20 12:41:01 UTC
This one is a bit confusing to me. I was testing without this patch applied:

Using biblionumber = 302

* Looked at the table, inventory number and call number were now showing
* Added an inventory number to one item, returend to page: column showed up
* Added a callnumber to one item, returned to page: column showed up.

So for items on first page it already works as expected.

* Added 20+ more items.
* Columns remained hidden on page 2 and were not visible at all.

Applied patch.

* My columns now show on the second page! :D
Comment 5 Katrin Fischer 2024-11-20 12:41:36 UTC
... forgot to say: in the end it was no longer confusion.  When I started typing out my test plan I had missed the bit about "second page".
Comment 6 Philip Orr 2024-11-20 13:18:35 UTC
Created attachment 174840 [details] [review]
Bug 38485: Take user's previous change into consideration for holdings tables col vis

A lot happened, hard to summarize everything.

Basically at this point, we have (among other things):
* fixed bug 38118 (but reintroduced it with bug 36640)
* fixed bug 38436 (the original report was that the is_hidden setting
  was no longer taken into effect on the holdings tables)

Here we are trying to deal with making user's previous changes
persistent across the pagination and col vis changes.

Test plan:
Confirm that hiding checkouts in settings hide the columns
Confirm that showing checkouts then change page (*pagination*, so you need
more than 20 items) and confirm that the columns are still the same
Add a callnumber for an item from page 2 and confirm that the column is
hidden on page 1 and shown on page 2
Hide callnumber from the settings and confirm that the column is hidden
on pages 1 and 2
Show callnumber from the holdings page and confirm that the column is
displayed on page 2, go page 1: hidden, go back page 2: displayed.

And certainly way more other things to test, just confirm that the
behaviours with this patch are consistent with what a end user would
expect.

Note for QA/self: this is not very nice as column-visibility.dt event
can change in datatables.js and not impact this one.
Also the following nice sounds good and should be added to datatables.js
+            if (recalc === false) return;

Signed-off-by: PhilipOrr <philip.orr@lmscloud.de>
Comment 7 Katrin Fischer 2024-11-20 13:22:12 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 8 Philip Orr 2024-11-20 13:23:00 UTC
tested everything in the test plan, also tested setting fields to hidden + "cannot be toggled".
Everything I could think of worked as the user would expect it to.