LG-7582: Add proofing components to IdV analytics events

[aduth]

🎫 Ticket

LG-7582

🛠 Summary of changes

To provide a means to disambiguate between in-person and unsupervised proofing events, it's proposed to add the user's proofing components to identity verification events, to allow for this and other similar filtering.

# Find IdV completion events for in-person proofing
filter name = 'IdV: review complete' and properties.event_properties.proofing_components.document_check = 'usps'

📜 Testing Plan

• Run updated analytics regression specs: rspec spec/features/idv/analytics_spec.rb

[sheldon-b]

LGTM

[zachmargolis]

LGTM

[aduth]

This feels rather hacky, but I was finding that the specs were failing due to what appeared to be the user record being outdated (user.proofing_component vs. user.reload.proofing_component had different results when using binding.pry). I suspected it's because the stubbed user was being cached across requests, which is unlike how it would behave in the real-world. Interesting that it only happened when updating proofing component for gpo_letter (pretty late in the flow) and not other proofing components.

[aduth]

I was observing some issues where the proofing components weren't being logged correctly. When debugging, it appeared that the user's proofing component was a new record with nil values, which I traced back to this usage of create_or_find_by. It could make sense if this logic would behave as an upsert without actually fetching the underlying record. At this point in the flow I would assume that a component should definitely exist, so the optimizations of create_or_find_by wouldn't seem to apply, or at least we definitely do want to fetch the record so that the logged proofing components are accurate.

Flagging since it's a bit more substantial a change than I would have expected to need to make here, and in case there's any drawbacks I'm not considering by swapping this to a find_or_create_by.

[aduth]

Marking this ready for review. There's been a handful of approvals already, but also quite a bit of revisions. I'll keep it open for the next day or so to allow time for any other comments and plan to merge sometime tomorrow morning.

[zachmargolis]

so if we're calling .decorate in the ApplicationController... we're decorating it basically everywhere (like 95% of callsites), which tells me we should probably just add whatever behavior here to the base class

[aduth]

so if we're calling .decorate in the ApplicationController... we're decorating it basically everywhere (like 95% of callsites), which tells me we should probably just add whatever behavior here to the base class

Yeah, that's true. I tried to find a middle-ground option by baking in the behaviors as an included module of the base class, but I was running into some issues with how the methods are dynamically defined. I can't recall the specifics of the issue, but I can try that again, and if all else fails, I can bake it into the class itself.

[aduth]

Got something working in 38235a73d.

[aduth]

(I also had an alternative approach which avoided it being a "concern", but unsure how I feel about referencing the base event method class like this)

DECORATED_METHODS.each do |method_name|
  define_method(method_name) do |**kwargs|
    original_method = AnalyticsEvents.instance_method(method_name)
    original_method.bind_call(self, **kwargs, **common_analytics_attributes)
  end
end

[zachmargolis]

I have one more idea, if we implement as_json in the ProofingComponentLogging class, we don't have to remember to call .to_h at each callsite here?

def as_json(*)
  to_h
end

then we can do:

Suggested change

	proofing_components: proofing_components.to_h,
	proofing_components: proofing_components,

[aduth]

I think that would be fine for the logging, but it may end up being a problem for how we're asserting expected calls to track_event, since it would then expect the instance and not the hash.

[zachmargolis]

Oh that's true. It looks like lots of the specs use kind_of(Hash) so for those at least it would be similar

[aduth]

I gave this a shot between 958a793a3 and f58a2f390. It required me to change how we assert against expected analytics properties in the FakeAnalytics class, though this feels like it's in line with how the logging works in the real-world.

[zachmargolis]

LGTM

Also one question: we still haven't 100% migrated the DOC_AUTH event, should we manually add this instrumentation inside the FlowStateMachine?

identity-idp/app/services/flow/flow_state_machine.rb

Lines 177 to 183 in b0e1dba

	def analytics_properties
	{
	flow_path: @flow.flow_path,
	step: current_step,
	step_count: current_flow_step_counts[current_step_name],
	}
	end

[zachmargolis]

unsure if it would too "sneaky" but what if we used prepend?

so

class Analytics
  include AnalyticsEvents
  prepend Idv::AnalyticsEventsEnhancer

and then this becomes...

Suggested change

	extend ActiveSupport::Concern
	included do
	DECORATED_METHODS.each do |method_name|
	alias_method "original_#{method_name}", method_name
	define_method(method_name) do |**kwargs|
	send("original_#{method_name}", **kwargs, **common_analytics_attributes)
	end
	end
	end
	DECORATED_METHODS.each do |method_name|
	define_method(method_name) do |**kwargs|
	super(**kwargs, **common_analytics_attributes)
	end
	end

[aduth]

Hm, the specs for the enhancer class don't seem to like the change.

diff --git a/app/services/analytics.rb b/app/services/analytics.rb
index a70bdaddc..fee5bf485 100644
--- a/app/services/analytics.rb
+++ b/app/services/analytics.rb
@@ -2,7 +2,7 @@
 
 class Analytics
   include AnalyticsEvents
-  include Idv::AnalyticsEventsEnhancer
+  prepend Idv::AnalyticsEventsEnhancer
 
   attr_reader :user, :request, :sp, :ahoy
 
diff --git a/app/services/idv/analytics_events_enhancer.rb b/app/services/idv/analytics_events_enhancer.rb
index cd0460db1..398d8a7f1 100644
--- a/app/services/idv/analytics_events_enhancer.rb
+++ b/app/services/idv/analytics_events_enhancer.rb
@@ -35,14 +35,10 @@ module Idv
       idv_start_over
     ].freeze
 
-    extend ActiveSupport::Concern
-
-    included do
-      DECORATED_METHODS.each do |method_name|
-        alias_method "original_#{method_name}", method_name
-        define_method(method_name) do |**kwargs|
-          send("original_#{method_name}", **kwargs, **common_analytics_attributes)
-        end
+    DECORATED_METHODS.each do |method_name|
+      alias_method "original_#{method_name}", method_name
+      define_method(method_name) do |**kwargs|
+        send("original_#{method_name}", **kwargs, **common_analytics_attributes)
       end
     end
 
diff --git a/spec/services/idv/analytics_events_enhancer_spec.rb b/spec/services/idv/analytics_events_enhancer_spec.rb
index a2f6fe79e..f4ce17b0a 100644
--- a/spec/services/idv/analytics_events_enhancer_spec.rb
+++ b/spec/services/idv/analytics_events_enhancer_spec.rb
@@ -8,7 +8,7 @@ describe Idv::AnalyticsEventsEnhancer do
       @called_kwargs = kwargs
     end
 
-    include Idv::AnalyticsEventsEnhancer
+    prepend Idv::AnalyticsEventsEnhancer
 
     attr_reader :user, :called_kwargs

Failure:

$ rspec spec/services/idv/analytics_events_enhancer_spec.rb

An error occurred while loading ./spec/services/idv/analytics_events_enhancer_spec.rb.
Failure/Error: alias_method "original_#{method_name}", method_name

NameError:
  undefined method `idv_cancellation_confirmed' for module `Idv::AnalyticsEventsEnhancer'

[aduth]

I'm happy to revisit this, but I won't consider it blocking since I'd like to get this merged today.

[aduth]

Also one question: we still haven't 100% migrated the DOC_AUTH event, should we manually add this instrumentation inside the FlowStateMachine?

Within the scope of the ticket, I'm not sure it's necessary, though I agree it'd probably make sense to be consistent with this, with the caveat that FlowStateMachine analytics probably needs a rethink anyways. Do we have an existing ticket for that?

I could write one and/or add to the scope of that.

[zachmargolis]

Do we have an existing ticket for that?

LG-6748