Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change persistence type to session rather than local #182

Merged
merged 8 commits into from
Nov 29, 2023

Conversation

antonymilne
Copy link
Contributor

@antonymilne antonymilne commented Nov 24, 2023

Description

Following conversations with the legal team, we've changed the persistence_type everywhere from local (the Dash default) to session (which is less persistent) and included a warning in the docs that it's the user's responsibility if they change it from this.

Checklist

  • I have not referenced individuals, products or companies in any commits, directly or indirectly
  • I have not added data or restricted code in any commits, directly or indirectly
  • I have updated the docstring of any public function/class/model changed
  • I have added tests to cover my changes (if applicable)

Types of changes

  • Docs/refactoring (non-breaking change which improves codebase)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

@antonymilne antonymilne changed the title Feature/session storage Change persistence type to session rather than local Nov 28, 2023
Copy link
Contributor

@petar-qb petar-qb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks good to me. The persistence type for all the components in the app is properly changed. Only the 'theme_selector' isn't adjusted (and it still uses the default persitence_type="local") which also looks correctly to me (I guess we always want to persist selected theme even when the browser tab is closed and reopened again.)

I'm just wondering why we decided to change the persistence_type to "session" in the first place. Do we consider this what the user expects to happen, or did we do it for some security reasons (if someone else opens the same app for the same machine, they can't know what values (besides "theme") were selected from the previous user), solve some existing unexpected behaviour or something else?

However +1 approval from my side 😄.

@lingyielia
Copy link
Contributor

I have the same question as Petar 😄

@antonymilne
Copy link
Contributor Author

Thanks for the reviews @lingyielia and @petar-qb! Actually leaving the theme selector with persistence_type="local" was not intentional but something that I missed in my find and replace. Good catch! Unless you think strongly otherwise then I would also change this to persistence_type="session", since I don't see why theme should persist more strongly than anything else like filter values.

As for the motivation for making this change: it's not to do with user expectation or security but purely for legal reasons. Possibly @Joseph-Perkins can explain more since he spoke to the legal team about it, but my understanding is that while there's room for interpretation as to what constitutes an essential vs. non-essential cookie, it's safest to persist these things as little as possible or you might be required to put up banners asking for users to accept cookies, which we don't want to do. So unless there's distinct advantage in using persistence_type="local" we should prefer "session".

N.B. here I talk about cookies, which as you know isn't actually the mechanism we use for persisting filter values etc. which is all done in browser storage. The laws apply exactly the same regardless of the mechanism, it's just often referred to as the Cookie Law because the most common place you see the effect is in needing to accept cookies.

@Joseph-Perkins
Copy link
Contributor

So far there was no specific user requirement to apply themes to local storage in the past. (I believe it was changed to local storage by default when all the other persistence instances where also defaulted to that)

Since there is an option in the dashboard configuration to default to a specific theme, then that can be used to provide some control of initial theme choices instead of using local storage, if needed

So in this case I think we can apply session storage to the themes without contradicting any known user requirements at this point

@antonymilne
Copy link
Contributor Author

Thanks @Joseph-Perkins, I've made the theme selector also use session storage 👍

@antonymilne antonymilne enabled auto-merge (squash) November 29, 2023 10:46
@antonymilne antonymilne merged commit 8a188aa into main Nov 29, 2023
25 checks passed
@antonymilne antonymilne deleted the feature/session-storage branch November 29, 2023 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants