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

FFS-2337: Multi-Provider Support in Javascript Front-end code #444

Merged
merged 54 commits into from
Feb 20, 2025

Conversation

jeffcatania-usds
Copy link
Contributor

@jeffcatania-usds jeffcatania-usds commented Feb 11, 2025

Ticket

Resolves FFS-2337.

Changes

  • Creates new "Adapter" classes to encapsulate income data provider sdk's
    • IncomeDataAdapter = an abstract class that is used to wrap a provider SDK
    • PinwheelIncomeDataAdapter = extends IncomeDataAdapter to implement Pinwheel SDK
    • createIncomeDataAdapter() = a factory method for selecting the right IncomeDataAdapter. Currently, only Pinwheel is allowed.
  • Replaces Pinwheel logic inside employer_search controller with the generic IncomeDataAdapter
  • Assumes a new "data-provider" attribute on the HTML tag that is used to select the provider. Currently this should be hardcoded "pinwheel"
  • Introduce global mocks into the test_setup.js file for API and load-script to reduce complexity of testing

Context for reviewers

  • This PR is primarily written in Typescript to take advantage of its Object-Oriented features

Acceptance testing

  • No acceptance testing needed
    • This change will not affect the user experience (bugfix, dependency updates, etc.)
  • Acceptance testing prior to merge
    • This change can be verified visually via screenshots attached below or by sending a link to a local development environment to the acceptance tester
    • Acceptance testing should be done by design for visual changes, product for behavior/logic changes, or both for changes that impact both.
  • Acceptance testing after merge
    • This change is hard to test locally, so we'll test it in the demo environment (deployed automatically after merge.)
    • Make sure to notify the team once this PR is merged so we don't inadvertently deploy the unaccepted change to production. (e.g. :alert: Deploy block! @ffs-eng I just merged PR [#123] and will be doing acceptance testing in demo - please don't deploy until I'm finished!)

@jeffcatania-usds jeffcatania-usds changed the title FFS-2337: Refactor Aggregator Modal trigger to support multi-provider FFS-2337: Multi-Provider Support in Javascript Front-end code Feb 12, 2025
@jeffcatania-usds jeffcatania-usds marked this pull request as ready for review February 12, 2025 03:46
Copy link
Contributor

@allthesignals allthesignals left a comment

Choose a reason for hiding this comment

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

Sorry, I have more comments coming, just haven't had time to dive in

Copy link
Contributor

@GeorgeCodes19 GeorgeCodes19 left a comment

Choose a reason for hiding this comment

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

There's a few optimizations that we can make as well as resolve some things like duplicate imports and proper TS configuration. We should definitely do away with using the any type.

Copy link
Contributor

@tdooner tdooner left a comment

Choose a reason for hiding this comment

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

I didn't review the tests very thoroughly, and may have missed something since I'm not as experienced at JS setup, but, with those caveats, it looks good to me. No blockers, but some suggestions to think about.

Do we want to run the tests automatically in CI? (I think we probably should!) Perhaps we can make a follow-up ticket for that.


export const createModalAdapter = (providerName: string) => {
switch (providerName) {
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make this "pinwheel" instead of default? And raise some kind of JS error if the providerName is unsupported?

Copy link
Contributor Author

@jeffcatania-usds jeffcatania-usds Feb 19, 2025

Choose a reason for hiding this comment

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

I don't see any other error throws in the JS code. What should the default behavior be? Redirect to /500.html - and will we have language specific implementations of this? Does something need to be logged to the track user action endpoint?

I wanted to keep Pinwheel as default until Argyle is added and then introduce the complexity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure the best error behavior, since we don't have any frontend error-tracking at all right now. new Error(...) would be fine with me for now, but I'm not sure whether there's a better pattern.

@allthesignals allthesignals merged commit 39acb23 into main Feb 20, 2025
15 checks passed
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