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

Explicitly log user-initiated cancellations of the Subscription Linking dialog #3423

Merged

Conversation

everyplace
Copy link
Contributor

@everyplace everyplace commented Feb 14, 2024

This PR adds an onCancel handler within the Subscription Linking flow, which now emits the NEW ACTION_SUBSCRIPTION_LINKING_CLOSE event. The second parameter in logSwgEvent specifies that this was a user-initaited action to disambiguate the two.

User-initiated event example, from when a user closes the window:

{
    "eventType": 1086,
    "eventOriginator": 1,
    "isFromUserAction": true,
    "additionalParameters": null,
    "timestamp": 1707883786769
}

@@ -69,6 +69,18 @@ export class SubscriptionLinkingFlow {
args,
/* shouldFadeBody= */ false
);

activityIframeView.onCancel(() => {
const CompletionStatus = AnalyticsEvent.EVENT_SUBSCRIPTION_LINKING_FAILED;
Copy link
Collaborator

@ChrisAntaki ChrisAntaki Feb 14, 2024

Choose a reason for hiding this comment

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

Could this variable name be camel cased?
go/tsjs-style#naming-rules-by-identifier-type

@ChrisAntaki
Copy link
Collaborator

Could you write a unit test (or tests) to cover this cancellation?

@@ -127,4 +131,15 @@ describes.realWin('SubscriptionLinkingFlow', (env) => {
).to.eventually.be.rejectedWith('Dialog error');
});
});

it.only('resolves promise with success=false when cancelled', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you remove the .only?

@ChrisAntaki ChrisAntaki merged commit d07d7a8 into subscriptions-project:main Feb 16, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants