search

18F / identity-idp

Login.gov Core App: Identity Provider (IdP)
https://secure.login.gov/
Other
524 stars 112 forks source link

Pass analytics to StepInfo preconditions proc #11259

Closed matthinz closed 1 month ago

matthinz commented 1 month ago

🎫 Ticket

Relates to work for LG-14393

🛠 Summary of changes

Over on #11254 there was a great suggestion to log an event when a certain step's preconditions fail. The problem was that step_info is implemented as a static method on controllers, and analytics is only available to individual instances.

So this PR updates the signature of the preconditions proc to include an analytics: argument, and ensures that it is passed properly by IdvStepConcern.

zachmargolis commented 1 month ago

Over on #11254 there was a great suggestion to log an event when a certain step's preconditions fail. The problem was that step_info is implemented as a static method on controllers, and analytics is only available to individual instances.

So this PR updates the signature of the preconditions proc to include an analytics: argument, and ensures that it is passed properly by IdvStepConcern.

I skimmed that PR and didn't see a good thread to follow onto... so some overall feedback:

I am skeptical of this approach! preconditions seems like a pure method that we would expect to be free of side-effects, and logging analytics is inherently a side effect. I feel like if we are actively doing a check and expecting something to happen after, that seems like a great candidate for a controller. Or maybe there's some sort of other "event" step we could create a hook for? Like on_precondition_check_fail or something? But to me, preconditions seems like a spot I would not expect this in.

matthinz commented 1 month ago

@zachmargolis Yeah, that's a fair point. Lemme noodle on this