[WEB-3376] - Mobile Usage Analytics #1504
Conversation
henry-tp
changed the title
|
/deploy qa2 |
|
henry-tp updated values.yaml file in qa2 |
|
henry-tp updated flux policies file in qa2 |
|
henry-tp deployed blip WEB-3376-mobile-analytics branch to qa2 namespace |
| const eventMetadata = { | ||
| clinicId, | ||
| clinician, | ||
| mobile, |
There was a problem hiding this comment.
KissMetrics automatically converts property names to Titlecase in their platform UI, so making this isMobile or isClinician turns it into Ismobile or Isclinician in KissMetrics, which is not very readable IMO. Maybe we should do snake_case?
app/bootstrap.js
Outdated
| // Empty values should be undefined, not null, to prevent sending blank query params | ||
| const clinicId = state?.blip?.selectedClinicId || undefined; | ||
| const loggedInUserId = state?.blip?.loggedInUserId; | ||
| const user = state?.blip?.allUsersMap?.[loggedInUserId]; |
There was a problem hiding this comment.
by the way Chris, I'm a fan of doing this kind of alignment personally, but if we're not a fan of this (or any of my other stylistic choices) feel free to tell me off 😋
There was a problem hiding this comment.
I think I'm not really actually a fan of this style. Probably prefer to match up with the rest of the codebase (I don't think we do this anywhere)
There was a problem hiding this comment.
No worries at all! Please don't ever hesitate to add in style preferences to reviews :)
| let selectedClinicId = appContext.store?.getState()?.blip?.selectedClinicId; | ||
| if (selectedClinicId) { | ||
| _.defaultsDeep(args, [, { clinicId: selectedClinicId }]); | ||
| } |
There was a problem hiding this comment.
My understanding around lodash and _.defaultsDeep is weak, but my interpretation is that the if condition is needed so that clinicId is undefined instead of null. Let me know if there is another idea behind the if statement that I missed.
There was a problem hiding this comment.
It was to ensure that there is no property on the resulting object that gets passed to the metric API call if there's not a current selectedClinicId. Having a property (e.g. something that can be gotten with hasOwnProperty) who's value is undefined is somewhat different than lacking the property entirely. By gating the defaultsDeep call, we don't add any properties with undefined values.
I guess another strategy would be to trim the undefineds out of the eventMetadata before passing to the defaultsDeep call via _.omitBy(eventMetadata, _.isUndefined) to prevent passing undefined properties through.
There was a problem hiding this comment.
That works! I can confirm that the undefined values are being successfully omitted out of the object.
There was a problem hiding this comment.
Hey actually, it looks like _.isNil captures both undefined and null, so we don't need to explicitly set empty values to undefined first (which is a little clunky IMO)
app/bootstrap.js
Outdated
| }; | ||
|
|
||
| // Empty values should be omitted from the metadata object to prevent sending blank query params | ||
| _.omitBy(eventMetadata, _.isNil); |
There was a problem hiding this comment.
_.omitBy doesn't mutate the target object
There was a problem hiding this comment.
Yikes, embarrassing. I had checked the output of _.omitBy() with console.log() but not that the actual variable eventMetadata variable after the change. Thanks for catching this
There was a problem hiding this comment.
The only reason I know to check is from having put those sorts of things into production myself :)
WEB-3376
Discussion
Per our slack convo, Bill is looking to keep false positives of Mobile Users to a minimum and is not concerned about false negatives (not capturing some legit phone users).
For this reason, I think we want to skip over MDN's recommendation of first looking for touchscreens, as touchscreen laptops will increase false positive rate. So that leaves us with searching for the
mobisubstring. I was not aware when I first started our discussion that we already have autils.isMobile()method, and it conveniently already uses that implementation substring search. Given that re-using theutils.isMobile()abstraction will help us increase consistency in the front-end and reduce complexity, I'm inclined to just do it that way.Not all events in the ticket requirements are testable yet because we simply don't allow personal users to do a lot in our platform on mobile. However, once we allow some functionality for personal users, the code in this PR should allow us to capture the metrics right away.
With these changes in KissMetrics, we can segment our users by
clinicianandmobile.