Skip to content

Commit 3d78a81

Browse files
author
Micajuine Ho
committed
Never use local consentRequired
1 parent f9e8bd4 commit 3d78a81

File tree

6 files changed

+29
-23
lines changed

6 files changed

+29
-23
lines changed

extensions/amp-consent/0.1/amp-consent.js

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -485,21 +485,23 @@ export class AmpConsent extends AMP.BaseElement {
485485

486486
/**
487487
* Create and set gdprApplies promise form consent manager.
488-
* Default value to `consentRequired`, if no `gdprApplies`
489-
* value is provided.
488+
* Default value to remote `consentRequired`, if no
489+
* `gdprApplies` value is provided.
490490
*
491+
* TODO(micajuinho) remove this method (and subsequent methods
492+
* in consent-state-manager) in favor of consolidation with
493+
* consentString
491494
*/
492495
setGdprApplies() {
493496
const responsePromise = this.getConsentRemote_();
494497
const gdprAppliesPromise = responsePromise.then((response) => {
495-
if (
496-
!response ||
497-
response['gdprApplies'] === undefined ||
498-
typeof response['gdprApplies'] !== 'boolean'
499-
) {
500-
return this.getConsentRequiredPromise_();
498+
if (!response) {
499+
return null;
501500
}
502-
return response['gdprApplies'];
501+
const gdprApplies = response['gdprApplies'];
502+
return gdprApplies === undefined || typeof gdprApplies !== 'boolean'
503+
? response['consentRequired']
504+
: gdprApplies;
503505
});
504506

505507
this.consentStateManager_.setConsentInstanceGdprApplies(gdprAppliesPromise);
@@ -520,6 +522,8 @@ export class AmpConsent extends AMP.BaseElement {
520522
this.consentStateManager_.setDirtyBit();
521523
}
522524

525+
// TODO(micajuineho) When we consolidate, add gdprApplies field
526+
// to be set with consentString.
523527
// Decision from promptUI takes precedence over consent decision from response
524528
if (
525529
!!response['consentRequired'] &&

extensions/amp-consent/0.1/consent-policy-manager.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,7 @@ export class ConsentPolicyManager {
275275
* @param {string} policyId
276276
* @return {!Promise<Object>}
277277
*/
278-
getGdprAppliesInfo(policyId) {
278+
getGdprApplies(policyId) {
279279
return this.whenPolicyResolved(policyId)
280280
.then(() => this.ConsentStateManagerPromise_)
281281
.then((manager) => {

extensions/amp-consent/0.1/consent-state-manager.js

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,6 @@ export class ConsentStateManager {
9191

9292
/**
9393
* Update consent instance state
94-
*
9594
* @param {CONSENT_ITEM_STATE} state
9695
* @param {string=} consentStr
9796
*/
@@ -166,7 +165,7 @@ export class ConsentStateManager {
166165
* Sets a promise which resolves to a boolean that is to be returned
167166
* from the remote endpoint.
168167
*
169-
* @param {!Promise<boolean>} gdprAppliesPromise
168+
* @param {!Promise<?boolean>} gdprAppliesPromise
170169
*/
171170
setConsentInstanceGdprApplies(gdprAppliesPromise) {
172171
devAssert(this.instance_, '%s: cannot find the instance', TAG);
@@ -195,7 +194,7 @@ export class ConsentStateManager {
195194
/**
196195
* Returns a promise that resolves to a gdprApplies value
197196
*
198-
* @return {?Promise<boolean>}
197+
* @return {?Promise<?boolean>}
199198
*/
200199
getConsentInstanceGdprApplies() {
201200
devAssert(this.instance_, '%s: cannot find the instance', TAG);
@@ -248,7 +247,9 @@ export class ConsentInstance {
248247
/** @public {?Promise<Object>} */
249248
this.sharedDataPromise = null;
250249

251-
/** @public {?Promise<boolean>} */
250+
// TODO(micajuineho) remove this in favor
251+
// of consolidation with consentString
252+
/** @public {?Promise<?boolean>} */
252253
this.gdprAppliesPromise = null;
253254

254255
/** @private {Promise<!../../../src/service/storage-impl.Storage>} */

extensions/amp-consent/0.1/test/test-amp-consent.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -422,7 +422,7 @@ describes.realWin(
422422
).to.eventually.be.true;
423423
});
424424

425-
it('defaults to inline config when checkConsentHref is not defined', async () => {
425+
it('never defaults to inline config when checkConsentHref is not defined', async () => {
426426
const inlineConfig = {
427427
'consentInstanceId': 'abc',
428428
'consentRequired': true,
@@ -434,7 +434,7 @@ describes.realWin(
434434
ampConsent
435435
.getConsentStateManagerForTesting()
436436
.getConsentInstanceGdprApplies()
437-
).to.eventually.be.true;
437+
).to.eventually.be.null;
438438
});
439439
});
440440

extensions/amp-consent/0.1/test/test-consent-policy-manager.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -576,23 +576,23 @@ describes.realWin(
576576
});
577577
});
578578

579-
describe('getGdprAppliesInfo', () => {
579+
describe('getGdprApplies', () => {
580580
let manager;
581581

582582
beforeEach(() => {
583583
manager = new ConsentPolicyManager(ampdoc);
584584
manager.setLegacyConsentInstanceId('ABC');
585585
});
586586

587-
it.only('should return gdprApplies data', async () => {
587+
it('should return gdprApplies value', async () => {
588588
manager.registerConsentPolicyInstance('default', {
589589
'waitFor': {
590590
'ABC': undefined,
591591
},
592592
});
593593
await macroTask();
594594
// Set above in getConsentInstanceGdprApplies mock
595-
await expect(manager.getGdprAppliesInfo()).to.eventually.be.false;
595+
await expect(manager.getGdprApplies()).to.eventually.be.false;
596596
});
597597
});
598598
}

src/consent.js

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,9 @@ export function getConsentPolicySharedData(element, policyId) {
5858
}
5959

6060
/**
61-
* TODO(micajuine-ho): Combine with getConsentPolicyState and
62-
* getGdprAppliesInfo to return a consentInfo object.
61+
* TODO(micajuine-ho): Combine with getConsentPolicyGdprApplies
62+
* and (getConsentType in future) getGdprAppliesInfo to return
63+
* a consentInfo object.
6364
* @param {!Element|!ShadowRoot} element
6465
* @param {string} policyId
6566
* @return {!Promise<string>}
@@ -83,14 +84,14 @@ export function getConsentPolicyInfo(element, policyId) {
8384
* @param {string} policyId
8485
* @return {!Promise<?boolean>}
8586
*/
86-
export function getGdprAppliesInfo(element, policyId) {
87+
export function getConsentPolicyGdprApplies(element, policyId) {
8788
// Return the stored gdpr applies value.
8889
return Services.consentPolicyServiceForDocOrNull(element).then(
8990
(consentPolicy) => {
9091
if (!consentPolicy) {
9192
return null;
9293
}
93-
return consentPolicy.getGdprAppliesInfo(/** @type {string} */ (policyId));
94+
return consentPolicy.getGdprApplies(/** @type {string} */ (policyId));
9495
}
9596
);
9697
}

0 commit comments

Comments
 (0)