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

Refactor stringifyFormReplacer helper function into more modular functions #259

Closed
erikphansen opened this issue Sep 11, 2018 · 2 comments
Closed
Assignees
Labels
[practice] engineering Engineering related work [type] debt Tech debt, refactors, maintenance issues

Comments

@erikphansen
Copy link
Contributor

Is your feature request related to a problem? Please describe.
The transformForSubmit helper function defaults to use the stringifyFormReplacer when processing form data for submission. The processing in stringifyFormReplacer performs a lot of magic and could likely have unintended side effects. This actually did happen while adding a new feature in vets.gov. This line in particular was responsible for stripping out a valid address object, leading to a lot of confusion when trying to figure out why the correct payload wasn't getting sent to the backend service.

Describe the solution you'd like
Perhaps the functionality in stringifyFormReplacer could be broken out into smaller parts and transformForSubmit could opt in to only the processing the user wanted. That's just a suggestion.

Additional context
I chatted with @annekainicUSDS about this on September 10, 2018. She determined that this particular code has been in use for a long time and is likely a relic from the original HCA on Vets.gov. It's possible that forms actually depend on this JSON transformation, so any solution would have to avoid breaking forms that are calling this function.

@dmethvin-gov
Copy link
Contributor

I'm working on #211 which is trying to address some of the same concerns regarding site-specific features and behaviors. There shouldn't be a global utility function that knows about the data representations of certain widgets and changes them, at least not at the USFS level. (Also note that we don't currently have any widgets or definitions specific to fields with the name address so this is especially magic right now.) If we need that sort of functionality in a generic way the widget itself should be doing the transformation, or if it's site-specific the USFS caller should be doing the transformation.

Users can affect the submit process by using formConfig.transformForSubmit or get even more control with formConfig.submit.

As part of #211 I have to pull out the vets-specific server responses and this issue involves wanting to change the call to transformForSubmit so it does transformations differently (but only in some cases). The only other thing submitForm does is actually submit the form, and several things there are also specific to vets.gov servers that need to be changed as part of #211 as well.

Once the fix for #211 lands it seems like formConfig.submit will need to be used anyway and maybe an internal transformForSubmit isn't needed? The alternative would seem to involve a lot of hooks and formConfig settings to do the right things at the right time (transform the data based on the server's requirements, set server-specific headers, process server-specific responses, etc.). We can always provide samples of what a formConfig.submit looks like for various cases.

@jcmeloni-usds jcmeloni-usds added [practice] engineering Engineering related work [type] debt Tech debt, refactors, maintenance issues labels Sep 20, 2018
@jcmeloni-usds jcmeloni-usds added this to the End of Phase 1 milestone Sep 20, 2018
@dmethvin-gov
Copy link
Contributor

I'm merging this into #211, the stringifyFormReplacer will only remove empty arrays and objects now. If there are widgets that need post-processing they would need to be processed outside USFS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[practice] engineering Engineering related work [type] debt Tech debt, refactors, maintenance issues
Projects
None yet
Development

No branches or pull requests

3 participants