Bug 33484 - Generalise table saveState for all tables
Summary: Generalise table saveState for all tables
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on: 22276 33426
Blocks:
  Show dependency treegraph
 
Reported: 2023-04-11 12:49 UTC by Martin Renvoize
Modified: 2024-01-09 14:22 UTC (History)
10 users (show)

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


Attachments
Bug 33484: Make saveState column handling global (1.98 KB, patch)
2023-04-11 14:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33484: Revert template change introduced in bug 22276 (2.10 KB, patch)
2023-04-11 14:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33484: Make saveState column handling global (1.98 KB, patch)
2023-04-11 14:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33484: Revert template change introduced in bug 22276 (2.10 KB, patch)
2023-04-11 14:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33484: Revert template change on suggestion.tt (1.68 KB, patch)
2023-04-11 14:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33484: Make saveState column handling global (1.98 KB, patch)
2023-04-20 10:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33484: Revert template change introduced in bug 22276 (2.10 KB, patch)
2023-04-20 10:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33484: Revert template change on suggestion.tt (1.68 KB, patch)
2023-04-20 10:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33484: Add support to kohaTable (AJAX Tables) (2.57 KB, patch)
2023-04-20 10:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33484: Add save state option to table settings (22.17 KB, patch)
2023-04-20 10:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33484: Schema Update (1.48 KB, patch)
2023-04-20 10:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33484: Make saveState column handling global (1.98 KB, patch)
2023-04-20 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33484: Revert template change introduced in bug 22276 (2.10 KB, patch)
2023-04-20 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33484: Revert template change on suggestion.tt (1.68 KB, patch)
2023-04-20 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33484: Add support to kohaTable (AJAX Tables) (2.57 KB, patch)
2023-04-20 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33484: Add save state option to table settings (22.17 KB, patch)
2023-04-20 16:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33484: Schema Update (1.48 KB, patch)
2023-04-20 16:17 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2023-04-11 12:49:58 UTC
We should really make bug 22276 work globally rather than having a bug per table to add this feature.
Comment 1 Martin Renvoize 2023-04-11 14:05:08 UTC
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.
Comment 2 Martin Renvoize 2023-04-11 14:05:10 UTC
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
Comment 3 Martin Renvoize 2023-04-11 14:10:30 UTC
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.
Comment 4 Martin Renvoize 2023-04-11 14:17:22 UTC
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.
Comment 5 Martin Renvoize 2023-04-11 14:17:25 UTC
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
Comment 6 Martin Renvoize 2023-04-11 14:17:27 UTC
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
Comment 7 Emily Lamancusa 2023-04-11 14:58:20 UTC
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)
Comment 8 Emily Lamancusa 2023-04-11 19:42:39 UTC
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)?
Comment 9 Katrin Fischer 2023-04-11 20:05:13 UTC
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...
Comment 10 Lisette Scheer 2023-04-19 16:10:13 UTC
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
Comment 11 Martin Renvoize 2023-04-20 08:20:48 UTC
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.
Comment 12 Martin Renvoize 2023-04-20 10:25:27 UTC Comment hidden (obsolete)
Comment 13 Martin Renvoize 2023-04-20 10:25:30 UTC Comment hidden (obsolete)
Comment 14 Martin Renvoize 2023-04-20 10:25:33 UTC Comment hidden (obsolete)
Comment 15 Martin Renvoize 2023-04-20 10:25:36 UTC Comment hidden (obsolete)
Comment 16 Martin Renvoize 2023-04-20 10:25:39 UTC Comment hidden (obsolete)
Comment 17 Martin Renvoize 2023-04-20 10:25:41 UTC Comment hidden (obsolete)
Comment 18 Martin Renvoize 2023-04-20 10:31:19 UTC
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.
Comment 19 Martin Renvoize 2023-04-20 10:31:53 UTC
PS, you'll need to run the DB update ;P
Comment 20 Lisette Scheer 2023-04-20 15:05:05 UTC
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
Comment 21 Martin Renvoize 2023-04-20 16:15:36 UTC
I can't replicate that I'm afraid.. I just freshly applied the bug and it's all working for me.
Comment 22 Martin Renvoize 2023-04-20 16:16:49 UTC Comment hidden (obsolete)
Comment 23 Martin Renvoize 2023-04-20 16:16:52 UTC Comment hidden (obsolete)
Comment 24 Martin Renvoize 2023-04-20 16:16:55 UTC Comment hidden (obsolete)
Comment 25 Martin Renvoize 2023-04-20 16:16:58 UTC Comment hidden (obsolete)
Comment 26 Martin Renvoize 2023-04-20 16:17:01 UTC Comment hidden (obsolete)
Comment 27 Martin Renvoize 2023-04-20 16:17:04 UTC Comment hidden (obsolete)
Comment 28 Martin Renvoize 2023-04-20 16:17:59 UTC
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.
Comment 29 Emily Lamancusa 2023-04-20 16:33:00 UTC
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.
Comment 30 Lisette Scheer 2023-04-20 17:03:48 UTC
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.
Comment 31 Emily Lamancusa 2023-04-26 21:26:02 UTC
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
Comment 32 Emily Lamancusa 2023-07-07 14:58:02 UTC
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);
-        }