refactor(email-submission/controller): refactored email submission

[seaerchin]

Problem

handleEmailSubmission uses alot of mutated variables and if/else, which could have been better written in order to avoid mental overhead on the maintainer and future developers

Solution

This PR refactors the handleEmailSubmission so that it reads in a sequential fashion and there is only one let declared variable.

Design Considerations

1. let scoped variable - spcpSubmissionFailure. This is because the flag is set only in 1 place and the frontend does do work if it is true. Although there could be a better alternative to avoid the let, it is typed as undefined | true for explicitness and i don't want to significantly refactor the controller just for this.
2. the generation of hashedFields/parsedResponses is still pretty unwieldy and done as a switch case - this is because MyInfo treats this differently

Manual Tests

• 1. Create a single field form without authentication and set its mode to email. Submitting the form should have the field data
• 2. Create a single field SP form and set its mode to email. Submitting the form should have the field data
• 3. Create a single field CP form and set its mode to email. Submitting the form should have the field data
• 4. Create a single field MyInfo form and set its mode to email. Submitting the form should have the field data
• 5. Submit storage mode form to email mode endpoint
• 6. Submit form which has been made private
• 7. Submit form which has been archived
• 8. Submit captcha-enabled form without captcha
• 9. Submit captcha-disabled form
• 10. Submit form which has exceeded submission limit
• 11. Submit form with invalid responses
• 12. Create multiple email fields with email confirmation, check that the correct confirmation email is sent for each one
• 13. Submit form with invalid attachments

[karrui]

@LoneRifle any conflicts here and what you're doing?

[LoneRifle]

Blocks #1571

[seaerchin]

Blocks #1571

@LoneRifle should be fine now; seeing as you will write unit tests for this file, lmk if it reads better/what you'd liek to see improved!

[seaerchin]

point 3 in design considerations

[seaerchin]

@karrui @mantariksh are there any issues apart from the unclear logging?

[mantariksh]

might it be better to extract this Result type into a separate type definition, and add it as a generic argument to this particular andThen? so it's very clear exactly what this andThen block should be returning

[seaerchin]

i agree with the first point. extracting it has value, because it helps the reader to understand this whole chunk without getting lost in the details.

for the second point, this is to aid in typehinting for typescript, and even if we were to add it as a generic argument to this particular andThen, we would still need the annotation on the errors just so that type level information is preserved and not lost (because of the ok).

i think this would impact readability just by virtue of how verbose it is (due to the errors) and wouldn't significantly impact clarity because the destructuring of the next andThen makes the shape and types clear

[mantariksh]

the intent of the code might be clearer if we prefix this with void and shift it above the call to res.json, to make it more obvious that we're not awaiting on the email confirmations

[seaerchin]

i combined it into a single map block and prefixed sendEmailConfirmations with void; main reason is because we need to call orElse on sendEmailConfirmations which requires typehints. The typehinting here is rather verbose due to the parameter forwarding

[mantariksh]

could you also add manual tests for the following (and anything else you can think of)? can't take any chances with email submissions endpoint.

• submit storage mode form to email mode endpoint
• submit form which has been made private
• submit form which has been archived
• submit captcha-enabled form without captcha
• submit captcha-disabled form
• submit form which has exceeded submission limit
• submit form with invalid responses
• create multiple email fields with email confirmation, check that the correct confirmation email is sent for each one

[seaerchin]

@mantariksh based on the old code, there are the following decision points

1. check if form is email mode
2. check if form is public
3. check if there is a captcha
4. check if form has hit submission limits
5. check if attachments are valid
6. sending of admin email
7. sending of email confirmation (to person who submits form)

i added 1 test based on point 5; unsure how to test point 7. i think the tests that you've described covers the rest of the decision points and i've verified them on local

[karrui]

lgtm; update your PR description? Remove the logErrorWithMeta block hehe since it is already removed