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

Reader: Add the subscription count to all reader track events, if available #3517

Merged
merged 1 commit into from
Feb 24, 2016

Conversation

blowery
Copy link
Contributor

@blowery blowery commented Feb 23, 2016

This should help us determine how many subscriptions an active user has.

@blowery blowery added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] Reader The reader site on Calypso. labels Feb 23, 2016
@blowery blowery added this to the Reader: Exploration milestone Feb 23, 2016
@blowery blowery self-assigned this Feb 23, 2016
@blowery blowery force-pushed the add/subs-count-reader-tracks branch from 11a7f24 to eea15a8 Compare February 23, 2016 21:58
@@ -77,5 +79,9 @@ export function recordUnfollow( url ) {
}

export function recordTrack( eventName, eventProperties ) {
const subCount = SubscriptionStore.getTotalSubscriptions();
Copy link
Contributor

Choose a reason for hiding this comment

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

One potential risk is that, in #3418, we switch to incrementing totalSubscriptions in the store and no longer use the subscription count from the first page of the response. This count mean that getTotalSubscriptions() reports 50, 100, 150.... as the subs get loaded in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmmm. We probably need to keep around the total number of subs. Sounds like we'll need to mod #3418.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

think we can land this and fixup #3418?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I've added a note to #3418.

@bluefuton
Copy link
Contributor

🚢

@bluefuton bluefuton added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 24, 2016
bluefuton added a commit that referenced this pull request Feb 24, 2016
Reader: Add the subscription count to all reader track events, if available
@bluefuton bluefuton merged commit 47a978b into master Feb 24, 2016
@bluefuton bluefuton deleted the add/subs-count-reader-tracks branch February 24, 2016 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Reader The reader site on Calypso.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants