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

Allow publisher defined consent policy #15724

Merged
merged 4 commits into from Jun 7, 2018

Conversation

zhouyx
Copy link
Contributor

@zhouyx zhouyx commented May 31, 2018

For #15290

#15325 introduced a list of predefined consent policy. They are data-block-on-consent=_if_responded, data-block-on-consent=_if_accepted and data-block-on-consent=_auto_reject. With those predefined consent policy, publisher can easily customize the consent blocking behavior of a given AMP component.

Individual AMP component can also overwrite the #getConsentPolicy() method to customize its own consent blocking behaviors. This PR allows publisher to customize blocking behaviors on top of that. So the order will be
publisher defined consent blocking behavior > AMP element defined consent blocking behaviors.

// data-block-on-consent attribute not set
return null;
}
if (policyId == '') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this will be a breaking change for docs with data-block-on-consent=default' now. We did document to not set value to the data-block-on-consentattribute, however I know there're usage ofdata-block-on-consent=defaultout there. Any ideas on what to do? I can easily add an extra check topolicyId == 'default'`

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm running to existing usages. Let's see what was the intention of such usage.

Just to clarify, so the distinction is:

  • data-block-on-consent leaves the consent handling to individual element (extension). Which by default is the same as _if_accepted. Element/extension can override this behavior.
  • data-block-on-consent=default is considered as pub explicitly want to have control over consent handling. Which right now is also _if_accepted. Pub can override this by modifying amp-consent config policy default.

BTW, now I feel the naming _if_accepted is a bit confusing. data-block-on-consent=til_accepted is more accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed to _till_accepted and _till_responded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify the behavior: publisher always have the final words on consent behavior using data-block-on-consent=_till_accepted/_till_responded/_auto_reject. That will block component at #buildCallback().

On top of that, individual extension can customize their behaviors like what they does today.

data-block-on-consent and data-block-on-consent='default' are exactly the same thing. data-block-on-consent and data-block-onconsent='_till_accepted are the same thing if the publisher doesn't customize the default consent blocking behavior, and the component doesn't have its blocking behavior customized.

I'll add all these to documentation once the feature is out.

* Get the consent policy to follow.
* @return {?string}
*/
getConsentPolicy_() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A comment here is that no element will be able to say I don't want to be blocked by amp-consent now. Even <amp-consent> it self can be blocked by data-block-on-consent : ( Which eventually lead to a deadlock.
We can either ignore the case, I don't believe anyone will put the data-block-on-consent attribute on the <amp-consent> component.
Or we can introduce another #shouldBeBlockedByConsent method. Any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

can you just use AmpConsent.getConsentPolicy() to force it to never block? (do we need a an _never policy preset?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought I think this is fine.
What we want to provide here is to allow publisher to make the final decision on consent blocking behaviors. I think it is the publisher's responsibility to use the feature correctly, and not introduce deadlock.

@jpettitt isn't _never preset equals to not adding data-block-on-consent. I'm open to more presets, but I'll add them upon requests.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants