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

VATEAM-90733: Normalize OMB info fields #2252

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

derekhouck
Copy link
Contributor

@derekhouck derekhouck commented Aug 29, 2024

Summary

Example output

  {
    "formId": "12345",
    // ...
    "ombInfo": {
      "expDate": "8/29/2025",
      "ombNumber": "1234-5678",
      "resBurden": 30
    },
    "chapters": [
      // ...
    ]
  }

Related issue(s)

Testing done

  • Added OMB info unit tests to the Digital Form post processor and the Digital Form GraphQL query
  • Pulled local Drupal data and confirmed the resulting output from content-build

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

I made the decision to format the Expiration Date at this stage. As far as I can tell, we won't need it as anything other than a formatted string in the Form Renderer app, and this seems like the least expensive place to do that formatting. I am open to other suggestions.

@derekhouck derekhouck requested a review from ryguyk August 29, 2024 18:43
@derekhouck derekhouck marked this pull request as ready for review August 29, 2024 18:44
@derekhouck derekhouck requested review from a team as code owners August 29, 2024 18:44
@va-vfs-bot va-vfs-bot temporarily deployed to master/main/90733-normalize-omb-info-section August 29, 2024 18:48 Inactive
Copy link
Contributor

@timcosgrove timcosgrove left a comment

Choose a reason for hiding this comment

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

I have no problem with any of this provided the various GraphQL tests pass, but will hold approval until reviewed by someone on the form builder team.

Copy link
Contributor

@ryguyk ryguyk left a comment

Choose a reason for hiding this comment

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

Great work. One requested change that we discussed around date conversion.

const formatDate = dateString =>
// Depending on what time zone our servers operate on, we may need to adjust
// this offset.
new Date(Date.parse(`${dateString}T04:00-05:00`)).toLocaleDateString();
Copy link
Contributor

Choose a reason for hiding this comment

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

Slack conversation about this: https://agilesix.slack.com/archives/D076J681TB5/p1725460391490419?thread_ts=1725460364.715049&cid=D076J681TB5

Takeaway: it might be better to not convert to a Date object (and thus introduce a time component) and instead just parse the input string to create the output string.

Copy link
Contributor

Choose a reason for hiding this comment

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

A possible starting point:

const removeLeadingZero = s => s.substr(0, 1) === '0' ? s.substr(1) : s;
const convertDate = dateInput => {
    const [year, month, day] = dateInput.split('-'); 	
    return `${removeLeadingZero(month)}/${removeLeadingZero(day)}/${year}`;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. I found a regex expression that reduced the amount of code needed for removeLeadingZero, but other than that, it's mostly your starting point.

@ryguyk ryguyk changed the base branch from integration-form-engine-poc to integration-form-engine-2 September 4, 2024 18:16
@va-vfs-bot va-vfs-bot temporarily deployed to master/main/90733-normalize-omb-info-section September 6, 2024 16:25 Inactive
Copy link
Contributor

@ryguyk ryguyk left a comment

Choose a reason for hiding this comment

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

Nice work!

@@ -11,6 +11,12 @@ const extractAdditionalFields = entity => {
};
const extractForms = resultObject => resultObject?.data?.nodeQuery?.entities;

const formatDate = dateString => {
const removeLeadingZero = s => s.replace(/^0+/, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

This will remove multiple leading zeroes, should they be present, which I think is noteworthy but also should be N/A. In fact, if it somehow were the case that a string came over with multiple leading zeroes, I think we would want them all removed, so this is great. Makes me wonder if removeLeadingZeroes is a better name, but that feels pedantic and possibly confusing given the context.

Nicely done!

@ryguyk ryguyk force-pushed the 90733-normalize-omb-info-section branch from f4446a6 to 680b6b5 Compare September 11, 2024 15:51
@va-vfs-bot va-vfs-bot temporarily deployed to master/main/90733-normalize-omb-info-section September 11, 2024 16:24 Inactive
@ryguyk ryguyk force-pushed the 90733-normalize-omb-info-section branch from 680b6b5 to 4d7dc20 Compare September 12, 2024 02:29
@ryguyk ryguyk requested a review from a team as a code owner September 12, 2024 02:29
@ryguyk ryguyk merged commit 52edb57 into integration-form-engine-2 Sep 12, 2024
17 checks passed
@ryguyk ryguyk deleted the 90733-normalize-omb-info-section branch September 12, 2024 12:18
@ryguyk ryguyk mentioned this pull request Sep 16, 2024
10 tasks
@derekhouck derekhouck restored the 90733-normalize-omb-info-section branch October 2, 2024 15:39
@derekhouck derekhouck deleted the 90733-normalize-omb-info-section branch October 2, 2024 15:45
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.

4 participants