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

[WIP] FFS-2326: Encapsulate Pinwheel responses into data objects #432

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tdooner
Copy link
Contributor

@tdooner tdooner commented Jan 31, 2025

Ticket

WIP as of 1/31.

Todo list:

  • Do the same conversion for all the other Pinwheel API endpoints
  • Test the flow in sandbox and development environments

Resolves FFS-2326.

Changes

Upgrade Rubocop, remove standard

Standard gem is not being actively used as a linter, and it's holding
back Rubocop's version. Let's remove it. It also seems to be used in the
scanning.rake file which has a bunch of code that doesn't apply to our
situation (since all our scanning work is being done by GH Actions).

Use activeresource gem for employment Pinwheel response

This will allow us to access their properties using dots, rather than
having objects whose keys are a mixture of symbols and strings.

This first one for the /employments endpoint is a proof of concept and
I'll be following up with the rest in subsequent commits.

Context for reviewers

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!)

Standard gem is not being actively used as a linter, and it's holding
back Rubocop's version. Let's remove it. It also seems to be used in the
`scanning.rake` file which has a bunch of code that doesn't apply to our
situation (since all our scanning work is being done by GH Actions).
This will allow us to access their properties using dots, rather than
having objects whose keys are a mixture of symbols and strings.

This first one for the /employments endpoint is a proof of concept and
I'll be following up with the rest in subsequent commits.
@tdooner tdooner marked this pull request as draft January 31, 2025 22:40
<%= table.with_data_point(:employment_start_date, summary[:employment]["start_date"]) %>
<%= table.with_data_point(:employment_end_date, summary[:employment]["termination_date"]) %>
<% employment = summary[:employment] %>
<%= table.with_data_point(:employer_phone, employment.employer_phone_number.value) %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh cool, so it handles nested stuff okay

@http.get(build_url("#{ACCOUNTS_ENDPOINT}/#{account_id}/employment")).body
json = @http.get(build_url("#{ACCOUNTS_ENDPOINT}/#{account_id}/employment")).body

Employment.new(json["data"], environment: @environment)
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I see, we're not having ARes issue requests. Makes sense why we have to do that stuff at the top.

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.

2 participants