Skip to content

LG-4312 Remove analytics_user override of default analytics user#4833

Merged
stevegsa merged 12 commits intomainfrom
stevegsa-rm-user-id-in-track-event
Apr 6, 2021
Merged

LG-4312 Remove analytics_user override of default analytics user#4833
stevegsa merged 12 commits intomainfrom
stevegsa-rm-user-id-in-track-event

Conversation

@stevegsa
Copy link
Contributor

No description provided.

@stevegsa stevegsa marked this pull request as ready for review March 25, 2021 04:50
@aduth
Copy link
Contributor

aduth commented Mar 25, 2021

I'm finding quite a few places where we currently pass in user_id as part of the hash. I only found one when the ticket was first created, in the hybrid image upload flow:

But now I see lots of others as well. Just a few examples:

user_id: existing_user.uuid,

Some of these might not be necessary to override, but in the end it seems not to be quite as safe or straight-forward to remove the attribute as I'd imagined when the ticket was first written.

That we have multiple ways and formats to pass user for analytics is still confusing. An alternative could be to flip it so we remove the analytics_user implementation, and instead always pass a user_id. We only have two instances of that:

def analytics_user
effective_user || super
end

def analytics_user
user_id_from_token ? User.find(user_id_from_token) : super
end

The frontend log would be easy enough to convert. CaptureDocController is tricky, since I think each step uses the controller, so having one place to control the analytics user is convenient 😕

Maybe we should leave it as it is?

@stevegsa stevegsa requested a review from aduth April 1, 2021 03:11
@stevegsa stevegsa changed the title LG-4312 Remove user_id attribute support from Analytics#track_event LG-4312 Remove analytics_user override of default analytics user and use user_id attribute in track_event hash Apr 1, 2021
@stevegsa stevegsa requested a review from aduth April 6, 2021 04:12
@stevegsa stevegsa changed the title LG-4312 Remove analytics_user override of default analytics user and use user_id attribute in track_event hash LG-4312 Remove analytics_user override of default analytics user Apr 6, 2021
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@stevegsa stevegsa merged commit 3fdd731 into main Apr 6, 2021
@stevegsa stevegsa deleted the stevegsa-rm-user-id-in-track-event branch April 6, 2021 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants