Skip to content

Commit

Permalink
<amp-consent> Local storage initial check (ampproject#25765)
Browse files Browse the repository at this point in the history 
* added experiment flag

* added configs

* changes to prod/canary config

* cleaning up

* more clean up

* merge logic in consent-config

* adding in userAssert for consentRequired

* Update canary-config.json

* Update prod-config.json

* Update amp-consent.js

* cleaning up

* deprecate promptIfUnknownForGeo

* consent intialize promise

* config merge then validate

* check if geoService can't identify

* verify consentRequired from endpoint

* removed consentState to string

* adding comment

* bug fixing and geoGroupUnknown

* adding in getConsentRequiredPromiseLegacy_()

* Update amp-consent.js

* requested changes

* suggested changes

* suggested fixes & fixing unit tests

* unit tests, better userAssert messages

* adding tests

* Suggested changes to tests + bug fixes

* starting to write tests for promptunknowngeo

* unit tests

* remove function*,  cleaning up

* sandbox change

* a little more cleaning up

* cleaning

* cleaning up logic

* fixing tests and promises

* testing

* bug fix

* unit test for cr

* test for non remote consentRequired

* migrating unit tests

* Cleaning code, adding thorough test

* get local storage first

* local storage priority and tests

* adding tests

* move logic to consentRequiredPromise

* Small changes

* fixing tests

* fixed tests, added config check

* Suggested Changes

* cleaning
  • Loading branch information
@caroqliu
Micajuine Ho authored and caroqliu committed Dec 5, 2019
1 parent 02d1804 commit c6ccb17
Show file tree
Hide file tree
Showing 3 changed files with 184 additions and 30 deletions.
37 changes: 30 additions & 7 deletions extensions/amp-consent/0.1/amp-consent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -446,13 +446,20 @@ export class AmpConsent extends AMP.BaseElement {
if (!isExperimentOn(this.win, 'amp-consent-geo-override')) {
return this.getConsentRequiredPromiseLegacy_();
}
if (typeof this.consentConfig_['consentRequired'] === 'boolean') {
return Promise.resolve(this.consentConfig_['consentRequired']);
}
return this.getConsentRemote_().then(consentInfo => {
const remoteResponse = consentInfo['consentRequired'];
return typeof remoteResponse === 'boolean' ? remoteResponse : true;
});
return this.consentStateManager_
.getLastConsentInstanceInfo()
.then(storedInfo => {
if (hasStoredValue(storedInfo)) {
return Promise.resolve(true);
}
const consentRequired = this.consentConfig_['consentRequired'];
if (typeof consentRequired === 'boolean') {
return Promise.resolve(consentRequired);
}
return this.getConsentRemote_().then(consentInfo => {
return !!consentInfo['consentRequired'];
});
});
}

/**
Expand Down Expand Up @@ -633,6 +640,22 @@ export class AmpConsent extends AMP.BaseElement {
});
});
}

/**
* @return {!Promise<boolean>}
* @visibleForTesting
*/
getConsentRequiredPromiseForTesting() {
return this.getConsentRequiredPromise_();
}

/**
* @return {boolean}
* @visibleForTesting
*/
getIsPromptUiOnForTesting() {
return this.isPromptUIOn_;
}
}

AMP.extension('amp-consent', '0.1', AMP => {
Expand Down
5 changes: 3 additions & 2 deletions extensions/amp-consent/0.1/consent-config.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -222,12 +222,13 @@ export class ConsentConfig {
* @return {!JsonObject}
*/
validateMergedGeoOverride_(mergedConfig) {
const consentRequired = mergedConfig['consentRequired'];
userAssert(
mergedConfig['consentRequired'] !== undefined,
typeof consentRequired === 'boolean' || consentRequired === 'remote',
'`consentRequired` is required',
TAG
);
if (mergedConfig['consentRequired'] === 'remote') {
if (consentRequired === 'remote') {
userAssert(
mergedConfig['checkConsentHref'],
'%s: `checkConsentHref` must be specified if `consentRequired` is remote',
Expand Down
172 changes: 151 additions & 21 deletions extensions/amp-consent/0.1/test/test-amp-consent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ describes.realWin(
let requestBody;
let ISOCountryGroups;
let xhrServiceMock;
let storageMock;

beforeEach(() => {
doc = env.win.document;
Expand Down Expand Up @@ -73,6 +74,16 @@ describes.realWin(
},
};

storageMock = {
get: name => {
return Promise.resolve(storageValue[name]);
},
set: (name, value) => {
storageValue[name] = value;
return Promise.resolve();
},
};

resetServiceForTesting(win, 'xhr');
registerServiceBuilder(win, 'xhr', function() {
return xhrServiceMock;
Expand All @@ -90,15 +101,7 @@ describes.realWin(

resetServiceForTesting(win, 'storage');
registerServiceBuilder(win, 'storage', function() {
return Promise.resolve({
get: name => {
return Promise.resolve(storageValue[name]);
},
set: (name, value) => {
storageValue[name] = value;
return Promise.resolve();
},
});
return Promise.resolve(storageMock);
});
});

Expand Down Expand Up @@ -205,16 +208,65 @@ describes.realWin(
it('read promptIfUnknown from server response', async () => {
await ampConsent.buildCallback();
await macroTask();
return ampConsent.getConsentRequiredPromise_().then(isRequired => {
expect(isRequired).to.be.true;
});
return ampConsent
.getConsentRequiredPromiseForTesting()
.then(isRequired => {
expect(isRequired).to.be.true;
});
});
});

describe('geo-override server communication', () => {
let ampConsent;
beforeEach(() => {
toggleExperiment(win, 'amp-consent-geo-override');
toggleExperiment(win, 'amp-consent-geo-override', true);
});

afterEach(() => {
toggleExperiment(win, 'amp-consent-geo-override', false);
});

it('checks local storage before making sever request', async () => {
const config = {
'consentInstanceId': 'abc',
'consentRequired': 'remote',
'checkConsentHref': 'https://geo-override-check2/',
};
const localStorageSpy = env.sandbox.spy(storageMock, 'get');
const fetchSpy = env.sandbox.spy(xhrServiceMock, 'fetchJson');

ampConsent = getAmpConsent(doc, config);
await ampConsent.buildCallback();
await macroTask();
expect(localStorageSpy).to.be.calledBefore(fetchSpy);
expect(fetchSpy).to.be.calledOnce;
expect(
(await ampConsent.consentStateManager_.getConsentInstanceInfo())[
'consentState'
]
).to.equal(CONSENT_ITEM_STATE.UNKNOWN);
expect(await ampConsent.getConsentRequiredPromiseForTesting()).to.be
.true;
});

it('respects exisitng local storage decision', async () => {
const config = {
'consentInstanceId': 'abc',
'consentRequired': 'remote',
'checkConsentHref': 'https://geo-override-false/',
};
storageValue = {
'amp-consent:abc': true,
};
ampConsent = getAmpConsent(doc, config);

await ampConsent.buildCallback();
await macroTask();
expect(
(await ampConsent.consentStateManager_.getConsentInstanceInfo())[
'consentState'
]
).to.equal(CONSENT_ITEM_STATE.ACCEPTED);
});

it('sends post request to server when consentRequired is remote', async () => {
Expand All @@ -226,7 +278,8 @@ describes.realWin(
ampConsent = getAmpConsent(doc, remoteConfig);
await ampConsent.buildCallback();
await macroTask();
expect(await ampConsent.getConsentRequiredPromise_()).to.be.false;
expect(await ampConsent.getConsentRequiredPromiseForTesting()).to.be
.false;
});

it('resolves consentRequired to remote response with old format', async () => {
Expand All @@ -240,7 +293,8 @@ describes.realWin(
ampConsent = getAmpConsent(doc, remoteConfig);
await ampConsent.buildCallback();
await macroTask();
expect(await ampConsent.getConsentRequiredPromise_()).to.be.true;
expect(await ampConsent.getConsentRequiredPromiseForTesting()).to.be
.true;
});

it('send post request to server with matched group', async () => {
Expand Down Expand Up @@ -283,7 +337,8 @@ describes.realWin(
ampConsent = getAmpConsent(doc, remoteConfig);
await ampConsent.buildCallback();
await macroTask();
expect(await ampConsent.getConsentRequiredPromise_()).to.be.true;
expect(await ampConsent.getConsentRequiredPromiseForTesting()).to.be
.true;
expect(ampConsent.matchedGeoGroup_).to.equal('na');
expect(requestBody).to.deep.equal({
'consentInstanceId': 'abc',
Expand All @@ -303,7 +358,8 @@ describes.realWin(
ampConsent = getAmpConsent(doc, remoteConfig);
await ampConsent.buildCallback();
await macroTask();
expect(await ampConsent.getConsentRequiredPromise_()).to.be.true;
expect(await ampConsent.getConsentRequiredPromiseForTesting()).to.be
.true;
});
});

Expand All @@ -327,15 +383,17 @@ describes.realWin(
ampConsent = new AmpConsent(consentElement);
ISOCountryGroups = ['unknown', 'testGroup'];
await ampConsent.buildCallback();
expect(await ampConsent.getConsentRequiredPromise_()).to.be.true;
expect(await ampConsent.getConsentRequiredPromiseForTesting()).to.be
.true;
});

it('not in geo group', async () => {
doc.body.appendChild(consentElement);
ampConsent = new AmpConsent(consentElement);
ISOCountryGroups = ['unknown'];
await ampConsent.buildCallback();
expect(await ampConsent.getConsentRequiredPromise_()).to.be.false;
expect(await ampConsent.getConsentRequiredPromiseForTesting()).to.be
.false;
});

it('geo override promptIfUnknown', async () => {
Expand All @@ -354,7 +412,8 @@ describes.realWin(
doc.body.appendChild(consentElement);
ampConsent = new AmpConsent(consentElement);
await ampConsent.buildCallback();
expect(await ampConsent.getConsentRequiredPromise_()).to.be.false;
expect(await ampConsent.getConsentRequiredPromiseForTesting()).to.be
.false;
});
});

Expand Down Expand Up @@ -485,6 +544,33 @@ describes.realWin(
});
});

afterEach(() => {
toggleExperiment(win, 'amp-consent-geo-override', false);
});

it('does not show promptUI if local storage has decision', async () => {
toggleExperiment(win, 'amp-consent-geo-override', true);
const config = {
'consentInstanceId': 'abc',
'consentRequired': 'remote',
'checkConsentHref': 'https://geo-override-check2/',
'promptUI': '123',
};
storageValue = {
'amp-consent:abc': true,
};

ampConsent = getAmpConsent(doc, config);
await ampConsent.buildCallback();
await macroTask();
expect(
(await ampConsent.consentStateManager_.getConsentInstanceInfo())[
'consentState'
]
).to.equal(CONSENT_ITEM_STATE.ACCEPTED);
expect(ampConsent.getIsPromptUiOnForTesting()).to.be.false;
});

it('update current displaying status', async () => {
await ampConsent.buildCallback();
await macroTask();
Expand Down Expand Up @@ -576,6 +662,50 @@ describes.realWin(
expect(postPromptUI).to.have.display('none');
});

describe('hide/show postPromptUI with local storage', () => {
beforeEach(() => {
toggleExperiment(win, 'amp-consent-geo-override', true);
defaultConfig = dict({
'consentInstanceId': 'ABC',
'consentRequired': true,
'postPromptUI': 'test2',
});
consentElement = createConsentElement(doc, defaultConfig);
postPromptUI = doc.createElement('div');
postPromptUI.setAttribute('id', 'test2');
consentElement.appendChild(postPromptUI);
doc.body.appendChild(consentElement);
ampConsent = new AmpConsent(consentElement);
});

afterEach(() => {
toggleExperiment(win, 'amp-consent-geo-override', false);
});

it('hides postPromptUI with no local storage decision', async () => {
await ampConsent.buildCallback();
expect(postPromptUI).to.have.display('none');
});

it('shows postPromptUI with local storage decision', async () => {
storageValue = {
'amp-consent:ABC': true,
};
await ampConsent.buildCallback();
ampConsent.element.classList.remove('i-amphtml-notbuilt');
// Wait for all modifications to the element to be applied.
await macroTask();

expect(
(await ampConsent.consentStateManager_.getConsentInstanceInfo())[
'consentState'
]
).to.equal(CONSENT_ITEM_STATE.ACCEPTED);
expect(postPromptUI).to.not.be.null;
expect(postPromptUI).to.not.have.display('none');
});
});

describe('hide/show postPromptUI', () => {
beforeEach(() => {
defaultConfig = dict({
Expand All @@ -596,7 +726,7 @@ describes.realWin(
ampConsent = new AmpConsent(consentElement);
});

it('hide postPromptUI', async () => {
it('hide postPromptUI with no local storage', async () => {
await ampConsent.buildCallback();
ampConsent.element.classList.remove('i-amphtml-notbuilt');
await macroTask();
Expand Down

0 comments on commit c6ccb17

Please sign in to comment.