Explicitly document Flow State Machine analytics events (LG-6748)#7191
Explicitly document Flow State Machine analytics events (LG-6748)#7191zachmargolis merged 15 commits intomainfrom
Conversation
changelog: Internal, Logging, Document doc auth analytics events
app/services/analytics_events.rb
Outdated
| # @identity.idp.previous_event_name IdV: in person proofing ssn visited | ||
| def idv_doc_auth_ssn_visited(**extra) | ||
| track_event('IdV: doc auth ssn visited', **extra) | ||
| end |
There was a problem hiding this comment.
per my RFC this is me collapsing two events that are essentially the same into one.
See the idv/analytics_spec.rb changes for a sense of what difference this would make to the actual flow
| def self.analytics_submitted_event | ||
| :idv_doc_auth_verify_document_status_submitted | ||
| end |
There was a problem hiding this comment.
Our existing pattern for a lot of the Flow State Machine has been to define things as constants and dynamically read them.
I broke from that pattern because I believe class methods are a better choice, and more conventional for Ruby.
There was a problem hiding this comment.
Do you think we should revisit some of the others? Which are you referring to? (STEP_INDICATOR_STEP? others?)
There was a problem hiding this comment.
I mean even the FLOW_STATE_MACHINE_SETTINGS constants count too in controllers.
I think we could file it as a nice-to-have that we switch to methods (maybe even add some syntax sugar/DSL-like methods to declare them), but our usages aren't complicate enough that it's a problem.
for example if we subclassed steps more, methods would be much better than constants because it's clearer to override a method than a constant.
| Time.zone.parse(value) | ||
| end | ||
|
|
||
| DOC_AUTH = 'Doc Auth' # visited or submitted is appended |
There was a problem hiding this comment.
LOL... so much more than "visited" or "submitted" were added to these 😭
| def self.analytics_submitted_event | ||
| :idv_doc_auth_verify_document_status_submitted | ||
| end |
There was a problem hiding this comment.
Do you think we should revisit some of the others? Which are you referring to? (STEP_INDICATOR_STEP? others?)
| analytics.public_send( | ||
| flow.step_handler(step).analytics_submitted_event, | ||
| **result.to_h.merge(analytics_properties), | ||
| ) |
There was a problem hiding this comment.
Trying to think how we validate that these methods are defined. I suppose we can rely on the fact that an error would be thrown in feature specs if they weren't?
There was a problem hiding this comment.
Yeah I've been relying on spec coverage, which is not a guarantee. Another approach I've been using to spot-check has been checking for all *step.rb and *action.rb files, but unfortunately due to some of the optional steps, not all steps have all analytics methods
There was a problem hiding this comment.
Another thing I discovered, the guards for if @analytics_id basically prevented us from trying to log information, I am trying removing that here to see we discover more errors: #7213
2 participants
🎫 Ticket
LG-6748
🛠 Summary of changes
Explicitly documents analytics events. I ran
spec/features/idv/analytics_spec.rbrepeatedly until it passed, but didn't give complete coverage.Draft mode because it's not 100% done. Here are the things I want to get to:
analytics_events.rbadd actual YARD doc params for each new event (I plan to copy fromidv/analytics_spec.rb)but maybe this could be in a follow-up PR? at least this way we get documentation that they existIdv::Steps::InheritedProofing::AgreementStepet al)📜 Testing Plan
NoMethodErrorson the various Step and Action classes