From 3341e1f3260a630fe6fdd8138ee6fe0c29699caa Mon Sep 17 00:00:00 2001 From: lannka Date: Wed, 29 Jun 2016 10:04:34 -0700 Subject: [PATCH 1/4] [amp-experiment] Exposes isDismissed() method in AmpUserNotification, as well as some minor code clean up. --- .../0.1/amp-user-notification.js | 71 ++++++++----------- .../0.1/test/test-amp-user-notification.js | 64 ++++++++++++++++- 2 files changed, 90 insertions(+), 45 deletions(-) diff --git a/extensions/amp-user-notification/0.1/amp-user-notification.js b/extensions/amp-user-notification/0.1/amp-user-notification.js index 3c042bf999d4..ac81a95d1473 100644 --- a/extensions/amp-user-notification/0.1/amp-user-notification.js +++ b/extensions/amp-user-notification/0.1/amp-user-notification.js @@ -1,5 +1,5 @@ /** - * Copyright 2015 The AMP HTML Authors. All Rights Reserved. + * Copyright 2016 The AMP HTML Authors. All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -30,15 +30,6 @@ import {xhrFor} from '../../../src/xhr'; const TAG = 'amp-user-notification'; -/** - * @export - * @typedef {{ - * elementId: string, - * ampUserId: string - * }} - */ -let PostRequestMetadataDef; - /** * @export * @typedef {{ @@ -239,21 +230,7 @@ export class AmpUserNotification extends AMP.BaseElement { /** @override */ shouldShow() { - let maybeCheckStoragePromise; - - if (this.persistDismissal_) { - maybeCheckStoragePromise = this.storagePromise_.then(storage => { - return storage.get(this.storageKey_); - }); - } else { - // Skip reading storage when not data-persist-dismissal. - maybeCheckStoragePromise = Promise.resolve(null); - } - - return maybeCheckStoragePromise.catch(reason => { - dev.error(TAG, 'Failed to read storage', reason); - return false; - }).then(dismissed => { + return this.isDismissed().then(dismissed => { if (dismissed) { // Consent has been accepted. Nothing more to do. return false; @@ -286,6 +263,25 @@ export class AmpUserNotification extends AMP.BaseElement { return this.dialogPromise_; } + /** + * Returns whether this notification has been dismissed and the dismissal + * has been persisted in storage. Returns false if storage throws error or + * 'data-persist-dismissal' is disabled. + * @return {!Promise} + */ + isDismissed() { + if (!this.persistDismissal_) { + return Promise.resolve(false); + } + return this.storagePromise_ + .then(storage => storage.get(this.storageKey_)) + .catch(reason => { + dev.error(TAG, 'Failed to read storage', reason); + return false; + }) + .then(persistedValue => !!persistedValue); + } + /** * Hides the current user notification and invokes the `dialogResolve_` * method. Removes the `.amp-active` class from the element. @@ -339,7 +335,7 @@ export class UserNotificationManager { * Retrieve a promise associated to an `amp-user-notification` component * that is resolved when user agrees to the terms. * @param {string} id - * @return {!Promise} + * @return {!Promise<>} */ get(id) { this.managerReadyPromise_.then(() => { @@ -347,19 +343,18 @@ export class UserNotificationManager { user.warn(TAG, `Did not find amp-user-notification element ${id}.`); } }); - return this.getElementDeferById_(id).promise; + return this.getOrCreateDeferById_(id).promise; } /** * Register an instance of `amp-user-notification`. * @param {string} id - * @param {!UserNotification} userNotification + * @param {!AmpUserNotification} userNotification * @return {!Promise} * @package */ registerUserNotification(id, userNotification) { - const deferred = this.getElementDeferById_(id); - + const deferred = this.getOrCreateDeferById_(id); // Compose the registered notifications into a promise queue // that blocks until one notification is dismissed. return this.nextInQueue_ = this.nextInQueue_ @@ -370,27 +365,19 @@ export class UserNotificationManager { } }); }) - .then(deferred.resolve) + .then(deferred.resolve.bind(this, userNotification)) .catch(rethrowAsync.bind(null, 'Notification service failed amp-user-notification', id)); } /** - * Retrieve UserNotificationDeferDef object. + * Retrieves UserNotificationDeferDef object. Creates an defer if it doesn't + * exist. * @param {string} id * @return {!UserNotificationDeferDef} * @private */ - getElementDeferById_(id) { - return this.createOrReturnDefer_(id); - } - - /** - * Create an defer if it doesnt exist, else just return the one in the - * registry. - * @return {!UserNotificationDeferDef} - */ - createOrReturnDefer_(id) { + getOrCreateDeferById_(id) { if (this.deferRegistry_[id]) { return this.deferRegistry_[id]; } diff --git a/extensions/amp-user-notification/0.1/test/test-amp-user-notification.js b/extensions/amp-user-notification/0.1/test/test-amp-user-notification.js index e783fa3c2cb6..9cfe9e973f0e 100644 --- a/extensions/amp-user-notification/0.1/test/test-amp-user-notification.js +++ b/extensions/amp-user-notification/0.1/test/test-amp-user-notification.js @@ -115,6 +115,57 @@ describe('amp-user-notification', () => { }); }); + it('isDismissed should return true if dismissal has been recorded', () => { + return getUserNotification(dftAttrs).then(el => { + const impl = el.implementation_; + impl.buildCallback(); + + storageMock.expects('get') + .withExactArgs('amp-user-notification:n1') + .returns(Promise.resolve(true)) + .once(); + return expect(impl.isDismissed()).to.eventually.equal(true); + }); + }); + + it('isDismissed should return false if dismissal has not been recorded', + () => { + return getUserNotification(dftAttrs).then(el => { + const impl = el.implementation_; + impl.buildCallback(); + + storageMock.expects('get') + .withExactArgs('amp-user-notification:n1') + .returns(Promise.resolve(null)) + .once(); + return expect(impl.isDismissed()).to.eventually.equal(false); + }); + }); + + it('isDismissed should return false if data-persist-dismissal=false', () => { + dftAttrs['data-persist-dismissal'] = false; + return getUserNotification(dftAttrs).then(el => { + const impl = el.implementation_; + impl.buildCallback(); + + storageMock.expects('get').never(); + return expect(impl.isDismissed()).to.eventually.equal(false); + }); + }); + + it('isDismissed should return false if storage throws error', () => { + return getUserNotification(dftAttrs).then(el => { + const impl = el.implementation_; + impl.buildCallback(); + + storageMock.expects('get') + .withExactArgs('amp-user-notification:n1') + .returns(Promise.reject('intentional')) + .once(); + return expect(impl.isDismissed()).to.eventually.equal(false); + }); + }); + it('shouldShow should return false if storage has been recorded', () => { return getUserNotification(dftAttrs).then(el => { const impl = el.implementation_; @@ -471,10 +522,17 @@ describe('amp-user-notification', () => { }; }); - it('should be able to get a resolved service', () => { - service.registerUserNotification('n1', tag); + it('should be able to get AmpUserNotification object by ID', () => { + let userNotification; - return service.get('n1'); + return getUserNotification().then(element => { + return new AmpUserNotification(element); + }).then(un => { + userNotification = un; + service.registerUserNotification('n1', userNotification); + }).then(() => { + return expect(service.get('n1')).to.eventually.equal(userNotification); + }); }); it('should queue up multiple amp-user-notification elements', () => { From 9ad156fa744e98337e50aac3c2e819137918e1db Mon Sep 17 00:00:00 2001 From: lannka Date: Wed, 29 Jun 2016 10:11:19 -0700 Subject: [PATCH 2/4] JS doc. --- extensions/amp-user-notification/0.1/amp-user-notification.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/amp-user-notification/0.1/amp-user-notification.js b/extensions/amp-user-notification/0.1/amp-user-notification.js index ac81a95d1473..1fa975f2c516 100644 --- a/extensions/amp-user-notification/0.1/amp-user-notification.js +++ b/extensions/amp-user-notification/0.1/amp-user-notification.js @@ -335,7 +335,7 @@ export class UserNotificationManager { * Retrieve a promise associated to an `amp-user-notification` component * that is resolved when user agrees to the terms. * @param {string} id - * @return {!Promise<>} + * @return {!Promise} */ get(id) { this.managerReadyPromise_.then(() => { From 2e32c20ecb3ef8e49ac62dfc4f92e9c6d744a346 Mon Sep 17 00:00:00 2001 From: lannka Date: Thu, 30 Jun 2016 08:59:12 -0700 Subject: [PATCH 3/4] Address comment. --- .../amp-user-notification/0.1/amp-user-notification.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/extensions/amp-user-notification/0.1/amp-user-notification.js b/extensions/amp-user-notification/0.1/amp-user-notification.js index 1fa975f2c516..ea8367f34196 100644 --- a/extensions/amp-user-notification/0.1/amp-user-notification.js +++ b/extensions/amp-user-notification/0.1/amp-user-notification.js @@ -1,5 +1,5 @@ /** - * Copyright 2016 The AMP HTML Authors. All Rights Reserved. + * Copyright 2015 The AMP HTML Authors. All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -275,11 +275,10 @@ export class AmpUserNotification extends AMP.BaseElement { } return this.storagePromise_ .then(storage => storage.get(this.storageKey_)) - .catch(reason => { + .then(persistedValue => !!persistedValue, reason => { dev.error(TAG, 'Failed to read storage', reason); return false; - }) - .then(persistedValue => !!persistedValue); + }); } /** From dcb6198f82f29e22ec0cc757fafeb6d7fa22cb64 Mon Sep 17 00:00:00 2001 From: lannka Date: Fri, 1 Jul 2016 11:21:16 -0700 Subject: [PATCH 4/4] Address comments. --- .../0.1/amp-user-notification.js | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/extensions/amp-user-notification/0.1/amp-user-notification.js b/extensions/amp-user-notification/0.1/amp-user-notification.js index ea8367f34196..572f7cb5b60d 100644 --- a/extensions/amp-user-notification/0.1/amp-user-notification.js +++ b/extensions/amp-user-notification/0.1/amp-user-notification.js @@ -65,6 +65,14 @@ class NotificationInterface { * @return {!Promise} */ show() {} + + /** + * Returns whether this notification has been dismissed and the dismissal + * has been persisted in storage. Returns false if storage throws error or + * 'data-persist-dismissal' is disabled. + * @return {!Promise} + */ + isDismissed() {} } /** @@ -263,12 +271,7 @@ export class AmpUserNotification extends AMP.BaseElement { return this.dialogPromise_; } - /** - * Returns whether this notification has been dismissed and the dismissal - * has been persisted in storage. Returns false if storage throws error or - * 'data-persist-dismissal' is disabled. - * @return {!Promise} - */ + /** @override */ isDismissed() { if (!this.persistDismissal_) { return Promise.resolve(false); @@ -334,7 +337,7 @@ export class UserNotificationManager { * Retrieve a promise associated to an `amp-user-notification` component * that is resolved when user agrees to the terms. * @param {string} id - * @return {!Promise} + * @return {!Promise} */ get(id) { this.managerReadyPromise_.then(() => { @@ -348,7 +351,7 @@ export class UserNotificationManager { /** * Register an instance of `amp-user-notification`. * @param {string} id - * @param {!AmpUserNotification} userNotification + * @param {!NotificationInterface} userNotification * @return {!Promise} * @package */