-
Notifications
You must be signed in to change notification settings - Fork 2
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-2490 Wrap Argyle Data Fetch API's #496
base: main
Are you sure you want to change the base?
Conversation
…cb3c/FFS-2490-fetch-argyle-data
…m string to floats
…cb3c/FFS-2490-fetch-argyle-data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just a bunch of style nitpicks.
app/app/services/argyle_service.rb
Outdated
@http.build_url(endpoint).to_s | ||
end | ||
|
||
def store_mock_response(responsePayload:, folderName: "other", fileName:) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idiomatic ruby would have these as snake_case. I'm going to follow up with a Rubocop rule to enforce this automatically.
}) | ||
expect(employment).to have_attributes( | ||
status: "terminated", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still some very strange indenting in here. Maybe installing some kind of ruby plugin to do indenting will allow it to autoformat correctly? Rubocop does not seem to have a linter for this one either (not 100% sure, but I tried the obvious ones to no avail).
app/app/services/argyle_service.rb
Outdated
store_mock_response( | ||
folderName: folderName, | ||
fileName: "request_paystubs", | ||
responsePayload: fetch_paystubs_api(user: argyle_user_id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, maybe at some point we should move these methods into a helper module of some sort so they're not chilling on ArgyleService
for the future.
Ticket
Resolves FFS-2490.
Changes
app/spec/support/fixtures/argyle
.Context for reviewers
Results:
Acceptance testing
: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!
)