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

Do not clear localstorage on logout #1929

Merged
merged 2 commits into from
Sep 26, 2023

Conversation

frederikrothenberger
Copy link
Contributor

@frederikrothenberger frederikrothenberger commented Sep 25, 2023

Deleting identity numbers on logout is unexpected and destructive. A change was requested to no longer do that.

Instead, we should give users a different way of managing the numbers shown on the landing page (to be solved as a separate PR).

Note: 3 lines were changed in the production code. The rest is adapting the e2e tests to use the pick identity flow instead of entering the identity number.

Deleting identity numbers on logout is unexpected and
destructive. A change was requested to no longer do that.

Instead, we should give users a different way of managing the
numbers shown on the landing page (to be solved as a separate PR).

Note: 3 lines were changed in the production code. The rest is adapting
the e2e tests to use the pick identity flow instead of entering it new.
Copy link
Collaborator

@lmuntaner lmuntaner left a comment

Choose a reason for hiding this comment

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

Why don't you clear the local storate between e2e tests?

I would differentiate e2e tests that require local storage filled and those that don't.

await browser.$("button[data-action='continue']").click();
const authenticateView = new AuthenticateView(browser);
await authenticateView.waitForDisplay();
await authenticateView.pickAnchor(userNumber);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this e2e depend on logging in at some point before? Or at least on creating the anchor in the same machine without clearing the local storage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, all tests that use the login flow star from a different screen now.

src/frontend/src/test-e2e/flows.ts Show resolved Hide resolved
@@ -32,16 +32,6 @@ export class WelcomeView extends View {
await this.browser.$("#loginButton").click();
await this.browser.$("#addNewDeviceButton").click();
}

async recover(): Promise<void> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this move? Does it have something to do with clearing the local storage on logging out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, see explanation here.

@@ -172,8 +165,8 @@ export const FLOWS = {
browser: WebdriverIO.Browser,
recoveryPhrase: string
): Promise<void> => {
const welcomeView = new WelcomeView(browser);
await welcomeView.recover();
const authenticateView = new AuthenticateView(browser);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change?

The user arrives at a different page if the storage is not cleared?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, see explanation here.

@frederikrothenberger
Copy link
Contributor Author

Why don't you clear the local storate between e2e tests?

I would differentiate e2e tests that require local storage filled and those that don't.

The local storage is cleared between each test. The reason why this has such a big impact on the e2e tests is because we have many variations of the initial landing screen:

There are more differences, but you get the idea. And now, having logout no longer clear the local storage, makes the e2e tests that use logout in the middle (i.e. create identity, logout, do some other action with sign-in / recovery) use a different screen.

@lmuntaner
Copy link
Collaborator

In the Flows, you have different names and methods depending on where the flow starts: registerNewIdentityWelcomeView, registerNewIdentityAuthenticateView, registerPinWelcomeView, ...

Wouldn't it make sense to rename also the login parts as well to be clear where the flow starts?

Does it make sense to add a login e2e test that checkts you can still login from the welcomeView after logging out?

I have the feeling that the e2e now stop testing some functionality and test a different one. The flows that you are changings, could still be tested, right?

Copy link
Collaborator

@lmuntaner lmuntaner left a comment

Choose a reason for hiding this comment

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

Discussed on slack. PR with more e2e will come later.

@frederikrothenberger frederikrothenberger force-pushed the frederik/keep-identity-nr-on-logout branch from 54d2dfa to 04d8eda Compare September 26, 2023 07:21
@frederikrothenberger frederikrothenberger added this pull request to the merge queue Sep 26, 2023
Merged via the queue into main with commit a7766b8 Sep 26, 2023
@frederikrothenberger frederikrothenberger deleted the frederik/keep-identity-nr-on-logout branch September 26, 2023 07:41
frederikrothenberger pushed a commit that referenced this pull request Sep 26, 2023
Due to the change in #1929 the login flow with unknown user number
was no longer tested. This  reintroduces these tests for both
webAuthn and PIN login.
frederikrothenberger pushed a commit that referenced this pull request Sep 26, 2023
Due to the change in #1929 the login flow with unknown user number
was no longer tested. This  reintroduces these tests for both
webAuthn and PIN login.
frederikrothenberger pushed a commit that referenced this pull request Sep 26, 2023
Due to the change in #1929 the login flow with unknown user number
was no longer tested. This  reintroduces these tests for both
webAuthn and PIN login.
frederikrothenberger pushed a commit that referenced this pull request Sep 26, 2023
Due to the change in #1929 the login flow with unknown user number
was no longer tested. This  reintroduces these tests for both
webAuthn and PIN login.
@frederikrothenberger
Copy link
Contributor Author

Discussed on slack. PR with more e2e will come later.

See #1933.

frederikrothenberger pushed a commit that referenced this pull request Sep 26, 2023
Due to the change in #1929 the login flow with unknown user number
was no longer tested. This  reintroduces these tests for both
webAuthn and PIN login.
github-merge-queue bot pushed a commit that referenced this pull request Sep 26, 2023
Due to the change in #1929 the login flow with unknown user number
was no longer tested. This  reintroduces these tests for both
webAuthn and PIN login.
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.

2 participants