10000 [🛁] Adding new tracking discover properties by eoji · Pull Request #704 · kickstarter/android-oss · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[🛁] Adding new tracking discover properties #704

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 8, 2020
Merged

[🛁] Adding new tracking discover properties #704

merged 1 commit into from
Jan 8, 2020

Conversation

eoji
Copy link
Contributor
@eoji eoji commented Jan 8, 2020

📲 What

Adding "Discover Properties" tracking group.

🤔 Why

The next next step of many for new tracking events 😛

🛠 How

  • Added new discover properties to TrackingClientType.
  • Implemented new discover property values in TrackingClient.
  • Testz

👀 See

nothing 2 c

📋 QA

All discovery events sent to the Lake 💧 should have these properties. This is a WIP so I'm only hitting staging.

Story 📖

Part of [NT-654]

@eoji
Added new discover properties to TrackingClientType.
bcb7b5f 
Implemented new discover property values in TrackingClient.
Testz
@eoji eoji requested a review from ifbarrera January 8, 2020 15:02
ifbarrera
ifbarrera approved these changes Jan 8, 2020
View reviewed changes
Copy link
@ifbarrera ifbarrera 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! Just one suggestion for an additional test.

app/src/test/java/com/kickstarter/libs/KoalaTest.kt
assertEquals(1, expectedProperties["page"])
assertEquals(15, expectedProperties["per_page"])
assertEquals(null, expectedProperties["discover_sort"])
assertNull(expectedProperties["discover_subcategory_id"])
Copy link

Choose a reason for hiding this comment

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

Should we also add a test for a subcategory to cover these 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.

So I do test that in LakeTest here https://github.com/kickstarter/android-oss/pull/704/files#diff-47aba103d20cb8a1a0a18e91c468769bR140. I've only been fixing tests in KoalaTest that break, not adding new ones since I will delete the whole class when we're done.

970C Copy link

Choose a reason for hiding this comment

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

okay cool sounds good!

@eoji eoji merged commit 68e434b into master Jan 8, 2020
@eoji eoji deleted the discovery-properties branch January 8, 2020 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ifbarrera ifbarrera ifbarrera approved these changes

Assignees
No one assigned
Labels
events 🛤
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eoji @ifbarrera
0