VAGOV-TEAM-89918: Create Form Config object from normalized input #31296

derekhouck · 2024-08-08T21:42:09Z

Are you removing, renaming or moving a folder in this PR?

No, I'm not changing any folders (skip to Summary and delete the rest of this section)
Yes, I'm removing, renaming or moving a folder

Summary

#31202 allowed forms to be rendered dynamically from existing Form Config objects, but we will not be receiving Form Config objects from content-build. This PR takes the output from department-of-veterans-affairs/content-build#2215 and creates a Form Config object from it. Actually connecting content-build and the Form Renderer app together will take place in department-of-veterans-affairs/va.gov-team#89988.

Related issue(s)

Create Form Config object from normalized form input va.gov-team#89918

QA Steps

/form-renderer/digital-form/2121212 should render a form with two variations of the Name and Date of Birth pattern.

Testing done

Added unit tests for the Form Config creation functions.
Manually tested Form Config creation with a hard-coded example normalized form.

What areas of the site does it impact?

This PR touches only our team-owned application (form-renderer). Notably, the app is not yet live.

Acceptance criteria

Quality Assurance & Testing

I fixed|updated|added unit tests and integration tests for each feature (if applicable).
No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
Linting warnings have been addressed
Documentation has been updated (link to documentation *if necessary)
Screenshot of the developed feature is added
Accessibility testing has been performed

Error Handling

Browser console contains no warnings or errors.
Events are being sent to the appropriate logging solution
Feature/bug has a monitor built into Datadog or Grafana (if applicable)

Authentication

Did you login to a local build and verify all authenticated routes work as expected with a test user

Requested Feedback

The schemas are currently hard-coded for the "Name and Date of Birth" pattern. That will change once we add in more patterns. Dynamically selecting proper schemas was not part of the scope of this ticket.

…orm-config-object

va-vfs-bot

ESLint is disabled

vets-website uses ESLint to help enforce code quality. In most situations we would like ESLint to remain enabled.

What you can do

See if the code can be refactored to avoid disabling ESLint, or wait for a VSP review.

va-vfs-bot · 2024-08-08T21:58:39Z

src/applications/form-renderer/tests/unit/utils/formConfig.unit.spec.js

@@ -0,0 +1,66 @@
+/* eslint-disable prefer-destructuring */


ESLint disabled here

src/applications/form-renderer/tests/unit/utils/formConfig.unit.spec.js

va-vfs-bot · 2024-08-08T21:58:39Z

src/applications/form-renderer/utils/formConfig.js

+    rootUrl: `${manifest.rootUrl}/${formId}`,
+    urlPrefix: `/${formId}/`,
+    trackingPrefix: `${formId}-`,
+    // eslint-disable-next-line no-console


ESLint disabled here

rmessina1010

LGTM

…orm-config-object

ryguyk

This looks really solid. I've left a question and a few comments. None of them are blocking, so I'm approving.

ryguyk · 2024-08-15T12:01:49Z

src/applications/form-renderer/_config/formConfig.js

+
+// This is a sample of the data structure produced by content-build.
+export const normalizedForm = {
+  id: 71160,


QUESTION
I thought we settled on cmsId here. Has that been changed?

It is interesting to me to have cmsId on the node but id on the chapters/steps/other things. Perhaps we need to iron this out a bit still.

We absolutely did, and cmsId appears on https://pr18811-ps4nwwul37jtyembecv4bg0gafmyl3oj.ci.cms.va.gov/data/cms/digital-forms.json. Guess I wrote this before we had that discussion. This is the downside to using mock data for testing, but doesn't actually impact anything at the moment, because I don't think we're using cmsId for anything on the front-end yet. Nonetheless, I'll update the mock form to reflect what is coming from content-build.

It is interesting to me to have cmsId on the node but id on the chapters/steps/other things. Perhaps we need to iron this out a bit still.

I felt no immediate need to disambiguate it as there are no other IDs on the chapter level competing for attention, but I could be persuaded to change the key label to chapterId, pageId, or stepId.

ryguyk · 2024-08-15T12:10:27Z

src/applications/form-renderer/utils/formConfig.js

+  chapters.forEach(chapter => {
+    const pages = {};
+    pages[chapter.id] = {
+      path: chapter.id.toString(),


MUSING
I wonder if we're going to want to generate a more human-readable path from a title field of some sort. Having a random ID in the url feels a bit odd. This can be addressed later, certainly.

That makes sense. The only issue with using the chapter title is that we cannot currently guarantee that every chapter title within a form will be unique. If we can guarantee that at the CMS level, I would feel comfortable using it instead of the chapter ID.

ryguyk · 2024-08-15T12:17:04Z

src/applications/form-renderer/utils/formConfig.js

+const formatChapters = chapters => {
+  const formattedChapters = {};
+
+  chapters.forEach(chapter => {


CONSIDER
My personal preference would be the use of reduce (rather than forEach) in cases like this where there are no side effects.

I always forget reduce exists, and whenever someone reminds me, I have go look up how it works again.

ryguyk · 2024-08-15T12:22:49Z

src/applications/form-renderer/utils/formConfig.js

+    const pages = {};
+    pages[chapter.id] = {


CONSIDER
It feels like it would be a bit more "immutable" to simply define pages to be an object with the key `chapter.id' all in one definition/declaration:

const pages = { [chapter.id]: { ... } }

This is a very small issue in this case, but something I personally like to do when possible.

Separately, this one-page-per-chapter code is feeling increasingly odd to me now that we're getting to the point of seeing things in the context of the formConfig. It might be the case that we settle on this constraint (one page per chapter - though I'm not sure that's likely), but it feels odd to see us indexing pages with the chapter id. Ultimately, that's not a problem in and of itself, but it feels slightly confusing to me at first glance. When browsing the code, it feels like it could be a mistake. We know it's not, so, for now, perhaps it could be useful to include a comment explaining this one-page-per-chapter setup under which we currently are operating.

I did it the way I did solely because I forgot how to define a dynamic key in an object literal. The code has now been updated.

Separately, this one-page-per-chapter code is feeling increasingly odd to me now that we're getting to the point of seeing things in the context of the formConfig. It might be the case that we settle on this constraint (one page per chapter - though I'm not sure that's likely), but it feels odd to see us indexing pages with the chapter id. Ultimately, that's not a problem in and of itself, but it feels slightly confusing to me at first glance. When browsing the code, it feels like it could be a mistake. We know it's not, so, for now, perhaps it could be useful to include a comment explaining this one-page-per-chapter setup under which we currently are operating.

I agree the code looks a little misleading here with chapter.id being used as a page key. There's a fundamental disagreement between VA.gov Design and Engineering teams over how VA.gov forms are structured, and we're trying to bridge the divide as best we can. As I mentioned above, if we can get to the point where we can guarantee the uniqueness of chapter and/or page titles in Drupal, we'll have a lot more flexibility to play around with this structure.

In the meantime, I've added a comment explaining why chapter.id is appearing as a key within a pages object.

* main: [SIMPLE-FORMS] feat: add accordion to new confirmation page (#31325) MHV-60214: Landing page typo (#31425) Remove pattern validation for service number (#31417) Update URL for 'Enrollment Manager FAQs' (#31371) Add MST info to SC intro (#31416) Add SC housing & living situation questions (#31391) SC prototype | Update beginning pages (#31354) Prevent multiple TOU requests (#31347) Add trace logging to unit test stability workflow (#31421) 90728 Combine street text fields in submit transformer - forms 10-7959c, 10-7959a (#31404) updated confirmation page: (#31394) #89775: Update transformer to pull modality for appointment from the preferredModality field [VA Requests] (#31380) VAGOV-TEAM-89918: Create Form Config object from normalized input (#31296) Add features flags for upcoming HCA initiatives (#31420) Fixes flaky medications unit test (#31422) added address line 3 and address line 4 to UI (#31423)

derekhouck and others added 4 commits August 6, 2024 16:11

Create initial createFormConfig function

389f996

Format chapters

0cdd626

Add normalized form to mockFetchFormConfigByFormId

56b90d4

Merge branch 'main' into 89918-pull-in-normalized-output-and-create-f…

ab0161a

…orm-config-object

derekhouck requested a review from ryguyk August 8, 2024 21:57

derekhouck marked this pull request as ready for review August 8, 2024 21:57

derekhouck requested a review from a team as a code owner August 8, 2024 21:57

va-vfs-bot temporarily deployed to master/89918-pull-in-normalized-output-and-create-form-config-object/main August 8, 2024 21:57 Inactive

va-vfs-bot requested a review from a team August 8, 2024 21:58

va-vfs-bot reviewed Aug 8, 2024

View reviewed changes

Remove redundant eslint-disable

8394fab

va-vfs-bot temporarily deployed to master/89918-pull-in-normalized-output-and-create-form-config-object/main August 8, 2024 22:06 Inactive

rmessina1010 previously approved these changes Aug 9, 2024

View reviewed changes

derekhouck mentioned this pull request Aug 13, 2024

VAGOV-TEAM-89988: Pull content-build output into Form Renderer #31346

Merged

12 tasks

Merge branch 'main' into 89918-pull-in-normalized-output-and-create-f…

ce894e9

…orm-config-object

va-vfs-bot temporarily deployed to master/89918-pull-in-normalized-output-and-create-form-config-object/main August 14, 2024 16:25 Inactive

Merge branch 'main' into 89918-pull-in-normalized-output-and-create-f…

34dc680

…orm-config-object

va-vfs-bot deployed to master/89918-pull-in-normalized-output-and-create-form-config-object/main August 14, 2024 20:09 View deployment

ryguyk previously approved these changes Aug 15, 2024

View reviewed changes

Update id to cmsID

d5c51de

derekhouck dismissed stale reviews from ryguyk and rmessina1010 via d5c51de August 15, 2024 14:38

Refactor formatChapters to use reduce

58254ed

va-vfs-bot had a problem deploying to master/89918-pull-in-normalized-output-and-create-form-config-object/main August 15, 2024 15:48 Error

Add comment explaining why we're using chapter IDs within pages

c90d2cf

va-vfs-bot had a problem deploying to master/89918-pull-in-normalized-output-and-create-form-config-object/main August 15, 2024 15:52 Error

derekhouck requested a review from ryguyk August 15, 2024 15:58

ryguyk approved these changes Aug 15, 2024

View reviewed changes

ryguyk merged commit f81a780 into main Aug 15, 2024
58 checks passed

ryguyk deleted the 89918-pull-in-normalized-output-and-create-form-config-object branch August 15, 2024 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VAGOV-TEAM-89918: Create Form Config object from normalized input #31296

VAGOV-TEAM-89918: Create Form Config object from normalized input #31296

derekhouck commented Aug 8, 2024 •

edited

Loading

va-vfs-bot left a comment

va-vfs-bot Aug 8, 2024

va-vfs-bot Aug 8, 2024

rmessina1010 left a comment

ryguyk left a comment

ryguyk Aug 15, 2024

derekhouck Aug 15, 2024

derekhouck Aug 15, 2024

ryguyk Aug 15, 2024

derekhouck Aug 15, 2024

ryguyk Aug 15, 2024

derekhouck Aug 15, 2024

ryguyk Aug 15, 2024

derekhouck Aug 15, 2024

derekhouck Aug 15, 2024

VAGOV-TEAM-89918: Create Form Config object from normalized input #31296

VAGOV-TEAM-89918: Create Form Config object from normalized input #31296

Conversation

derekhouck commented Aug 8, 2024 • edited Loading

Are you removing, renaming or moving a folder in this PR?

Summary

Related issue(s)

QA Steps

Testing done

What areas of the site does it impact?

Acceptance criteria

Quality Assurance & Testing

Error Handling

Authentication

Requested Feedback

va-vfs-bot left a comment

Choose a reason for hiding this comment

ESLint is disabled

What you can do

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmessina1010 left a comment

Choose a reason for hiding this comment

ryguyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekhouck commented Aug 8, 2024 •

edited

Loading