fix(encrypt-submission): add missing ndi response fields #7425

g-tejas · 2024-06-21T07:48:18Z

Problem

Previously for email notifications for storage mode forms, for MyInfo fields, only the [MyInfo] Name gets added, and not the "sgID Validate NRIC". This PR adds them.

Closes FRM-1752

Breaking Changes

No - this PR is backwards compatible

Tests

Without NRIC Masking

1. Create storage mode form, and add admin emails.
2. Add my info fields and make form public
3. Fill in a form and submit
4. Receive email and observe that the sgID Validated NRIC field exists. The NRIC must be unmasked

With NRIC Masking

1. Create storage mode form, and add admin emails.
2. Enable NRIC Masking
3. Add my info fields and make form public
4. Fill in a form and submit
5. Receive email and observe that the sgID Validated NRIC field exists. The NRIC in the email must be masked

g-tejas · 2024-06-21T07:48:34Z

fix(tests): fix e2e testing for email notifications on encrypt-forms #7426
fix(encrypt-submission): add missing ndi response fields #7425 👈
develop

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @g-tejas and the rest of your teammates on Graphite

KenLSM

LGTM. How did we miss this 🙈

kevin9foong · 2024-06-23T16:18:37Z

hi @g-tejas @KenLSM

just a small thing to point out, it seems that if we add the ndi responses to the response object
then the sdID validate NRIC will be stored twice in the submission's database entry.
once under the encryptedContent field (current file change) and another time during the verifiedContent field (in the submitEncryptModeForm method).

this causes

duplication of nric field in 2 fields of the submission entry in the db
nric masking has to mask at 2 places

I was wondering if a better approach is to reduce the duplication of extracting the sgid from jwt token/OidcService & storage of nric field in the saved submission entry in the db

I propose an alternative implementation:

currently, the nric is stored and encrypted in the verifiedContent field after being loaded/extracted in submitEncryptModeForm
instead of extracting/loading the nric twice, shall we append this fetched sgid nric directly into the mail message payload instead? this would reduce the duplication mentioned above

Details:
The src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts, line 135 onwards loads the nric once again and stores this duplicate nric in the "verifiedContent" field on top of the "encryptedContent" field.

linear · 2024-06-24T03:09:51Z

FRM-1752 Add missing NDI response fields

before nric masking was merged

src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts

kevin9foong · 2024-06-28T06:30:10Z

hey @g-tejas, the code looks good! please let me know your thoughts on the PR comment

kevin9foong

Lgtm!

…ng_for_email_notifications_on_encrypt-forms fix(tests): fix e2e testing for email notifications on encrypt-forms

kevin9foong · 2024-07-02T03:21:22Z

__tests__/e2e/helpers/verifySubmission.ts

@@ -120,6 +123,7 @@ export const verifyEmailSubmission = async ({
      getResponseTitle(field, { mode: FormResponseMode.Email }),
      ...responseArray,
    ])
+    if (!submission.attachments) continue


perhaps we can change this to if ... { } instead of continue. justification: might be easier to understand for future modifications to not accidentally include test code that is not executed

kevin9foong · 2024-07-02T03:23:16Z

__tests__/e2e/helpers/verifySubmission.ts

@@ -193,6 +211,7 @@ export const verifyEncryptSubmission = async (
        getResponseTitle(field, { mode: FormResponseMode.Email }),
        ...responseArray,
      ])
+      if (!submission.attachments) continue


same as above

src/app/modules/submission/email-submission/__tests__/email-submission.controller.spec.ts

src/app/modules/submission/encrypt-submission/__tests__/encrypt-submission.controller.spec.ts

src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts

* fix(encrypt-submission): add missing ndi response fields * fix(tests): fix e2e testing for email notifs on encrypt-form * fix: remove attachment tests temporarily * fix: fix comment for email test * test: add encrypt tests for nric masking in email notifs * test: enable jest tests * fix: fix comment for tests * test: fix verifyEncryptSubmission test

g-tejas marked this pull request as ready for review June 21, 2024 07:48

g-tejas marked this pull request as draft June 21, 2024 07:48

g-tejas marked this pull request as ready for review June 21, 2024 07:48

g-tejas requested a review from KenLSM June 21, 2024 07:48

g-tejas mentioned this pull request Jun 21, 2024

fix(tests): fix e2e testing for email notifications on encrypt-forms #7426

Merged

1 task

g-tejas force-pushed the 06-21-fix_encrypt-submission_add_missing_ndi_response_fields branch from 8ed3504 to 66d2d9d Compare June 21, 2024 09:18

g-tejas requested a review from kevin9foong June 21, 2024 09:22

KenLSM previously approved these changes Jun 23, 2024

View reviewed changes

KenLSM self-requested a review June 24, 2024 02:49

g-tejas force-pushed the 06-21-fix_encrypt-submission_add_missing_ndi_response_fields branch 2 times, most recently from c4bf485 to 2b5ddcb Compare June 27, 2024 12:34

fix(encrypt-submission): add missing ndi response fields

e1d4d71

g-tejas force-pushed the 06-21-fix_encrypt-submission_add_missing_ndi_response_fields branch from 2b5ddcb to e1d4d71 Compare June 27, 2024 12:36

fix(tests): fix e2e testing for email notifs on encrypt-form

dd8db09

kevin9foong reviewed Jun 28, 2024

View reviewed changes

src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts Outdated Show resolved Hide resolved

g-tejas force-pushed the 06-21-fix_encrypt-submission_add_missing_ndi_response_fields branch from b0ac37d to 07ec710 Compare June 28, 2024 08:26

kevin9foong approved these changes Jun 29, 2024

View reviewed changes

g-tejas force-pushed the 06-21-fix_encrypt-submission_add_missing_ndi_response_fields branch 5 times, most recently from 884dc49 to 5cd89a5 Compare July 1, 2024 01:50

Merge pull request #7426 from opengovsg/06-21-fix_tests_fix_e2e_testi…

0b7b4bc

…ng_for_email_notifications_on_encrypt-forms fix(tests): fix e2e testing for email notifications on encrypt-forms

g-tejas force-pushed the 06-21-fix_encrypt-submission_add_missing_ndi_response_fields branch from 5cd89a5 to 0b7b4bc Compare July 1, 2024 08:46

fix: remove attachment tests temporarily

5d3a5e4

g-tejas added 3 commits July 1, 2024 18:24

fix: fix comment for email test

4c0933d

test: add encrypt tests for nric masking in email notifs

37700a7

test: enable jest tests

ede5dc2

kevin9foong reviewed Jul 2, 2024

View reviewed changes

fix: fix comment for tests

8bd99d4

kevin9foong reviewed Jul 2, 2024

View reviewed changes

test: fix verifyEncryptSubmission test

d215238

g-tejas merged commit 7c5f436 into develop Jul 2, 2024
15 checks passed

g-tejas deleted the 06-21-fix_encrypt-submission_add_missing_ndi_response_fields branch July 2, 2024 05:15

KenLSM mentioned this pull request Jul 4, 2024

build: release v6.132.0 #7473

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(encrypt-submission): add missing ndi response fields #7425

fix(encrypt-submission): add missing ndi response fields #7425

g-tejas commented Jun 21, 2024 •

edited

Loading

g-tejas commented Jun 21, 2024 •

edited

Loading

KenLSM left a comment

kevin9foong commented Jun 23, 2024 •

edited

Loading

linear bot commented Jun 24, 2024

kevin9foong commented Jun 28, 2024

kevin9foong left a comment

kevin9foong Jul 2, 2024 •

edited

Loading

kevin9foong Jul 2, 2024

fix(encrypt-submission): add missing ndi response fields #7425

fix(encrypt-submission): add missing ndi response fields #7425

Conversation

g-tejas commented Jun 21, 2024 • edited Loading

Problem

Tests

g-tejas commented Jun 21, 2024 • edited Loading

KenLSM left a comment

Choose a reason for hiding this comment

kevin9foong commented Jun 23, 2024 • edited Loading

linear bot commented Jun 24, 2024

kevin9foong commented Jun 28, 2024

kevin9foong left a comment

Choose a reason for hiding this comment

kevin9foong Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

kevin9foong Jul 2, 2024

Choose a reason for hiding this comment

g-tejas commented Jun 21, 2024 •

edited

Loading

g-tejas commented Jun 21, 2024 •

edited

Loading

kevin9foong commented Jun 23, 2024 •

edited

Loading

kevin9foong Jul 2, 2024 •

edited

Loading