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

Feedback on failing authentication #188

Merged
merged 9 commits into from
Nov 17, 2019

Conversation

jejoivanic
Copy link
Contributor

Description

This PR provides:

  • A way for the user to handle the error response from the authentication request and display whatever the user wants in case of failure.
  • Unit tests and e2e tests have been implemented.
  • For the default AuthenticationLayout, added snackbar on failing authentication.

Closes #180

Type of change

  • 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 not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Apart from running all previous tests (unit/e2e) two different e2e tests were added. Those are

  • Testing snackbar visibility and content on failing authentication for invalid username
  • Testing snackbar visibility and content on failing authentication for invalid password

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@jejoivanic
Copy link
Contributor Author

Why is this a WIP?

I implemented a solution which requires the user to handle the asyn response from the authentication with a then. In this then's callback the user receives a parameter which represents the error of the response if any. If not, the layout will change and nothing should be received in this parameter.

...
  this.va.login(user, password)
    .then(err => {
      if (err)
        doSomething()
    })
...

I've thought about this solution but I don't really know if this is the right approach for this, however I could not find a better way. I've consider passing also the user, to resemble other libraries which, in similar cases, defines a callback like (data, err) => { ... }.
That's my only concern about this solution. I'll wait for some comments or feedback.

@jejoivanic
Copy link
Contributor Author

I also modified the response from the demo server to return 401 status when the user or password is incorrect.

…return an async resource so the user can chain the authentication resultand handle the error response.
@jejoivanic jejoivanic force-pushed the 180-feedback-on-failed-authentication branch from 15f35c0 to b009086 Compare November 8, 2019 14:22
@sgobotta
Copy link
Member

sgobotta commented Nov 16, 2019

Not really sure what's going on. My guess is that the snackbar disappears before Cypress finally finds it in the virtual dom.

Note: this only happens during the CI, local e2e tests pass successfully.

@sgobotta
Copy link
Member

sgobotta commented Nov 16, 2019

Keeps failing, even with a timeout of 0. I'm going to temporarily remove those tests. It's not a big trouble since the plan is to move the demo to another repository, therefore e2e tests will not be part of the source code anymore.

@sgobotta sgobotta force-pushed the 180-feedback-on-failed-authentication branch from 9cfd008 to 3326fe1 Compare November 16, 2019 23:45
@sgobotta
Copy link
Member

End to end tests for AuthLayout snackbar were removed in 3326fe1

@sgobotta sgobotta changed the title [WIP] Feedback on failing authentication Feedback on failing authentication Nov 17, 2019
@sgobotta sgobotta merged commit 6b7a938 into develop Nov 17, 2019
@sgobotta sgobotta deleted the 180-feedback-on-failed-authentication branch February 9, 2020 21:14
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.

None yet

2 participants