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

Add a notification on password reset #14968

Merged

Conversation

mrmir
Copy link
Contributor

@mrmir mrmir commented Oct 6, 2021

What type of PR is this? (check all applicable)

  • Feature

Description

Changes made:

  1. Adds a notice on clicking the "Send me password reset instructions" button

Related Tickets & Documents

Resolves #10130

QA Instructions, Screenshots, Recordings

  1. Visit /users/password/new
  2. Enter email ID and click submit
  3. A notice should appear on the /enter page that the app is redirected to

UI accessibility concerns?

No

Added/updated tests?

  • [ x] No, and this is why: Existing tests should suffice since only one line has been added

[optional] What gif best describes this PR or how it makes you feel?

First time!

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Oct 6, 2021
@CLAassistant
Copy link

CLAassistant commented Oct 6, 2021

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2021

Thank you for opening this PR! We appreciate you!

For all pull requests coming from third-party forks we will need to
review the PR before we can process it through our CI pipelines.

A Forem Team member will review this contribution and get back to
you as soon as possible!

@citizen428
Copy link
Contributor

citizen428 commented Oct 7, 2021

The reason we don't send a notification is that we didn't want to leak any information as to whether or not an address actually exists. Previously we showed either that the notification was sent or an error. I think a generic unconditional message is ok.

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 7, 2021
@rhymes
Copy link
Contributor

rhymes commented Oct 7, 2021

@citizen428 @mrmir I think the fix is correct but while I was testing it I noticed the redirect doesn't work at all, on DEV it lands on a 404 page and I get a routing error locally as well:

Screen.Recording.2021-10-07.at.12.36.13.mov

which means that the feature might not be working at all. Maybe we want to hold off on merging this until we understand why the feature is not working

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Oct 7, 2021
@mrmir
Copy link
Contributor Author

mrmir commented Oct 7, 2021

@rhymes I don't know what's happening in your tests, but I just checked again and it seems to be redirecting fine for me. I'm running the app via bin/startup after following the developer documentation setup.

image

@benhalpern
Copy link
Contributor

Seeing if closing and re-opening will trigger tests appropriately

@benhalpern benhalpern closed this Oct 7, 2021
@benhalpern benhalpern reopened this Oct 7, 2021
@msarit
Copy link
Contributor

msarit commented Oct 7, 2021

@benhalpern Are we sure the author does not need to add/modify any tests?
I could've sworn there's an E2E test for this flow. Perhaps its straightforward enough not to need one...

@benhalpern
Copy link
Contributor

@benhalpern Are we sure the author does not need to add/modify any tests?

Yeah @msarit. The PR in the end really doesn't affect functionality enough to need a whole new tests. If regression tests pass we're good to go here.

The problem was just that the PR never even kicked off a Travis job. Tests weren't failing, they just never started. So now they ran and are green so I'm going to merge this.

@benhalpern benhalpern merged commit 79afcba into forem:main Oct 7, 2021
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a notification when resetting password
6 participants