Bug 24958 - Remember last selected tab in SQL reports
Summary: Remember last selected tab in SQL reports
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Owen Leonard
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 29225
  Show dependency treegraph
 
Reported: 2020-03-23 19:23 UTC by Owen Leonard
Modified: 2021-10-14 12:33 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
20.11.00


Attachments
Bug 24958: Remember last selected tab in SQL reports (2.25 KB, patch)
2020-04-06 15:58 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24958: Remember last selected tab in SQL reports (5.46 KB, patch)
2020-04-09 13:20 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24958: Remember last selected tab in SQL reports (5.58 KB, patch)
2020-04-14 18:56 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24958: Remember last selected tab in SQL reports (5.63 KB, patch)
2020-04-20 09:55 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 24958: Remember last selected tab in SQL reports (6.22 KB, patch)
2020-06-15 19:23 UTC, Owen Leonard
Details | Diff | Splinter Review
Local Storage setting on log out (46.33 KB, image/png)
2020-06-22 15:23 UTC, Sally
Details
Bug 24958: Remember last selected tab in SQL reports (7.12 KB, patch)
2020-09-11 18:21 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24958: Remember last selected tab in SQL reports (7.16 KB, patch)
2020-09-25 10:13 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 24958: Remember last selected tab in SQL reports (7.24 KB, patch)
2020-09-26 17:30 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2020-03-23 19:23:09 UTC
I think it would be useful for the saved reports page to remember which tab the user last suggested. I propose to set a cookie to store the information.
Comment 1 Owen Leonard 2020-04-06 15:58:02 UTC Comment hidden (obsolete)
Comment 2 Devinim 2020-04-06 19:33:01 UTC
Hi Owen,
The previously selected tab became selected. However, the data is incorrect. The listed reports came from "All" tab.
Comment 3 Owen Leonard 2020-04-09 13:20:27 UTC Comment hidden (obsolete)
Comment 4 Nicolas Legrand 2020-04-10 13:22:15 UTC
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?
Comment 5 Owen Leonard 2020-04-14 18:56:37 UTC Comment hidden (obsolete)
Comment 6 Nicolas Legrand 2020-04-20 09:55:11 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2020-04-20 11:49:43 UTC
Should not we use localStorage or sessionStorage instead?
Comment 8 Owen Leonard 2020-04-20 11:55:00 UTC
(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.
Comment 9 Jonathan Druart 2020-04-20 12:01:52 UTC
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?
Comment 10 Owen Leonard 2020-04-20 12:52:36 UTC
(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.
Comment 11 Owen Leonard 2020-04-20 15:07:07 UTC
(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.
Comment 12 Owen Leonard 2020-06-15 19:23:48 UTC
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.
Comment 13 Sally 2020-06-22 10:54:10 UTC
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.
Comment 14 Owen Leonard 2020-06-22 12:25:44 UTC
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
Comment 15 Sally 2020-06-22 15:23:38 UTC
Created attachment 106168 [details]
Local Storage setting on log out
Comment 16 Sally 2020-06-22 15:29:16 UTC
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.
Comment 17 Owen Leonard 2020-06-24 11:20:26 UTC
I was unable to reproduce this problem on Chrome 83 on Windows 10. I'm not sure where to go next with it!
Comment 18 Sally 2020-06-25 09:47:25 UTC
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 :)
Comment 19 Jonathan Druart 2020-06-25 10:47:11 UTC
I confirm the patch works for me.
However I am wondering if the flush should not happen in auth.tt instead.
Comment 20 Owen Leonard 2020-07-16 18:14:00 UTC
(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?
Comment 21 Jonathan Druart 2020-07-27 10:54:00 UTC
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.
Comment 22 Owen Leonard 2020-09-11 18:21:41 UTC
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.
Comment 23 Fridolin Somers 2020-09-25 10:13:54 UTC
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>
Comment 24 Fridolin Somers 2020-09-25 10:15:52 UTC
Nice.

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

I signoff.
Comment 25 Katrin Fischer 2020-09-26 17:30:08 UTC
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>
Comment 26 Jonathan Druart 2020-09-29 12:29:47 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 27 Lucas Gass 2020-10-20 16:25:06 UTC
enhancement will not be backported to 20.05.x