Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add submission limits for storage mode form submissions #1097

Merged
merged 9 commits into from
Feb 27, 2021

Conversation

frankchn
Copy link
Contributor

@frankchn frankchn commented Feb 7, 2021

Problem

This pull request adds a submission limits feature for storage.

Closes #148

Solution

Instead of using the formStatisticsTotal collection, I am simply querying SubmissionModel.countDocuments({ form: form._id }) for the number of submissions up till now. I think this is probably fine as the submission model has an index on the form field and we are already using that to count the total number of submissions for export and attachment downloads and those seem reasonably fast.

Otherwise, we are allowing users to specify a submission limit, but will simply show them an error message instead of attempting to deactivate the form, which is more complicated and requires us to touch the form document itself instead of simply checking for number of submissions before we render the form and before the form is submitted.

This also doesn't require re-CAPTCHA to be turned on (maybe we should?).

Features:

  • New field to let the user specify how many responses they want.
  • Checks for submission limits and shows the user an error message if they visit a form that has exceeded the maximum limit

Before & After Screenshots

image
image

Tests

  • Create a form, and set the limit to zero.
  • Verify that the form is not visible.
  • Set the limit to 1 and submit the form.
  • Verify that the submission goes through
  • Visit the form again and make sure that the form is not visible any more.
  • Set the limit to 2 and open two tabs.
  • Attempt to submit the form in both tabs. Only one should work.

Deploy Notes

This introduces a new submissionLimit field in the Form collection.

Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to make after discussion

  1. Ship form deactivation without notification feature
  2. Form status should be set to deactivated when submission limit is reached (it would be good to log this)
  3. Align UI to design screens

@frankchn
Copy link
Contributor Author

The response limit is off:
image

The response limit is on and above the current response count:
image

The response limit is on and below the current response count:
image

@frankchn
Copy link
Contributor Author

@liangyuanruo I am not sure how to fix these build errors, can you take a look? Thanks!

src/app/modules/form/public-form/public-form.middlewares.ts(28,13): error TS2345: Argument of type '(queryResults: number) => void | Promise<Response<any, Record<string, any>, number>>' is not assignable to parameter of type '(value: number) => Response<any, Record<string, any>, number> | PromiseLike<Response<any, Record<string, any>, number>>'.
  Type 'void | Promise<Response<any, Record<string, any>, number>>' is not assignable to type 'Response<any, Record<string, any>, number> | PromiseLike<Response<any, Record<string, any>, number>>'.
    Type 'void' is not assignable to type 'Response<any, Record<string, any>, number> | PromiseLike<Response<any, Record<string, any>, number>>'.
src/app/modules/form/public-form/public-form.middlewares.ts(33,13): error TS2741: Property 'action' is missing in type '{ form: any; }' but required in type '{ [other: string]: any; action: string; }'.

Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be launched for both Storage and Email mode forms, and add tests.

@frankchn
Copy link
Contributor Author

The email submission route (as defined in email-submission.routes.ts) doesn't support const { form } = req as WithForm<typeof req> and I am not sure where that is added in the processing stack, so I can't enable it at the moment.

@frankchn frankchn force-pushed the frank-update-count branch 2 times, most recently from 98fbf82 to b2e5684 Compare February 21, 2021 06:31
@liangyuanruo
Copy link
Contributor

The email submission route (as defined in email-submission.routes.ts) doesn't support const { form } = req as WithForm<typeof req> and I am not sure where that is added in the processing stack, so I can't enable it at the moment.

Ah, I think this is because we recently refactored to combine the email submission middlewares together into a single handleEmailSubmission function for better type safety since there will be no need for type assertions. We'll eventually combine the remaining code into a handleEncryptSubmission function too.

I think you can get around this by first extracting all the business logic from the checkFormSubmissionLimitAndDeactivate middleware into a service, then using that in both the new middleware as well as to extend the handleEmailSubmission function...

Does this make sense?

Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up from discussions:

  1. drop hasSubmissionLimit boolean
  2. refactor logic out of checkFormSubmissionLimitAndDeactivate middleware into a service function for reuse with both storage and email mode forms

@frankchn
Copy link
Contributor Author

@liangyuanruo made all the changes -- PTAL, thanks!

src/types/form.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @frankchn - clarified with @syan-syan and I think we need some further refinements to both the processing flow and data model.

Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @frankchn , almost there!

Co-authored-by: Yuan Ruo <yuanruo@data.gov.sg>
@frankchn frankchn merged commit d65b4ae into develop Feb 27, 2021
@frankchn frankchn deleted the frank-update-count branch February 27, 2021 03:39
@r00dgirl
Copy link
Contributor

r00dgirl commented Mar 2, 2021

changes from reviewing staging

  • tooltip: Form will automatically deactivate when set response limit is reached. Enable reCaptcha to prevent spam submissions from triggering this limit.
  • “Maximum number of responses allowed”
  • error message below the field change from blue to red

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Response cap/limit
3 participants