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.)
| # | ||
| # 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.
lmgeorge
force-pushed
the
lmgeorge/LG-14064
branch
from
31ba61d to
ebff0c5
Compare
lmgeorge
force-pushed
the
lmgeorge/LG-14064
branch
from
ebff0c5 to
0a1ce51
Compare
| DOCUMENTATION_OPTIONAL_PARAMS = [ | ||
| :user_id, | ||
| *AnalyticsEventsDocumenter::DOCUMENTATION_OPTIONAL_PARAMS.map(&:to_sym), | ||
| ].uniq.freeze |
There was a problem hiding this comment.
A/N: There is a mismatch between how UndocumentedParamsChecker and AnalyticsEventsDocumenter load method definitions for events.
The Idv::AnalyticsEventsEnhancer uses metaprogramming to redefine the method signature of some events to add additional data to the event. As these parameters are not defined on the original method declaration in analytics_events.rb, there is no good way to document these runtime parameters since AnalyticsEventsDocumenter does not use the same runtime method definitions that Analytics and FakeAnalytics define.
It is beyond the scope of this PR to fully fix these behavioral discrepancies, so this is a workaround until a better solution is found.
**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
…al params to be like AnalyticsEventsDocumenter
lmgeorge
force-pushed
the
lmgeorge/LG-14064
branch
from
e30bd0b to
4f3757d
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 and 2) events solely used in jobs. Anything that doesn't fall neatly in either category should just be put in the
IGNORED_METHODSconstant and those 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.
Non-Goals
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.