LG-14064: Make IdV event enhancement opt-out #11588
Conversation
lmgeorge
added
status - ready for review
ruby
| *EXCLUDED_FRONTEND_EVENT_METHODS, | ||
| *EXCLUDED_JOB_EVENT_METHODS, | ||
| *EXCLUDED_MISC_EVENT_METHODS, | ||
| ].uniq.freeze |
There was a problem hiding this comment.
A/N: Using a Set causes some unexpected behaviors when doing collection comparisons as it behaves more like a Hash than an Array. Switched to using uniq since the const is immutable anyway.
| @@ -120,7 +99,7 @@ module AnalyticsEventsEnhancer | |||
| idv_final | |||
| idv_please_call_visited | |||
| idv_start_over | |||
| ].to_set.freeze | |||
| ].uniq.freeze | |||
There was a problem hiding this comment.
A/N: Using a Set causes some unexpected behaviors when doing collection comparisons as it behaves more like a Hash than an Array. Switched to using uniq since the const is immutable anyway. (This uniq call is superfluous as the list is so short, but keeping it here for consistency in behavior.)
| :idv_warning_shown, | ||
| ].freeze | ||
|
|
||
| EXCLUDED_JOB_EVENT_METHODS = [ |
There was a problem hiding this comment.
Is there a way we could just detect that idv_session is not available / present and not attempt to enhance in that case? That way we don't have to maintain a list of events that fire in the worker
There was a problem hiding this comment.
That's what I would love to move to but felt like that might be beyond the scope of the ticket. Open to moving back to draft and rethinking that though!
There was a problem hiding this comment.
I really like this idea, but I also agree that it's a deviation from what the story says.
| # | ||
| # Additionally, +profile_history+, the list of a User's profiles | ||
| # (sorted by creaton date, oldest to newest), may be added to events, but this is opt-in only. | ||
| # See {AnalyticsEventsEnhancer::METHODS_WITH_PROFILE_HISTORY} for the list of included events. |
There was a problem hiding this comment.
I'm going to approve this because it successfully implements what's asked in the story and is well-written.
I think it would be neat to do what Matt suggested in a comment and look for the presence of an IDV session, but the last day of the sprint is probably a bad time to completely rewrite your PR. 👼 Might be a neat followup later?
Tests appear to have gone off the rails (pun semi-intended) for reasons unrelated to your code.
| :idv_warning_shown, | ||
| ].freeze | ||
|
|
||
| EXCLUDED_JOB_EVENT_METHODS = [ |
There was a problem hiding this comment.
I really like this idea, but I also agree that it's a deviation from what the story says.
**Why**
* When Idv::AnalyticsEventEnhancer was first introduced it was opt-in,
but this no longer makes sense for our current analytics tracking
needs.
* While a better long-term strategy would be to eliminate the event
enhancer, this should be addressed in future work given its complexity.
**How**
* Broke up the IGNORED_METHODS constant into 3 buckets:
1) frontend events
2) events solely used in jobs
3) a miscellaneous bucket
These contexts may not have access to the proofing session info. For
the miscellaneous bucket, additions should be minimal.
* Added specs to ensure that the list of event methods referenced don't
get out of sync with AnalyticsEvents and that there is no overlap
between ignored and explicitly opted-in methods.
changelog: Internal, IdV Analytics, Make IdV event enhancement opt-out
lmgeorge
force-pushed
the
lmgeorge/LG-14064
branch
from
31ba61d to
ebff0c5
Compare
Why
When Idv::AnalyticsEventEnhancer was first introduced it was opt-in, but this no longer makes sense for our current analytics tracking needs.
While a better long-term strategy would be to eliminate the event enhancer, this should be addressed in future work given its complexity.
How
Broke up the IGNORED_METHODS constant into 3 buckets: 1) frontend events 2) events solely used in jobs 3) a miscellaneous bucket These contexts may not have access to the proofing session info. For the miscellaneous bucket, additions should be minimal.
Added specs to ensure that the list of event methods referenced don't get out of sync with AnalyticsEvents and that there is no overlap between ignored and explicitly opted-in methods.
changelog: Internal, IdV Analytics, Make IdV event enhancement opt-out
🎫 Ticket
Link to the relevant ticket:
LG-14064
📜 Testing Plan
Minimal testing needed as no functionality is changing.