Viewing rooms tracking [GALL-2785]

[mdole]

Update 5/8

Ok! Been a longer road to get here than anticipated, but I think this one's ready for review.

This PR adds tracking + tracking tests for taps and impressions in viewing rooms. That's pretty much it!

---

This PR will implement tracking for Viewing Rooms. Still a bit of clarity needed around exactly what we're tracking and how we're doing it; I'll check in with Louis and continue to update here.

Jira ticket

[mdole]

I felt like I shouldn't have to specify these on this event since this is wrapped in a <ProvideScreenTracking> component that includes these values in its info object - but without them, when I checked what was printed to the console when that event was fired, it only included action_name:

[Fri May 08 2020 13:03:42.163]  LOG      [Event tracked] {
  "action_name": "bodyImpression",
}

What am I missing here?

[pvinis]

Hm, this should work as you suggest. I'll take a look.

[mdole]

Pavlos and I looked at this together. From what we can tell, the issue is that if the useTracking hook is in the same component whose returned component has the <ProvideScreenTracking /> wrapper, the hook is called before the wrapper is rendered so it doesn't include any of the properties from the wrapper. @pvinis anything you'd add to that?

[pvinis]

Yup. I'd like to have this documented somewhere, so I might make a quick PR for this. Hopefully, we will not fall on this edge case again/often. Nice debugging session though :D

[pvinis]

I always feel these tracking objects are getting in the way of the actual code when reading. Would it make sense to keep them on the bottom of the file like the graphql stuff, something like

const tracks = {
tappedArtworkGroup: item => {
                  action_name: Schema.ActionNames.TappedArtworkGroup,
                  context_module: Schema.ContextModules.ViewingRoomArtworkRail,
                  destination_screen: Schema.PageNames.ArtworkPage,
                  destination_screen_owner_id: item?.node?.internalID,
                  destination_screen_owner_slug: item?.node?.slug,
                  type: "thumbnail",
                },
otherEvent: { ... },
}

and then in the place we use them, like here, we just do tracking.trackEvent(tappedArtworkGroup(item)) and it's just one line? Maybe even tracks.tappedArtworkGroup(item) if we modify a bit more.

@artsy/mobile-experience tagging for opinions.

[brainbicycle]

I think it makes sense, FWIW we are going to have similar events for all home rails so I was planning on having a helper function that just creates these events depending on the home rail type so hopefully that will clean things up a little. As a general pattern though I think defining all the tracking events at end of file or a separate file makes sense to me, I prefer to inline as well. Also we are planning on moving to use https://github.com/artsy/cohesion in the future so potentially can add any shared boilerplate analytics stuff there as well.

[mdole]

Good thought! Took a stab at this in f58e6c1; I'm sure we can figure out a helper function that works more broadly but it definitely feels a bit cleaner at least

[mikehrom]

i believe all events need to have a context_screen_owner_type as a minimum, and then context_screen_owner_id and context_screen_owner_slug if applicable

[mdole]

yep! the actual events DO have those properties - they come from the <ProvideScreenTracking /> that wraps the whole dealio. they're not reflected in the test because when we test we don't have that wrapper. does leave a bit of a hole in the tests though...not sure if there's a good way to handle that

Example tracking call:

[mikehrom]

destination_screen_owner_type as well. we've started defining these in detail in cohesion: https://cohesion.artsy.net/interfaces/_schema_events_tap_.tappedartworkgroup.html

[louislecluse]

hm yes you are right this should be destination_screen_owner_type

[mdole]

Good catch! updated in 64bbc3a

[pvinis]