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

[EP-404] QA Phase 1 Fix User Properties #1417

Merged
merged 6 commits into from
Apr 7, 2021

Conversation

moderateepheezy
Copy link
Contributor

📲 What

Change how we send launched_project_count and created_project_count for User Properties to Segment.

🤔 Why

The api returns created_project_count, this is the count of the project that you've created, but on segment user_created_project_count definition is different, to get the value you'll need created_project_count from api + draft_project_count from the api, while launched_project_count on segment is created_project_count from the api.

@codecov
Copy link

codecov bot commented Apr 6, 2021

Codecov Report

Merging #1417 (0493aee) into master (05716b7) will decrease coverage by 0.01%.
The diff coverage is 44.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1417      +/-   ##
==========================================
- Coverage   86.10%   86.09%   -0.02%     
==========================================
  Files        1106     1106              
  Lines       97930    97964      +34     
==========================================
+ Hits        84327    84340      +13     
- Misses      13603    13624      +21     
Impacted Files Coverage Δ
KsApi/models/lenses/UserLenses.swift 5.23% <0.00%> (-0.03%) ⬇️
KsApi/models/lenses/User.StatsLenses.swift 18.00% <20.58%> (-1.24%) ⬇️
KsApi/models/User.swift 89.05% <100.00%> (+0.08%) ⬆️
KsApi/models/UserTests.swift 100.00% <100.00%> (ø)
Library/Tracking/KSRAnalytics.swift 85.37% <100.00%> (+0.08%) ⬆️
Library/Tracking/KSRAnalyticsTests.swift 100.00% <100.00%> (ø)
Library/Navigation.swift 77.25% <0.00%> (+0.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05716b7...0493aee. Read the comment docs.

@singhhari singhhari self-assigned this Apr 7, 2021
Library/Tracking/KSRAnalytics.swift Show resolved Hide resolved
props["is_admin"] = user?.isAdmin
props["launched_projects_count"] = user?.stats.memberProjectsCount
props["launched_projects_count"] = user?.stats.createdProjectsCount
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. I think it would be very useful to have all this info documented somewhere so we have a reference in the future.

props["created_projects_count"] = (user?.stats.createdProjectsCount ?? 0) +
(user?.stats
.draftProjectsCount ??
0) // Stats.createdProjectsCount is the count of projects user has lauched only, while event property `created_projects_count` includes Stats.createdProject + Stats.draftProjectsCount
Copy link
Contributor

Choose a reason for hiding this comment

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

typos and still unclear. move this above line 1813 with the following description:

the product/insights team definition of created_projects_count is the sum of createdProjectsCount and draftProjectsCount

props["is_admin"] = user?.isAdmin
props["launched_projects_count"] = user?.stats.memberProjectsCount
props["launched_projects_count"] = user?.stats
.createdProjectsCount // event property`launched_projects_count` = Stats.createdProjectsCount
Copy link
Contributor

Choose a reason for hiding this comment

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

change to:

product and insights defines launched_projects_count as only the createdProjectsCount

Copy link
Contributor

@singhhari singhhari left a comment

Choose a reason for hiding this comment

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

lgtm

@moderateepheezy moderateepheezy merged commit 8cb7a65 into master Apr 7, 2021
@moderateepheezy moderateepheezy deleted the EP-404-qa-phase-1-fix-user-properties branch April 7, 2021 19:00
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

2 participants