Hi Owen,
The previously selected tab became selected. However, the data is incorrect. The listed reports came from "All" tab.

a gut yor Owen,

this doesn't seem to work for me. I don't see anymore any “Cookies.set("sql_reports_activetab", activeTab );” in your new patch, maybe that's why it's not set?

Should not we use localStorage or sessionStorage instead?

(In reply to Jonathan Druart from comment #7)
> Should not we use localStorage or sessionStorage instead?

localStorage is not designed for session-only storage, and cookies are. Using localStorage instead of cookies requires a layer of session management that isn't necessary with a cookie.

sessionStorage *should* be an option, but it only works for the current tab, and that limitation is not acceptable.

Cookies will be send to the server and back to the client, here we do not need that.

What about using localStorage and clear it on logout?

(In reply to Jonathan Druart from comment #9)
> Cookies will be send to the server and back to the client, here we do not
> need that.

We don't need it, but it's not very burdensome.

> What about using localStorage and clear it on logout?

Certainly possible, but it requires that we add each session-only localStorage item to a function which runs at logout. My vote is to use the browser feature which most closely matches our requirement.

(In reply to Owen Leonard from comment #10)
> Certainly possible, but it requires that we add each session-only
> localStorage item to a function which runs at logout.

...which we'll have to do anyway for cookies which need to be cleared on logout (instead of when the browser is closed). So never mind, don't listen to me.

Created attachment 105890 [details] [review]
Bug 24958: Remember last selected tab in SQL reports

This patch modifies the saved reports page so that it remembers, for the
duration of the browser session, the last active tab.

To test you should have multiple reports in multiple report groups.
Apply the patch and go to Reports -> Saved reports.

- Select a tab to filter the table of saved reports to a particular
  report group.
- Navigate away from the page
- Return to the save reports page. The tab you previously selected
  should be selected again.
- Restart your browser and return to the saved reports page. The tab
  should no longer be preselected.

Update: The tabs filtering JavaScript has been moved to a separate
function so that the function can be triggered by both the "create"
event (when the tabs are initialized) and the "activate" event (when a
tab is selected).

Update II: Persistence is now enable through localStorage instead of
Cookies. The localStorage item is now cleared during the logOut
function.

The function works well, but the test failed at this point:  "The localStorage item is now cleared during the logOut function."

Even when the user is logged out and the browser is restarted, the localStorage setting persists and does not clear.

Thanks for testing Sally. It seems to be working for me. Here's the process I use for testing (using Chrome on Mac):

https://zivotdesign.com/p/view.php?p=15928287071339

Created attachment 106168 [details]
Local Storage setting on log out

Ace, thanks, Owen.  Yeah, we're definitely following the same process - just a different end result.  I've added a screenshot so you can see the storage persisting after I've logged out.

My work PC is Windows, Chrome 83.

It's a managed browser, so I just tested on my Chromebook to see if that resolved it, but I get the same error.  That's Chrome OS, Chrome 83.

I was unable to reproduce this problem on Chrome 83 on Windows 10. I'm not sure where to go next with it!

Hi Owen,

This was baffling me too.  As you couldn't replicate it, I had another test to see if I could help:

The value in localStorage still persisted with the ordinary tests.

I noticed you were using a different account, so on the off chance, I created a new superlibrarian instead of using the koha default log in, but the value still persisted.

I then looked at the Diff on the bug, and ran the JS command from the console: 
 $(localStorage.removeItem("sql_reports_activetab"));

This removed the storage element, so it verified that the JS you've used was correct and there's nothing awry with the browsers I'm using.

I noticed that the command is next to the clearHoldFor() function in the JS file, so I tested to see if the clear hold function still worked, and sure enough:

- Load patron
- Use 'search to hold' function
- Log out
- Log back in
- Load record
- Place hold button still recalls the patron from step 1

So the JS command is correct, but perhaps the placement in the file has broken the functionality?

Hope that helps :)

I confirm the patch works for me.
However I am wondering if the flush should not happen in auth.tt instead.

(In reply to Sally from comment #18)
> - Place hold button still recalls the patron from step 1

I don't get this result either :(

(In reply to Jonathan Druart from comment #19)
> I confirm the patch works for me.
> However I am wondering if the flush should not happen in auth.tt instead.

Are you suggesting we move all the cookie and local-storage clearing to auth.tt? I added my change to staff-global.js because that's where similar functions were being executed.

Isn't it better to clear this data when the user logs out rather than when logging in? Or am I misunderstanding your suggestion?

Owen,

a user can be logged out when they click the logout link (what your patch supports) but also if 1. the session expired or even 2. they don't have necessary permissions.
In those 2 cases (maybe not exhaustive) the code you added won't be triggered (I have not test, only read the code) and the variables won't be removed.

To be on the safe side maybe we should have the code in both places.

Created attachment 109967 [details] [review]
Bug 24958: Remember last selected tab in SQL reports

This patch modifies the saved reports page so that it remembers, for the
duration of the browser session, the last active tab.

To test you should have multiple reports in multiple report groups.
Apply the patch and go to Reports -> Saved reports.

- Select a tab to filter the table of saved reports to a particular
  report group.
- Navigate away from the page
- Return to the save reports page. The tab you previously selected
  should be selected again.
- Restart your browser and return to the saved reports page. The tab
  should no longer be preselected.

Update: The tabs filtering JavaScript has been moved to a separate
function so that the function can be triggered by both the "create"
event (when the tabs are initialized) and the "activate" event (when a
tab is selected).

Update II: Persistence is now enable through localStorage instead of
Cookies. The localStorage item is now cleared during the logOut
function.

Update III: The logOut() function in staff-global.js is now called by
auth.tt to ensure that tabs are not remembered across sessions.

Created attachment 110741 [details] [review]
Bug 24958: Remember last selected tab in SQL reports

This patch modifies the saved reports page so that it remembers, for the
duration of the browser session, the last active tab.

To test you should have multiple reports in multiple report groups.
Apply the patch and go to Reports -> Saved reports.

- Select a tab to filter the table of saved reports to a particular
  report group.
- Navigate away from the page
- Return to the save reports page. The tab you previously selected
  should be selected again.
- Restart your browser and return to the saved reports page. The tab
  should no longer be preselected.

Update: The tabs filtering JavaScript has been moved to a separate
function so that the function can be triggered by both the "create"
event (when the tabs are initialized) and the "activate" event (when a
tab is selected).

Update II: Persistence is now enable through localStorage instead of
Cookies. The localStorage item is now cleared during the logOut
function.

Update III: The logOut() function in staff-global.js is now called by
auth.tt to ensure that tabs are not remembered across sessions.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Nice.

I've seen comment 21 but not sure I understand all of it.

I signoff.

Created attachment 110785 [details] [review]
Bug 24958: Remember last selected tab in SQL reports

This patch modifies the saved reports page so that it remembers, for the
duration of the browser session, the last active tab.

To test you should have multiple reports in multiple report groups.
Apply the patch and go to Reports -> Saved reports.

- Select a tab to filter the table of saved reports to a particular
  report group.
- Navigate away from the page
- Return to the save reports page. The tab you previously selected
  should be selected again.
- Restart your browser and return to the saved reports page. The tab
  should no longer be preselected.

Update: The tabs filtering JavaScript has been moved to a separate
function so that the function can be triggered by both the "create"
event (when the tabs are initialized) and the "activate" event (when a
tab is selected).

Update II: Persistence is now enable through localStorage instead of
Cookies. The localStorage item is now cleared during the logOut
function.

Update III: The logOut() function in staff-global.js is now called by
auth.tt to ensure that tabs are not remembered across sessions.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Pushed to master for 20.11, thanks to everybody involved!

enhancement will not be backported to 20.05.x