feat(sms-limiting): sms counts retrieval #2276

seaerchin · 2021-06-30T04:13:56Z

Problem

Part of allowing sms limiting is to have a way to count the number of free sms that an agency user has sent out thus far. Because the frontend also requires this count to determine the appropriate state on the modal, there is also a new endpoint exposed in this PR

Solution

Relevant technical overview (per the design doc) is here

Some relevant technical choices are listed below for discussion

isOnboardedAccount set using a pre-save hook. This is done so that the updating/setting is automatic, and developers don't have to be concerned with this in the future.
Exposing the endpoint to count is done on SmsController - decided on this because other controllers don't seem too concerned with the sms counts. Arguably, this could be done on AdminFormController or UserController but those seem to be more concerned with the properties of the form/user respectively. (am ok w/ changing if needed tho)

Breaking Changes

Yes - this PR contains breaking changes
- Details ...
No - this PR is backwards compatible

Manual Tests
Because this endpoint (and method) will be used subsequently to determine whether the form should be disabled or not on the frontend, this needs to be tested for speed.

the test plan is as follows on staging:

Populate the staging db with 2.5k sms counts.
next, query the endpoint at /:formId([a-fA-F0-9]{24})/verified-sms/count/free
log the time taken for this operation
Repeat the above 3 steps for the various warning limits (2.5k, 5k, 7.5k; 10k for disabled, with different emails)

karrui · 2021-06-30T09:03:20Z

isOnboardedAccount set using a pre-save hook. This is done so that the updating/setting is automatic, and developers don't have to be concerned with this in the future.

Just take note that our current way of manually onboarding users will not trigger this pre-save hook; will need to add another manual check when we do so until we have an automated way to do this on the GUI

seaerchin · 2021-06-30T10:36:24Z

@karrui could you help me understand this? my understanding is that we save the credentials in AWS secrets manager and subsequently, update all forms of the specified user to have the specified message service id. if that's the case, wouldn't this check work because we are checking the form's msgsrvcid, which would already have been updated?

karrui · 2021-07-01T02:15:50Z

@karrui could you help me understand this? my understanding is that we save the credentials in AWS secrets manager and subsequently, update all forms of the specified user to have the specified message service id. if that's the case, wouldn't this check work because we are checking the form's msgsrvcid, which would already have been updated?

Pre-save hooks don't run when we update using an external GUI such as Robo3T

src/app/routes/api/v3/sms/sms.routes.ts

tests/unit/backend/helpers/jest-db.ts

src/app/services/sms/sms_count.server.model.ts

tests/unit/backend/helpers/jest-db.ts

src/app/services/sms/sms.controller.ts

src/app/services/sms/sms.service.ts

seaerchin · 2021-07-01T09:28:28Z

i think i'll change this so that the endpoint is rooted at /api/v3/admin/forms/:formId/verified-sms/count/free. However, not going to change the controller to sms controller due to point #2 in the PR write up (unless you think it should change also)

also, i'll rebase this off develop (sorry kr) because i didn't take into account config initially, and it's been changed due to feature manager removal.

… to add/delete the key

… formsg's credentials

… set property

…rdedAccount

…fault credentials

…e counts and msgsrvcid fix(sms.controller.ts): added mapRouteError for sms controller

… as it sohuld be for the admin

…eve counts

… tests breaking

renamed to smsCountsDto because it is used to transfer data b/w fe and be. against giving it a more specific name because this is a type signifying a transfer of sms count data. context for what this sms counts actually means should be given by the caller

src/app/routes/api/v3/admin/forms/admin-forms.form.routes.ts

src/app/services/sms/__tests__/sms_count.server.model.spec.ts

karrui · 2021-07-05T05:31:01Z

src/app/services/sms/__tests__/sms_count.server.model.spec.ts

+      expect(actualSavedObject).toEqual(expected)
+    })
+
+    it('should save successfully and set isOnboarded to false when sms is sent using default credentials', async () => {


I think we should separate the tests into default -> false, custom -> true tests

src/app/services/sms/sms.controller.ts

src/app/services/sms/sms.util.ts

src/app/services/sms/sms.types.ts

…val from sms to admin-form

karrui

lgtm, just need to log that one extra error

src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts

src/app/services/sms/sms.service.ts

seaerchin · 2021-07-14T02:39:00Z

Tested on staging - counts retrieval on staging takes ~70ms for 10k sms counts.

* feat(sms_count.server.model): adds new isOnboardedAcc key and scripts to add/delete the key * refactor(sms.types): adds property that checks if a sms is sent using formsg's credentials * refactor(sms_count.server.model): adds pre-save hook to automatically set property * feat(sms.types): updated typings of IVerificationsmscount * test(sms_count.server): fixed tests and added extra tests for isOnboardedAccount * feat(sms.service): adds new method to retrieve free sms counts of a user * test(sms.service): adds new unit tests for sms count retrieval * test(sms_count.server.model): updated test case for a client using default credentials * feat(sms.controller): added new sms controller iwth method to retrieve counts and msgsrvcid fix(sms.controller.ts): added mapRouteError for sms controller * feat(routes): exposes new route at sms/:userId/:formId * fix(sms.controller): removed sms count retrieval requiring the userId as it sohuld be for the admin * refactor(sms): count retrieval no longer required userId * test(sms.controller): added unit tests for controller method to retrieve counts * fix(sms.controller): changed to relative imports in spec to avoid e2e tests breaking * refactor(sms_count): sms service wraps model count documents call * test(sms.service): reworked tests to account for mocking of db * refactor(sms.controller): removed extra middleware from sms controller * test(sms.controller.spec): renamed sms controller's exposed method in tests * refactor(sms.routes): removed unused sms route * refactor(sms.controller): changed counts retrieval to be solely focused on counts * feat(admin-forms.form.routes): adds new route for retrieving free sms ocunts for admin of a form * refactor(sms_count): shifts default msgsrvcid to be from config, helper method defined in test * test(sms.controller.spec): updated tests to use mocks * test(sms_count.server.model.spec): updated db tests to use smsConfig * fix(v3.routes): removed extra import statement * refactor(types): renamed smscounts meta and shifted it to api renamed to smsCountsDto because it is used to transfer data b/w fe and be. against giving it a more specific name because this is a type signifying a transfer of sms count data. context for what this sms counts actually means should be given by the caller * test(sms_count.server.model.spec): updated test to use sms_config * docs(sms): updated docs for sms counts retrieval in controller and route * test(sms.service.spec): made tests more explicit * refactor(admin-form.controller): shifts over handler for count retrieval from sms to admin-form * test(admin-form.controller.spec): shifts over associated tests * refactor(sms_count.server.model.spec): changed tests structure * chore(sms.util): removed unneeded method * refactor(sms.types): added in docs for retrieveFreeSmsCounts * fix(admin-forms.form.routes): removed erroneous import * fix(sms.service): adds error logging

* feat(sms-limiting): sms counts retrieval (#2276) * feat(sms_count.server.model): adds new isOnboardedAcc key and scripts to add/delete the key * refactor(sms.types): adds property that checks if a sms is sent using formsg's credentials * refactor(sms_count.server.model): adds pre-save hook to automatically set property * feat(sms.types): updated typings of IVerificationsmscount * test(sms_count.server): fixed tests and added extra tests for isOnboardedAccount * feat(sms.service): adds new method to retrieve free sms counts of a user * test(sms.service): adds new unit tests for sms count retrieval * test(sms_count.server.model): updated test case for a client using default credentials * feat(sms.controller): added new sms controller iwth method to retrieve counts and msgsrvcid fix(sms.controller.ts): added mapRouteError for sms controller * feat(routes): exposes new route at sms/:userId/:formId * fix(sms.controller): removed sms count retrieval requiring the userId as it sohuld be for the admin * refactor(sms): count retrieval no longer required userId * test(sms.controller): added unit tests for controller method to retrieve counts * fix(sms.controller): changed to relative imports in spec to avoid e2e tests breaking * refactor(sms_count): sms service wraps model count documents call * test(sms.service): reworked tests to account for mocking of db * refactor(sms.controller): removed extra middleware from sms controller * test(sms.controller.spec): renamed sms controller's exposed method in tests * refactor(sms.routes): removed unused sms route * refactor(sms.controller): changed counts retrieval to be solely focused on counts * feat(admin-forms.form.routes): adds new route for retrieving free sms ocunts for admin of a form * refactor(sms_count): shifts default msgsrvcid to be from config, helper method defined in test * test(sms.controller.spec): updated tests to use mocks * test(sms_count.server.model.spec): updated db tests to use smsConfig * fix(v3.routes): removed extra import statement * refactor(types): renamed smscounts meta and shifted it to api renamed to smsCountsDto because it is used to transfer data b/w fe and be. against giving it a more specific name because this is a type signifying a transfer of sms count data. context for what this sms counts actually means should be given by the caller * test(sms_count.server.model.spec): updated test to use sms_config * docs(sms): updated docs for sms counts retrieval in controller and route * test(sms.service.spec): made tests more explicit * refactor(admin-form.controller): shifts over handler for count retrieval from sms to admin-form * test(admin-form.controller.spec): shifts over associated tests * refactor(sms_count.server.model.spec): changed tests structure * chore(sms.util): removed unneeded method * refactor(sms.types): added in docs for retrieveFreeSmsCounts * fix(admin-forms.form.routes): removed erroneous import * fix(sms.service): adds error logging * feat(sms-limiting): prerelease (#2500) * refactor(admin-form/controller): removed env var being sent and changed quota to be on method * refactor(sms.types): updated disableSms to use mongoose types; updated typing of verification schema * fix(sms.config): added missing property

seaerchin requested review from karrui and mantariksh June 30, 2021 04:14

seaerchin force-pushed the feat/sms-counts-retrieval branch from b153443 to 882365c Compare June 30, 2021 06:36

seaerchin requested review from chowyiyin, yong-jie and tshuli June 30, 2021 06:37

karrui requested changes Jul 1, 2021

View reviewed changes

seaerchin mentioned this pull request Jul 1, 2021

feat(sms-limiting): admin facing frontend #2280

Merged

7 tasks

seaerchin added 16 commits July 1, 2021 17:33

feat(sms_count.server.model): adds new isOnboardedAcc key and scripts…

389d04d

… to add/delete the key

refactor(sms.types): adds property that checks if a sms is sent using…

2d15046

… formsg's credentials

refactor(sms_count.server.model): adds pre-save hook to automatically…

6879c08

… set property

feat(sms.types): updated typings of IVerificationsmscount

acc2121

test(sms_count.server): fixed tests and added extra tests for isOnboa…

7ef3f12

…rdedAccount

feat(sms.service): adds new method to retrieve free sms counts of a user

dced045

test(sms.service): adds new unit tests for sms count retrieval

65e4535

test(sms_count.server.model): updated test case for a client using de…

e76303d

…fault credentials

feat(sms.controller): added new sms controller iwth method to retriev…

df9411a

…e counts and msgsrvcid fix(sms.controller.ts): added mapRouteError for sms controller

feat(routes): exposes new route at sms/:userId/:formId

54fff31

fix(sms.controller): removed sms count retrieval requiring the userId…

7ad271f

… as it sohuld be for the admin

refactor(sms): count retrieval no longer required userId

0d181c9

test(sms.controller): added unit tests for controller method to retri…

98f6ac6

…eve counts

fix(sms.controller): changed to relative imports in spec to avoid e2e…

e32cd09

… tests breaking

refactor(sms_count): sms service wraps model count documents call

766ca02

test(sms.service): reworked tests to account for mocking of db

fcb489d

seaerchin force-pushed the feat/sms-limiting branch from 02b0b0f to 3870a78 Compare July 1, 2021 09:35

seaerchin force-pushed the feat/sms-counts-retrieval branch from 882365c to fcb489d Compare July 1, 2021 09:35

seaerchin added 7 commits July 1, 2021 18:21

test(sms.controller.spec): updated tests to use mocks

1b32026

test(sms_count.server.model.spec): updated db tests to use smsConfig

a3ba586

fix(v3.routes): removed extra import statement

7f1722a

test(sms_count.server.model.spec): updated test to use sms_config

aba1b3e

docs(sms): updated docs for sms counts retrieval in controller and route

895fde8

test(sms.service.spec): made tests more explicit

5a3fa4e

seaerchin requested a review from karrui July 2, 2021 04:27

seaerchin mentioned this pull request Jul 5, 2021

feat(sms-limiting): disable sms verifications toggling #2300

Merged

karrui reviewed Jul 5, 2021

View reviewed changes

seaerchin added 5 commits July 7, 2021 14:57

refactor(admin-form.controller): shifts over handler for count retrie…

5ec22f3

…val from sms to admin-form

test(admin-form.controller.spec): shifts over associated tests

d20174f

refactor(sms_count.server.model.spec): changed tests structure

6c58b8b

chore(sms.util): removed unneeded method

9dfbc54

refactor(sms.types): added in docs for retrieveFreeSmsCounts

6dbb74b

seaerchin requested a review from karrui July 7, 2021 07:14

fix(admin-forms.form.routes): removed erroneous import

9d58ab8

seaerchin force-pushed the feat/sms-counts-retrieval branch from 70ba350 to 9d58ab8 Compare July 7, 2021 07:31

karrui approved these changes Jul 12, 2021

View reviewed changes

src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts Show resolved Hide resolved

src/app/services/sms/sms.service.ts Show resolved Hide resolved

fix(sms.service): adds error logging

92838a8

seaerchin merged commit 7f730dd into feat/sms-limiting Jul 14, 2021

seaerchin deleted the feat/sms-counts-retrieval branch July 14, 2021 02:53

seaerchin mentioned this pull request Aug 2, 2021

feat: sms limiting #2504

Merged

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sms-limiting): sms counts retrieval #2276

feat(sms-limiting): sms counts retrieval #2276

seaerchin commented Jun 30, 2021 •

edited

Loading

karrui commented Jun 30, 2021

seaerchin commented Jun 30, 2021 •

edited

Loading

karrui commented Jul 1, 2021

seaerchin commented Jul 1, 2021

karrui Jul 5, 2021

karrui left a comment

seaerchin commented Jul 14, 2021

feat(sms-limiting): sms counts retrieval #2276

feat(sms-limiting): sms counts retrieval #2276

Conversation

seaerchin commented Jun 30, 2021 • edited Loading

Problem

Solution

karrui commented Jun 30, 2021

seaerchin commented Jun 30, 2021 • edited Loading

karrui commented Jul 1, 2021

seaerchin commented Jul 1, 2021

karrui Jul 5, 2021

Choose a reason for hiding this comment

karrui left a comment

Choose a reason for hiding this comment

seaerchin commented Jul 14, 2021

seaerchin commented Jun 30, 2021 •

edited

Loading

seaerchin commented Jun 30, 2021 •

edited

Loading