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

Subscriptions: Better platform selection #14004

Merged
merged 21 commits into from Mar 15, 2018
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/amp-subscriptions.amp.html
Expand Up @@ -27,7 +27,7 @@
"services": [
{
"authorizationUrl": "//localhost:8000/subscription/2/entitlements",
"pingbackUrl": "//lipsum.com/pingback"
"pingbackUrl": "//lipsum.com/pingback",
"actions":{
"login": "//lipsum.com/login",
"subscribe": "//lipsum.com/subscribe"
Expand Down
Expand Up @@ -162,9 +162,14 @@ export class GoogleSubscriptionsPlatform {
}

/**
* Perdforms the pingback to the subscription platform
* Performs the pingback to the subscription platform
*/
pingback() {}

/** override */
Copy link
Contributor

Choose a reason for hiding this comment

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

@override

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

supportsCurrentViewer() {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please return this.isGoogleViewer_ here since that PR is merged now.

}
}


Expand Down
90 changes: 42 additions & 48 deletions extensions/amp-subscriptions/0.1/amp-subscriptions.js
Expand Up @@ -17,15 +17,14 @@
import {CSS} from '../../../build/amp-subscriptions-0.1.css';
import {Dialog} from './dialog';
import {Entitlement} from './entitlement';
import {EntitlementStore} from './entitlement-store';
import {LocalSubscriptionPlatform} from './local-subscription-platform';
import {PageConfig, PageConfigResolver} from '../../../third_party/subscriptions-project/config';
import {PlatformStore} from './platform-store';
import {Renderer} from './renderer';
import {ServiceAdapter} from './service-adapter';
import {SubscriptionPlatform} from './subscription-platform';
import {ViewerTracker} from './viewer-tracker';
import {dev, user} from '../../../src/log';
import {dict} from '../../../src/utils/object';
import {installStylesForDoc} from '../../../src/style-installer';
import {tryParseJson} from '../../../src/json';

Expand Down Expand Up @@ -60,11 +59,8 @@ export class SubscriptionService {
/** @private {?JsonObject} */
this.platformConfig_ = null;

/** @private @const {!Object<string, !SubscriptionPlatform>} */
this.subscriptionPlatforms_ = dict();

/** @private {?EntitlementStore} */
this.entitlementStore_ = null;
/** @private {?PlatformStore} */
this.platformStore_ = null;

/** @const @private {!Element} */
this.configElement_ = user().assertElement(configElement);
Expand Down Expand Up @@ -108,10 +104,12 @@ export class SubscriptionService {
*/
initializeLocalPlatforms_(serviceConfig) {
if ((serviceConfig['serviceId'] || 'local') == 'local') {
this.subscriptionPlatforms_['local'] = new LocalSubscriptionPlatform(
this.ampdoc_,
serviceConfig,
this.serviceAdapter_
this.platformStore_.resolvePlatform('local',
new LocalSubscriptionPlatform(
this.ampdoc_,
serviceConfig,
this.serviceAdapter_
)
);
}
}
Expand Down Expand Up @@ -147,8 +145,8 @@ export class SubscriptionService {
matchedServiceConfig,
this.serviceAdapter_);

this.subscriptionPlatforms_[subscriptionPlatform.getServiceId()] =
subscriptionPlatform;
this.platformStore_.resolvePlatform(subscriptionPlatform.getServiceId(),
subscriptionPlatform);

this.fetchEntitlements_(subscriptionPlatform);
});
Expand Down Expand Up @@ -182,7 +180,7 @@ export class SubscriptionService {
'Product id is null'
));
entitlement.setCurrentProduct(productId);
this.entitlementStore_.resolveEntitlement(serviceId, entitlement);
this.platformStore_.resolveEntitlement(serviceId, entitlement);
}

/**
Expand Down Expand Up @@ -217,19 +215,17 @@ export class SubscriptionService {
const serviceIds = this.platformConfig_['services'].map(service =>
service['serviceId'] || 'local');

this.entitlementStore_ = new EntitlementStore(serviceIds);
this.platformStore_ = new PlatformStore(serviceIds);

this.platformConfig_['services'].forEach(service => {
this.initializeLocalPlatforms_(service);
});

for (const platformKey in this.subscriptionPlatforms_) {
if (this.subscriptionPlatforms_.hasOwnProperty(platformKey)) {
const subscriptionPlatform =
this.subscriptionPlatforms_[platformKey];
this.fetchEntitlements_(subscriptionPlatform);
}
}
this.platformStore_.getAllResolvedPlatforms().forEach(
subscriptionPlatform => {
this.fetchEntitlements_(subscriptionPlatform);
}
);

this.startAuthorizationFlow_();
});
Expand All @@ -249,7 +245,7 @@ export class SubscriptionService {
* @private
*/
startAuthorizationFlow_() {
this.entitlementStore_.getGrantStatus()
this.platformStore_.getGrantStatus()
.then(grantState => {this.processGrantState_(grantState);});

this.selectAndActivatePlatform_();
Expand All @@ -258,15 +254,15 @@ export class SubscriptionService {
/** @private */
selectAndActivatePlatform_() {
const requireValuesPromise = Promise.all([
this.entitlementStore_.getGrantStatus(),
this.entitlementStore_.selectPlatform(),
this.platformStore_.getGrantStatus(),
this.platformStore_.selectPlatform(),
]);

return requireValuesPromise.then(resolvedValues => {
const grantState = resolvedValues[0];
const selectedEntitlement = resolvedValues[1];

dev().assert(this.viewTrackerPromise_, 'viewer tracker promise is null');
const selectedPlatform = resolvedValues[1];
const selectedEntitlement = this.platformStore_.getResolvedEntitlementFor(
selectedPlatform.getServiceId());

/** @type {!RenderState} */
const renderState = {
Expand All @@ -276,26 +272,24 @@ export class SubscriptionService {
granted: grantState,
};

const selectedPlatform = dev().assert(
this.subscriptionPlatforms_[selectedEntitlement.service],
'Selected service not registered');

selectedPlatform.activate(renderState);

this.viewTrackerPromise_.then(() => {
const localPlatform = /** @type {!LocalSubscriptionPlatform} */ (
user().assert(this.subscriptionPlatforms_['local'],
'Local platform is not registered'));

if (selectedPlatform.isPingbackEnabled()) {
selectedPlatform.pingback(selectedEntitlement);
}

if (selectedPlatform.getServiceId() !== localPlatform.getServiceId()
&& localPlatform.isPingbackEnabled()) {
localPlatform.pingback(selectedEntitlement);
}
});
if (this.viewTrackerPromise_) {
this.viewTrackerPromise_.then(() => {
const localPlatform = /** @type {!LocalSubscriptionPlatform} */ (
user().assert(this.platformStore_.getLocalPlatform(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the user().assert() should just go to getLocalPlatform implementation itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, it already is, should just remove from here

'Local platform is not registered'));

if (selectedPlatform.isPingbackEnabled()) {
selectedPlatform.pingback(selectedEntitlement);
}

if (selectedPlatform.getServiceId() !== localPlatform.getServiceId()
&& localPlatform.isPingbackEnabled()) {
localPlatform.pingback(selectedEntitlement);
}
});
}
});
}

Expand All @@ -316,7 +310,7 @@ export class SubscriptionService {
*/
reAuthorizePlatform(subscriptionPlatform) {
return this.fetchEntitlements_(subscriptionPlatform).then(() => {
this.entitlementStore_.reset();
this.platformStore_.reset();
this.startAuthorizationFlow_();
});
}
Expand All @@ -327,7 +321,7 @@ export class SubscriptionService {
*/
delegateActionToLocal(action) {
const localPlatform = /** @type {LocalSubscriptionPlatform} */ (
dev().assert(this.subscriptionPlatforms_['local'],
dev().assert(this.platformStore_.getLocalPlatform(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Then this goes away as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, it already is, should just remove from here

'Local platform is not registered'));

localPlatform.executeAction(action);
Expand Down
Expand Up @@ -218,6 +218,11 @@ export class LocalSubscriptionPlatform {
});
});
}

/** override */
Copy link
Contributor

Choose a reason for hiding this comment

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

@override

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

supportsCurrentViewer() {
return false;
}
}

/**
Expand Down