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
ampproject · Nov 8, 2018 · 005e97d · 005e97d
1 parent 7e124b6
commit 005e97d
Show file tree

Hide file tree

Showing 12 changed files with 159 additions and 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<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.
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -175,6 +175,10 @@ export class ViewerSubscriptionPlatform {
   activate() {
   }
 
+  /** @override */
+  reset() {
+  }
+
   /** @override */
   isPingbackEnabled() {
     return this.platform_.isPingbackEnabled();