Skip to content

Commit

Permalink
♻️ Create enums for Actions and ActionResults (#22947)
Browse files Browse the repository at this point in the history 
* use enums instead of strings

* add missing ;

* use correct enum, remove extra space

* remove extra space
  • Loading branch information
@mborof @chenshay
mborof authored and chenshay committed Jun 24, 2019
1 parent 9c6f331 commit 5d25607
Show file tree
Hide file tree
Showing 12 changed files with 125 additions and 93 deletions.
58 changes: 31 additions & 27 deletions extensions/amp-subscriptions-google/0.1/amp-subscriptions-google.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@
* limitations under the License.
*/

import {
Action,
ActionStatus,
SubscriptionAnalyticsEvents,
} from '../../amp-subscriptions/0.1/analytics';
import {CSS} from '../../../build/amp-subscriptions-google-0.1.css';
import {
ConfiguredRuntime,
Expand All @@ -27,7 +32,6 @@ import {
} from '../../amp-subscriptions/0.1/entitlement';
import {PageConfig} from '../../../third_party/subscriptions-project/config';
import {Services} from '../../../src/services';
import {SubscriptionAnalyticsEvents} from '../../amp-subscriptions/0.1/analytics';
import {SubscriptionsScoreFactor} from '../../amp-subscriptions/0.1/score-factors.js';
import {installStylesForDoc} from '../../../src/style-installer';
import {parseUrlDeprecated} from '../../../src/url';
Expand Down Expand Up @@ -99,8 +103,8 @@ export class GoogleSubscriptionsPlatform {
this.onLinkComplete_();
this.subscriptionAnalytics_.actionEvent(
this.getServiceId(),
'link',
'success'
Action.LINK,
ActionStatus.SUCCESS
);
// TODO(dvoytenko): deprecate separate "link" events.
this.subscriptionAnalytics_.serviceEvent(
Expand All @@ -110,15 +114,15 @@ export class GoogleSubscriptionsPlatform {
});
this.runtime_.setOnFlowStarted(e => {
if (
e.flow == 'subscribe' ||
e.flow == 'contribute' ||
e.flow == 'showContributionOptions' ||
e.flow == 'showOffers'
e.flow == Action.SUBSCRIBE ||
e.flow == Action.CONTRIBUTE ||
e.flow == Action.SHOW_CONTRIBUTION_OPTIONS ||
e.flow == Action.SHOW_OFFERS
) {
this.subscriptionAnalytics_.actionEvent(
this.getServiceId(),
e.flow,
'started'
ActionStatus.STARTED
);
}
});
Expand All @@ -127,24 +131,24 @@ export class GoogleSubscriptionsPlatform {
this.onLinkComplete_();
this.subscriptionAnalytics_.actionEvent(
this.getServiceId(),
'link',
'rejected'
Action.LINK,
ActionStatus.REJECTED
);
// TODO(dvoytenko): deprecate separate "link" events.
this.subscriptionAnalytics_.serviceEvent(
SubscriptionAnalyticsEvents.LINK_CANCELED,
this.getServiceId()
);
} else if (
e.flow == 'subscribe' ||
e.flow == 'contribute' ||
e.flow == 'showContributionOptions' ||
e.flow == 'showOffers'
e.flow == Action.SUBSCRIBE ||
e.flow == Action.CONTRIBUTE ||
e.flow == Action.SHOW_CONTRIBUTION_OPTIONS ||
e.flow == Action.SHOW_OFFERS
) {
this.subscriptionAnalytics_.actionEvent(
this.getServiceId(),
e.flow,
'rejected'
ActionStatus.REJECTED
);
}
});
Expand All @@ -153,12 +157,12 @@ export class GoogleSubscriptionsPlatform {
});
this.runtime_.setOnSubscribeResponse(promise => {
promise.then(response => {
this.onSubscribeResponse_(response, 'subscribe');
this.onSubscribeResponse_(response, Action.SUBSCRIBE);
});
});
this.runtime_.setOnContributionResponse(promise => {
promise.then(response => {
this.onSubscribeResponse_(response, 'contribute');
this.onSubscribeResponse_(response, Action.CONTRIBUTE);
});
});

Expand All @@ -185,8 +189,8 @@ export class GoogleSubscriptionsPlatform {
this.runtime_.linkAccount();
this.subscriptionAnalytics_.actionEvent(
this.getServiceId(),
'link',
'started'
Action.LINK,
ActionStatus.STARTED
);
// TODO(dvoytenko): deprecate separate "link" events.
this.subscriptionAnalytics_.serviceEvent(
Expand All @@ -207,7 +211,7 @@ export class GoogleSubscriptionsPlatform {
/** @private */
onNativeSubscribeRequest_() {
this.maybeComplete_(
this.serviceAdapter_.delegateActionToLocal('subscribe')
this.serviceAdapter_.delegateActionToLocal(Action.SUBSCRIBE)
);
}

Expand Down Expand Up @@ -235,7 +239,7 @@ export class GoogleSubscriptionsPlatform {
this.subscriptionAnalytics_.actionEvent(
this.getServiceId(),
eventType,
'success'
ActionStatus.SUCCESS
);
}

Expand Down Expand Up @@ -379,21 +383,21 @@ export class GoogleSubscriptionsPlatform {
* subscribe flows elsewhere since they are invoked after
* offer selection.
*/
if (action == 'subscribe') {
if (action == Action.SUBSCRIBE) {
this.runtime_.showOffers({
list: 'amp',
isClosable: true,
});
return Promise.resolve(true);
}
if (action == 'contribute') {
if (action == Action.CONTRIBUTE) {
this.runtime_.showContributionOptions({
list: 'amp',
isClosable: true,
});
return Promise.resolve(true);
}
if (action == 'login') {
if (action == Action.LOGIN) {
this.runtime_.linkAccount();
return Promise.resolve(true);
}
Expand All @@ -405,7 +409,7 @@ export class GoogleSubscriptionsPlatform {
const opts = options ? options : {};

switch (action) {
case 'subscribe':
case Action.SUBSCRIBE:
element.textContent = '';
this.runtime_.attachButton(element, options, () => {});
break;
Expand All @@ -415,7 +419,7 @@ export class GoogleSubscriptionsPlatform {
opts.theme = 'light';
opts.lang = userAssert(
element.getAttribute('subscriptions-lang'),
'subscribe-smartbutton must have a language attrbiute'
'subscribe-smartbutton must have a language attribute'
);
this.runtime_.attachSmartButton(element, opts, () => {});
break;
Expand All @@ -424,7 +428,7 @@ export class GoogleSubscriptionsPlatform {
opts.theme = 'dark';
opts.lang = userAssert(
element.getAttribute('subscriptions-lang'),
'subscribe-smartbutton must have a language attrbiute'
'subscribe-smartbutton must have a language attribute'
);
this.runtime_.attachSmartButton(element, opts, () => {});
break;
Expand Down
38 changes: 21 additions & 17 deletions extensions/amp-subscriptions-google/0.1/test/test-amp-subscriptions-google.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@
* limitations under the License.
*/

import {
Action,
ActionStatus,
SubscriptionAnalytics,
} from '../../../amp-subscriptions/0.1/analytics';
import {
ConfiguredRuntime,
Entitlements,
Expand All @@ -27,7 +32,6 @@ import {GoogleSubscriptionsPlatform} from '../amp-subscriptions-google';
import {PageConfig} from '../../../../third_party/subscriptions-project/config';
import {ServiceAdapter} from '../../../amp-subscriptions/0.1/service-adapter';
import {Services} from '../../../../src/services';
import {SubscriptionAnalytics} from '../../../amp-subscriptions/0.1/analytics';
import {SubscriptionsScoreFactor} from '../../../amp-subscriptions/0.1/score-factors';

const PLATFORM_ID = 'subscribe.google.com';
Expand Down Expand Up @@ -269,7 +273,7 @@ describes.realWin('amp-subscriptions-google', {amp: true}, env => {
it('should delegate login when linking not requested', () => {
serviceAdapterMock
.expects('delegateActionToLocal')
.withExactArgs('login')
.withExactArgs(Action.LOGIN)
.returns(Promise.resolve(false))
.once();
callback(callbacks.loginRequest)({linkRequested: false});
Expand All @@ -280,7 +284,7 @@ describes.realWin('amp-subscriptions-google', {amp: true}, env => {
platform.isGoogleViewer_ = false;
serviceAdapterMock
.expects('delegateActionToLocal')
.withExactArgs('login')
.withExactArgs(Action.LOGIN)
.returns(Promise.resolve(false))
.once();
callback(callbacks.loginRequest)({linkRequested: true});
Expand All @@ -295,7 +299,7 @@ describes.realWin('amp-subscriptions-google', {amp: true}, env => {
it('should reauthorize on complete linking', () => {
analyticsMock
.expects('actionEvent')
.withExactArgs(PLATFORM_ID, 'link', 'success')
.withExactArgs(PLATFORM_ID, Action.LINK, ActionStatus.SUCCESS)
.once();
serviceAdapterMock.expects('resetPlatforms').once();
callback(callbacks.linkComplete)();
Expand All @@ -304,7 +308,7 @@ describes.realWin('amp-subscriptions-google', {amp: true}, env => {
it('should reauthorize on canceled linking', () => {
analyticsMock
.expects('actionEvent')
.withExactArgs(PLATFORM_ID, 'link', 'rejected')
.withExactArgs(PLATFORM_ID, Action.LINK, ActionStatus.REJECTED)
.once();
serviceAdapterMock.expects('resetPlatforms').once();
callback(callbacks.flowCanceled)({flow: 'linkAccount'});
Expand All @@ -313,23 +317,23 @@ describes.realWin('amp-subscriptions-google', {amp: true}, env => {
it('should log subscribe start', () => {
analyticsMock
.expects('actionEvent')
.withExactArgs(PLATFORM_ID, 'subscribe', 'started')
.withExactArgs(PLATFORM_ID, Action.SUBSCRIBE, ActionStatus.STARTED)
.once();
callback(callbacks.flowStarted)({flow: 'subscribe'});
callback(callbacks.flowStarted)({flow: Action.SUBSCRIBE});
});

it('should log subscribe cancel', () => {
analyticsMock
.expects('actionEvent')
.withExactArgs(PLATFORM_ID, 'subscribe', 'rejected')
.withExactArgs(PLATFORM_ID, Action.SUBSCRIBE, ActionStatus.REJECTED)
.once();
callback(callbacks.flowCanceled)({flow: 'subscribe'});
callback(callbacks.flowCanceled)({flow: Action.SUBSCRIBE});
});

it('should reauthorize on complete subscribe', () => {
analyticsMock
.expects('actionEvent')
.withExactArgs(PLATFORM_ID, 'subscribe', 'success')
.withExactArgs(PLATFORM_ID, Action.SUBSCRIBE, ActionStatus.SUCCESS)
.once();
const promise = Promise.resolve();
const response = new SubscribeResponse(
Expand All @@ -353,7 +357,7 @@ describes.realWin('amp-subscriptions-google', {amp: true}, env => {
it('should delegate native subscribe request', () => {
serviceAdapterMock
.expects('delegateActionToLocal')
.withExactArgs('subscribe')
.withExactArgs(Action.SUBSCRIBE)
.returns(Promise.resolve(false))
.once();
callback(callbacks.subscribeRequest)();
Expand All @@ -363,7 +367,7 @@ describes.realWin('amp-subscriptions-google', {amp: true}, env => {
const loginResult = Promise.resolve(true);
serviceAdapterMock
.expects('delegateActionToLocal')
.withExactArgs('login')
.withExactArgs(Action.LOGIN)
.returns(loginResult)
.once();
callback(callbacks.loginRequest)({linkRequested: false});
Expand All @@ -376,7 +380,7 @@ describes.realWin('amp-subscriptions-google', {amp: true}, env => {
const loginResult = Promise.resolve(false);
serviceAdapterMock
.expects('delegateActionToLocal')
.withExactArgs('login')
.withExactArgs(Action.LOGIN)
.returns(loginResult)
.once();
callback(callbacks.loginRequest)({linkRequested: false});
Expand All @@ -389,7 +393,7 @@ describes.realWin('amp-subscriptions-google', {amp: true}, env => {
const loginResult = Promise.resolve(true);
serviceAdapterMock
.expects('delegateActionToLocal')
.withExactArgs('subscribe')
.withExactArgs(Action.SUBSCRIBE)
.returns(loginResult)
.once();
callback(callbacks.subscribeRequest)();
Expand Down Expand Up @@ -498,19 +502,19 @@ describes.realWin('amp-subscriptions-google', {amp: true}, env => {

it('should show offers if subscribe action is delegated', () => {
const executeStub = platform.runtime_.showOffers;
platform.executeAction('subscribe');
platform.executeAction(Action.SUBSCRIBE);
expect(executeStub).to.be.calledWith({list: 'amp', isClosable: true});
});

it('should show contributions if contribute action is delegated', () => {
const executeStub = platform.runtime_.showContributionOptions;
platform.executeAction('contribute');
platform.executeAction(Action.CONTRIBUTE);
expect(executeStub).to.be.calledWith({list: 'amp', isClosable: true});
});

it('should link accounts if login action is delegated', () => {
const executeStub = platform.runtime_.linkAccount;
platform.executeAction('login');
platform.executeAction(Action.LOGIN);
expect(executeStub).to.be.calledWith();
});

Expand Down
9 changes: 5 additions & 4 deletions extensions/amp-subscriptions/0.1/actions.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* limitations under the License.
*/

import {ActionStatus} from './analytics';
import {assertHttpsUrl, parseQueryString} from '../../../src/url';
import {dev, userAssert} from '../../../src/log';
import {dict} from '../../../src/utils/object';
Expand Down Expand Up @@ -116,7 +117,7 @@ export class Actions {

dev().fine(TAG, 'Start action: ', url, action);

this.analytics_.actionEvent(LOCAL, action, 'started');
this.analytics_.actionEvent(LOCAL, action, ActionStatus.STARTED);
const dialogPromise = this.openPopup_(url);
const actionPromise = dialogPromise
.then(result => {
Expand All @@ -126,15 +127,15 @@ export class Actions {
const s = query['success'];
const success = s == 'true' || s == 'yes' || s == '1';
if (success) {
this.analytics_.actionEvent(LOCAL, action, 'success');
this.analytics_.actionEvent(LOCAL, action, ActionStatus.SUCCESS);
} else {
this.analytics_.actionEvent(LOCAL, action, 'rejected');
this.analytics_.actionEvent(LOCAL, action, ActionStatus.REJECTED);
}
return success || !s;
})
.catch(reason => {
dev().fine(TAG, 'Action failed: ', action, reason);
this.analytics_.actionEvent(LOCAL, action, 'failed');
this.analytics_.actionEvent(LOCAL, action, ActionStatus.FAILED);
if (this.actionPromise_ == actionPromise) {
this.actionPromise_ = null;
}
Expand Down
19 changes: 19 additions & 0 deletions extensions/amp-subscriptions/0.1/analytics.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,25 @@ export const SubscriptionAnalyticsEvents = {
LINK_CANCELED: 'subscriptions-link-canceled',
};

/** @enum {string} */
export const Action = {
LOGIN: 'login',
LOGOUT: 'logout',
LINK: 'link',
SUBSCRIBE: 'subscribe',
CONTRIBUTE: 'contribute',
SHOW_CONTRIBUTION_OPTIONS: 'showContributionOptions',
SHOW_OFFERS: 'showOffers',
};

/** @enum {string} */
export const ActionStatus = {
STARTED: 'started',
REJECTED: 'rejected',
FAILED: 'failed',
SUCCESS: 'success',
};

export class SubscriptionAnalytics {
/**
* Creates an instance of SubscriptionAnalytics.
Expand Down

0 comments on commit 5d25607

Please sign in to comment.