VATEAM-88634: Create a normalization layer for Digital Forms

[derekhouck]

Summary

Normalizes how a Digital Form object looks for post-processing.

Before

{
  "data": {
    "nodeQuery": {
      "entities": [
        {
          "nid": 71159,
          "entityLabel": "Form with One Step",
          "fieldVaFormNumber": "1111111",
          "fieldOmbNumber": "1111-1111",
          "fieldChapters": [
            {
              "entity": {
                "entityId": "158252",
                "type": {
                  "entity": {
                    "entityId": "digital_form_name_and_date_of_bi",
                    "entityLabel": "Name and Date of Birth"
                  }
                },
                "fieldTitle": "The Only Step",
                "fieldIncludeDateOfBirth": true
              }
            }
          ]
        },
        {
          "nid": 71160,
          "entityLabel": "Form with Two Steps",
          "fieldVaFormNumber": "2121212",
          "fieldOmbNumber": "1212-1212",
          "fieldChapters": [
            {
              "entity": {
                "entityId": "158253",
                "type": {
                  "entity": {
                    "entityId": "digital_form_name_and_date_of_bi",
                    "entityLabel": "Name and Date of Birth"
                  }
                },
                "fieldTitle": "First Step",
                "fieldIncludeDateOfBirth": true
              }
            },
            {
              "entity": {
                "entityId": "158254",
                "type": {
                  "entity": {
                    "entityId": "digital_form_name_and_date_of_bi",
                    "entityLabel": "Name and Date of Birth"
                  }
                },
                "fieldTitle": "Second Step",
                "fieldIncludeDateOfBirth": false
              }
            }
          ]
        }
      ]
    }
  }
}

After

[
  {
    "cmsId": 71159,
    "title": "Form with One Step",
    "formId": "1111111",
    "ombNumber": "1111-1111",
    "chapters": [
      {
        "id": 158252,
        "chapterTitle": "The Only Step",
        "type": "digital_form_name_and_date_of_bi",
        "pageTitle": "Name and Date of Birth",
        "additionalFields": {
          "includeDateOfBirth": true
        }
      }
    ]
  },
  {
    "cmsId": 71160,
    "title": "Form with Two Steps",
    "formId": "2121212",
    "ombNumber": "1212-1212",
    "chapters": [
      {
        "id": 158253,
        "chapterTitle": "First Step",
        "type": "digital_form_name_and_date_of_bi",
        "pageTitle": "Name and Date of Birth",
        "additionalFields": {
          "includeDateOfBirth": true
        }
      },
      {
        "id": 158254,
        "chapterTitle": "Second Step",
        "type": "digital_form_name_and_date_of_bi",
        "pageTitle": "Name and Date of Birth",
        "additionalFields": {
          "includeDateOfBirth": false
        }
      }
    ]
  }
]

Related issue(s)

• Create a normalization layer for KISS data va.gov-team#88634

Testing done

• Added unit tests for the Digital Form post processor
• Updated unit test for the Digital Form query object
• Pulled Drupal data from the integration branch deployment to get the before & after outputs above.

What areas of the site does it impact?

Only affects the output of digital-forms.json, which is not currently generated in production.

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

Is this the format desired for the normalization layer? Can you foresee any situations where this format will cause issues?

[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]

ESLint disabled here

[va-vfs-bot]

ESLint disabled here

[va-vfs-bot]

ESLint disabled here

[va-vfs-bot]

ESLint disabled here

[va-vfs-bot]

ESLint disabled here

[ryguyk]

Let's quickly chat about this, but it looks great!

[ryguyk]

Just acknowledging here that this structure assumes the one-page-per-chapter model, which I'm not sure is going to stand the test of time. For now, though, this is fine.

[derekhouck]

Acknowledged. That's a bridge we'll cross when we get there.

[ryguyk]

LET'S DISCUSS

I think we need to align on what a form id is. I do think we want to capture the node/entity id somewhere, so this looks good, but my model up to this point (when working on parsing the form id from the url) has been that the form number would have to be available somewhere. I think we might want to add something like:

{
   ...
   formId: form.fieldVaFormNumber,
   ...
}

We might even need to format it in some way (strip spaces, convert to lowercase, etc.):

{
   ...
   formId: formatFormNumber(form.fieldVaFormNumber),
   ...
}

[derekhouck]

Per our discussion, formId has been added, id has been renamed to cmsId, and subTitle has been removed.

[ryguyk]

Same as above - this can just be forms.map(normalizeForm). Again, this might be a personal preference.

[derekhouck]

Done

[ryguyk]

This can just be chapters.map(normalizeChapter), though that's a matter of preference I suppose.

[derekhouck]

Done

[ryguyk]

What happens if resultObject isn't shaped properly? I think we might want to be defensive here with some optional chaining and appropriate checks.

[derekhouck]

Fixed and tests added for this scenario.

[ryguyk]

This looks very, very nice. Some very robust application code.

I added some commentary around one of the tests, but it's not something I consider blocking approval. I'll give you some time if you feel compelled to change anything or tell me I'm crazy, otherwise I'll get this merged.

[ryguyk]

I love the dependency injection 😍

[ryguyk]

CONSIDER

It feels like this should be a spy rather than a stub.

Perhaps more pertinently, though, it feels like we shouldn't need to spy on the specific implementation (drupalUtils.logDrupal), but rather take advantage of the dependency injection you've beautifully included in your application code to pass a dummy logger (see line 177).

[derekhouck]

Fixed

[ryguyk]

CONSIDER

Connected to comment above (line 111). Do we need to import an actual implementation and pass it in here? It feels like we would want one of two things here:

1. Do not pass a logger in here and test that the default logger is called. So, spy on drupalUtils.logDrupal but don't pass it in explicitly.
2. (Probably more valuable) Utilize the dependency injection, which allows us to better isolate the function in question, and pass a dummy logger.

It's arguable we'd want to do both of these, but, in that case, we would need two separate calls to postProcessDigitalForm. I personally find more value in the second case, but there's always an "it depends".

[derekhouck]

Spying on drupalUtils.logDrupal didn't work for me. Not sure why. But I agree, now that we have the dependency injection, we don't actually need to spy on logDrupal and can just pass in a sinon spy/stub to see that it is being called. I'll update the tests.

[derekhouck]

Fixed.