refactor/submit-form: reduce possibility of race conditions in submission flow

[liangyuanruo]

Problem

Addresses #1044

Solution

I did not manage to reproduce the bug in question. This is not a surprise as concurrency bugs are indeterministic by definition. However, based on the symptom and rarity of the bug, it is possible that the behavior is a result of edge cases caused by poor integration with AngularJS. Multiple code paths were thoroughly examined, and the appropriate refactor performed to improve code clarity and reduce the likelihood of bugs.

1. controllerState variable refactor

The controllerState variable is used to store the cloned form object in the savedForm property before fields were locked/disabled. It was previously not attached to the directive scope, but simply as a const which makes it a possible candidate to resolve the concurrency bug as AngularJS would not be tracking changes to this variable during the digest cycle.

2. submitFormMain function refactor

The need to clone the form object before calling this function was removed by refactoring to avoid assigning to the form.attachment property (it is a very bad practice to mutate function arguments). This not only removed the need for an expensive clone operation, but also guarantees that there can be no divergence between the form object in the controller scope vs the one used in the submitForm function, since the call to submitForm(scope.form) is a reference to the same form object.

Almost all the logic necessary to generate the POST body has also been moved into the Form class as instance methods, with the exception of captcha. Internal methods have also been prefixed with an underscore to discourage external use.

Improvements:

• Improved code cleanliness and efficiency

Bug Fixes:

• Possibly resolves Possible concurrency bug causing empty values in Storage Mode submissions #1044

Tests

Manually tested with storage mode submissions with and without attachment fields. As this is sensitive code with low test coverage, the reviewer is invited to step through the commits to verify that the new implementation is sound.

[mantariksh]

lgtm!

[mantariksh]

we are initialising the SDK in production mode here, even if the code is running in dev/staging. this works fine for now because the SDK's crypto.encrypt function does not depend on FormSG's public key, but may lead to unexpected behaviour in future if this class has to call other SDK methods.