Skip to content

Add ability to read events from the IRS Attempts API#6375

Merged
jmhooper merged 7 commits intomainfrom
jmhooper-render-irs-attempts-api-events
May 23, 2022
Merged

Add ability to read events from the IRS Attempts API#6375
jmhooper merged 7 commits intomainfrom
jmhooper-render-irs-attempts-api-events

Conversation

@jmhooper
Copy link
Contributor

This commit adds an API that allows for reading and acknowledging events from the IRS attempts API.

This API is based loosely on RFC 8936. Currently it does not recognize the returnImmediately param and always returns immediately.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM, small comments

Copy link
Contributor

Choose a reason for hiding this comment

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

the logs are JSON, to save ourselves a JSON parse later, should this just be the JSON array or hash or whatever?

Also is this accepting data wholesale from partners into our logs? I'm a little wary of giving partners freeform access to write to our logs because PII may end up there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, yeah I hadn't considered PII but was nervous about just dropping stuff in here. Not super sure how to approach it cause I imagine we do want to be able to see these errors.

Comment on lines 46 to 50
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to change, this is just me thinking aloud... as we've migrated other events to the new typed AnalyticsEvents, we've been checking for strings ex

expect().to receive(:track_event).with('The String Key', value: 1)

or the

expect(analytics).to have_logged_event('string key', value: 1)

but maybe this works just as well? Do we have a preference we should aim for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot about have_logged_event which I think I actually like a little bit better.

Comment on lines 6 to 11
Copy link
Contributor

Choose a reason for hiding this comment

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

if we switch the let to let!( we can let Rspec eager load these and simplify the before block:

Suggested change
events_to_acknowledge
events_to_render
end
let(:events_to_acknowledge) do
end
let!(:events_to_acknowledge) do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so this gets tricky because of the .clear_attempts! call in the before action. That runs after the let! block so the events get created and then immediately destroyed 😭

jmhooper added 7 commits May 23, 2022 14:20
This commit adds an API that allows for reading and acknowledging events from the IRS attempts API.

This API is based losely on [RFC 8936](https://datatracker.ietf.org/doc/html/rfc8936). Currently it does not recognize the `returnImmediately` param and always returns immediately.

changelog: Upcoming Features, IRS Attempts API, The ability to acknowledge IRS attempts API events was added
@jmhooper jmhooper force-pushed the jmhooper-render-irs-attempts-api-events branch from ba92240 to d4e5b73 Compare May 23, 2022 18:21
@jmhooper jmhooper merged commit b1b4aa0 into main May 23, 2022
@jmhooper jmhooper deleted the jmhooper-render-irs-attempts-api-events branch May 23, 2022 20:46
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