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): disable sms verifications toggling #2300

Merged
merged 15 commits into from
Jul 26, 2021

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Jul 5, 2021

Problem

This PR addresses the edge case when the admin wishes to perform some operation on the form field but should be prohibited from doing so because their sms limit has been exceeded. This PR is dependent on #2276

Solution

The solution consists of 2 portions.

Checking if the operation should be prohibited

  • this is done though a service method called shouldUpdateFormField, where the applicable scenario is checked - the field is of type Mobile and it's verifiable. When this occurs, the free sms counts used of the admin is retrieved and checked to see if the limit has been exceeded, and a Result is returned accordingly.

Preventing operation if it should be prohibited

  • because the type returned in the error case is Err<some base type>, the error returned will be propagated and short-circuited, preventing the operation from taking place

Technical Choices

  1. Placing of shouldUpdateFormField in controller rather than service.
    • the choice to update should be clearly visible and not abstracted by the service
    • choice to prohibit the update should be a controller level responsibility; service should just perform a single function

@seaerchin seaerchin force-pushed the feat/disable-sms-verifications branch from 2a5405c to 7a0e48b Compare July 5, 2021 03:24
@seaerchin seaerchin requested a review from karrui July 8, 2021 04:10
* @return err(SmsLimitExceededError) if the form admin went over the free sm limit
* while attempting to toggle verification on for a mobile form field
*/
export const shouldUpdateFormField = (
Copy link
Contributor

Choose a reason for hiding this comment

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

now that I'm reading this function signature again, should we make it clearer so ppl reading only the controller can understand what this function does?

Can we further split this function up so it can be extended if needed?

const shouldUpdateFormField = (...) => {
  switch (formField.fieldType) {
    case BasicField.Mobile:
      return assessMobileFieldUpdateAllowed(...)
    default:
      return okAsync(form)
}

assessMobileFieldUpdateAllowed can still be a function in this admin-form service since it's still acting on form fields.

If not, we can also move this field type check into the controller and call assessMobileFieldUpdateAllowed. This makes the pipelines clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i was thinking about doing the split-up but was wary about premature optimization actually HAHA now that you've raised it, i'll split it up!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done here

Base automatically changed from feat/sms-counts-retrieval to feat/sms-limiting July 14, 2021 02:53
seaerchin added 15 commits July 14, 2021 11:58
…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
…h 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
@seaerchin seaerchin force-pushed the feat/disable-sms-verifications branch from 4f36305 to 8b5d20c Compare July 14, 2021 03:59
@seaerchin seaerchin requested a review from karrui July 14, 2021 04:39
@seaerchin seaerchin merged commit 9f389ce into feat/sms-limiting Jul 26, 2021
@seaerchin seaerchin deleted the feat/disable-sms-verifications branch July 26, 2021 03:08
seaerchin added a commit that referenced this pull request Aug 3, 2021
* 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
seaerchin added a commit that referenced this pull request Aug 11, 2021
* feat(sms-limiting): email for disabling sms verification (#2133)

* 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(sms limit): sms warning for different limits (#2104)

* 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(verified-sms): disabling sms verifications (#2262)

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(sms-limiting): admin facing frontend (#2280)

* 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(sms-limiting): disable sms verifications toggling (#2300)

* 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(sms-limiting): check admin sms count on public submission (#2326)

* 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

* refactor(configure-mobile.client.directive): updated to account for new BE api

* chore(types): clean up unused types

* test(verification): fixed import path in tests

fix(configure-mobile.client.directive): fixed import path

* fix(mail.service): updated to cc rather than send individual mail

* chore(create-isonboardedacc): updated db script to be more specific

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

* fix(form.server.model): updated disable to only affect forms w/o msg srvc name

* test(form.server.model): updated tests to check that only not onboarded forms are disabled

* feat(sms-limiting): ui changes for modal (#2541)

* 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(sms-limiting): disabled email (#2532)

* 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
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.

2 participants