Skip to content

Pass analytics to StepInfo preconditions proc#11259

Closed
matthinz wants to merge 3 commits intomainfrom
matthinz/precondition-analytics
Closed

Pass analytics to StepInfo preconditions proc#11259
matthinz wants to merge 3 commits intomainfrom
matthinz/precondition-analytics

Conversation

@matthinz
Copy link
Contributor

🎫 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.

We'd like to log an event from certain preconditions procs. This just updates the proc signature to include an `analytics:` argument.

[skip changelog]
@matthinz matthinz requested a review from a team September 18, 2024 18:11
@user = user
end

def steps
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes to this method:

  1. Made it public (so I could use it in a spec)
  2. Added @steps ||= to the beginning so steps are cached
  3. Added .freeze to the object definition

Copy link
Contributor

Choose a reason for hiding this comment

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

what's the purpose of the freeze? was code modifying this sneakily?

}.freeze
end

def step_allowed?(key:)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes to this one:

  1. Made public
  2. Added analytics: arg

@zachmargolis
Copy link
Contributor

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
Copy link
Contributor Author

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

@matthinz matthinz closed this Sep 18, 2024
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