Skip to content

RAS-1614: Fix Frontstage request_password_change logic#1063

Merged
SteveScorfield merged 16 commits intomainfrom
fix-request-password-change-logic
Jun 13, 2025
Merged

RAS-1614: Fix Frontstage request_password_change logic#1063
SteveScorfield merged 16 commits intomainfrom
fix-request-password-change-logic

Conversation

@SteveScorfield
Copy link
Copy Markdown
Contributor

@SteveScorfield SteveScorfield commented May 30, 2025

What and why?

A bug was found where if a user tries to reset their password multiple times and let the token expire. As a result, the token was deleted (null value) in the DB and then a 500 was shown to the user as a result of a conditional. The fix introduced will prevent this from happening. I've added more tests to cover further scenarios around password resetting.

Note: I think I have covered all potential unit test scenarios but let me know if you see other areas.

How to test?

  1. Build out your dev env
  2. Run acceptance tests
  3. Navigate to the 'Forgot password' page
  4. Enter an email of a user you know is in the DB
  5. You should receive the usual reset password message
  6. Using a DB tool of you choice, goto the 'respondent' table of the 'partysvc' schema
  7. Locate the ID of the user, then using the 'UPDATE SET' SQL commands, set the 'password_verifcation_token' value to 'NULL'
  8. Navigate back to 'Forgot password' page and enter the same email. You should receive 'An error has occurred' message
  9. Next deploy this PR to your dev env
  10. Navigate back to the 'Forgot password' page and enter the same email
  11. You should get the correct 'Check your email' message page

Run unit and acceptance tests

Jira

https://jira.ons.gov.uk/browse/RAS-1614

@SteveScorfield SteveScorfield requested a review from a team as a code owner May 30, 2025 12:16
@SteveScorfield
Copy link
Copy Markdown
Contributor Author

/deploy scorfs

@ras-rm-pr-bot
Copy link
Copy Markdown
Collaborator

Deploying to dev cluster with following parameters:

  • namespace: scorfs

  • tag: pr-1063

  • configBranch: main

  • paramKey: ``

  • paramValue: ``

Copy link
Copy Markdown
Contributor

@matthew-robinson-ons matthew-robinson-ons left a comment

Choose a reason for hiding this comment

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

The implementation works to resolve this issue found. Some queries around the testing

Copy link
Copy Markdown
Contributor

@matthew-robinson-ons matthew-robinson-ons left a comment

Choose a reason for hiding this comment

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

Happy this fixes the bug and testing is improved

Copy link
Copy Markdown
Contributor

@LJBabbage LJBabbage left a comment

Choose a reason for hiding this comment

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

Realistically we shouldnt be client testing and it should be at function level, however the code is sub optimal as the view shouldn't be doing all this and should be rationalised properly. We don't have to change this now mind.

@SteveScorfield
Copy link
Copy Markdown
Contributor Author

/deploy scorfs

@ras-rm-pr-bot
Copy link
Copy Markdown
Collaborator

Deploying to dev cluster with following parameters:

  • namespace: scorfs

  • tag: pr-1063

  • configBranch: main

  • paramKey: ``

  • paramValue: ``

@SteveScorfield
Copy link
Copy Markdown
Contributor Author

/deploy scorfs

@ras-rm-pr-bot
Copy link
Copy Markdown
Collaborator

Deploying to dev cluster with following parameters:

  • namespace: scorfs

  • tag: pr-1063

  • configBranch: main

  • paramKey: ``

  • paramValue: ``

@SteveScorfield
Copy link
Copy Markdown
Contributor Author

/deploy scorfs

@ras-rm-pr-bot
Copy link
Copy Markdown
Collaborator

Deploying to dev cluster with following parameters:

  • namespace: scorfs

  • tag: pr-1063

  • configBranch: main

  • paramKey: ``

  • paramValue: ``

Copy link
Copy Markdown
Contributor

@LJBabbage LJBabbage left a comment

Choose a reason for hiding this comment

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

A bit of redundancy in the tests, but it seems to work

@SteveScorfield SteveScorfield merged commit 88d5140 into main Jun 13, 2025
3 checks passed
@SteveScorfield SteveScorfield deleted the fix-request-password-change-logic branch June 13, 2025 09:08
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.

4 participants