-
Notifications
You must be signed in to change notification settings - Fork 326
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
adding autoedits onboarding setup for dotcom users #6463
base: hitesh/make-autoedits-reactive
Are you sure you want to change the base?
adding autoedits onboarding setup for dotcom users #6463
Conversation
742874b
to
ab9ec69
Compare
b08f12a
to
81b0302
Compare
2018bc0
to
0f22a0e
Compare
ab63f60
to
0fb8d6d
Compare
!isUserEligibleForAutoeditsFeature( | ||
autoeditsFeatureFlagEnabled, | ||
authStatus, | ||
userProductSubscription | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this check one level higher to the registerAutoEdits
function in main.ts
. If a user is not eligible, we should not attempt to call the createAutoEditsProvider
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any specific reason why main.ts
could be better here ?
I liked it here since that puts all the logic for creating/decide if not to create provider in a single place. Also, if the logic becomes more complex (such as the proposal to show pop-up to the non eligible users ) we want to add condition under the if, so we can do this in the same file.
Do you think it would be okay to just keep it here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed we still have a check for the feature flag at the top level, but now I see that we moved all conditions into one place. That was the primary purpose of this suggestion 👍
) | ||
) { | ||
throw new Error( | ||
'User is not eligible for auto-edits. Can not initialize auto-edits provider.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be helpful for debugging if we named the reason here or at least logged it to the output channel. That way, when users come to us with questions about this error, we can triage it faster.
private async getAutoEditsOnboardingNotificationCount(): Promise<number> { | ||
return await localStorage.getAutoEditsOnboardingNotificationCount() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: no need to await
in cases where we only need to return a promise
private async getAutoEditsOnboardingNotificationCount(): Promise<number> { | |
return await localStorage.getAutoEditsOnboardingNotificationCount() | |
} | |
private getAutoEditsOnboardingNotificationCount(): Promise<number> { | |
return localStorage.getAutoEditsOnboardingNotificationCount() | |
} |
|
||
private async isUserEligibleForAutoeditsOnboarding(): Promise<boolean> { | ||
const authStatus = currentAuthStatus() | ||
const productSubsubscription = await currentUserProductSubscription() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const productSubsubscription = await currentUserProductSubscription() | |
const productSubscription = await currentUserProductSubscription() |
export function isUserEligibleForAutoeditsFeature( | ||
autoeditsFeatureFlagEnabled: boolean, | ||
authStatus: AuthStatus, | ||
productSubsubscription: UserProductSubscription | null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
productSubsubscription: UserProductSubscription | null | |
productSubscription: UserProductSubscription | null |
|
||
private async isAutoeditsNotificationsUnderLimit(): Promise<boolean> { | ||
const count = await this.getAutoEditsOnboardingNotificationCount() | ||
return count < this.MAX_AUTO_EDITS_ONBOARDING_NOTIFICATIONS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we decide to show it multiple times, we should add a progressive timeout between prompts to avoid coming across as spammy.
auto-edits
feature.auto-edits
feature based on the conditions:auto-edits
, we show a pop up to the user (max 3 times). Clicking on the pop-up enables auto-edits for the user.@aramaraju Does the conditions above looks okay to you ?
auto-edits.onboarding.mp4
Test plan
CI