LG-6111 - Event tracking for personal key dl

[gsa-manish]

No description provided.

[aduth]

Sorry to adjust again, but I noticed that in the upcoming reimplementation of this step, this event was implemented in #6257 as ...

Suggested change

	trackEvent('IdV: personal key downloaded');
	trackEvent('IdV: download personal key');

It would be good to keep these in sync so we don't have to worry about querying for two event names.

[gsa-manish]

No worries..agreed on keeping them in sync.

Updated.

[aduth]

LGTM 👍

I notice that the event name was previously "IdV: personal key downloaded" when it was removed in #6162. That being said, the front-end logging will prefix any logged event with "Frontend:", so the event name would be different anyways.

(cc @zachmargolis)

[zachmargolis]

I notice that the event name was previously "IdV: personal key downloaded" when it was removed in #6162. That being said, the front-end logging will prefix any logged event with "Frontend:", so the event name would be different anyways.

Good catch on the Frontend: prefix!

1. What if we allowlisted the event so we could drop the "Frontend:" prefix?

   identity-idp/app/controllers/frontend_log_controller.rb

   Lines 8 to 13 in 700f883

   	EVENT_MAP = {
   	'IdV: personal key visited' => :idv_personal_key_visited,
   	'IdV: personal key submitted' => :idv_personal_key_submitted,
   	'IdV: personal key confirm visited' => :idv_personal_key_confirm_visited,
   	'IdV: personal key confirm submitted' => :idv_personal_key_confirm_submitted,
   	}.transform_values { |method| AnalyticsEvents.instance_method(method) }.freeze

2. We have the old method, let's remove the @deprecated since the allowlist would use it?

   identity-idp/app/services/analytics_events.rb

   Lines 610 to 615 in 700f883

   	# @deprecated
   	# A user has downloaded their personal key. This event is no longer emitted.
   	# @identity.idp.previous_event_name IdV: download personal key
   	def idv_personal_key_downloaded
   	track_event('IdV: personal key downloaded')
   	end

The documentation tracks the previous name of the event as well, so I think we should use the method

[aduth]

1. What if we allowlisted the event so we could drop the "Frontend:" prefix?

I wondered about this, but one thing I worry about is consistency around when and why we want the prefix in some cases and not others for frontend-logged events. If we want to avoid it altogether, I'd almost rather just get rid of the prefix, and force all events to be added to the allowlist.

[zachmargolis]

I wondered about this, but one thing I worry about is consistency around when and why we want the prefix in some cases and not others for frontend-logged events. If we want to avoid it altogether, I'd almost rather just get rid of the prefix, and force all events to be added to the allowlist.

Yes I am a big fan of that! Only issue is that in the ~15 minute deploy window for new events from the client, old servers may reject.

But I think it's worth a shot, we could even tag the Frontend: prefixed as @identity.idp.previous_event_name so that the documentation can capture both new + old

[gsa-manish]

I wondered about this, but one thing I worry about is consistency around when and why we want the prefix in some cases and not others for frontend-logged events. If we want to avoid it altogether, I'd almost rather just get rid of the prefix, and force all events to be added to the allowlist.

Yes I am a big fan of that! Only issue is that in the ~15 minute deploy window for new events from the client, old servers may reject.

But I think it's worth a shot, we could even tag the Frontend: prefixed as @identity.idp.previous_event_name so that the documentation can capture both new + old

Thinking we should do this as a separate refactor story?

[zachmargolis]

Thinking we should do this as a separate refactor story?

Yes, migrating the rest of frontend events should totally be a different story (or stories, if we group them in batches of 5)

But for this one, we should definitely add idv_personal_key_downloaded to the allowlist

[gsa-manish]

Thinking we should do this as a separate refactor story?

Yes, migrating the rest of frontend events should totally be a different story (or stories, if we group them in batches of 5)

But for this one, we should definitely add idv_personal_key_downloaded to the allowlist

Cool. Added, take a look.

[zachmargolis]

Thanks! Can you remove the @deprecated here?

identity-idp/app/services/analytics_events.rb

Lines 610 to 615 in 700f883

	# @deprecated
	# A user has downloaded their personal key. This event is no longer emitted.
	# @identity.idp.previous_event_name IdV: download personal key
	def idv_personal_key_downloaded
	track_event('IdV: personal key downloaded')
	end

[gsa-manish]

Ah yes.. done.

[zachmargolis]

LGTM