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 backend validation for form feedback submission #3941

Merged
merged 23 commits into from
Jul 4, 2022

Conversation

hanstirtaputra
Copy link
Contributor

@hanstirtaputra hanstirtaputra commented Jun 2, 2022

Problem

This enforces backend validation form feedback submissions. It's in line with the UI behaviour where we can only submit feedback once per submission

Solution

  • Add new /api/v3/forms/{formId}/submissions/{submissionId}/feedback endpoint to support this
  • Link Angular FE to the new feedback endpoint

Created new controller and route instead of replacing the pre-existing one as we still want to preserve the old /api/v3/forms/{formId}/feedback route

Breaking Changes

  • Yes - this PR contains breaking changes
  • No - this PR is backwards compatible

Tests

  • Test that submitting a feedback with submissionId for a form without a prior form submission with the same submissionId returns 404
  • Test that submitting feedback twice for the same submissionId and formId returns 422
  • Test that feedback using only endpoint of /api/v3/forms/{formId}/feedback still works
  • Test that old client works with new server both locally and on staging (load the submit feedback page on the old client, deploy the new version of the code, then submit feedback to see that feedback submission still works)

Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

a few more thoughts:

  1. have you tested that the old client works with the new server? would be good to do so both locally and on staging (load the submit feedback page on the old client, deploy the new version of the code, then submit feedback and make sure it works as expected)
  2. do you think we need to create an index on the new submissionId field?
  3. what are the pros and cons of this approach vs creating a unique constraint on the submissionId?

shared/types/form/form_feedback.ts Outdated Show resolved Hide resolved
src/app/models/form_feedback.server.model.ts Outdated Show resolved Hide resolved
src/app/modules/feedback/feedback.controller.ts Outdated Show resolved Hide resolved
src/app/routes/api/v3/forms/public-forms.routes.ts Outdated Show resolved Hide resolved
tests/unit/backend/helpers/jest-db.ts Show resolved Hide resolved
@hanstirtaputra hanstirtaputra temporarily deployed to staging-al2 June 8, 2022 05:57 Inactive
@hanstirtaputra
Copy link
Contributor Author

have you tested that the old client works with the new server? would be good to do so both locally and on staging (load the submit feedback page on the old client, deploy the new version of the code, then submit feedback and make sure it works as expected)

Yep tested both locally and on staging and it works as expected - feedbacks are still submitted and reflected in the feedback data. Upon reload it switches to the new endpoint and continues to work with the same behaviour.

do you think we need to create an index on the new submissionId field?

Yes I think so since we will be searching by submissionId in our validation, I've updated the PR to add an index on submissionId field for formfeedback collection

tests/unit/backend/helpers/jest-db.ts Outdated Show resolved Hide resolved
src/app/modules/feedback/feedback.error.ts Outdated Show resolved Hide resolved
src/app/modules/feedback/feedback.error.ts Outdated Show resolved Hide resolved
src/app/modules/feedback/feedback.util.ts Outdated Show resolved Hide resolved
src/app/modules/feedback/feedback.util.ts Outdated Show resolved Hide resolved
src/app/modules/feedback/feedback.util.ts Outdated Show resolved Hide resolved
@mantariksh
Copy link
Contributor

mantariksh commented Jun 14, 2022

Test that a non-existent submissionId returns 404

can we make this test clearer pls? other engineers should be able to carry out the tests without having read your code changes

Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

almost there, just a couple of nits!

@hanstirtaputra hanstirtaputra force-pushed the feat/form-feedback-validation branch from f31648a to 40f2072 Compare June 28, 2022 02:09
@hanstirtaputra hanstirtaputra force-pushed the feat/form-feedback-validation branch from 40f2072 to c0d6450 Compare July 4, 2022 05:41
@hanstirtaputra hanstirtaputra temporarily deployed to staging-al2 July 4, 2022 05:42 Inactive
Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

lgtm other than one nit

src/app/modules/feedback/feedback.controller.ts Outdated Show resolved Hide resolved
@hanstirtaputra hanstirtaputra merged commit 7d094c2 into develop Jul 4, 2022
@hanstirtaputra hanstirtaputra deleted the feat/form-feedback-validation branch July 4, 2022 07:02
@justynoh justynoh mentioned this pull request Jul 12, 2022
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.

2 participants