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: sms limiting #2504

Merged
merged 17 commits into from
Aug 11, 2021
Merged

feat: sms limiting #2504

merged 17 commits into from
Aug 11, 2021

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Aug 2, 2021

Problem

This PR adds sms limiting!

This PR comprises of a few sections:

  1. email sending (warning/disabled)
  2. admin facing frontend (conditional modal showing depending on whether admin has exceeded limit)
  3. backend changes (pre-save hook on smscounts; checking whether to send email or not; checking whether we should disable the admin's ability to send sms)

More relevant information can be found in the individual PRs!

This PR has 2 unreviewed commits here and here which does 2 things. It removes the smsVerificationLimit env var and removes unused typings.

Closes #1935

Solution

see individual PR

Breaking Changes

  • Yes - this PR contains breaking changes
    • This PR updates the FE to call a new API and has a new UI

Manual Tests

Happy path without db access

  • Create 2 new forms with a mobile field and sms verifications enabled
  • On 1 form, request to verify your phone number.
  • Return to the form and attempt to add a new mobile number field. The count should be updated
  • Attempt to edit the other mobile number field. The count should also be updated.
  • Go to the other form, and repeat the above 2 tests.

Warning email tests - requires db migration

  • Create 2 forms with verifiable mobile fields. ensure that 1 of them has the msgSrvcName property set.
  • Create a form with a verifiable mobile field. Insert 2499 smses into the db (see the misc section down below).
  • Open the form and subsequently, the mobile field. Verify that the counts has been updated.
  • request for otp verification. Should receive an email warning form admin about the limit. Attempt to toggle the mobile field on again - this should succeed and show the updated count.
  • Verify that there is a warning email receieved.
  • Ensure that the email received has 2 form titles. (the 2 forms w/o msgSrvcName)
  • Ensure that the form with msgSrvcName is not shown in the email
  • Repeat the above steps for 4999/7499 smses and verify the result

Disabled email tests - requires db access

Have the edit mobile field window open before proceeeding. Toggle on the sms verification feature and do not save. Remain on the modal. Create another form in the staging db and set a msgSrvcName so that it acts as an onboarded form

  • Have a window/tab with the form open prior to inserting smses.
  • Insert 10k smses. attempt to verify with the tab/window from previously. this should succeed. next, refresh the public forms page/navigate back (now the form has >10k smses). the verify option should not be there.
  • Now attempt to save the edit mobile field modal as an admin (with otp verification toggled on). There should be a toast informing you that the update is invalid.
  • Attempt to duplicate the mobile field. This should work. Click on the mobile field. It should be disabled.
  • Attempt to duplicate this form. This should work. Verify that the duplicated form also has sms verification disabled.
  • Attempt to use the form as a template. This should work. Verify that the duplicated form also has sms verification disabled.
  • Close the edit mobile field modal and reopen it. Verify that the toggle is greyed out.
  • Verify that you have received an email warning you that your forms have sms verifications disabled.
  • Transfer the form to ${name}+1@${domain here} and make them the owner. Open the form and verify that sms counts have been reset.
  • Ensure that the other form with msgSrvcName is not affected and mobile number field can still be toggled

Misc

db script to insert sms counts into staging db:

/*
Populates the database with the given number of sms in the sms_counts collection
*/

// == PRE-UPDATE CHECKS ==
const formId = ObjectId('insert credentials here')
const formAdminId = ObjectId('insert credentials here')
const formAdminEmail = 'insert credentials here'
const msgSrvcSid = 'insert credentials here'
const numberToCreate = 2500
// A: Retrieve smscounts of the user before updating
db.getCollection('smscounts').countDocuments({
  'formAdmin.userId': formAdminId,
  smsType: 'VERIFICATION',
  isOnboardedAccount: false,
})

// == UPDATE ==
// B: Insert smscount document into smscounts collection
const documentsToCreate = Array(numberToCreate).fill({
  smsType: 'VERIFICATION',
  form: formId,
  formAdmin: {
    email: formAdminEmail,
    userId: formAdminId,
  },
  msgSrvcSid,
  logType: 'SUCCESS',
  createdAt: ISODate('2021-06-30T07:41:53.768Z'),
  isOnboardedAccount: false,
  __v: 0,
})
db.getCollection('smscounts').insertMany(documentsToCreate)

// == POST-UPDATE CHECKS ==
// Check smscounts of the user after updating
// Should be equivalent to A + B
db.getCollection('smscounts').countDocuments({
  'formAdmin.userId': formAdminId,
  smsType: 'VERIFICATION',
  isOnboardedAccount: false,
})

@seaerchin
Copy link
Contributor Author

seaerchin commented Aug 2, 2021

i can rebase to remove #2276 and #2250 since both of em will be merged into develop

@seaerchin seaerchin marked this pull request as draft August 2, 2021 08:51
@timotheeg timotheeg marked this pull request as ready for review August 3, 2021 01:41
@seaerchin seaerchin marked this pull request as draft August 3, 2021 03:11
seaerchin and others added 6 commits August 3, 2021 12:40
* feat(templates): add html template for email when verification is disabled

* feat(mail.utils.ts): added new method for generation of email html

* feat(mail.service): new service method to send mail when sms verification disabled

* chore(sms-verification-disabled.view): updated wording

* test(mail.service.spec): adds tests for verification disabled email sending
* feat(templates): adds new sms verification warning template

* feat(mail.service): adds new method to send sms verification warning mail

* chore(templates): added colon for proper punctuation for sms verification template

* chore(sms-verification-warning): updated wording to reflect limit is on admin level

* refactor(mail.service): updated the api for mail service
feat(sms_count.server.model): adds new isOnboardedAcc key and scripts to add/delete the key

chore(sms_count.server.model): remove unrelated code changes

chore(scripts): added extra checks for verifiction counts

style(scripts): updated whitespace for formatting

feat(form.server.model): add new static method for disabling sms verifications for an admin

feat(admin-form.service): adds new service method to disable sms verifications for a certain user

fix(form.server.model): disabling sms verifications does not show as an update

fix(types): updated return type for disabling sms and added extra db types

test(form.server.model): added unit tests for disabling sms on the model scehma

test(admin-form.service.spec): added tests for disabling sms on admin service

chore(form.server.model): addressed PR comments

refactor(sms.config): shifts limiting to be on env var

refactor(sms.config): reworked to use env-vars
* feat(configure-mobile.client.directive): updated modal to use new messages with hard-coded data

* refactor(verification.constants): shifted sms_verification_limit to shared/util/verification

* feat(configure-mobile.client): added frontend ui for toggling on/off

* feat(configure-mobile.client.view): added lock and disabled state when limit exceeded

* refactor(configure-mobile.client.view): changed text message to be error themed

* refactor(edit-form.css): added inline-padding for error message display

* fix(configure-mobile.client.view): updated form twilio account link

* feat(configure-mobile.client.directive): added loading state for toggle

* refactor(configure-mobile.client.view): update wording for error message

* fix(configure-mobile.client.view.html): added ng-show so that error only shows when limit exceeded

* feat(public/services): adds new smsservice

* feat(configure-mobile.client.directive): connected sms counts to fetch from backend

* refactor(pop-up-modal): adds variable to check if we should display red button

\

* test(smsservice): adds tests for sms service

* refactor(configure-mobile.client): adds error reporting and handling on FE

* refactor(smsservice): renamed SmsService to AdminMetaService

* refactor(adminmetaservice): renamed method for greater clarity

* chore(configure-mobile.client): addressed PR comments

* refactor(configure-mobile.client): uses env var instead of using constants

* refactor(mail.service): changes to use env var instead of const

* refactor(adminmetaservice): updated definitions to fit BE

* refactor(configure-mobile.client): changed condition to be same as BE

* refactor(configure-mobile.client.directive): extracts formatting to own function
* feat(verification): added utility method to check if admin has exceeded sms limit

* refactor(admin-form.controller): adds new step to check if admin has exceeded the free sms limit

the service method returns an error on limit being exceeded so that field creation short-circuits
and we hit the mapErr step immediately

* test(admin-form.service): added unit tests for isAdminOverFreeSmsLimit

* refactor(types): added more refined form and field types together with typeguards

added an onboardedForm type so that we can check if a form is onboarded. this is restricted to
subtypes of IForm because the context of msgSrvcName is unique to forms and not just any generic
type. Added a verifiable mobile field type that checks if the field is toggled to have
isVerifiableOn

* refactor(admin-form): adds checks to prevent admin updates when the fe state is inconsistent with be

* refactor(admin-form.service): optmised to avoid unnecessary query

* test(admin-form.serivce): adds unit tests for shouldUpdateFormField

* docs(admin-form.controller): updated documentation for 409s when sms limit is exceeded

* test(admin-form.controller.spec): updated with unit tests for 409s

* test(admin-forms.form.routes.spec): fixed failing test

* test(admin-form.controller): updated faulty tests

* refactor(mail.service): changes to use env var instead of const

* chore(form.utils): addressed PR comments

* test(admin-form.service.spec): updated test structure and description for greater clarity

* refactor(admin-form.service): splits field specific logic from checking field type
* feat(form.utils): adds util method to check if a form is onboarded

* feat(verification.util): adds util method for checking if an admin has exceed limit

this is duplicated from another PR because they are only tangentially dependent.

* feat(admin-form.service): adds method to send email/disable verification when sms limits are reached

* refactor(verification.controller): adds step to check sms counts for otp generation

* refactor(admin-form.service): simplify check by removing redundant neverthrow usage

* test(admin-form.service.spec): adds unit tests for new methods

* fix(verification.controller): shifted order so that retrieveFullForm is awaited on

* test(verification.controller.spec): adds check for disabling mail

* refactor(admin-form.service): shifted initialization of admin id below

* refactor(verification): updated to use env var rather than const

* refactor(admin-form.service): removed export of helper method; tests now shifted to wrapper method

* refactor(form.utils): removed duplicate method due to PR separation

* refactor(verification): shifted over processing admin sms count to verification module
@seaerchin seaerchin marked this pull request as ready for review August 3, 2021 05:09
@seaerchin seaerchin requested review from karrui and mantariksh August 3, 2021 05:40
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, reviewed last two commits only

@seaerchin seaerchin requested a review from karrui August 10, 2021 06:02
seaerchin and others added 3 commits August 10, 2021 15:43
* refactor(pop-up-modal): removed isImportant prop as it is now unused

* chore(configure-mobile.client): updated modal wording

* refactor(configure-mobile.client): added sms counts figure

* refactor(configure-mobile.client): updated stats to not show if loading or error

* chore(configure-mobile.client): removed extra html property
* feat(form.server.model): added new method to retrieve public ofrms with sms verifications

* feat(form.service): adds new method to retrieve public forms w/ sms verif enabled

* refactor(mail.service): changed warning mails to only be sent to public forms w/ active sms verif

* refactor(mail.service): updated disabled mail to have all forms for admin/single form for collab

* fix(views/templates): updated wording for warning/disabled mails

* fix(verification): fixed lowercase k

* chore(templates): disabled email templates updated to match issue copy

* fix(templates): updated email template for warning emails

chore(template): removed fullstop so it looks more natural

* fix(mail.service): updated mail service + tests so that numbers have commas

* fix(form.server.model): updated read pref to secondary

* fix(form.server.model): updated retrieve method to only affect forms that are not onboarded

* docs(mail.service): updated documentation for using toLocaleString

* test(form.server.model): updated test to match only verifiable public forms that are not onboarded
@seaerchin seaerchin merged commit 6f3c9b6 into develop Aug 11, 2021
@seaerchin seaerchin deleted the feat/sms-limiting branch August 11, 2021 01:54
This was referenced Aug 11, 2021
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.

SMS verifications free tier limit
2 participants