Bug 31250 - Don't remove advanced/basic cataloging editor cookie on logout
Summary: Don't remove advanced/basic cataloging editor cookie on logout
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-07-27 20:48 UTC by Lucas Gass
Modified: 2022-11-20 00:34 UTC (History)
8 users (show)

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


Attachments
Bug 31250: Deny clearing cookies with numeric suffix (2.71 KB, patch)
2022-07-28 12:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31250: Add catalogue_editor_ cookie to koha-conf deny list (1.53 KB, patch)
2022-07-28 12:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31250: Deny clearing cookies with numeric suffix (3.28 KB, patch)
2022-08-10 13:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31250: Add catalogue_editor_ cookie to koha-conf deny list (1.55 KB, patch)
2022-08-10 13:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31250: (QA follow-up) The future will be easier (5.84 KB, patch)
2022-08-18 14:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31250: (QA follow-up) The future will be easier (6.12 KB, patch)
2022-08-22 08:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31250: Deny clearing cookies with numeric suffix (3.33 KB, patch)
2022-09-02 16:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 31250: Deny clearing cookies with numeric suffix (3.33 KB, patch)
2022-09-02 16:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 31250: Add catalogue_editor_ cookie to koha-conf deny list (1.61 KB, patch)
2022-09-02 16:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 31250: (QA follow-up) The future will be easier (6.13 KB, patch)
2022-09-02 16:06 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass 2022-07-27 20:48:03 UTC
Bug 29957 removed cookies upon logout and I think this is impacting catalogers choice of advanced vs basic cataloging editor.

Before Bug 29957 a cataloger could set the editor to basic or advanced and that choice would be remembered after logout. Now catalogers who logout and log back in will always be defaulted to the basic editor.
Comment 1 Liz Rea 2022-07-27 20:50:34 UTC
On bug 22957 there was a comment: 

"I'd love to see a 'Private computer' checkbox on login screens next and have a list of cookies to keep should that checkbox be ticked at login vs a completely public computer where I still think we should remove all by default." 

I think this is not the worst ever solution for this. It happens to be how the bank, for example, often does it.
Comment 2 Lucas Gass 2022-07-27 20:51:31 UTC
(In reply to Liz Rea from comment #1)
> On bug 22957 there was a comment: 
> 
> "I'd love to see a 'Private computer' checkbox on login screens next and
> have a list of cookies to keep should that checkbox be ticked at login vs a
> completely public computer where I still think we should remove all by
> default." 
> 
> I think this is not the worst ever solution for this. It happens to be how
> the bank, for example, often does it.

This does seem like the best solution for this, IMHO.
Comment 3 Marcel de Rooy 2022-07-28 12:46:35 UTC
The referenced bug allows you to specify cookies not to be removed:

See koha-conf.xml

 <!-- Uncomment lines like hereunder to not clear cookies at logout:
      The cookie name is case sensitive.
     <do_not_remove_cookie>KohaOpacLanguage</do_not_remove_cookie>
 -->

The problem is that it sets a cookie for each user:
Cookies.set( 'catalogue_editor_[% logged_in_user.borrowernumber | html %]', 'advanced', { expires: 365, path: '/', sameSite: 'Lax'  } );

Adding a fix for that
Comment 4 Marcel de Rooy 2022-07-28 12:47:12 UTC
Created attachment 138203 [details] [review]
Bug 31250: Deny clearing cookies with numeric suffix

This change allows us to add catalogue_editor_ to the deny list
in koha-conf.xml and keep cookies like catalogue_editor_123.

Test plan:
Run t/CookieManager.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2022-07-28 12:54:44 UTC
Created attachment 138204 [details] [review]
Bug 31250: Add catalogue_editor_ cookie to koha-conf deny list

Test plan:
Add this change to your koha-conf.xml.
Flush, restart.
Test if the cookie is kept now in the interface.
Comment 6 Marcel de Rooy 2022-07-28 13:09:29 UTC
Lucas, I could imagine that we call this a bug for backporting. Especially the first patch. Note that we do not touch koha-conf at upgrades, so it still needs koha-conf editing. Which might make backporting a bit less interesting..
Comment 7 Liz Rea 2022-08-02 18:53:51 UTC
knowing it *can* be fixed is a huge thing - I agree it's interesting for a backport. :D
Comment 8 Nick Clemens 2022-08-10 13:22:11 UTC
Created attachment 138981 [details] [review]
Bug 31250: Deny clearing cookies with numeric suffix

This change allows us to add catalogue_editor_ to the deny list
in koha-conf.xml and keep cookies like catalogue_editor_123.

Test plan:
Run t/CookieManager.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

To test:
1 - Sign in to staff client
2 - Search for records and edit one
3 - Switch to advanced editor
4 - View cookies (F12/developer panel/storage tab)
5 - Note cookie like 'catalogue_editor_##' with value 'advanced'
6 - Log out
7 - note cookie value deleted
8 - Log in and search/edit a record
9 - Basic editor loads
10 - Apply patch
11 - Add line to koha-conf as described in second patch
12 - Restart all
13 - Switch to advanced editor
14 - Cookie value updated
15 - Logout
16 - Cookie value remains
17 - Log in and search/edit
18 - Confirm advanced editor loads

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Nick Clemens 2022-08-10 13:22:14 UTC
Created attachment 138982 [details] [review]
Bug 31250: Add catalogue_editor_ cookie to koha-conf deny list

Test plan:
Add this change to your koha-conf.xml.
Flush, restart.
Test if the cookie is kept now in the interface.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Jonathan Druart 2022-08-17 10:05:11 UTC
hum, why not supporting a wildcard then? catalogue_editor_*

Seems more flexible.
Comment 11 Marcel de Rooy 2022-08-17 11:14:42 UTC
(In reply to Jonathan Druart from comment #10)
> hum, why not supporting a wildcard then? catalogue_editor_*
> 
> Seems more flexible.

We do not need it currently. Can be done on a later report imo. This is ready to go.
Comment 12 Jonathan Druart 2022-08-17 13:19:59 UTC
(In reply to Marcel de Rooy from comment #11)
> (In reply to Jonathan Druart from comment #10)
> > hum, why not supporting a wildcard then? catalogue_editor_*
> > 
> > Seems more flexible.
> 
> We do not need it currently. Can be done on a later report imo. This is
> ready to go.

How will you modified the config files? Once this is pushed you will have to keep the behaviour. If we propose a generic solution from the beginning, future will be easier ;)
Comment 13 Marcel de Rooy 2022-08-18 13:41:17 UTC
(In reply to Jonathan Druart from comment #12)
> How will you modified the config files? Once this is pushed you will have to
> keep the behaviour. If we propose a generic solution from the beginning,
> future will be easier ;)

All improvements in the future :)
Will think a bit about it.
Comment 14 Marcel de Rooy 2022-08-18 14:35:51 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2022-08-22 08:25:39 UTC
Created attachment 139571 [details] [review]
Bug 31250: (QA follow-up) The future will be easier

As requested by Jonathan, we need more flexibility ;)
Here it comes.

Test plan:
Run t/CookieManager.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Kyle M Hall 2022-09-02 16:03:23 UTC
Created attachment 140163 [details] [review]
Bug 31250: Deny clearing cookies with numeric suffix

This change allows us to add catalogue_editor_ to the deny list
in koha-conf.xml and keep cookies like catalogue_editor_123.

Test plan:
Run t/CookieManager.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

To test:
1 - Sign in to staff client
2 - Search for records and edit one
3 - Switch to advanced editor
4 - View cookies (F12/developer panel/storage tab)
5 - Note cookie like 'catalogue_editor_##' with value 'advanced'
6 - Log out
7 - note cookie value deleted
8 - Log in and search/edit a record
9 - Basic editor loads
10 - Apply patch
11 - Add line to koha-conf as described in second patch
12 - Restart all
13 - Switch to advanced editor
14 - Cookie value updated
15 - Logout
16 - Cookie value remains
17 - Log in and search/edit
18 - Confirm advanced editor loads

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2022-09-02 16:06:15 UTC
Created attachment 140164 [details] [review]
Bug 31250: Deny clearing cookies with numeric suffix

This change allows us to add catalogue_editor_ to the deny list
in koha-conf.xml and keep cookies like catalogue_editor_123.

Test plan:
Run t/CookieManager.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

To test:
1 - Sign in to staff client
2 - Search for records and edit one
3 - Switch to advanced editor
4 - View cookies (F12/developer panel/storage tab)
5 - Note cookie like 'catalogue_editor_##' with value 'advanced'
6 - Log out
7 - note cookie value deleted
8 - Log in and search/edit a record
9 - Basic editor loads
10 - Apply patch
11 - Add line to koha-conf as described in second patch
12 - Restart all
13 - Switch to advanced editor
14 - Cookie value updated
15 - Logout
16 - Cookie value remains
17 - Log in and search/edit
18 - Confirm advanced editor loads

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2022-09-02 16:06:26 UTC
Created attachment 140165 [details] [review]
Bug 31250: Add catalogue_editor_ cookie to koha-conf deny list

Test plan:
Add this change to your koha-conf.xml.
Flush, restart.
Test if the cookie is kept now in the interface.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Kyle M Hall 2022-09-02 16:06:29 UTC
Created attachment 140166 [details] [review]
Bug 31250: (QA follow-up) The future will be easier

As requested by Jonathan, we need more flexibility ;)
Here it comes.

Test plan:
Run t/CookieManager.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Tomás Cohen Arazi 2022-09-12 11:44:24 UTC
(In reply to Marcel de Rooy from comment #13)
> (In reply to Jonathan Druart from comment #12)
> > How will you modified the config files? Once this is pushed you will have to
> > keep the behaviour. If we propose a generic solution from the beginning,
> > future will be easier ;)
> 
> All improvements in the future :)
> Will think a bit about it.

Have you thougth about it?
Comment 21 Marcel de Rooy 2022-09-12 12:11:52 UTC
(In reply to Tomás Cohen Arazi from comment #20)
> (In reply to Marcel de Rooy from comment #13)
> > (In reply to Jonathan Druart from comment #12)
> > > How will you modified the config files? Once this is pushed you will have to
> > > keep the behaviour. If we propose a generic solution from the beginning,
> > > future will be easier ;)
> > 
> > All improvements in the future :)
> > Will think a bit about it.
> 
> Have you thougth about it?

Yes and I added a follow-up. Allowing the required flexibility via regexes.
Comment 22 Marcel de Rooy 2022-09-12 12:12:39 UTC
(In reply to Marcel de Rooy from comment #21)
> (In reply to Tomás Cohen Arazi from comment #20)
> > (In reply to Marcel de Rooy from comment #13)
> > > (In reply to Jonathan Druart from comment #12)
> > > > How will you modified the config files? Once this is pushed you will have to
> > > > keep the behaviour. If we propose a generic solution from the beginning,
> > > > future will be easier ;)
> > > 
> > > All improvements in the future :)
> > > Will think a bit about it.
> > 
> > Have you thougth about it?
> 
> Yes and I added a follow-up. Allowing the required flexibility via regexes.

Which is the generic solution mentioned.
Comment 23 Tomás Cohen Arazi 2022-09-12 18:37:07 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 24 Lucas Gass 2022-10-31 20:48:40 UTC
Backported to 22.05.x for upcoming 22.05.07 release
Comment 25 Arthur Suzuki 2022-11-14 14:08:37 UTC
applied to 21.11.x for 21.11.14
Comment 26 Victor Grousset/tuxayo 2022-11-20 00:34:09 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document it seems, marking resolved.