Bug 38436 - Adjust code for column visibility (after DataTables upgrade)
Summary: Adjust code for column visibility (after DataTables upgrade)
Status: Needs documenting
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: Paul Derscheid
URL:
Keywords:
: 38456 (view as bug list)
Depends on: 36640 38391
Blocks: 38595 38906 38482 38485
  Show dependency treegraph
 
Reported: 2024-11-13 16:46 UTC by Caroline Cyr La Rose
Modified: 2025-01-16 13:08 UTC (History)
7 users (show)

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


Attachments
Bug 38436: Do not try to build the table if tab is not there (1.15 KB, patch)
2024-11-15 15:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Store bKohaColumnsUseNames in data instead of DT's settings (2.62 KB, patch)
2024-11-15 15:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Store loaded_from_state in data instead of DT's settings (5.15 KB, patch)
2024-11-15 15:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Adjust column names for the holdings tables (14.38 KB, patch)
2024-11-15 15:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Rename columns_settings.columnname in DB (1.28 KB, patch)
2024-11-15 15:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Fix column visibility when bKohaColumnsUseNames is used (6.99 KB, patch)
2024-11-15 15:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Adjust columns_settings.inc for KohaTable (4.73 KB, patch)
2024-11-15 15:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Restore hide columns when empty (1.23 KB, patch)
2024-11-15 15:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Do not hide all if none to hide (1.07 KB, patch)
2024-11-15 15:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Fix visibility when bKohaColumnsUseNames is not set (2.01 KB, patch)
2024-11-15 21:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Adjust Vue (1.25 KB, patch)
2024-11-16 07:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Do not deal with 'Columns' button if no table settings (2.08 KB, patch)
2024-11-16 07:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: (follow-up) Adjust columns_settings.inc for KohaTable (1.29 KB, patch)
2024-11-18 13:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Fix visibility when bKohaColumnsUseNames is not set (2.35 KB, patch)
2024-11-18 13:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Fix patron categories (6.16 KB, patch)
2024-11-18 13:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: (follow-up) Fix visibility when bKohaColumnsUseNames is not set (2.36 KB, patch)
2024-11-18 13:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Fix patron categories (6.16 KB, patch)
2024-11-18 13:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Adjust columns_settings.inc for KohaTable (4.73 KB, patch)
2024-11-18 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Restore hide columns when empty (1.23 KB, patch)
2024-11-18 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Do not hide all if none to hide (1.07 KB, patch)
2024-11-18 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Fix visibility when bKohaColumnsUseNames is not set (2.01 KB, patch)
2024-11-18 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Adjust Vue (1.25 KB, patch)
2024-11-18 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Do not deal with 'Columns' button if no table settings (2.08 KB, patch)
2024-11-18 14:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38069: (follow-up) Fix jump to the correct table (1.31 KB, patch)
2024-11-18 14:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: (follow-up) Adjust columns_settings.inc for KohaTable (1.29 KB, patch)
2024-11-18 14:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: (follow-up) Fix visibility when bKohaColumnsUseNames is not set (2.36 KB, patch)
2024-11-18 14:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Fix patron categories (6.16 KB, patch)
2024-11-18 14:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Do not try to build the table if tab is not there (1.20 KB, patch)
2024-11-18 15:40 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 38436: Store bKohaColumnsUseNames in data instead of DT's settings (2.67 KB, patch)
2024-11-18 15:40 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 38436: Store loaded_from_state in data instead of DT's settings (5.19 KB, patch)
2024-11-18 15:40 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 38436: Adjust column names for the holdings tables (14.42 KB, patch)
2024-11-18 15:40 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 38436: Rename columns_settings.columnname in DB (1.33 KB, patch)
2024-11-18 15:40 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 38436: Fix column visibility when bKohaColumnsUseNames is used (7.04 KB, patch)
2024-11-18 15:41 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 38436: Adjust columns_settings.inc for KohaTable (4.78 KB, patch)
2024-11-18 15:41 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 38436: Restore hide columns when empty (1.28 KB, patch)
2024-11-18 15:41 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 38436: Do not hide all if none to hide (1.11 KB, patch)
2024-11-18 15:41 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 38436: Fix visibility when bKohaColumnsUseNames is not set (2.05 KB, patch)
2024-11-18 15:41 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 38436: Adjust Vue (1.30 KB, patch)
2024-11-18 15:41 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 38436: Do not deal with 'Columns' button if no table settings (2.12 KB, patch)
2024-11-18 15:41 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 38069: (follow-up) Fix jump to the correct table (1.36 KB, patch)
2024-11-18 15:41 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 38436: (follow-up) Adjust columns_settings.inc for KohaTable (1.33 KB, patch)
2024-11-18 15:41 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 38436: (follow-up) Fix visibility when bKohaColumnsUseNames is not set (2.41 KB, patch)
2024-11-18 15:41 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 38436: Fix patron categories (6.21 KB, patch)
2024-11-18 15:41 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 38436: Do not try to build the table if tab is not there (1.26 KB, patch)
2024-11-18 17:05 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38436: Store bKohaColumnsUseNames in data instead of DT's settings (2.72 KB, patch)
2024-11-18 17:05 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38436: Store loaded_from_state in data instead of DT's settings (5.25 KB, patch)
2024-11-18 17:05 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38436: Adjust column names for the holdings tables (14.48 KB, patch)
2024-11-18 17:05 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38436: Rename columns_settings.columnname in DB (1.39 KB, patch)
2024-11-18 17:05 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38436: Fix column visibility when bKohaColumnsUseNames is used (7.09 KB, patch)
2024-11-18 17:05 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38436: Adjust columns_settings.inc for KohaTable (4.84 KB, patch)
2024-11-18 17:05 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38436: Restore hide columns when empty (1.33 KB, patch)
2024-11-18 17:05 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38436: Do not hide all if none to hide (1.17 KB, patch)
2024-11-18 17:05 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38436: Fix visibility when bKohaColumnsUseNames is not set (2.11 KB, patch)
2024-11-18 17:05 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38436: Adjust Vue (1.36 KB, patch)
2024-11-18 17:05 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38436: Do not deal with 'Columns' button if no table settings (2.18 KB, patch)
2024-11-18 17:05 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38069: (follow-up) Fix jump to the correct table (1.42 KB, patch)
2024-11-18 17:05 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38436: (follow-up) Adjust columns_settings.inc for KohaTable (1.39 KB, patch)
2024-11-18 17:05 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38436: (follow-up) Fix visibility when bKohaColumnsUseNames is not set (2.47 KB, patch)
2024-11-18 17:05 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38436: Fix patron categories (6.26 KB, patch)
2024-11-18 17:05 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38436: (QA follow-up) Tidy atomicupdate file to satisfy qa script (1.10 KB, patch)
2024-11-18 17:06 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38436: Do not try to build the table if tab is not there (1.26 KB, patch)
2024-11-19 10:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Store bKohaColumnsUseNames in data instead of DT's settings (2.73 KB, patch)
2024-11-19 10:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Do not try to build the table if tab is not there (1.26 KB, patch)
2024-11-19 10:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Store bKohaColumnsUseNames in data instead of DT's settings (2.73 KB, patch)
2024-11-19 10:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Store loaded_from_state in data instead of DT's settings (5.25 KB, patch)
2024-11-19 10:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Adjust column names for the holdings tables (14.49 KB, patch)
2024-11-19 10:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Rename columns_settings.columnname in DB (1.39 KB, patch)
2024-11-19 10:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Fix column visibility when bKohaColumnsUseNames is used (7.09 KB, patch)
2024-11-19 10:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Adjust columns_settings.inc for KohaTable (4.84 KB, patch)
2024-11-19 10:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Restore hide columns when empty (1.33 KB, patch)
2024-11-19 10:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Do not hide all if none to hide (1.17 KB, patch)
2024-11-19 10:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Fix visibility when bKohaColumnsUseNames is not set (2.11 KB, patch)
2024-11-19 10:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Adjust Vue (1.36 KB, patch)
2024-11-19 10:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Do not deal with 'Columns' button if no table settings (2.18 KB, patch)
2024-11-19 10:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: (follow-up) Adjust columns_settings.inc for KohaTable (1.39 KB, patch)
2024-11-19 10:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: (follow-up) Fix visibility when bKohaColumnsUseNames is not set (2.47 KB, patch)
2024-11-19 10:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Fix patron categories (6.27 KB, patch)
2024-11-19 10:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Fix OPAC tables (16.07 KB, patch)
2024-11-20 09:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38436: Adjust DB update output (944 bytes, patch)
2024-11-25 11:09 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 Caroline Cyr La Rose 2024-11-13 16:46:36 UTC
In the holdings table in the staff interface, if a column has a value, it is show regardless of the table settings.

I think this is a regression from previous behaviour. I tested it on 23.05 and 24.05 and the table settings are taken into account and hide the undesired column.  A use case example is for single-library system who want to simplify/shorten the holdings table, they might choose to hide the holding library column as it's always going to be the same as the home library. Having those two columns only doubles the information uselessly for them.

To recreate :

1. View a record in the staff interface
2. In another tab, go to Administration > Table settings > Catalog > holdings_table and check 'is hidden by default' for holdings_holdingbranch
3. Go back to the tab with the record and refresh the page
   --> The 'Current library' column is still there
Comment 1 Jonathan Druart 2024-11-14 11:29:30 UTC
It was an illusion.

It could have worked in simple situation, but say you have:
https://snipboard.io/TpDSZ2.jpg

I don't think it could work with the code I have in front of my eyes...

To be confirmed however. I am tempted to suggest a proper fix instead of a hack but 24.11 is quite close now :-/
Comment 2 Jonathan Druart 2024-11-14 11:30:25 UTC
Needs to be confirmed however, I am currently trapped between main (which has several DT changes already) and bug 36640. I am trying to work on top of those ones.
Comment 3 Jonathan Druart 2024-11-14 15:53:53 UTC
Pfiou, tricky one here!

So keep in mind that we have "Save state" (bug 33484) AND "DataTables upgrade" (bug 36640) that happened in main.

We are now on top of that, and things are tricky.

I really feel like the previous versions were buggy. They may have kind of worked, but they were not fully working (regarding the different possible configurations).

I will be back on Monday, but if you some of you want to test, there is a remote branch on my gitlab repo: bug_38436

You can test the patron search and the items table.

There are still some work to do:
* Adjust the column names in DB
* Adjust the other tables using bKohaColumnsUseNames
* Adjust KohaTable code (in columns_settings.inc)

I will continue next week but it would be great to know if the expected behaviour is the one from this branch.
Comment 4 Jonathan Druart 2024-11-14 15:54:17 UTC
Switched to "Needs Signoff" but it's not ready for inclusion.
Comment 5 Jonathan Druart 2024-11-15 13:44:08 UTC
*** Bug 38433 has been marked as a duplicate of this bug. ***
Comment 6 Jonathan Druart 2024-11-15 15:17:32 UTC
*** Bug 38456 has been marked as a duplicate of this bug. ***
Comment 7 Jonathan Druart 2024-11-15 15:18:23 UTC
Created attachment 174587 [details] [review]
Bug 38436: Do not try to build the table if tab is not there

If we don't have the "other holdings" tab, no need to try to build the
table.
Comment 8 Jonathan Druart 2024-11-15 15:18:25 UTC
Created attachment 174588 [details] [review]
Bug 38436: Store bKohaColumnsUseNames in data instead of DT's settings

DataTable's settings do no longer allow to store custom properties.
Here we are passing bKohaColumnsUseNames to the constructor, but we
cannot retrieve it later from DT's api.

We need to store it in data().

Test plan:
0. Do not apply this patch
1. In _dt_visibility add console.log(settings.bKohaColumnsUseNames);
   where relevant
2. Open your browser, clear localStorage entries (we want to reach the
   set_default() code)
3. Hit http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=4
Notice that the value is true then false (displayed once per columns).
This is because we are calling _dt_visibility twice:
 * with the 'settings' we passed to the constructor:
1044         [hidden_ids, included_ids] = _dt_visibility(table_settings, settings, this)
 * from set_default() where we pass the settings retrieved from DT's api
900         [hidden_ids, included_ids] = _dt_visibility(table_settings, settings, $("#"+settings.nTable.id));

4. Apply this patch, console.log again and confirm that it's now true.

Notice that everything is then buggy...
Comment 9 Jonathan Druart 2024-11-15 15:18:28 UTC
Created attachment 174589 [details] [review]
Bug 38436: Store loaded_from_state in data instead of DT's settings

This has been introduced by bug 33484 as we needed to know if the state
was loaded (from URL or localStorage).

If the state is loaded then we need to:
1. not redirect if only one patron to display (to prevent a blocking
   situation)
2. display the table (ie. not defer loading)
Comment 10 Jonathan Druart 2024-11-15 15:18:31 UTC
Created attachment 174590 [details] [review]
Bug 38436: Adjust column names for the holdings tables

Removing the prefix holdings_ and otherholdings_
Could have been done at the code level but does not seem necessary to
have different column names
Comment 11 Jonathan Druart 2024-11-15 15:18:34 UTC
Created attachment 174591 [details] [review]
Bug 38436: Rename columns_settings.columnname in DB
Comment 12 Jonathan Druart 2024-11-15 15:18:36 UTC
Created attachment 174592 [details] [review]
Bug 38436: Fix column visibility when bKohaColumnsUseNames is used

When the columns displayed in the table can vary (depending on sysprefs)
we use bKohaColumnsUseNames. This allows us to use the name of the
columns instead of there order number.

The code was totally wrong, and didn't retrieve the correct number.

A good example to confirm that is to add:
 console.log("%s is at %s".format(this.columnname, used_id));
at the end of the `if ( use_names ) {` block of _dt_visibility.

If at least one column is hidden by default and cannot be toggled (and
so won't appear in the list of the columns of the "Column visibility" button),
there will be a shift.

This line was definitelly wrong:
 var named_id = $( 'thead th[data-colname="' + this.columnname + '"]', selector ).index( selector + ' th' );
We cannot rely on the order of the visible columns, we need to retrieve
the list of the columns using DT's api.

Now the columns are retrieved using their class names, and from DT's
api.
The code is way nicer and more inline with how we are supposed to play
with DT.
Look at those 2 lines to pass the list of columns to the colvis button:
  let included_columns = table_settings.columns.filter(c => !c.cannot_be_toggled);
  columns: included_columns.map(c => '.' + c.columnname).join(',')

Test plan:
Play intensively with the items table (detail.pl), but also with the
other tables not using bKohaColumnsUseNames (eg. patrons search)
You need to set column as hidden by default and/or cannot be toggled.
You need to play with the "Columns" button.
Note that you need to remove the localStorage keys starting with 'DataTables_'
to retrieve the visibility settings from what is defined in the admin area.
Comment 13 Jonathan Druart 2024-11-15 15:18:39 UTC
Created attachment 174593 [details] [review]
Bug 38436: Adjust columns_settings.inc for KohaTable
Comment 14 Jonathan Druart 2024-11-15 15:18:42 UTC
Created attachment 174594 [details] [review]
Bug 38436: Restore hide columns when empty

If a column has no data on the items table we should hide the column.
Comment 15 Jonathan Druart 2024-11-15 15:18:45 UTC
Created attachment 174595 [details] [review]
Bug 38436: Do not hide all if none to hide
Comment 16 Philip Orr 2024-11-15 18:56:06 UTC
I tested on ktd. The holdings table on detail.pl does indeed behave as expected with this patch.
However the "Columns" button in the table of patron results in patron search now no longer works, the screen is grayed out as if the menu was open, but there is no menu after clicking on "Columns".
(I first searched for some patrons and when the table of results was shown I clicked on "Columns").
Comment 17 Philip Orr 2024-11-15 18:57:21 UTC
(also qa says:
´´´
 WARN   installer/data/mysql/atomicupdate/bug_38436.pl
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 0, now: 2)
´´´)
Comment 18 Jonathan Druart 2024-11-15 21:08:25 UTC
Created attachment 174634 [details] [review]
Bug 38436: Fix visibility when bKohaColumnsUseNames is not set

Typically the patrons search.
Comment 19 Jonathan Druart 2024-11-15 21:09:50 UTC
(In reply to Philip Orr from comment #16)
> I tested on ktd.

Thanks for testing!

> The holdings table on detail.pl does indeed behave as
> expected with this patch.

Great!

> However the "Columns" button in the table of patron results in patron search
> now no longer works, the screen is grayed out as if the menu was open, but
> there is no menu after clicking on "Columns".

Oops, yes. I was too focused on the items table and its particularities it seems.

(In reply to Philip Orr from comment #17)
> (also qa says:
> ´´´
>  WARN   installer/data/mysql/atomicupdate/bug_38436.pl
>    WARN   tidiness
>                 The file is less tidy than before (bad/messy lines before:
> 0, now: 2)
> ´´´)

Can be ignored.
Comment 20 Jonathan Druart 2024-11-16 07:06:45 UTC
Created attachment 174641 [details] [review]
Bug 38436: Adjust Vue
Comment 21 Jonathan Druart 2024-11-16 07:06:48 UTC
Created attachment 174642 [details] [review]
Bug 38436: Do not deal with 'Columns' button if no table settings

If the table does not have table settings we should not deal with the
column visibility and not display the 'Columns' button.
Comment 22 Paul Derscheid 2024-11-18 11:46:52 UTC
Don't know whether this is related to these patches:

/cgi-bin/koha/admin/columns_settings.pl?module=catalogue&page=detail&table=holdings_table

Uncaught Error: Syntax error, unrecognized expression: #detail|holdings_table
Comment 23 Paul Derscheid 2024-11-18 11:53:23 UTC
Also unsure whether this is local but I experience this:
> I tested on ktd. The holdings table on detail.pl does indeed behave as expected with this patch.
However the "Columns" button in the table of patron results in patron search now no longer works, the screen is grayed out as if the menu was open, but there is no menu after clicking on "Columns".
(I first searched for some patrons and when the table of results was shown I clicked on "Columns").

But the other way around. The columns button in detail.pl yields a backdrop with no options. The patron search does behave as expected.
Comment 24 Jonathan Druart 2024-11-18 12:22:52 UTC
(In reply to Paul Derscheid from comment #22)
> Don't know whether this is related to these patches:
> 
> /cgi-bin/koha/admin/columns_settings.
> pl?module=catalogue&page=detail&table=holdings_table
> 
> Uncaught Error: Syntax error, unrecognized expression: #detail|holdings_table

Attached a follow-up on bug 38069.
Comment 25 Jonathan Druart 2024-11-18 13:50:53 UTC
Created attachment 174688 [details] [review]
Bug 38436: (follow-up) Adjust columns_settings.inc for KohaTable

Previous commit didn't adjust columns_settings.inc
Comment 26 Jonathan Druart 2024-11-18 13:50:55 UTC
Created attachment 174689 [details] [review]
Bug 38436: Fix visibility when bKohaColumnsUseNames is not set

Previous commit didn't adjust columns_settings.inc
Comment 27 Jonathan Druart 2024-11-18 13:50:59 UTC
Created attachment 174690 [details] [review]
Bug 38436: Fix patron categories

Make it use bKohaColumnsUseNames as we are building the columns
depending on sysprefs.

Note that the hard-coded list of columns to export is wrong
  exportColumns: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12],
Must use .noExport instead.
Comment 28 Jonathan Druart 2024-11-18 13:53:21 UTC
(In reply to Jonathan Druart from comment #27)
> Note that the hard-coded list of columns to export is wrong
>   exportColumns: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12],
> Must use .noExport instead.

Opened bug 38472.
Comment 29 Jonathan Druart 2024-11-18 13:57:28 UTC
Created attachment 174691 [details] [review]
Bug 38436: (follow-up) Fix visibility when bKohaColumnsUseNames is not set

Previous commit didn't adjust columns_settings.inc
Comment 30 Jonathan Druart 2024-11-18 13:57:31 UTC
Created attachment 174692 [details] [review]
Bug 38436: Fix patron categories

Make it use bKohaColumnsUseNames as we are building the columns
depending on sysprefs.

Note that the hard-coded list of columns to export is wrong
  exportColumns: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12],
Must use .noExport instead.
Comment 31 Jonathan Druart 2024-11-18 14:03:45 UTC
Created attachment 174693 [details] [review]
Bug 38436: Adjust columns_settings.inc for KohaTable
Comment 32 Jonathan Druart 2024-11-18 14:03:48 UTC
Created attachment 174694 [details] [review]
Bug 38436: Restore hide columns when empty

If a column has no data on the items table we should hide the column.
Comment 33 Jonathan Druart 2024-11-18 14:03:51 UTC
Created attachment 174695 [details] [review]
Bug 38436: Do not hide all if none to hide
Comment 34 Jonathan Druart 2024-11-18 14:03:54 UTC
Created attachment 174696 [details] [review]
Bug 38436: Fix visibility when bKohaColumnsUseNames is not set

Typically the patrons search.
Comment 35 Jonathan Druart 2024-11-18 14:03:57 UTC
Created attachment 174697 [details] [review]
Bug 38436: Adjust Vue
Comment 36 Jonathan Druart 2024-11-18 14:04:00 UTC
Created attachment 174698 [details] [review]
Bug 38436: Do not deal with 'Columns' button if no table settings

If the table does not have table settings we should not deal with the
column visibility and not display the 'Columns' button.
Comment 37 Jonathan Druart 2024-11-18 14:04:03 UTC
Created attachment 174699 [details] [review]
Bug 38069: (follow-up) Fix jump to the correct table
Comment 38 Jonathan Druart 2024-11-18 14:04:06 UTC
Created attachment 174700 [details] [review]
Bug 38436: (follow-up) Adjust columns_settings.inc for KohaTable

Previous commit didn't adjust columns_settings.inc
Comment 39 Jonathan Druart 2024-11-18 14:04:09 UTC
Created attachment 174701 [details] [review]
Bug 38436: (follow-up) Fix visibility when bKohaColumnsUseNames is not set

Previous commit didn't adjust columns_settings.inc
Comment 40 Jonathan Druart 2024-11-18 14:04:12 UTC
Created attachment 174702 [details] [review]
Bug 38436: Fix patron categories

Make it use bKohaColumnsUseNames as we are building the columns
depending on sysprefs.

Note that the hard-coded list of columns to export is wrong
  exportColumns: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12],
Must use .noExport instead.
Comment 41 Jonathan Druart 2024-11-18 14:10:27 UTC
(In reply to Jonathan Druart from comment #40)
> Created attachment 174702 [details] [review] [review]
> Bug 38436: Fix patron categories
> 
> Make it use bKohaColumnsUseNames as we are building the columns
> depending on sysprefs.

This is an important one to keep in mind as I am pretty sure there are others in the wild. If the column list is empty it means that the number of columns defined in the settings is not the same as the one in the table. In that case we MUST use bKohaColumnsUseNames and make adjustement to the th, but maybe also to the config (which is actually a bug as we are supposed to keep them in sync).
Comment 42 Katrin Fischer 2024-11-18 15:26:18 UTC
Where are we at here?
Comment 43 Paul Derscheid 2024-11-18 15:27:04 UTC
Working on it, testing this is a bit of work.
Comment 44 Owen Leonard 2024-11-18 15:40:49 UTC
Created attachment 174721 [details] [review]
Bug 38436: Do not try to build the table if tab is not there

If we don't have the "other holdings" tab, no need to try to build the
table.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 45 Owen Leonard 2024-11-18 15:40:51 UTC
Created attachment 174722 [details] [review]
Bug 38436: Store bKohaColumnsUseNames in data instead of DT's settings

DataTable's settings do no longer allow to store custom properties.
Here we are passing bKohaColumnsUseNames to the constructor, but we
cannot retrieve it later from DT's api.

We need to store it in data().

Test plan:
0. Do not apply this patch
1. In _dt_visibility add console.log(settings.bKohaColumnsUseNames);
   where relevant
2. Open your browser, clear localStorage entries (we want to reach the
   set_default() code)
3. Hit http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=4
Notice that the value is true then false (displayed once per columns).
This is because we are calling _dt_visibility twice:
 * with the 'settings' we passed to the constructor:
1044         [hidden_ids, included_ids] = _dt_visibility(table_settings, settings, this)
 * from set_default() where we pass the settings retrieved from DT's api
900         [hidden_ids, included_ids] = _dt_visibility(table_settings, settings, $("#"+settings.nTable.id));

4. Apply this patch, console.log again and confirm that it's now true.

Notice that everything is then buggy...

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 46 Owen Leonard 2024-11-18 15:40:53 UTC
Created attachment 174723 [details] [review]
Bug 38436: Store loaded_from_state in data instead of DT's settings

This has been introduced by bug 33484 as we needed to know if the state
was loaded (from URL or localStorage).

If the state is loaded then we need to:
1. not redirect if only one patron to display (to prevent a blocking
   situation)
2. display the table (ie. not defer loading)

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 47 Owen Leonard 2024-11-18 15:40:56 UTC
Created attachment 174724 [details] [review]
Bug 38436: Adjust column names for the holdings tables

Removing the prefix holdings_ and otherholdings_
Could have been done at the code level but does not seem necessary to
have different column names

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 48 Owen Leonard 2024-11-18 15:40:58 UTC
Created attachment 174725 [details] [review]
Bug 38436: Rename columns_settings.columnname in DB

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 49 Owen Leonard 2024-11-18 15:41:01 UTC
Created attachment 174726 [details] [review]
Bug 38436: Fix column visibility when bKohaColumnsUseNames is used

When the columns displayed in the table can vary (depending on sysprefs)
we use bKohaColumnsUseNames. This allows us to use the name of the
columns instead of there order number.

The code was totally wrong, and didn't retrieve the correct number.

A good example to confirm that is to add:
 console.log("%s is at %s".format(this.columnname, used_id));
at the end of the `if ( use_names ) {` block of _dt_visibility.

If at least one column is hidden by default and cannot be toggled (and
so won't appear in the list of the columns of the "Column visibility" button),
there will be a shift.

This line was definitelly wrong:
 var named_id = $( 'thead th[data-colname="' + this.columnname + '"]', selector ).index( selector + ' th' );
We cannot rely on the order of the visible columns, we need to retrieve
the list of the columns using DT's api.

Now the columns are retrieved using their class names, and from DT's
api.
The code is way nicer and more inline with how we are supposed to play
with DT.
Look at those 2 lines to pass the list of columns to the colvis button:
  let included_columns = table_settings.columns.filter(c => !c.cannot_be_toggled);
  columns: included_columns.map(c => '.' + c.columnname).join(',')

Test plan:
Play intensively with the items table (detail.pl), but also with the
other tables not using bKohaColumnsUseNames (eg. patrons search)
You need to set column as hidden by default and/or cannot be toggled.
You need to play with the "Columns" button.
Note that you need to remove the localStorage keys starting with 'DataTables_'
to retrieve the visibility settings from what is defined in the admin area.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 50 Owen Leonard 2024-11-18 15:41:03 UTC
Created attachment 174727 [details] [review]
Bug 38436: Adjust columns_settings.inc for KohaTable

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 51 Owen Leonard 2024-11-18 15:41:06 UTC
Created attachment 174728 [details] [review]
Bug 38436: Restore hide columns when empty

If a column has no data on the items table we should hide the column.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 52 Owen Leonard 2024-11-18 15:41:08 UTC
Created attachment 174729 [details] [review]
Bug 38436: Do not hide all if none to hide

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 53 Owen Leonard 2024-11-18 15:41:10 UTC
Created attachment 174730 [details] [review]
Bug 38436: Fix visibility when bKohaColumnsUseNames is not set

Typically the patrons search.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 54 Owen Leonard 2024-11-18 15:41:13 UTC
Created attachment 174731 [details] [review]
Bug 38436: Adjust Vue

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 55 Owen Leonard 2024-11-18 15:41:15 UTC
Created attachment 174732 [details] [review]
Bug 38436: Do not deal with 'Columns' button if no table settings

If the table does not have table settings we should not deal with the
column visibility and not display the 'Columns' button.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 56 Owen Leonard 2024-11-18 15:41:18 UTC
Created attachment 174733 [details] [review]
Bug 38069: (follow-up) Fix jump to the correct table

https://bugs.koha-community.org/show_bug.cgi?id=38436
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 57 Owen Leonard 2024-11-18 15:41:20 UTC
Created attachment 174734 [details] [review]
Bug 38436: (follow-up) Adjust columns_settings.inc for KohaTable

Previous commit didn't adjust columns_settings.inc

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 58 Owen Leonard 2024-11-18 15:41:23 UTC
Created attachment 174735 [details] [review]
Bug 38436: (follow-up) Fix visibility when bKohaColumnsUseNames is not set

Previous commit didn't adjust columns_settings.inc

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 59 Owen Leonard 2024-11-18 15:41:25 UTC
Created attachment 174736 [details] [review]
Bug 38436: Fix patron categories

Make it use bKohaColumnsUseNames as we are building the columns
depending on sysprefs.

Note that the hard-coded list of columns to export is wrong
  exportColumns: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12],
Must use .noExport instead.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 60 Paul Derscheid 2024-11-18 17:05:08 UTC
Created attachment 174740 [details] [review]
Bug 38436: Do not try to build the table if tab is not there

If we don't have the "other holdings" tab, no need to try to build the
table.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 61 Paul Derscheid 2024-11-18 17:05:11 UTC
Created attachment 174741 [details] [review]
Bug 38436: Store bKohaColumnsUseNames in data instead of DT's settings

DataTable's settings do no longer allow to store custom properties.
Here we are passing bKohaColumnsUseNames to the constructor, but we
cannot retrieve it later from DT's api.

We need to store it in data().

Test plan:
0. Do not apply this patch
1. In _dt_visibility add console.log(settings.bKohaColumnsUseNames);
   where relevant
2. Open your browser, clear localStorage entries (we want to reach the
   set_default() code)
3. Hit http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=4
Notice that the value is true then false (displayed once per columns).
This is because we are calling _dt_visibility twice:
 * with the 'settings' we passed to the constructor:
1044         [hidden_ids, included_ids] = _dt_visibility(table_settings, settings, this)
 * from set_default() where we pass the settings retrieved from DT's api
900         [hidden_ids, included_ids] = _dt_visibility(table_settings, settings, $("#"+settings.nTable.id));

4. Apply this patch, console.log again and confirm that it's now true.

Notice that everything is then buggy...

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 62 Paul Derscheid 2024-11-18 17:05:15 UTC
Created attachment 174742 [details] [review]
Bug 38436: Store loaded_from_state in data instead of DT's settings

This has been introduced by bug 33484 as we needed to know if the state
was loaded (from URL or localStorage).

If the state is loaded then we need to:
1. not redirect if only one patron to display (to prevent a blocking
   situation)
2. display the table (ie. not defer loading)

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 63 Paul Derscheid 2024-11-18 17:05:18 UTC
Created attachment 174743 [details] [review]
Bug 38436: Adjust column names for the holdings tables

Removing the prefix holdings_ and otherholdings_
Could have been done at the code level but does not seem necessary to
have different column names

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 64 Paul Derscheid 2024-11-18 17:05:21 UTC
Created attachment 174744 [details] [review]
Bug 38436: Rename columns_settings.columnname in DB

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 65 Paul Derscheid 2024-11-18 17:05:24 UTC
Created attachment 174745 [details] [review]
Bug 38436: Fix column visibility when bKohaColumnsUseNames is used

When the columns displayed in the table can vary (depending on sysprefs)
we use bKohaColumnsUseNames. This allows us to use the name of the
columns instead of there order number.

The code was totally wrong, and didn't retrieve the correct number.

A good example to confirm that is to add:
 console.log("%s is at %s".format(this.columnname, used_id));
at the end of the `if ( use_names ) {` block of _dt_visibility.

If at least one column is hidden by default and cannot be toggled (and
so won't appear in the list of the columns of the "Column visibility" button),
there will be a shift.

This line was definitelly wrong:
 var named_id = $( 'thead th[data-colname="' + this.columnname + '"]', selector ).index( selector + ' th' );
We cannot rely on the order of the visible columns, we need to retrieve
the list of the columns using DT's api.

Now the columns are retrieved using their class names, and from DT's
api.
The code is way nicer and more inline with how we are supposed to play
with DT.
Look at those 2 lines to pass the list of columns to the colvis button:
  let included_columns = table_settings.columns.filter(c => !c.cannot_be_toggled);
  columns: included_columns.map(c => '.' + c.columnname).join(',')

Test plan:
Play intensively with the items table (detail.pl), but also with the
other tables not using bKohaColumnsUseNames (eg. patrons search)
You need to set column as hidden by default and/or cannot be toggled.
You need to play with the "Columns" button.
Note that you need to remove the localStorage keys starting with 'DataTables_'
to retrieve the visibility settings from what is defined in the admin area.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 66 Paul Derscheid 2024-11-18 17:05:28 UTC
Created attachment 174746 [details] [review]
Bug 38436: Adjust columns_settings.inc for KohaTable

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 67 Paul Derscheid 2024-11-18 17:05:31 UTC
Created attachment 174747 [details] [review]
Bug 38436: Restore hide columns when empty

If a column has no data on the items table we should hide the column.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 68 Paul Derscheid 2024-11-18 17:05:34 UTC
Created attachment 174748 [details] [review]
Bug 38436: Do not hide all if none to hide

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 69 Paul Derscheid 2024-11-18 17:05:37 UTC
Created attachment 174749 [details] [review]
Bug 38436: Fix visibility when bKohaColumnsUseNames is not set

Typically the patrons search.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 70 Paul Derscheid 2024-11-18 17:05:41 UTC
Created attachment 174750 [details] [review]
Bug 38436: Adjust Vue

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 71 Paul Derscheid 2024-11-18 17:05:44 UTC
Created attachment 174751 [details] [review]
Bug 38436: Do not deal with 'Columns' button if no table settings

If the table does not have table settings we should not deal with the
column visibility and not display the 'Columns' button.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 72 Paul Derscheid 2024-11-18 17:05:47 UTC
Created attachment 174752 [details] [review]
Bug 38069: (follow-up) Fix jump to the correct table

https://bugs.koha-community.org/show_bug.cgi?id=38436
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 73 Paul Derscheid 2024-11-18 17:05:51 UTC
Created attachment 174753 [details] [review]
Bug 38436: (follow-up) Adjust columns_settings.inc for KohaTable

Previous commit didn't adjust columns_settings.inc

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 74 Paul Derscheid 2024-11-18 17:05:54 UTC
Created attachment 174754 [details] [review]
Bug 38436: (follow-up) Fix visibility when bKohaColumnsUseNames is not set

Previous commit didn't adjust columns_settings.inc

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 75 Paul Derscheid 2024-11-18 17:05:57 UTC
Created attachment 174755 [details] [review]
Bug 38436: Fix patron categories

Make it use bKohaColumnsUseNames as we are building the columns
depending on sysprefs.

Note that the hard-coded list of columns to export is wrong
  exportColumns: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12],
Must use .noExport instead.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 76 Paul Derscheid 2024-11-18 17:06:01 UTC
Created attachment 174756 [details] [review]
Bug 38436: (QA follow-up) Tidy atomicupdate file to satisfy qa script
Comment 77 Paul Derscheid 2024-11-18 17:07:36 UTC
I have thoroughly tested the tables. I think this is good to go but which such a big impact it's hard to be sure.

Anyway, passing QA.
Comment 78 Michaela Sieber 2024-11-18 18:07:05 UTC
Before you push this, please test this scenario :

search for "Programming the Perl DBI"  (= koha/catalogue/detail.pl?biblionumber=13 )

Check that the item has no itemcallnumber, that's why the column 'Call number' is not displayed

Check that the itemcallnumber in the admin table settings is NOT hidden by default (checkbox is not active)

Go back to biblionumber=13 and edit the item.
Add an itemcallnumber in the editor (Field  o - Full call number)

save the item.

Go again to the "Normal" detail view.


Now I have the problem, that I still don't see the column Call number.
Comment 79 Katrin Fischer 2024-11-18 18:19:08 UTC
(In reply to Michaela Sieber from comment #78)
> Before you push this, please test this scenario :
> 
> search for "Programming the Perl DBI"  (=
> koha/catalogue/detail.pl?biblionumber=13 )
> 
> Check that the item has no itemcallnumber, that's why the column 'Call
> number' is not displayed
> 
> Check that the itemcallnumber in the admin table settings is NOT hidden by
> default (checkbox is not active)
> 
> Go back to biblionumber=13 and edit the item.
> Add an itemcallnumber in the editor (Field  o - Full call number)
> 
> save the item.
> 
> Go again to the "Normal" detail view.
> 
> 
> Now I have the problem, that I still don't see the column Call number.

Jonathan, can you please have a look?

Michaela, if you still have that sandbox: could you try un-checking the "save state" in the table configuration and re-test? My guess would be that when you first viewed the table it saved the "state" and that made the column not appear when the data was filled in.
Comment 80 Paul Derscheid 2024-11-18 18:21:01 UTC
Testing this scenario.
Comment 81 Paul Derscheid 2024-11-18 18:35:32 UTC
Hi Michaela, I just tried to reproduce the problem you described in comment #78 but fortunately it's not an issue.

As cait suggested this is remedied by disabling the 'Save state' checkbox in the table configuration.
Comment 82 Paul Derscheid 2024-11-18 18:37:02 UTC
There's something weird going on here.

When doing QA, I resolved a merge conflict on Patch #8: Restore hide columns when empty.

I'd assume that when reattaching the patches after QA, that this would've been resolved.

It's still an issue, though. Maybe I am just to blind to spot it today.

Any help welcome! :)
Comment 83 Jonathan Druart 2024-11-19 05:52:21 UTC
(In reply to Paul Derscheid from comment #82)
> There's something weird going on here.
> 
> When doing QA, I resolved a merge conflict on Patch #8: Restore hide columns
> when empty.

Not sure what happened but my local branch still applies. I have added Owen and your's signed-off-by lines and push to my gitlab repo.
Comment 84 Jonathan Druart 2024-11-19 06:06:55 UTC
(In reply to Michaela Sieber from comment #78)
> Before you push this, please test this scenario :
> 
> search for "Programming the Perl DBI"  (=
> koha/catalogue/detail.pl?biblionumber=13 )
> 
> Check that the item has no itemcallnumber, that's why the column 'Call
> number' is not displayed
> 
> Check that the itemcallnumber in the admin table settings is NOT hidden by
> default (checkbox is not active)
> 
> Go back to biblionumber=13 and edit the item.
> Add an itemcallnumber in the editor (Field  o - Full call number)
> 
> save the item.
> 
> Go again to the "Normal" detail view.
> 
> 
> Now I have the problem, that I still don't see the column Call number.

Yes, this is a tricky one.
I thought I could fix this behaviour here but failed. It's reported separately on bug 38433 (reopened now).
If we are saving the state (column visibility), it is saved automatically when a visibility changes, manually or with this behaviour we have on this table (hide the column if empty).
To distinguish them we need to store separately the state desired by the user, and the "automatic state" generated by this "hide if empty" behaviour.

It should not be very hard, but this is too late for 24.11.00

If this is too confusing, maybe a solution for 24.11.00 could be to disable "save state" by default for this table, with a note in the setting.
Comment 85 Jonathan Druart 2024-11-19 06:09:00 UTC
Another suggestion: have a "show not empty columns" link at the top of the table if at least one column has data.
Comment 86 Michaela Sieber 2024-11-19 08:37:41 UTC
(In reply to Jonathan Druart from comment #84)
> (In reply to Michaela Sieber from comment #78)
> > Before you push this, please test this scenario :
> > 
> > search for "Programming the Perl DBI"  (=
> > koha/catalogue/detail.pl?biblionumber=13 )
> > 
> > Check that the item has no itemcallnumber, that's why the column 'Call
> > number' is not displayed
> > 
> > Check that the itemcallnumber in the admin table settings is NOT hidden by
> > default (checkbox is not active)
> > 
> > Go back to biblionumber=13 and edit the item.
> > Add an itemcallnumber in the editor (Field  o - Full call number)
> > 
> > save the item.
> > 
> > Go again to the "Normal" detail view.
> > 
> > 
> > Now I have the problem, that I still don't see the column Call number.
> 
> Yes, this is a tricky one.
> I thought I could fix this behaviour here but failed. It's reported
> separately on bug 38433 (reopened now).

Yes it is tricky. I read all your thoughts in this bug , thats why I tested it with the mentioned workflow

And to add new data to an item record is not a "special" use case, this is daily business in a library

> If we are saving the state (column visibility), it is saved automatically
> when a visibility changes, manually or with this behaviour we have on this
> table (hide the column if empty).
> To distinguish them we need to store separately the state desired by the
> user, and the "automatic state" generated by this "hide if empty" behaviour.

Exactly, that is the only solution.

> It should not be very hard, but this is too late for 24.11.00

Unfortunately.
 
> If this is too confusing, maybe a solution for 24.11.00 could be to disable
> "save state" by default for this table, with a note in the setting.

Yes, it definitly should be disabled because the feature makes no sense as it works not as an end-user would expect it.
Comment 87 Jonathan Druart 2024-11-19 09:29:32 UTC
(In reply to Michaela Sieber from comment #86)
> (In reply to Jonathan Druart from comment #84)
> > If this is too confusing, maybe a solution for 24.11.00 could be to disable
> > "save state" by default for this table, with a note in the setting.
> 
> Yes, it definitly should be disabled because the feature makes no sense as
> it works not as an end-user would expect it.

See bug 38482.
Comment 88 Jonathan Druart 2024-11-19 10:20:26 UTC
Created attachment 174766 [details] [review]
Bug 38436: Do not try to build the table if tab is not there

If we don't have the "other holdings" tab, no need to try to build the
table.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 89 Jonathan Druart 2024-11-19 10:20:29 UTC
Created attachment 174767 [details] [review]
Bug 38436: Store bKohaColumnsUseNames in data instead of DT's settings

DataTable's settings do no longer allow to store custom properties.
Here we are passing bKohaColumnsUseNames to the constructor, but we
cannot retrieve it later from DT's api.

We need to store it in data().

Test plan:
0. Do not apply this patch
1. In _dt_visibility add console.log(settings.bKohaColumnsUseNames);
   where relevant
2. Open your browser, clear localStorage entries (we want to reach the
   set_default() code)
3. Hit http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=4
Notice that the value is true then false (displayed once per columns).
This is because we are calling _dt_visibility twice:
 * with the 'settings' we passed to the constructor:
1044         [hidden_ids, included_ids] = _dt_visibility(table_settings, settings, this)
 * from set_default() where we pass the settings retrieved from DT's api
900         [hidden_ids, included_ids] = _dt_visibility(table_settings, settings, $("#"+settings.nTable.id));

4. Apply this patch, console.log again and confirm that it's now true.

Notice that everything is then buggy...

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 90 Jonathan Druart 2024-11-19 10:20:55 UTC
Created attachment 174768 [details] [review]
Bug 38436: Do not try to build the table if tab is not there

If we don't have the "other holdings" tab, no need to try to build the
table.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 91 Jonathan Druart 2024-11-19 10:20:59 UTC
Created attachment 174769 [details] [review]
Bug 38436: Store bKohaColumnsUseNames in data instead of DT's settings

DataTable's settings do no longer allow to store custom properties.
Here we are passing bKohaColumnsUseNames to the constructor, but we
cannot retrieve it later from DT's api.

We need to store it in data().

Test plan:
0. Do not apply this patch
1. In _dt_visibility add console.log(settings.bKohaColumnsUseNames);
   where relevant
2. Open your browser, clear localStorage entries (we want to reach the
   set_default() code)
3. Hit http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=4
Notice that the value is true then false (displayed once per columns).
This is because we are calling _dt_visibility twice:
 * with the 'settings' we passed to the constructor:
1044         [hidden_ids, included_ids] = _dt_visibility(table_settings, settings, this)
 * from set_default() where we pass the settings retrieved from DT's api
900         [hidden_ids, included_ids] = _dt_visibility(table_settings, settings, $("#"+settings.nTable.id));

4. Apply this patch, console.log again and confirm that it's now true.

Notice that everything is then buggy...

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 92 Jonathan Druart 2024-11-19 10:21:02 UTC
Created attachment 174770 [details] [review]
Bug 38436: Store loaded_from_state in data instead of DT's settings

This has been introduced by bug 33484 as we needed to know if the state
was loaded (from URL or localStorage).

If the state is loaded then we need to:
1. not redirect if only one patron to display (to prevent a blocking
   situation)
2. display the table (ie. not defer loading)

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 93 Jonathan Druart 2024-11-19 10:21:06 UTC
Created attachment 174771 [details] [review]
Bug 38436: Adjust column names for the holdings tables

Removing the prefix holdings_ and otherholdings_
Could have been done at the code level but does not seem necessary to
have different column names

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 94 Jonathan Druart 2024-11-19 10:21:09 UTC
Created attachment 174772 [details] [review]
Bug 38436: Rename columns_settings.columnname in DB

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 95 Jonathan Druart 2024-11-19 10:21:13 UTC
Created attachment 174773 [details] [review]
Bug 38436: Fix column visibility when bKohaColumnsUseNames is used

When the columns displayed in the table can vary (depending on sysprefs)
we use bKohaColumnsUseNames. This allows us to use the name of the
columns instead of there order number.

The code was totally wrong, and didn't retrieve the correct number.

A good example to confirm that is to add:
 console.log("%s is at %s".format(this.columnname, used_id));
at the end of the `if ( use_names ) {` block of _dt_visibility.

If at least one column is hidden by default and cannot be toggled (and
so won't appear in the list of the columns of the "Column visibility" button),
there will be a shift.

This line was definitelly wrong:
 var named_id = $( 'thead th[data-colname="' + this.columnname + '"]', selector ).index( selector + ' th' );
We cannot rely on the order of the visible columns, we need to retrieve
the list of the columns using DT's api.

Now the columns are retrieved using their class names, and from DT's
api.
The code is way nicer and more inline with how we are supposed to play
with DT.
Look at those 2 lines to pass the list of columns to the colvis button:
  let included_columns = table_settings.columns.filter(c => !c.cannot_be_toggled);
  columns: included_columns.map(c => '.' + c.columnname).join(',')

Test plan:
Play intensively with the items table (detail.pl), but also with the
other tables not using bKohaColumnsUseNames (eg. patrons search)
You need to set column as hidden by default and/or cannot be toggled.
You need to play with the "Columns" button.
Note that you need to remove the localStorage keys starting with 'DataTables_'
to retrieve the visibility settings from what is defined in the admin area.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 96 Jonathan Druart 2024-11-19 10:21:16 UTC
Created attachment 174774 [details] [review]
Bug 38436: Adjust columns_settings.inc for KohaTable

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 97 Jonathan Druart 2024-11-19 10:21:20 UTC
Created attachment 174775 [details] [review]
Bug 38436: Restore hide columns when empty

If a column has no data on the items table we should hide the column.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 98 Jonathan Druart 2024-11-19 10:21:23 UTC
Created attachment 174776 [details] [review]
Bug 38436: Do not hide all if none to hide

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 99 Jonathan Druart 2024-11-19 10:21:27 UTC
Created attachment 174777 [details] [review]
Bug 38436: Fix visibility when bKohaColumnsUseNames is not set

Typically the patrons search.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 100 Jonathan Druart 2024-11-19 10:21:29 UTC
Created attachment 174778 [details] [review]
Bug 38436: Adjust Vue

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 101 Jonathan Druart 2024-11-19 10:21:33 UTC
Created attachment 174779 [details] [review]
Bug 38436: Do not deal with 'Columns' button if no table settings

If the table does not have table settings we should not deal with the
column visibility and not display the 'Columns' button.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 102 Jonathan Druart 2024-11-19 10:21:36 UTC
Created attachment 174780 [details] [review]
Bug 38436: (follow-up) Adjust columns_settings.inc for KohaTable

Previous commit didn't adjust columns_settings.inc

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 103 Jonathan Druart 2024-11-19 10:21:39 UTC
Created attachment 174781 [details] [review]
Bug 38436: (follow-up) Fix visibility when bKohaColumnsUseNames is not set

Previous commit didn't adjust columns_settings.inc

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 104 Jonathan Druart 2024-11-19 10:21:42 UTC
Created attachment 174782 [details] [review]
Bug 38436: Fix patron categories

Make it use bKohaColumnsUseNames as we are building the columns
depending on sysprefs.

Note that the hard-coded list of columns to export is wrong
  exportColumns: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12],
Must use .noExport instead.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 105 Jonathan Druart 2024-11-20 09:34:59 UTC
Created attachment 174821 [details] [review]
Bug 38436: Fix OPAC tables

This patch sync columns_settings.inc for the OPAC-side

It however removes 'api' from datatables.js as it is never used.
'api' was renamed 'kohaTable' on bug 29408 but OPAC was forgotten.

Better to not have to deal with this unused code for now, we will
reintroduce it if needed later.
Comment 106 Jonathan Druart 2024-11-20 09:35:42 UTC
Please test this last patch if you can. It fixes (at least) the holdings table at the OPAC.
Comment 107 Katrin Fischer 2024-11-20 10:58:36 UTC
Going in...
Comment 108 Katrin Fischer 2024-11-20 13:21:34 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 109 Katrin Fischer 2024-11-20 17:57:10 UTC
(In reply to Katrin Fischer from comment #108)
> Pushed for 24.11!
> 
> Well done everyone, thank you!

Will fix the database update.
Comment 110 Katrin Fischer 2024-11-20 18:10:17 UTC
(In reply to Katrin Fischer from comment #109)
> (In reply to Katrin Fischer from comment #108)
> > Pushed for 24.11!
> > 
> > Well done everyone, thank you!
> 
> Will fix the database update.

REGEXP_REPLACE was an issue with old versions of MySQL, but I think it's in the past now. (bug 29805)
Comment 111 Katrin Fischer 2024-11-20 18:23:40 UTC
Database update pushed.
Comment 112 Jonathan Druart 2024-11-25 11:09:41 UTC
Created attachment 174967 [details] [review]
Bug 38436: Adjust DB update output
Comment 113 Katrin Fischer 2024-11-25 11:12:48 UTC
Picked last follow-up "Adjust DB..." for main.
Comment 114 Wainui Witika-Park 2024-12-16 02:49:10 UTC
(In reply to Katrin Fischer from comment #113)
> Picked last follow-up "Adjust DB..." for main.

Should I do the same for 24.05?
Comment 115 Katrin Fischer 2024-12-16 13:44:43 UTC
Hi Wainui, this depends on the datatables update not in 24.05
Comment 116 Wainui Witika-Park 2024-12-16 23:16:31 UTC
(In reply to Katrin Fischer from comment #115)
> Hi Wainui, this depends on the datatables update not in 24.05

Ok thanks Katrin!

Not backporting this to 24.05