LG-7058 | IrsAttemptsApi::Tracker sets up base parameters#6664
Conversation
changelog: Internal: Attempts API: Establish base event attributes
n1zyy
left a comment
n1zyy
left a comment
There was a problem hiding this comment.
This is still a WIP, but I wanted to push this up before the weekend and see if it breaks anything.
|
|
||
| return nil unless spi.present? | ||
| new(spi).link_identity | ||
| end |
There was a problem hiding this comment.
This works and is in keeping with some of the existing patterns, but it feels a bit ugly to me.
| sp1 = create_identity(user, 'http://localhost:3000', 'UUID1') | ||
| create_identity(user, 'urn:gov:gsa:openidconnect:test', 'UUID2') | ||
| sp1 = create_service_provider_identity(user, 'http://localhost:3000', 'UUID1') | ||
| create_service_provider_identity(user, 'urn:gov:gsa:openidconnect:test', 'UUID2') |
There was a problem hiding this comment.
I renamed this which created a lot of noise.
| end | ||
|
|
||
| context 'and there is no service provider identity' do | ||
| it 'returns nil' do |
There was a problem hiding this comment.
Does this seem like reasonable behavior?
|
|
||
| def create_agency_identity(user, agency, uuid) | ||
| AgencyIdentity.create!(user: user, agency: agency, uuid: uuid) | ||
| end |
There was a problem hiding this comment.
This is only used in one place, but I wanted to follow the pattern of the existing create_identity. I paid a bit of a price in terms of PR noise, though...
| sp: service_provider, | ||
| enabled_for_session: enabled_for_session, | ||
| ) | ||
| end |
There was a problem hiding this comment.
To be clear, this isn't testing anything new, just passing in everything now needed.
Trying to reason about here a good test for this should go, though... I feel like there should probably be an end-to-end test somewhere for this.
|
With these applied, a login event has this for |
changelog: Internal, Attempts API, Establish base event attributes
n1zyy
force-pushed
the
mattw/LG-7058_attempt_event_new
branch
from
7391687 to
1c1f3c6
Compare
n1zyy
changed the title
zachmargolis
left a comment
zachmargolis
left a comment
There was a problem hiding this comment.
LGTM, some small comments
| ).take | ||
|
|
||
| return nil unless spi.present? | ||
| new(spi).link_identity |
There was a problem hiding this comment.
I would love a version of this method that is read-only and doesn't attempt to link? But I'm not sure that would work here, for the AttemptsAPI.... since usually the ID is only created after an account is created + consented to? Maybe we put in a pin and circle back
There was a problem hiding this comment.
I was unclear on some of the existing semantics here. Just to be clear, are you suggesting to leave this as-is for now but consider circling back on this later? Or are you saying that this is will prematurely create these links and should be changed now?
There was a problem hiding this comment.
Sorry that was a mess of a comment. Let's leave this as-is
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
2 participants
Why: Simplifies the addition of new events. There are a bunch of base parameters used in every event, so let's set them up in the constructor rather than in every call to
track_event.For bonus points, this hashes our session ID, and passes in a couple newly-requested parameters like device ID.
changelog: Internal: Attempts API: Establish base event attributes