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 discovery properties & events [Part 1] #984

Merged
merged 13 commits into from
Dec 10, 2019

Conversation

ifbarrera
Copy link
Contributor

@ifbarrera ifbarrera commented Dec 9, 2019

πŸ“² What

Part 1 of cleaning up our discovery, search, and activities events and properties.

πŸ€” Why

We're doing some cleanup of our events.

πŸ›  How

  • Removing and renaming some discover default properties
  • removing a bunch of deprecated events that we no longer care about
  • updating some of the logic around when events fire

♿️ Accessibility

N/A

🏎 Performance

N/A

βœ… Acceptance criteria

When testing, be sure to enable KOALA_TRACKING in the Kickstarter-iOS scheme.

  • The event Explore Page Viewed fires anytime a new discovery page is viewed. It should not fire on pagination.
  • The event Explore Sort Clicked fires any time a sort is tapped (Ending soon, Popular, etc).
  • The event Activity Feed Viewed fires any time a logged in user views their activity feed. It should not fire on pagination.
  • The event Editorial Card Clicked fires if an editorial card (ex. Go rewardless) is tapped.
  • The event Collection Viewed fires when an editorial collection of projects is viewed (ex. "Go rewardless")
  • The event Filter Clicked fires when a category from the Explore dropdown is selected
  • The event Search Page Viewed fires when the search tab is viewed
  • The event Search Results Loaded fires after entering a search query when results have loaded

Default discovery properties should include the following properties:

  • discover_recommended
  • discover_social
  • discover_staff_picks
  • discover_starred
  • discover_tag
  • discover_everything
  • discover_sort
  • discover_ref_tag
  • search_term
  • discover_category_id
  • discover_category_name
  • discover_parent_category_id
  • discover_parent_category_name

@nativeksr
Copy link
Collaborator

nativeksr commented Dec 9, 2019

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

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 lgtm, minor comments.

public func trackDiscoveryPullToRefresh() {
self.track(event: "Triggered Refresh")
public func trackEditorialHeaderTapped(refTag: RefTag) {
self.track(event: "Editorial Card Clicked", properties: ["ref_tag": refTag.stringTag])
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this not include Discover Properties? Like:

discoveryProperties(from: params).withAllValuesFrom(["ref_tag": refTag.stringTag])

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 don't think its really useful in this case because the next event should be Collection Viewed which does include discover properties πŸ€·β€β™€


private func discoveryProperties(
from params: DiscoveryParams,
prefix _: String = "discover_"
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unused in this function, do we want to use it on line 2194?

@@ -198,3 +198,25 @@ extension RefTag: Argo.Decodable {
}
}
}

extension RefTag {
public static func fromParams(_ params: DiscoveryParams) -> RefTag {
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that this is extracted from the VM we probably want to add a test for it.

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.

:shipit:

@ifbarrera ifbarrera merged commit e3748d2 into master Dec 10, 2019
@ifbarrera ifbarrera deleted the NT-645-clean-discover-properties branch December 10, 2019 15:00
Scollaco pushed a commit that referenced this pull request Dec 13, 2019
* WIP cleaning up discovery properties

* Cleaning up events

* Updating tests

* Formatting

* Reverting scheme change

* Line length

* Remove category_projects_count prop

* Updates

* Formatting

* Cleanup

* Formatting

* RefTag fromParams tests and snake-casing search term
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.

None yet

3 participants