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

Reset the UI once any action completes: delegated or non-delegated #19178

Merged
merged 4 commits into from Nov 8, 2018
Merged
Show file tree
Hide file tree
Changes from all 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
Expand Up @@ -90,6 +90,11 @@ export class GoogleSubscriptionsPlatform {
this.runtime_.setOnLinkComplete(() => {
this.onLinkComplete_();
});
this.runtime_.setOnFlowCanceled(e => {
if (e.flow == 'linkAccount') {
this.onLinkComplete_();
}
});
this.runtime_.setOnNativeSubscribeRequest(() => {
this.onNativeSubscribeRequest_();
});
Expand Down Expand Up @@ -128,7 +133,6 @@ export class GoogleSubscriptionsPlatform {

/** @private */
onLinkComplete_() {
this.runtime_.reset();
this.serviceAdapter_.reAuthorizePlatform(this);
}

Expand Down Expand Up @@ -156,7 +160,6 @@ export class GoogleSubscriptionsPlatform {
*/
onSubscribeResponse_(response) {
response.complete().then(() => {
this.runtime_.reset();
this.serviceAdapter_.reAuthorizePlatform(this);
});
}
Expand Down Expand Up @@ -203,6 +206,11 @@ export class GoogleSubscriptionsPlatform {
}
}

/** @override */
reset() {
this.runtime_.reset();
}

/**
* Returns if pingback is enabled for this platform
* @return {boolean}
Expand Down
Expand Up @@ -59,6 +59,8 @@ describes.realWin('amp-subscriptions-google', {amp: true}, env => {
sandbox.stub(ConfiguredRuntime.prototype, 'setOnLoginRequest'),
linkComplete:
sandbox.stub(ConfiguredRuntime.prototype, 'setOnLinkComplete'),
flowCanceled:
sandbox.stub(ConfiguredRuntime.prototype, 'setOnFlowCanceled'),
subscribeRequest:
sandbox.stub(ConfiguredRuntime.prototype,
'setOnNativeSubscribeRequest'),
Expand All @@ -84,6 +86,12 @@ describes.realWin('amp-subscriptions-google', {amp: true}, env => {
return stub.args[0][0];
}

it('should reset runtime on platform reset', () => {
expect(methods.reset).to.not.be.called;
platform.reset();
expect(methods.reset).to.be.calledOnce.calledWithExactly();
});

it('should listen on callbacks', () => {
expect(callbacks.loginRequest).to.be.calledOnce;
});
Expand Down Expand Up @@ -220,7 +228,13 @@ describes.realWin('amp-subscriptions-google', {amp: true}, env => {
.withExactArgs(platform)
.once();
callback(callbacks.linkComplete)();
expect(methods.reset).to.be.calledOnce.calledWithExactly();
});

it('should reauthorize on canceled linking', () => {
serviceAdapterMock.expects('reAuthorizePlatform')
.withExactArgs(platform)
.once();
callback(callbacks.flowCanceled)({flow: 'linkAccount'});
});

it('should reauthorize on complete subscribe', () => {
Expand All @@ -232,12 +246,7 @@ describes.realWin('amp-subscriptions-google', {amp: true}, env => {
.once();
callback(callbacks.subscribeResponse)(Promise.resolve(response));
expect(methods.reset).to.not.be.called;
return promise.then(() => {
// Skip microtask.
return Promise.resolve();
}).then(() => {
expect(methods.reset).to.be.calledOnce.calledWithExactly();
});
return promise;
});

it('should delegate native subscribe request', () => {
Expand Down
3 changes: 3 additions & 0 deletions extensions/amp-subscriptions/0.1/amp-subscriptions.js
Expand Up @@ -485,6 +485,9 @@ export class SubscriptionService {
reAuthorizePlatform(subscriptionPlatform) {
this.platformStore_.resetEntitlementFor(
subscriptionPlatform.getServiceId());
this.platformStore_.getAvailablePlatforms()
.forEach(platform => platform.reset());
this.renderer_.toggleLoading(true);
return this.fetchEntitlements_(subscriptionPlatform).then(() => {
this.subscriptionAnalytics_.serviceEvent(
SubscriptionAnalyticsEvents.PLATFORM_REAUTHORIZED,
Expand Down
34 changes: 33 additions & 1 deletion extensions/amp-subscriptions/0.1/dialog.js
Expand Up @@ -44,6 +44,9 @@ export class Dialog {
/** @private {?Element} */
this.content_ = null;

/** @private {!Promise} */
this.lastAction_ = Promise.resolve();

const doc = this.ampdoc_.win.document;

this.wrapper_ = createElementWithAttributes(
Expand Down Expand Up @@ -93,6 +96,34 @@ export class Dialog {
* @return {!Promise}
*/
open(content, showCloseAction = true) {
return this.action_(() => this.open_(content, showCloseAction));
}

/**
* Closes the dialog.
* @return {!Promise}
*/
close() {
return this.action_(() => this.close_());
}

/**
* @param {!Function} action
* @return {!Promise}
* @private
*/
action_(action) {
return this.lastAction_ = this.lastAction_.then(action);
}

/**
* Opens the dialog with the specified content.
* @param {!Element} content
* @param {boolean=} showCloseAction
* @return {!Promise}
* @private
*/
open_(content, showCloseAction = true) {
if (this.content_) {
this.wrapper_.replaceChild(content, this.content_);
} else {
Expand Down Expand Up @@ -131,8 +162,9 @@ export class Dialog {
/**
* Closes the dialog.
* @return {!Promise}
* @private
*/
close() {
close_() {
if (!this.visible_) {
return Promise.resolve();
}
Expand Down
Expand Up @@ -16,6 +16,7 @@

import {Entitlement} from './entitlement';
import {Services} from '../../../src/services';
import {dict} from '../../../src/utils/object';
import {evaluateExpr} from './expr';

/**
Expand Down Expand Up @@ -57,11 +58,22 @@ export class LocalSubscriptionPlatformRenderer {
]);
}

/**
* Resets all the rendered content back to normal.
* @return {!Promise}
*/
reset() {
// Close dialog. Ignored if the dialog is not currently open.
this.dialog_.close();
// Hide subscriptions sections.
return this.renderActionsInNode_(dict(), this.rootNode_, () => false);
}

/**
* @param {!JsonObject} renderState
*/
renderActions_(renderState) {
this.renderActionsInNode_(renderState, this.rootNode_);
this.renderActionsInNode_(renderState, this.rootNode_, evaluateExpr);
jpettitt marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -92,7 +104,8 @@ export class LocalSubscriptionPlatformRenderer {
/** @type {!JsonObject} */(authResponse);
return this.renderActionsInNode_(
renderState,
element);
element,
evaluateExpr);
});
}
const clone = candidate.cloneNode(true);
Expand All @@ -111,10 +124,11 @@ export class LocalSubscriptionPlatformRenderer {
* Renders actions inside a given node according to an authResponse
* @param {!JsonObject} renderState
* @param {!Node} rootNode
* @param {function(string, !JsonObject):boolean} evaluateExpr
* @return {!Promise<Node>}
* @private
*/
renderActionsInNode_(renderState, rootNode) {
renderActionsInNode_(renderState, rootNode, evaluateExpr) {
return this.ampdoc_.whenReady().then(() => {
// Find the matching actions and sections and make them visible if
// evalutes to true.
Expand All @@ -130,13 +144,15 @@ export class LocalSubscriptionPlatformRenderer {
candidate.classList.add('i-amphtml-subs-display');
if (candidate.getAttribute('subscriptions-service')
&& candidate.getAttribute('subscriptions-action')
&& candidate.getAttribute('subscriptions-decorate') !== 'false') {
&& candidate.getAttribute('subscriptions-decorate') !== 'false'
&& !candidate.hasAttribute('i-amphtml-subs-decorated')) {
this.serviceAdapter_.decorateServiceAction(
candidate,
candidate.getAttribute('subscriptions-service'),
candidate.getAttribute('subscriptions-action'),
null
);
candidate.setAttribute('i-amphtml-subs-decorated', true);
}
} else {
candidate.classList.remove('i-amphtml-subs-display');
Expand Down
Expand Up @@ -162,6 +162,11 @@ export class LocalSubscriptionPlatform {
});
}

/** @override */
reset() {
this.renderer_.reset();
}

/** @override */
executeAction(action) {
const actionExecution = this.actions_.execute(action);
Expand Down
5 changes: 5 additions & 0 deletions extensions/amp-subscriptions/0.1/subscription-platform.js
Expand Up @@ -43,6 +43,11 @@ export class SubscriptionPlatform {
*/
activate(unusedEntitlement) {}

/**
* Reset the platform and renderer.
*/
reset() {}

/**
* Returns if pingback is enabled for this platform.
* @return {boolean}
Expand Down
28 changes: 26 additions & 2 deletions extensions/amp-subscriptions/0.1/test/test-amp-subscriptions.js
Expand Up @@ -488,11 +488,35 @@ describes.fakeWin('AmpSubscriptions', {amp: true}, env => {
granted: true, grantReason: GrantReason.SUBSCRIBER});
sandbox.stub(platform, 'getEntitlements')
.callsFake(() => Promise.resolve(entitlement));
const resetStub = sandbox.stub(subscriptionService.platformStore_,
const resetEntitlementsStub = sandbox.stub(
subscriptionService.platformStore_,
'resetEntitlementFor');
sandbox.stub(subscriptionService, 'startAuthorizationFlow_');
return subscriptionService.reAuthorizePlatform(platform).then(() => {
expect(resetStub).to.be.calledOnce.calledWith('local');
expect(resetEntitlementsStub).to.be.calledOnce.calledWith('local');
});
});

it('should reset UI in all platforms on re-authorization', () => {
const entitlement = Entitlement.empty('local');
sandbox.stub(platform, 'getEntitlements')
.callsFake(() => Promise.resolve(entitlement));
sandbox.stub(subscriptionService.platformStore_, 'resetEntitlementFor');
sandbox.stub(subscriptionService, 'startAuthorizationFlow_');

const localResetStub = sandbox.stub(platform, 'reset');
subscriptionService.platformStore_.resolvePlatform('local', platform);

const otherPlatform = new LocalSubscriptionPlatform(ampdoc,
serviceConfig.services[0],
serviceAdapter);
const otherResetStub = sandbox.stub(otherPlatform, 'reset');
subscriptionService.platformStore_.resolvePlatform(
'other', otherPlatform);

return subscriptionService.reAuthorizePlatform(platform).then(() => {
expect(localResetStub).to.be.calledOnce;
expect(otherResetStub).to.be.calledOnce;
});
});
});
Expand Down
16 changes: 14 additions & 2 deletions extensions/amp-subscriptions/0.1/test/test-dialog.js
Expand Up @@ -58,11 +58,11 @@ describes.realWin('AmpSubscriptions Dialog', {amp: true}, env => {

it('should open content when invisible', () => {
const promise = dialog.open(content, false);
expect(content.parentNode).to.equal(dialog.getRoot());
expect(dialog.getRoot()).to.have.display('none');
expect(dialog.isVisible()).to.be.true;
return vsync.mutatePromise(() => {}).then(() => {
// First vsync displays the dialog.
expect(content.parentNode).to.equal(dialog.getRoot());
expect(dialog.isVisible()).to.be.true;
expect(dialog.getRoot()).to.have.display('block');
const styles = getComputedStyle(dialog.getRoot());
expect(styles.transform).to.contain('17');
Expand Down Expand Up @@ -106,6 +106,18 @@ describes.realWin('AmpSubscriptions Dialog', {amp: true}, env => {
});
});

it('should re-open after close', () => {
return dialog.open(content, false).then(() => {
expect(dialog.isVisible()).to.be.true;
return dialog.close();
}).then(() => {
expect(dialog.isVisible()).to.be.false;
return dialog.open(content, false);
}).then(() => {
expect(dialog.isVisible()).to.be.true;
});
});

it('should show close button', () => {
doc.body.classList.add('i-amphtml-subs-grant-yes');
return dialog.open(content, true).then(() => {
Expand Down
Expand Up @@ -111,6 +111,15 @@ describes.realWin('local-subscriptions-rendering', {amp: true}, env => {
expect(delegateUIStub).to.be.called;
});
});

it('should hide sections on reset', () => {
return renderer.render({subscribed: true}).then(() => {
displayed([actions2]);
return renderer.reset();
}).then(() => {
displayed([]);
});
});
});

describe('dialog renderer', () => {
Expand Down Expand Up @@ -150,7 +159,7 @@ describes.realWin('local-subscriptions-rendering', {amp: true}, env => {

afterEach(() => {
templatesMock.verify();
// dialogMock.verify();
dialogMock.verify();
});

it('should render an element', () => {
Expand Down Expand Up @@ -190,10 +199,15 @@ describes.realWin('local-subscriptions-rendering', {amp: true}, env => {
});
});

it('should ignore rendering if nothign found', () => {
it('should ignore rendering if nothing found', () => {
templatesMock.expects('renderTemplate').never();
dialogMock.expects('open').never();
return renderer.render({value: 'C'});
});

it('should hide the dialog on reset', () => {
dialogMock.expects('close').once();
return renderer.reset();
});
});
});
Expand Up @@ -270,6 +270,13 @@ describes.fakeWin('LocalSubscriptionsPlatform', {amp: true}, env => {
expect(renderStub).to.be.calledOnce;
});
});

it('should reset renderer\'s on reset', () => {
const resetStub =
sandbox.stub(localSubscriptionPlatform.renderer_, 'reset');
localSubscriptionPlatform.reset();
expect(resetStub).to.be.calledOnce;
});
});

describe('pingback', () => {
Expand Down
Expand Up @@ -175,6 +175,10 @@ export class ViewerSubscriptionPlatform {
activate() {
}

/** @override */
reset() {
}

/** @override */
isPingbackEnabled() {
return this.platform_.isPingbackEnabled();
Expand Down