refactor: encapsulate parsed responses (part 1) #1140

tshuli · 2021-02-15T03:20:47Z

Problem

Part 1 of #1104

Solution

This PR addresses the comments made in refactor: encapsulate parsedResponses #1070 and refactors the implementation of feat: mask cp uid in autoreply to respondent #1109 into class methods instead of a standalone masking function
As per the objectives stated in Encapsulate the construction and representation of parsed responses into a class #1104, subsequent PR will encapsulate the data processing functions in the middlewaresEmailSubmissionsMiddleware.validateEmailSubmission, AdminFormController.passThroughSpcp,
SpcpController.appendVerifiedSPCPResponses into the same class

Tests

Create a corppass form with email autoreply and pdf response. Submit form. Check that the autoreply email and pdf response both have a masked UID number with only the last 4 characters shown (e.g. *****123A)
Add a text field to the form with the title 'CorpPass Validated UID'. Check that the response on the text field is not masked in autoreply.
Check that the email response to the admin still has the full UID number.

src/app/modules/submission/email-submission/email-submission.util.ts

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

src/app/modules/submission/email-submission/email-submission.util.ts

src/types/express.locals.ts

liangyuanruo

To fix merge conflicts

mantariksh

some small nits + the comment on renaming, otherwise lgtm

src/app/modules/submission/email-submission/email-submission.service.ts

src/app/modules/submission/email-submission/email-submission.util.ts

…cessary Co-authored-by: Yuan Ruo <yuanruo@data.gov.sg>

…teFormattedDataForOneField

tshuli · 2021-03-04T03:43:39Z

@liangyuanruo merge conflicts resolved, for re-review

liangyuanruo · 2021-03-04T05:13:53Z

src/app/services/mail/mail.service.ts

+    dataCollationData,
    formData,


As we're encapsulating the complexity into the EmailDataObj class, could you combine the function arguments to accept an instance of that class instead? the service APIs should not need to know or ask for internal representations.

liangyuanruo

I also see a lot of integration tests had to be updated, which seems fairly troublesome. One of the goals of encapsulation is to make the code more testable. Instead of long-winded integration tests, perhaps we could simply be writing unit tests against the EmailDataObj instead?

liangyuanruo

Post-discussion follow up

To use EmailDataObj in its entirety downstream
Add unit tests for the EmailDataObj class

mantariksh · 2021-03-04T07:20:23Z

I also see a lot of integration tests had to be updated, which seems fairly troublesome.

I think the only integration test file which had to be updated was email-submissions.server.controller.spec.js, which only covers middleware used in the preview submissions endpoint. We can probably delete that test file as soon as the preview submissions middleware functions are combined into a controller. the individual functions are all in typescript already.

liangyuanruo · 2021-03-05T03:26:36Z

src/app/modules/submission/email-submission/email-submission.service.ts

 export const createEmailData = (
  parsedResponses: ProcessedFieldResponse[],
  hashedFields: Set<string>,
+  authType: AuthType = AuthType.NIL,
 ): EmailData => {
-  // First, get an array of email data for each response
-  // Each field has an array of email data to accommodate table fields,
-  // which have multiple rows of data per field. Hence flatten and maintain
-  // the order of responses.
-  return (
-    parsedResponses
-      .flatMap((response) => createEmailDataForOneField(response, hashedFields))
-      // Then reshape such that autoReplyData, jsonData and formData are each arrays
-      .reduce<EmailData>(
-        (acc, dataForOneField) => {
-          if (dataForOneField.autoReplyData) {
-            acc.autoReplyData.push(dataForOneField.autoReplyData)
-          }
-          if (dataForOneField.jsonData) {
-            acc.jsonData.push(dataForOneField.jsonData)
-          }
-          acc.formData.push(dataForOneField.formData)
-          return acc
-        },
-        {
-          autoReplyData: [],
-          jsonData: [],
-          formData: [],
-        },
-      )
-  )
+  return new SubmissionEmailObj(parsedResponses, hashedFields, authType)
 }


createEmailData is now a dumb wrapper around the SubmissionEmailObj constructor and offers no abstraction. can we get rid of it and just call new SubmissionEmailObj(...) directly?

Alternatively - keep createEmailData but have it return a Result for type safety? Either in this PR or the next one.

tshuli · 2021-03-08T05:50:30Z

thanks @liangyuanruo for reviewing, will continue on the issues raised in the next PR

tshuli changed the title ~~Refactor encapsulate parsed responses 1~~ refactor: encapsulate parsed responses (part 1) Feb 15, 2021

tshuli marked this pull request as ready for review February 15, 2021 03:28

tshuli requested review from liangyuanruo and mantariksh February 15, 2021 07:29

mantariksh suggested changes Feb 18, 2021

View reviewed changes

tshuli force-pushed the refactor-encapsulate-parsed-responses-1 branch from 865b7fa to 0aeae38 Compare February 22, 2021 03:57

tshuli mentioned this pull request Feb 22, 2021

refactor: encapsulate parsedResponses #1070

Closed

3 tasks

tshuli requested a review from mantariksh February 22, 2021 05:43