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

[amp-experiment] Exposes isDismissed() method in AmpUserNotification #3832

Merged
merged 4 commits into from
Jul 1, 2016
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 27 additions & 41 deletions extensions/amp-user-notification/0.1/amp-user-notification.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,6 @@ import {xhrFor} from '../../../src/xhr';
const TAG = 'amp-user-notification';


/**
* @export
* @typedef {{
* elementId: string,
* ampUserId: string
* }}
*/
let PostRequestMetadataDef;

/**
* @export
* @typedef {{
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -286,6 +263,24 @@ 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<boolean>}
*/
isDismissed() {
Copy link
Member

Choose a reason for hiding this comment

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

can you add the signature to NotificationInterface

if (!this.persistDismissal_) {
return Promise.resolve(false);
}
return this.storagePromise_
.then(storage => storage.get(this.storageKey_))
.then(persistedValue => !!persistedValue, reason => {
Copy link
Member

Choose a reason for hiding this comment

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

any reason why we're doing the coercion here instead of the previous then success handler?

also prefer .catch for error handler.

Copy link
Contributor

Choose a reason for hiding this comment

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

also prefer .catch for error handler.

I told him to combine since !!value can never throw, so no reason to create another promise in the chain to catch it and any previous rejections.

Copy link
Member

Choose a reason for hiding this comment

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

nvm i see why now. this lgtm.

dev.error(TAG, 'Failed to read storage', reason);
return false;
});
}

/**
* Hides the current user notification and invokes the `dialogResolve_`
* method. Removes the `.amp-active` class from the element.
Expand Down Expand Up @@ -339,27 +334,26 @@ 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<!AmpUserNotification>}
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

lets switch this one to NotificationInterface too.

*/
get(id) {
this.managerReadyPromise_.then(() => {
if (this.win_.document.getElementById(id) == null) {
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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wondering if we had a bug here. Should this be returned inside the managerReadyPromise chain? @erwinmombay

Copy link
Member

Choose a reason for hiding this comment

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

looking at the code again, it shouldn't be a bug. although yes a bit weird. I don't think we need to be dependent on managerReady (viewer + dom ready) to return the promise (since that promise is resolved independently). this was really just to give the publisher dev a warning if they mistyped the notification "id". although you could argue as that being a user error instead of a warning and that we could move this dom scan to the url replacement code.

* @param {!UserNotification} userNotification
* @param {!AmpUserNotification} userNotification
Copy link
Member

Choose a reason for hiding this comment

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

this probably should have been NotificationInterface in the first place.

* @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_
Expand All @@ -370,27 +364,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];
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
Expand Down Expand Up @@ -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', () => {
Expand Down