From c315cfe115d34fdd85ef9a2c229fe4bed97a1921 Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Wed, 7 Nov 2018 16:10:51 -0800 Subject: [PATCH] Reset the UI once any action completes: delegated or non-delegated (#19178) * Reset the UI once any action completes: delegated or non-delegated * fixes * fixes * fixes --- .../0.1/amp-subscriptions-google.js | 12 +++++-- .../0.1/test/test-amp-subscriptions-google.js | 23 +++++++++---- .../0.1/amp-subscriptions.js | 3 ++ extensions/amp-subscriptions/0.1/dialog.js | 34 ++++++++++++++++++- .../local-subscription-platform-renderer.js | 24 ++++++++++--- .../0.1/local-subscription-platform.js | 5 +++ .../0.1/subscription-platform.js | 5 +++ .../0.1/test/test-amp-subscriptions.js | 28 +++++++++++++-- .../amp-subscriptions/0.1/test/test-dialog.js | 16 +++++++-- .../test/test-local-subscription-rendering.js | 18 ++++++++-- .../0.1/test/test-local-subscriptions.js | 7 ++++ .../0.1/viewer-subscription-platform.js | 4 +++ 12 files changed, 159 insertions(+), 20 deletions(-) diff --git a/extensions/amp-subscriptions-google/0.1/amp-subscriptions-google.js b/extensions/amp-subscriptions-google/0.1/amp-subscriptions-google.js index b27dd9d582fec..caba78e4aa0f1 100644 --- a/extensions/amp-subscriptions-google/0.1/amp-subscriptions-google.js +++ b/extensions/amp-subscriptions-google/0.1/amp-subscriptions-google.js @@ -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_(); }); @@ -128,7 +133,6 @@ export class GoogleSubscriptionsPlatform { /** @private */ onLinkComplete_() { - this.runtime_.reset(); this.serviceAdapter_.reAuthorizePlatform(this); } @@ -156,7 +160,6 @@ export class GoogleSubscriptionsPlatform { */ onSubscribeResponse_(response) { response.complete().then(() => { - this.runtime_.reset(); this.serviceAdapter_.reAuthorizePlatform(this); }); } @@ -203,6 +206,11 @@ export class GoogleSubscriptionsPlatform { } } + /** @override */ + reset() { + this.runtime_.reset(); + } + /** * Returns if pingback is enabled for this platform * @return {boolean} diff --git a/extensions/amp-subscriptions-google/0.1/test/test-amp-subscriptions-google.js b/extensions/amp-subscriptions-google/0.1/test/test-amp-subscriptions-google.js index b81ea7b447e12..97ffbcebf45b8 100644 --- a/extensions/amp-subscriptions-google/0.1/test/test-amp-subscriptions-google.js +++ b/extensions/amp-subscriptions-google/0.1/test/test-amp-subscriptions-google.js @@ -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'), @@ -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; }); @@ -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', () => { @@ -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', () => { diff --git a/extensions/amp-subscriptions/0.1/amp-subscriptions.js b/extensions/amp-subscriptions/0.1/amp-subscriptions.js index 36e27c341e7d6..d93c6c84954f8 100644 --- a/extensions/amp-subscriptions/0.1/amp-subscriptions.js +++ b/extensions/amp-subscriptions/0.1/amp-subscriptions.js @@ -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, diff --git a/extensions/amp-subscriptions/0.1/dialog.js b/extensions/amp-subscriptions/0.1/dialog.js index 839dcd93692d8..a4a78c1575867 100644 --- a/extensions/amp-subscriptions/0.1/dialog.js +++ b/extensions/amp-subscriptions/0.1/dialog.js @@ -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( @@ -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 { @@ -131,8 +162,9 @@ export class Dialog { /** * Closes the dialog. * @return {!Promise} + * @private */ - close() { + close_() { if (!this.visible_) { return Promise.resolve(); } diff --git a/extensions/amp-subscriptions/0.1/local-subscription-platform-renderer.js b/extensions/amp-subscriptions/0.1/local-subscription-platform-renderer.js index a7e8db8374ad3..b2dbd28e0f85e 100644 --- a/extensions/amp-subscriptions/0.1/local-subscription-platform-renderer.js +++ b/extensions/amp-subscriptions/0.1/local-subscription-platform-renderer.js @@ -16,6 +16,7 @@ import {Entitlement} from './entitlement'; import {Services} from '../../../src/services'; +import {dict} from '../../../src/utils/object'; import {evaluateExpr} from './expr'; /** @@ -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); } /** @@ -92,7 +104,8 @@ export class LocalSubscriptionPlatformRenderer { /** @type {!JsonObject} */(authResponse); return this.renderActionsInNode_( renderState, - element); + element, + evaluateExpr); }); } const clone = candidate.cloneNode(true); @@ -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} * @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. @@ -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'); diff --git a/extensions/amp-subscriptions/0.1/local-subscription-platform.js b/extensions/amp-subscriptions/0.1/local-subscription-platform.js index 0f3a6bdc88d5b..ed35b399748e8 100644 --- a/extensions/amp-subscriptions/0.1/local-subscription-platform.js +++ b/extensions/amp-subscriptions/0.1/local-subscription-platform.js @@ -162,6 +162,11 @@ export class LocalSubscriptionPlatform { }); } + /** @override */ + reset() { + this.renderer_.reset(); + } + /** @override */ executeAction(action) { const actionExecution = this.actions_.execute(action); diff --git a/extensions/amp-subscriptions/0.1/subscription-platform.js b/extensions/amp-subscriptions/0.1/subscription-platform.js index 09cc25834e684..11b5c7798fa6f 100644 --- a/extensions/amp-subscriptions/0.1/subscription-platform.js +++ b/extensions/amp-subscriptions/0.1/subscription-platform.js @@ -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} diff --git a/extensions/amp-subscriptions/0.1/test/test-amp-subscriptions.js b/extensions/amp-subscriptions/0.1/test/test-amp-subscriptions.js index a13917c56154c..516d2829dec3b 100644 --- a/extensions/amp-subscriptions/0.1/test/test-amp-subscriptions.js +++ b/extensions/amp-subscriptions/0.1/test/test-amp-subscriptions.js @@ -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; }); }); }); diff --git a/extensions/amp-subscriptions/0.1/test/test-dialog.js b/extensions/amp-subscriptions/0.1/test/test-dialog.js index c87bb814ba261..7f31127416325 100644 --- a/extensions/amp-subscriptions/0.1/test/test-dialog.js +++ b/extensions/amp-subscriptions/0.1/test/test-dialog.js @@ -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'); @@ -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(() => { diff --git a/extensions/amp-subscriptions/0.1/test/test-local-subscription-rendering.js b/extensions/amp-subscriptions/0.1/test/test-local-subscription-rendering.js index bbe0451dbaaca..a75f0f16aa0bc 100644 --- a/extensions/amp-subscriptions/0.1/test/test-local-subscription-rendering.js +++ b/extensions/amp-subscriptions/0.1/test/test-local-subscription-rendering.js @@ -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', () => { @@ -150,7 +159,7 @@ describes.realWin('local-subscriptions-rendering', {amp: true}, env => { afterEach(() => { templatesMock.verify(); - // dialogMock.verify(); + dialogMock.verify(); }); it('should render an element', () => { @@ -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(); + }); }); }); diff --git a/extensions/amp-subscriptions/0.1/test/test-local-subscriptions.js b/extensions/amp-subscriptions/0.1/test/test-local-subscriptions.js index 789b5307408c0..55a34bfcaa6c4 100644 --- a/extensions/amp-subscriptions/0.1/test/test-local-subscriptions.js +++ b/extensions/amp-subscriptions/0.1/test/test-local-subscriptions.js @@ -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', () => { diff --git a/extensions/amp-subscriptions/0.1/viewer-subscription-platform.js b/extensions/amp-subscriptions/0.1/viewer-subscription-platform.js index 7c1cfd97ba18e..424f1c1d8b002 100644 --- a/extensions/amp-subscriptions/0.1/viewer-subscription-platform.js +++ b/extensions/amp-subscriptions/0.1/viewer-subscription-platform.js @@ -175,6 +175,10 @@ export class ViewerSubscriptionPlatform { activate() { } + /** @override */ + reset() { + } + /** @override */ isPingbackEnabled() { return this.platform_.isPingbackEnabled();