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

Reset store on logout #6694

Merged
merged 2 commits into from May 2, 2022
Merged

Reset store on logout #6694

merged 2 commits into from May 2, 2022

Conversation

elizavetaRa
Copy link
Member

Description

When logging out, only some parts of vuex store were reset to default. This caused bugs by switching to another account that has some other/missing settings. For example, if the account has no quota, the quota of the previously logged in account was shown. We have fixed this by resetting the store on logout with reset function (vuex extensions library).

Related Issue

How Has This Been Tested?

logging in, logging out, logging in

  • test case 1: logging in with account with quota, logging out, logging in with account without quota --> quota not shown
  • test case 2: logging in with account with navItems, logging out, logging in with account with another set of navItems --> they are displayed correctly
  • ...

Screenshots (if appropriate):

Types of changes

  • 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)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

@update-docs
Copy link

update-docs bot commented Mar 31, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@ownclouders
Copy link
Contributor

Results for oC10Files3 https://drone.owncloud.com/owncloud/web/24286/18/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUILogin-oauthLogin_feature-L22.png

webUILogin-oauthLogin_feature-L22.png

💥 The acceptance tests pipeline failed. The build has been cancelled.

@pascalwengerter
Copy link
Contributor

Results for oC10Files3 https://drone.owncloud.com/owncloud/web/24286/18/1
💥 The acceptance tests failed on retry. Please find the screenshots inside ...
💥 The acceptance tests pipeline failed. The build has been cancelled.

Fails as the redirect with OAuth2 in combination with OC10 fails with these changes. I'll try to take a look with @kulmann tomorrow

packages/web-runtime/src/store/user.js Outdated Show resolved Hide resolved
@elizavetaRa
Copy link
Member Author

@kulmann implemented an action for clearing dynamic items as you asked, testing by us with switching between different accounts works correctly.

@kulmann
Copy link
Member

kulmann commented Apr 25, 2022

@kulmann implemented an action for clearing dynamic items as you asked, testing by us with switching between different accounts works correctly.

Hm. 🤔 it needs at least a rebase to pass CI. But I see another issue: The oCIS settings service UI adds dynamic nav items and there is no logic in your PR for re-adding certain dynamic nav items. See https://github.com/owncloud/ocis/blob/44e8b6e7be947efcabda1074776c045164021cf3/extensions/settings/ui/components/SettingsApp.vue#L106
In other words, while your PR fixes what you do with dynamic nav items, it kills functionality of the settings UI.
I'm starting to think that we need a complete re-bootstrapping...

@sonarcloud
Copy link

sonarcloud bot commented Apr 27, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Turns out that clearing the dynamic nav items is ok. The ADD_NAV_ITEM mutation in the web runtime is implemented in a way that allows setting the same nav item again over and over. In fact, all known apps use the dynamic nav item init during render time, thus they already get re-created when needed.

Thank you for the contribution! 🤩

@kulmann kulmann merged commit 5aed53f into owncloud:master May 2, 2022
@elizavetaRa elizavetaRa deleted the upstream_logout branch May 2, 2022 09:53
@micbar micbar mentioned this pull request May 5, 2022
45 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cached parts of the interface by switching between accounts not updating
4 participants