Skip to content

LG-13138 Avoid sending account reset notification on duplicate submission#12205

Merged
kevinsmaster5 merged 8 commits intomainfrom
LG-13138-kmas-avoid-sending-duplicate-reset-email
Jun 16, 2025
Merged

LG-13138 Avoid sending account reset notification on duplicate submission#12205
kevinsmaster5 merged 8 commits intomainfrom
LG-13138-kmas-avoid-sending-duplicate-reset-email

Conversation

@kevinsmaster5
Copy link
Copy Markdown
Contributor

@kevinsmaster5 kevinsmaster5 commented May 23, 2025

🎫 Ticket

Link to the relevant ticket:
LG-13138

🛠 Summary of changes

Updates the "Yes, continue..." button to use a simple_form component adding built-in protection against double clicking the button.

Set the rate limiter timing to 2 minutes so the page response will have time to finish POSTing regardless of multiple button mashing and slow connection. If the user ended up needing to re-submit a request the limiter should expire by the time they return there.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

The simulated email pop up should only appear once and not every time you clicked.

@kevinsmaster5 kevinsmaster5 marked this pull request as ready for review May 27, 2025 15:21
@kevinsmaster5 kevinsmaster5 requested a review from a team May 27, 2025 15:21
@kevinsmaster5 kevinsmaster5 force-pushed the LG-13138-kmas-avoid-sending-duplicate-reset-email branch from d992300 to 65053f0 Compare May 28, 2025 20:13
@mdiarra3
Copy link
Copy Markdown
Contributor

looks good can u add a test

Copy link
Copy Markdown
Contributor

@jmdembe jmdembe left a comment

Choose a reason for hiding this comment

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

Changes make sense.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mentioned here, but I think a more comprehensive mitigation is needed based on reading the ticket

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have added rate limiting with d4577ff
I left in the UI change so I think unless they have JS turned off they'll never hit the rate limit but it's there as an additional stopgap.

@kevinsmaster5 kevinsmaster5 force-pushed the LG-13138-kmas-avoid-sending-duplicate-reset-email branch from ecfe2f2 to d4577ff Compare May 30, 2025 13:21
@kevinsmaster5 kevinsmaster5 requested review from a team and mitchellhenke May 30, 2025 17:34
@kevinsmaster5
Copy link
Copy Markdown
Contributor Author

looks good can u add a test
Test added with 1543f16

@kevinsmaster5 kevinsmaster5 force-pushed the LG-13138-kmas-avoid-sending-duplicate-reset-email branch from 1543f16 to 3d21240 Compare June 3, 2025 14:10
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what happens if account reset request isnt created? right now it looks like we take them to account reset confirm, this could cause confusion for users that may hit this limit naturally especially if its 2 requests per 2 days.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I set the limiter to 2 minutes because the real world thing this is trying to prevent (the controller not POSTing in time with button mashing) I expect should be over by then. 542a916

@kevinsmaster5 kevinsmaster5 force-pushed the LG-13138-kmas-avoid-sending-duplicate-reset-email branch from 3d21240 to 8c32a87 Compare June 5, 2025 19:22
Copy link
Copy Markdown
Contributor

@mitchellhenke mitchellhenke Jun 10, 2025

Choose a reason for hiding this comment

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

I don't have strong opinions on what the values here should be, but it should be monitored for potential adjustments

@kevinsmaster5 kevinsmaster5 force-pushed the LG-13138-kmas-avoid-sending-duplicate-reset-email branch from 8c32a87 to a142974 Compare June 12, 2025 19:57
@kevinsmaster5 kevinsmaster5 force-pushed the LG-13138-kmas-avoid-sending-duplicate-reset-email branch from a142974 to 3f62400 Compare June 12, 2025 20:15
Copy link
Copy Markdown
Contributor

@mdiarra3 mdiarra3 left a comment

Choose a reason for hiding this comment

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

LGTM

@kevinsmaster5 kevinsmaster5 merged commit 69f88a3 into main Jun 16, 2025
1 check passed
@kevinsmaster5 kevinsmaster5 deleted the LG-13138-kmas-avoid-sending-duplicate-reset-email branch June 16, 2025 15:19
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