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

FORMS-867: Export CSV submission columns issue (new validation for 'form' keyword in fieldnames) #1033

Merged
merged 4 commits into from
Sep 26, 2023

Conversation

bcvesalink
Copy link
Contributor

Description

This PR is to fix use cases when user would name one of the fields as form which is system keyword, so we added additional validation so users can't use this API name for fields

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read the CONTRIBUTING doc
  • I have checked that unit tests pass locally with my changes
  • I have run the npm script lint on the frontend and backend
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have approval from the product owner for the contribution in this pull request

Further comments

@bcvesalink bcvesalink self-assigned this Sep 21, 2023
@github-actions
Copy link

github-actions bot commented Sep 21, 2023

Coverage Report (Application)

Totals Coverage
Statements: 41.36% ( 2566 / 6204 )
Methods: 36.29% ( 315 / 868 )
Lines: 46.81% ( 1710 / 3653 )
Branches: 32.14% ( 541 / 1683 )

@github-actions
Copy link

github-actions bot commented Sep 21, 2023

Coverage Report (Frontend)

Totals Coverage
Statements: 69.18% ( 1091 / 1577 )
Methods: 65.87% ( 193 / 293 )
Lines: 73.92% ( 768 / 1039 )
Branches: 53.06% ( 130 / 245 )

@@ -529,6 +532,18 @@ export default {
this.addPatchToHistory();
}
}
this.canSave = true;
modified?.components?.map((comp) => {
if (comp.key === 'form') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

My concern here is that it's a breaking change. A form that was fine before this PR will produce an error when edited and saved after this PR, even if there are no changes to the form.

Maybe this isn't a big deal - how hard would it be to identify how many forms already in production use "form" as one of the field names? Maybe we could work with those form owners to fix their forms before the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WalterMoar there are only 2 published forms which have form as a field name, should we just notify them?

@bcvesalink bcvesalink merged commit 1bbf2ec into master Sep 26, 2023
@bcvesalink bcvesalink deleted the FORMS-867-export-column-issue branch September 26, 2023 21:08
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