We should really make bug 22276 work globally rather than having a bug per table to add this feature.
Created attachment 149444 [details] [review] Bug 33484: Make saveState column handling global This patch updates the handling for saveState added in bug 22276 to work globally for all KohaTables that pass a table_settings hash.
Created attachment 149445 [details] [review] Bug 33484: Revert template change introduced in bug 22276 With the globalisation of the stateSave handling introduced by bug 22276 we can remove the localised changes to the template in branches.tt
To Test. 1) Confirm the test plan for bug 22276 still passes. 2) Check other instances where KohaTable is used and table_settings (i.e. you can choose what columns to display) is passed.. all should now save and restore state upon navigating back to them I'm not 100% sure about the original patch.. I feel it does more than expected in so much as saveState also saves and restores pagination, search filters etc etc. Secondly.. whilst I do this for the KohaTable (found in column_settings.inc).. I haven't added it to the DT REST wrapper.. or Vue components.. we should really do all three... but I wanted to start getting feedback first.
Created attachment 149447 [details] [review] Bug 33484: Make saveState column handling global This patch updates the handling for saveState added in bug 22276 to work globally for all KohaTables that pass a table_settings hash.
Created attachment 149448 [details] [review] Bug 33484: Revert template change introduced in bug 22276 With the globalisation of the stateSave handling introduced by bug 22276 we can remove the localised changes to the template in branches.tt
Created attachment 149449 [details] [review] Bug 33484: Revert template change on suggestion.tt This reverts commit 2b0693dbfd6aa2dd2c7c91428b23aba717ccc0af as it's no longer required once we globalise the logic as we do in this patchset
This works on some pages, but not on others. I have a meeting in a few minutes and so can't test them all at the moment, but this is what I've found so far: Pages that work: pendingreserves.pl suggestions.pl Pages that don't retain the save state: branches.pl members-home.pl (table id: memberresults)
Update: The tables that aren't retaining the save state are generated with the REST API wrapper from datatables.js, not the KohaTable wrapper from column_settings.inc. All the tables I've tested that use the KohaTable wrapper do retain the save state as intended. I think I do need to set this in its current state to Failed QA for causing a regression on branches.tt, though. As for the fact that saveState saves pagination and filters, there are certainly times when that's less desirable, but there are times when it is desirable too. Part of what spurred me to create bug 33426 was that our selectors want that behavior on the purchase suggestion table. Also, to be fair, the same could be said for saving the column configuration - there are definitely times when staff hide certain columns to make a page print more nicely, but don't necessarily need that state to persist. That being said...there are definitely some tables where saving the search filters is almost never going to be desirable. For example, on holdings_table in detail.pl, because the search filter on one bib will carry over to a different bib where that search filter may not even make sense. Not sure what the right answer is here... Go back to adding saveState only to certain tables individually, where it's actually needed? Add it globally but make it configurable (which would add some scope creep)?
Hi Emily, thanks for your comments! I hope I won't get myself into trouble for this... but could it be a flag we set on the column configuration page? But that would bring us back to having a different behaviour for column controlled vs. column uncontrolled tables...
Would it be possible to have it save as part of the saveState an option for keeping or clearing filters and have it clear filters by default, but someone could turn it on for a table if they wanted? Lisette
I've got a patch for the AJAX datatables now, and I'm working through making it all configurable through the table settings page. I'll need a hand working through the Vue table component, but I don't imagine it'll be too honerous. I'll throw the patches up as soon as I've got the configuration patch working.
Created attachment 149925 [details] [review] Bug 33484: Make saveState column handling global This patch updates the handling for saveState added in bug 22276 to work globally for all KohaTables that pass a table_settings hash.
Created attachment 149926 [details] [review] Bug 33484: Revert template change introduced in bug 22276 With the globalisation of the stateSave handling introduced by bug 22276 we can remove the localised changes to the template in branches.tt
Created attachment 149927 [details] [review] Bug 33484: Revert template change on suggestion.tt This reverts commit 2b0693dbfd6aa2dd2c7c91428b23aba717ccc0af as it's no longer required once we globalise the logic as we do in this patchset
Created attachment 149928 [details] [review] Bug 33484: Add support to kohaTable (AJAX Tables)
Created attachment 149929 [details] [review] Bug 33484: Add save state option to table settings
Created attachment 149930 [details] [review] Bug 33484: Schema Update
OK.. we now have this applying to KohaTable and kohaTable components (<KohaTable isn't yet covered). We also have the requested switches so one can enable/disable the filter state saving from the admin table settings page. Feel free to test and give feedback.
PS, you'll need to run the DB update ;P
None of the koha tables show anymore and the console is showing these errors/warnings: caught SyntaxError: Unexpected token '<<' jquery-3.6.0.min_22.1200016.js:2 jQuery.Deferred exception: $(...).kohaTable is not a function TypeError: $(...).kohaTable is not a function at HTMLDocument.<anonymous> (https://staff-save2.sandbox.bywatersolutions.com/cgi-bin/koha/admin/branches.pl:1632:45) at e (https://staff-save2.sandbox.bywatersolutions.com/intranet-tmpl/lib/jquery/jquery-3.6.0.min_22.1200016.js:2:30038) at t (https://staff-save2.sandbox.bywatersolutions.com/intranet-tmpl/lib/jquery/jquery-3.6.0.min_22.1200016.js:2:30340) undefined S.Deferred.exceptionHook @ jquery-3.6.0.min_22.1200016.js:2 jquery-3.6.0.min_22.1200016.js:2 Uncaught TypeError: $(...).kohaTable is not a function at HTMLDocument.<anonymous> (branches.pl:1632:45) at e (jquery-3.6.0.min_22.1200016.js:2:30038) at t (jquery-3.6.0.min_22.1200016.js:2:30340) Lisette
I can't replicate that I'm afraid.. I just freshly applied the bug and it's all working for me.
Created attachment 149974 [details] [review] Bug 33484: Make saveState column handling global This patch updates the handling for saveState added in bug 22276 to work globally for all KohaTables that pass a table_settings hash.
Created attachment 149975 [details] [review] Bug 33484: Revert template change introduced in bug 22276 With the globalisation of the stateSave handling introduced by bug 22276 we can remove the localised changes to the template in branches.tt
Created attachment 149976 [details] [review] Bug 33484: Revert template change on suggestion.tt This reverts commit 2b0693dbfd6aa2dd2c7c91428b23aba717ccc0af as it's no longer required once we globalise the logic as we do in this patchset
Created attachment 149977 [details] [review] Bug 33484: Add support to kohaTable (AJAX Tables)
Created attachment 149978 [details] [review] Bug 33484: Add save state option to table settings
Created attachment 149979 [details] [review] Bug 33484: Schema Update
Re-attached the patches, but nothings changed in them.. The message you report looks like a merge issue, but I can't see any such markers in the patches.
I can't reproduce the error on KTD either. Lisette, can you give more information about how to reproduce it? I am noticing so far a few other unexpected behaviors so far, though: - Holds to Pull saves the client-side state regardless of the configuration on the admin table settings page - Holds Queue appears to be working correctly for the most part. If you disable state saving from the admin table settings page, the table will continue to save the client-side state for the rest of the session, but will respect the configuration if you start a new session. (I'm not sure if that's an error or if it's the expected behavior.) - memberresultst and branches.pl (both of which use the kohaTable API wrapper rather than the KohaTable wrapper) appear to be working correctly for the most part. However, if the admin table settings hide a column by default, that column stays hidden even when state saving is enabled. Other aspects of client-side state saving (hiding columns that are shown by default, entries per page) work, though. Also, this was the case for branches.pl in master, too, so it wasn't introduced by this patch. I really like where this is going - but I think it's going to need more thorough/systematic testing than what I've done so far to catch any anomalies.
I'm no longer getting that error, but it's still not working. Likely a problem with my KTD. I'll try and sort that out before testing again.
I wrote up a test plan to keep myself organized while testing different tables and states...sharing in case someone else gets to it before me and would like to use it. (also open to feedback :) ) Considerations: - test a variety of tables - must include both tables that use KohaTable (column_settings.inc) and kohaTable (datatables.js) For reference: tables that use kohaTable (based on a git grep): - patron search (patrons_table) - acqui/parcel.tt (pending_orders) - admin/background_jobs.tt (table_jobs) - admin/branches.tt (libraries) - admin/cities.tt (table_cities) - admin/identity_provider_domains.tt (identity_provider_domains) - admin/identity_providers.tt (identity_providers) - admin/search_filters.tt (search_filters_table) - admin/smtp_servers.tt (smtp_servers) - catalogue/detail.tt (table_concerns) - catalogue/detail.tt (bundle_table) - cataloguing/concerns.tt (table_concerns) - pos/register.tt (table_cashups) - tools/quotes.tt (quotes) Test plan per table: 1. Edit the config to hide some columns and disable "save state" 2. Test that the "clean" page matches the config settings 3. Make some client-side changes a. Hide one or more showing columns b. Show one or more hidden columns c. Change number of results to display d. Add a search filter 4. Navigate away from that page and back 5. Confirm that the state was not saved and the page matches the config settings 6. Navigate away from that page 7. Edit the config to enable "save state" 8. Return to the page and confirm that it matches the config settings 9. Make some client-side changes a. Hide one or more showing columns b. Show one or more hidden columns c. Change number of results to display d. Add a search filter 10. Navigate away from the page and back 11. Confirm that all of the changes were retained
Taking another stab at testing this. I'm still getting a few tables (holds to pull and saved SQL reports, at least) that will continue to save the local configuration indefinitely if a local save state was ever created, even if it was later disabled in the configuration. I couldn't figure out why it happens with some tables and not others, but it did clear up when I tried moving the block that clears the local save state (if configured not to save) to occur before trying to load the local save state: Lines ~21-53 (columns_settings.inc): + if ( table_settings.hasOwnProperty('default_save_state') && table_settings['default_save_state'] === 0 ) { + localStorage.removeItem(table_key); + } + // enable stateSave dt_parameters['stateSave'] = true; dt_parameters['stateSaveCallback'] = function( settings, data ) { localStorage.setItem( table_key, JSON.stringify(data) ) } dt_parameters['stateLoadCallback'] = function(settings) { return JSON.parse( localStorage.getItem(table_key) ) } // merge stateSave for column settings var local_settings = localStorage.getItem(table_key); var system_settings = table_settings['columns']; var columns_settings = get_columns_saved_state(local_settings, system_settings); $(columns_settings).each( function() { var named_id = $( 'thead th[data-colname="' + this.columnname + '"]', selector ).index( selector + ' th' ); var used_id = dt_parameters.bKohaColumnsUseNames ? named_id : counter; if ( used_id == -1 ) return; if ( this['is_hidden'] == "1" ) { hidden_ids.push( used_id ); } if ( this['cannot_be_toggled'] == "0" ) { included_ids.push( used_id ); } counter++; }); - if ( table_settings.hasOwnProperty('default_save_state') && table_settings['default_save_state'] === 0 ) { - localStorage.removeItem(table_key); - }