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

Extract NewPasswordPage from SignInPage, and make new route #7826

Merged

Conversation

jonboiser
Copy link
Contributor

@jonboiser jonboiser commented Mar 9, 2021

Summary

  1. Adds a new NewPasswordPage, which is assigned to a new route /set-password
  2. Adds a test for the new component
  3. In the test environment, adds a global.flushPromises utility, following some advice in these docs
  4. Refactors some tests to remove the hacky multiple calls to nextTick.

Reviewer guidance

  1. Does the Sign-In page still work in "learner has to update password when facility changes password requirement setting from not-required to required" flow?
  2. Is the handling for edge cases good enough? e.g. when a user goes directly to a /set-password URL and the learner has already set their password, or the username query param is invalid.

References

Closes #7321

(We should write separate issues for the other suggested refactors, since they are a little bit more complicated).

This might add some tech debt because the error states just redirect back to the sign-in page. But this is not a regression since these errors were unhandled originally. We would need some specs on how to be more informative to the user when these errors occur.


Contributor Checklist

PR process:

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Testing:

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

Reviewer Checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md
    edit

@jonboiser jonboiser added this to the 0.15.0 milestone Mar 9, 2021
@jonboiser jonboiser added APP: User Re: User app (sign-in, sign-up, user profile, etc.) changelog Important user-facing changes labels Mar 9, 2021
@@ -72,8 +72,7 @@ function makeWrapper(options) {
describe('ActivityList component', () => {
it('on first render, calls the notification resource', async () => {
const { fetchMock, wrapper } = makeWrapper({});
await wrapper.vm.$nextTick();
await wrapper.vm.$nextTick();
await global.flushPromises();
Copy link
Member

Choose a reason for hiding this comment

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

Swanky

Comment on lines -152 to -156
<!-- TODO: This can be its own separate component -->
<!--
Learner was created without a password, but now must create one.
This ought to be routed separately.
-->
Copy link
Member

Choose a reason for hiding this comment

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

From TODO to TODONE 👊

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for this!

Once the red x below is a green check this should be good to go!

@jonboiser jonboiser merged commit 73b9fc0 into learningequality:develop Mar 12, 2021
@jonboiser jonboiser deleted the sign-in-page-improvements branch March 12, 2021 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: User Re: User app (sign-in, sign-up, user profile, etc.) changelog Important user-facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants