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

feat: enforce max 10 OTP requests for verified field #3952

Merged
merged 3 commits into from
Jun 6, 2022

Conversation

mantariksh
Copy link
Contributor

@mantariksh mantariksh commented Jun 3, 2022

Problem

There is no limit to the number of OTPs users can request for a given transaction ID. This allows users to continually request for OTPs, which can be costly in the case of SMSes.

Solution

Enforce a limit of maximum 10 OTPs before user needs to refresh.

Breaking Changes

  • No - this PR is backwards compatible

Screenshots

Screenshot 2022-06-03 at 3 51 20 PM

Tests

  • Requesting OTP for the 11th time results in the error seen in above screenshot
  • Refreshing page clears error, allowing user to verify the field
  • Entering a wrong OTP results in hashRetries incrementing but not otpRequests
  • Requesting for a new OTP results in otpRequests incrementing and hashRetries being reset to 0
  • Request an OTP on the old server, then switch over to the new server. Requesting another OTP results in the otpRequests key being added to the field object in the database, with a value of 1.

@mantariksh mantariksh temporarily deployed to staging-al2 June 3, 2022 07:54 Inactive
@mantariksh mantariksh marked this pull request as ready for review June 3, 2022 08:26
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

lgtm wah anti writing code again how does this feel

src/app/modules/verification/verification.util.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@hanstirtaputra hanstirtaputra left a comment

Choose a reason for hiding this comment

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

LGTM

@mantariksh mantariksh merged commit 346e58c into develop Jun 6, 2022
@mantariksh mantariksh deleted the feat/limit-sms-generation branch June 6, 2022 02:55
@tshuli tshuli mentioned this pull request Jun 7, 2022
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.

3 participants