Cid optout action #9952
Cid optout action #9952
Conversation
| return this.getCidService_() | ||
| .then(cid => cid.optOutOfCid()) | ||
| .then(this.dismiss.bind(this)) | ||
| .catch(reason => { |
There was a problem hiding this comment.
Nit: move this into the first then (nested), so that optout always dismisses regardless of error:
cidService()
.then(cid => cid.optOutOfCid().catch(reason => {}))
.then(this.dismiss)
| .then(this.dismiss.bind(this), reason => { | ||
| dev().error('amp-user-notification', | ||
| 'Failed to opt out of Cid', reason); | ||
| // If optout fails, dismiss notification without persisting. |
There was a problem hiding this comment.
This seemed to me as the most reasonable thing to do if opting out fails, thoughts?
src/service/cid-impl.js
Outdated
| const CID_OPTOUT_STORAGE_KEY = 'amp-cid-optout'; | ||
|
|
||
| // TODO(aghassemi): replace with real viewer message when implemented. | ||
| const CID_OPTOUT_VIEWER_MESSAGE = 'cidOptout'; |
There was a problem hiding this comment.
@lannka Does viewer already have support for sending a message to inform user has opted out? For now I am just sending a message with this key
There was a problem hiding this comment.
yes, they're working on that. the key is cidOptOut
There was a problem hiding this comment.
fixed the key to match
src/service/cid-impl.js
Outdated
| return isOptedOutOfCid(this.ampdoc).then(optedOut => { | ||
| // TODO(aghassemi): null or '' ? | ||
| if (optedOut) { | ||
| return null; |
There was a problem hiding this comment.
so, although get can already resolve to null, I have seen places that do not check for null (e.g extractClientIdFromGaCookie). Maybe '' is better? thoughts?
There was a problem hiding this comment.
done, also moved the check after consent check (since user can optout while consent is pending)
src/service/cid-impl.js
Outdated
| viewerForDoc(ampdoc).sendMessage(CID_OPTOUT_VIEWER_MESSAGE); | ||
|
|
||
| // Store the optout bit in storage | ||
| return storageForDoc(ampdoc).then(storage => { |
There was a problem hiding this comment.
do we want localStorage or storage service here?
There was a problem hiding this comment.
will user be able to ever opt-in again? is there such an API?
There was a problem hiding this comment.
nope, do we need that @cramforce?
| * @private | ||
| */ | ||
| getCidService_() { | ||
| cidForDoc(this.element); |
There was a problem hiding this comment.
ugh, extension was not type-checked, added type checking (caught an unquoted JSON issues as well)
src/service/cid-impl.js
Outdated
| return isOptedOutOfCid(this.ampdoc).then(optedOut => { | ||
| // TODO(aghassemi): null or '' ? | ||
| if (optedOut) { | ||
| return null; |
src/service/cid-impl.js
Outdated
| viewerForDoc(ampdoc).sendMessage(CID_OPTOUT_VIEWER_MESSAGE); | ||
|
|
||
| // Store the optout bit in storage | ||
| return storageForDoc(ampdoc).then(storage => { |
src/service/cid-impl.js
Outdated
| const CID_OPTOUT_STORAGE_KEY = 'amp-cid-optout'; | ||
|
|
||
| // TODO(aghassemi): replace with real viewer message when implemented. | ||
| const CID_OPTOUT_VIEWER_MESSAGE = 'cidOptout'; |
There was a problem hiding this comment.
yes, they're working on that. the key is cidOptOut
src/service/cid-impl.js
Outdated
| export function optOutOfCid(ampdoc) { | ||
|
|
||
| // Tell the viewer that user has opted out. | ||
| viewerForDoc(ampdoc).sendMessage(CID_OPTOUT_VIEWER_MESSAGE); |
There was a problem hiding this comment.
does it make sense to move this little thing into cid-impl? so we have centralized place managing the CID APIs
There was a problem hiding this comment.
this whole thing is in amp-impl.js, did you mean somewhere else?
| .then(this.dismiss.bind(this), reason => { | ||
| dev().error('amp-user-notification', | ||
| 'Failed to opt out of Cid', reason); | ||
| // If optout fails, dismiss notification without persisting. |
| */ | ||
| dismiss() { | ||
| dismiss(opt_forceNoPersist = false) { |
There was a problem hiding this comment.
Danger! This requires auditing every call to dismiss, since there may have been a values passed into it through callbacks (the thens are already doing it in this PR).
There was a problem hiding this comment.
wow good catch Justin. Only issue was registerAction ( which actually was caught after I enabled type checking ).
Anyway now I am a bit worried about this. Should we start preferring explicit () => func() over bind?
There was a problem hiding this comment.
Type checking will often have a hard time finding these since the type of the arg will often be ?
|
@cramforce @lannka @jridgewell PTAL |
| */ | ||
| dismiss() { | ||
| dismiss(opt_forceNoPersist = false) { |
There was a problem hiding this comment.
Type checking will often have a hard time finding these since the type of the arg will often be ?
| * @return {!Promise} | ||
| * @visibleForTesting | ||
| */ | ||
| export function optOutOfCid(ampdoc) { |
There was a problem hiding this comment.
was this because you had some trouble with the tests?
if so, check out #10200
Closes #9810