GH-2048: Display premium icons & subscription info & GH-2036: Unlock Plus features on GBE/Midnight for all Premium & Plus users #546
Conversation
…ader, and settings panel
… subscriber badge icon
…as a premium subscription
| </g> | ||
| </svg> | ||
| <span>{ t('ghostery_plus') }</span> | ||
| {/* Upselling plus for all users who are not premium subscribers */} |
wlycdgr
May 11, 2020
Member
Much closer to what's going on, but let's tweak this a little more to something like "Show premium icon to premium users and plus icon to basic and plus users" so the comment doesn't leave the reader confused about whether we are mistakenly upselling plus again to people who are already plus subscribers.
Much closer to what's going on, but let's tweak this a little more to something like "Show premium icon to premium users and plus icon to basic and plus users" so the comment doesn't leave the reader confused about whether we are mistakenly upselling plus again to people who are already plus subscribers.
benstrumeyer
May 12, 2020
Author
Contributor
Ah okay. Will make comments as clear and descriptive as possible from now on
Ah okay. Will make comments as clear and descriptive as possible from now on
wlycdgr
May 12, 2020
Member
Awesome, thank you for your patience with my nittiness about this
Awesome, thank you for your patience with my nittiness about this
|
|
||
| let subscriptionData = sub.find(subscription => subscription.productName.includes('Ghostery Premium')); | ||
| if (subscriptionData) { | ||
| callback({ subscriptionData }); | ||
| } | ||
|
|
||
| subscriptionData = sub.find(subscription => subscription.productName.includes('Ghostery Plus')); | ||
| callback({ subscriptionData }); |
wlycdgr
May 11, 2020
•
Member
See my comments on Account#getUserSubscriptionData, especially the last update after getting the feedback from Tino. Note that we need to make sure we pass an empty object to the callback, and not undefined (the fail return value of Array.prototype.find), in the event that neither subscription is found. We also don't need to test the return value for whether it's an array because Account#getUserSubscriptionData will now guarantee it returns an array.
See my comments on Account#getUserSubscriptionData, especially the last update after getting the feedback from Tino. Note that we need to make sure we pass an empty object to the callback, and not undefined (the fail return value of Array.prototype.find), in the event that neither subscription is found. We also don't need to test the return value for whether it's an array because Account#getUserSubscriptionData will now guarantee it returns an array.
benstrumeyer
May 12, 2020
Author
Contributor
Changed to return only the highest tier subscription, and made sure to return an empty object instead of undefined!
Changed to return only the highest tier subscription, and made sure to return an empty object instead of undefined!
…iptions, and the background handler to return the highest tier subscription
|
Looks good! |
…on in account#getUserSubscriptionData
…miumAccess and plusAccess
…k, and user.subscription for UI
benstrumeyer
changed the title
| _premiumSubscriber = () => { | ||
| const { loggedIn, user } = this.props; | ||
|
|
||
| return loggedIn && (user && user.subscriptionsPremium); | ||
| return loggedIn && (user && user.premiumAccess); | ||
| } | ||
|
|
||
| /** |
wlycdgr
May 13, 2020
Member
Can we rename this and the matching _plusSubscriber helper to reflect that they now return access status rather than subscription status?
Can we rename this and the matching _plusSubscriber helper to reflect that they now return access status rather than subscription status?
benstrumeyer
May 13, 2020
Author
Contributor
Changed to _hasPlusAccess() and _hasPremiumAccess()!
Changed to _hasPlusAccess() and _hasPremiumAccess()!
| @@ -517,7 +517,7 @@ class Stats extends React.Component { | |||
| return selectionData; | |||
| } | |||
|
|
|||
| _isPlus = props => props.user && props.user.subscriptionsPlus; | |||
| _isPlus = props => props.user && props.user.plusAccess; | |||
wlycdgr
May 13, 2020
Member
Similarly, let's rename this helper to _hasPlusAccess
Similarly, let's rename this helper to _hasPlusAccess
benstrumeyer
May 13, 2020
Author
Contributor
Renamed!
Renamed!
| @@ -329,7 +329,7 @@ class Stats extends React.Component { | |||
| monthlyAverageData: clearData, | |||
| dailyAverageData: clearData, | |||
| showResetModal: false, | |||
| showPitchModal: (!this.props.user || !this.props.user.subscriptionsPlus), | |||
| showPitchModal: (!this.props.user || !this.props.user.plusAccess), | |||
wlycdgr
May 13, 2020
Member
While you're in there, can you destructure this.props.user like you've done elsewhere?
While you're in there, can you destructure this.props.user like you've done elsewhere?
benstrumeyer
May 13, 2020
Author
Contributor
Done!
Done!
| const hasPremiumAccess = user && user.premiumAccess; | ||
| const hasPlusAccess = user && user.plusAccess; |
wlycdgr
May 13, 2020
Member
Man I'm SO ready for optional chaining to be a thing
Man I'm SO ready for optional chaining to be a thing
benstrumeyer
May 13, 2020
Author
Contributor
Ooh, I've never heard of optional chaining. Looks interesting
Ooh, I've never heard of optional chaining. Looks interesting
| @@ -493,7 +494,6 @@ class Account { | |||
| conf.paid_subscription = true; | |||
| dispatcher.trigger('conf.save.paid_subscription'); | |||
| } | |||
| conf.account.subscriptionData = data || null; | |||
wlycdgr
May 13, 2020
Member
I think we still need to save this, no?
I think we still need to save this, no?
benstrumeyer
May 13, 2020
Author
Contributor
Thank god for PR reviews, my bad!
Thank god for PR reviews, my bad!
| currentAccount.user.plusAccess = account.hasScopesUnverified(['subscriptions:plus']) | ||
| || account.hasScopesUnverified(['subscriptions:premium']); | ||
| currentAccount.user.premiumAccess = account.hasScopesUnverified(['subscriptions:premium']); |
wlycdgr
May 13, 2020
Member
I still wanna move this into Account but let's worry about that after. We can sneak it into one of the related tickets once this is in testing.
I still wanna move this into Account but let's worry about that after. We can sneak it into one of the related tickets once this is in testing.
benstrumeyer
May 13, 2020
Author
Contributor
I'll make a note!
I'll make a note!
…ubscriptionData in account#setSubscriptionData
|
|
||
| return ( | ||
| <PromoModalContainer | ||
| type={PREMIUM} | ||
| location="panel" | ||
| isPlus={isPlus} | ||
| isPlus={hasPlusAccess} |
wlycdgr
May 13, 2020
Member
Since we only use the return value here, we can just call this.hasPlusAccess directly here like you do with this._hasPremiumAccess at the top of the function (also, don't forget the leading underscore :) )
Since we only use the return value here, we can just call this.hasPlusAccess directly here like you do with this._hasPremiumAccess at the top of the function (also, don't forget the leading underscore :) )
benstrumeyer
May 13, 2020
Author
Contributor
Gotcha, I will be more thorough!
Gotcha, I will be more thorough!
|
LGTM. Smoke tested. |
GH-2048
GH-2036
Tickets: