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
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 :-/
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.
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.
Switched to "Needs Signoff" but it's not ready for inclusion.
*** Bug 38433 has been marked as a duplicate of this bug. ***
*** Bug 38456 has been marked as a duplicate of this bug. ***
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.
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...
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)
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
Created attachment 174591 [details] [review] Bug 38436: Rename columns_settings.columnname in DB
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.
Created attachment 174593 [details] [review] Bug 38436: Adjust columns_settings.inc for KohaTable
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.
Created attachment 174595 [details] [review] Bug 38436: Do not hide all if none to hide
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").
(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) ´´´)
Created attachment 174634 [details] [review] Bug 38436: Fix visibility when bKohaColumnsUseNames is not set Typically the patrons search.
(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.
Created attachment 174641 [details] [review] Bug 38436: Adjust Vue
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.
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
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.
(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.
Created attachment 174688 [details] [review] Bug 38436: (follow-up) Adjust columns_settings.inc for KohaTable Previous commit didn't adjust columns_settings.inc
Created attachment 174689 [details] [review] Bug 38436: Fix visibility when bKohaColumnsUseNames is not set Previous commit didn't adjust columns_settings.inc
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.
(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.
Created attachment 174691 [details] [review] Bug 38436: (follow-up) Fix visibility when bKohaColumnsUseNames is not set Previous commit didn't adjust columns_settings.inc
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.
Created attachment 174693 [details] [review] Bug 38436: Adjust columns_settings.inc for KohaTable
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.
Created attachment 174695 [details] [review] Bug 38436: Do not hide all if none to hide
Created attachment 174696 [details] [review] Bug 38436: Fix visibility when bKohaColumnsUseNames is not set Typically the patrons search.
Created attachment 174697 [details] [review] Bug 38436: Adjust Vue
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.
Created attachment 174699 [details] [review] Bug 38069: (follow-up) Fix jump to the correct table
Created attachment 174700 [details] [review] Bug 38436: (follow-up) Adjust columns_settings.inc for KohaTable Previous commit didn't adjust columns_settings.inc
Created attachment 174701 [details] [review] Bug 38436: (follow-up) Fix visibility when bKohaColumnsUseNames is not set Previous commit didn't adjust columns_settings.inc
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.
(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).
Where are we at here?
Working on it, testing this is a bit of work.
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>
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>
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>
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>
Created attachment 174725 [details] [review] Bug 38436: Rename columns_settings.columnname in DB Signed-off-by: Owen Leonard <oleonard@myacpl.org>
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>
Created attachment 174727 [details] [review] Bug 38436: Adjust columns_settings.inc for KohaTable Signed-off-by: Owen Leonard <oleonard@myacpl.org>
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>
Created attachment 174729 [details] [review] Bug 38436: Do not hide all if none to hide Signed-off-by: Owen Leonard <oleonard@myacpl.org>
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>
Created attachment 174731 [details] [review] Bug 38436: Adjust Vue Signed-off-by: Owen Leonard <oleonard@myacpl.org>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Created attachment 174756 [details] [review] Bug 38436: (QA follow-up) Tidy atomicupdate file to satisfy qa script
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.
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.
(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.
Testing this scenario.
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.
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! :)
(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.
(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.
Another suggestion: have a "show not empty columns" link at the top of the table if at least one column has data.
(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.
(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.
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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.
Please test this last patch if you can. It fixes (at least) the holdings table at the OPAC.
Going in...
Pushed for 24.11! Well done everyone, thank you!
(In reply to Katrin Fischer from comment #108) > Pushed for 24.11! > > Well done everyone, thank you! Will fix the database update.
(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)
Database update pushed.
Created attachment 174967 [details] [review] Bug 38436: Adjust DB update output
Picked last follow-up "Adjust DB..." for main.
(In reply to Katrin Fischer from comment #113) > Picked last follow-up "Adjust DB..." for main. Should I do the same for 24.05?
Hi Wainui, this depends on the datatables update not in 24.05
(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