Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NT-645] Clean user properties & session properties #990

Merged
merged 11 commits into from
Dec 16, 2019

Conversation

ifbarrera
Copy link
Contributor

@ifbarrera ifbarrera commented Dec 13, 2019

#📲 What

Adds clean user properties and session properties, with tests.

🤔 Why

Part of the data cleanup project.

🛠 How

  • cleans up defaultProperties and rename to sessionProperties
  • adds userProperties to decouple user props from session props

Now, instead of having user properties in the defaultProperties function, we build them separately and join all properties together with:

let props = self.sessionProperties()
    .withAllValuesFrom(userProperties())
    .withAllValuesFrom(eventProperties)

Note that unfortunately, we're not in a good place to test some of the session properties that don't have mocks. I've made a note and filed a separate tech debt ticket to address this so we can be sure to test all session properties in the near future.

♿️ Accessibility

N/A

🏎 Performance

N/A

✅ Acceptance criteria

  • Verify that all the tests in KoalaTests.swift are correct

Verify that the following session properties are sent on every tracking event:

  • apple_pay_capable
  • apple_pay_device
  • cellular_connection
  • client_type
  • current_variants
  • device
  • device_fingerprint
  • device_format
  • device_manufacturer
  • device_model
  • device_orientation
  • distinct_id
  • enabled_features
  • iphone_uuid
  • is_voiceover_running
  • mp_lib
  • os
  • os_version
  • time
  • app_version
  • app_release // might re-name to app_build
  • screen_width
  • user_agent
  • user_logged_in
  • wifi_connection
  • client_platform

Verify that the following user properties are sent on every tracking event:

  • user_is_admin // only if logged in
  • user_backed_projects_count // only if logged in
  • user_country
  • user_facebook_account // only if logged in
  • user_watched_projects_count // only if logged in
  • user_uid // only if logged in

⏰ TODO

  • finalize exact native session props

@ifbarrera ifbarrera marked this pull request as ready for review December 13, 2019 20:21
Copy link
Contributor

@Scollaco Scollaco left a comment

Choose a reason for hiding this comment

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

Good job, @ifbarrera . I just have a question regarding a property name, but it looks good otherwise.

Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

Nice!

props["device_manufacturer"] = "Apple"
props["device_model"] = Koala.deviceModel
props["device_orientation"] = self.deviceOrientation
props["distinct_id"] = self.distinctId
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed on Slack this, device_fingerprint and iphone_uuid all appear to be the same value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll bring it up with insights and remove one property in a follow-up PR if they're ok with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not remove two props?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry yes. Consolidate to one prop.

XCTAssertEqual(device.systemVersion, properties?["os_version"] as? String)
XCTAssertEqual(UInt(screen.bounds.width), properties?["screen_width"] as? UInt)
XCTAssertEqual(UInt(screen.bounds.height), properties?["screen_height"] as? UInt)
// FIXME: The following properties cannot be reliably tested
Copy link
Contributor

Choose a reason for hiding this comment

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

Best to keep the FIXME: here or track in a Jira ticket?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's in both, I can remove it from here if you think it's too noisy.

@ifbarrera ifbarrera merged commit 5c7c98b into master Dec 16, 2019
@ifbarrera ifbarrera deleted the NT-645-user-properties branch December 16, 2019 22:50
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.

3 participants