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.

Sign up for GitHub

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

Jump to bottom

✨Add Consent State Macro into Comscore's Vendor Script #22641

Merged
merged 3 commits into from
Jul 16, 2019
Merged

✨Add Consent State Macro into Comscore's Vendor Script #22641

merged 3 commits into from
Jul 16, 2019

Conversation

wroberts-comscore
Copy link
Contributor

Description

  • Adding the consentState Macro into Comscore's UDM Tag as 'cs_amp_consent'
  • Comscore Jira ticket: [TAG-4790]
  • Note: Cannot find Comscore's CLA agreement. Sent two emails to cla-submissions@google.com and have not received a response. Please advise

@wroberts-comscore
Adding two new labels, cs_consent for the consent value and cs_ucfr f…
bc39ac4 
…or the translated value. Insufficient should return 0, sufficient should return 1, and anything else will return an empty string in cs_ucfr
@wroberts-comscore
Removing comscore translated label for now, collecting consent state …
54bf139 
…to determine if it is inline with our expectations, TAG-4790
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@torch2424
Copy link
Contributor

@wroberts-comscore

Hello! So it seems you have not signed the CLA. Is this still an issue on your end, and want this merged?

If so, please follow the instructions given by the bot 😄 Unless you have a corporate CLA, which we can then try to figure out another way.

Triaging to @zhouyx , since you would be most familiar with this existing config 😄

@torch2424 torch2424 requested a review from zhouyx June 18, 2019 01:47
@wroberts-comscore
Copy link
Contributor Author

@torch2424 I've been in contact with taymonbeal from the AMP slack channel, he's been assisting us in getting the CLA figured out (We needed a new one as all the previous points of contact have left Comscore).

Someone from Comscore's side will then be reviewing the previous CLA for comparing the terms and then they should be able to sign the new one. I am hopeful that will happen this week and we can move this PR along the chain.

Please let me know if you need any further information.

@zhouyx
Copy link
Contributor

zhouyx commented Jun 18, 2019

Hello @wroberts-comscore. Thank you for submitting the change. There is one thing that I'd like to confirm with you. The request ping will wait for the consent_state to resolve when there's <amp-consent> element placed on the page, which means it's possible that the pageview never get sent. Is that something you're aware of and expect? Thank you

@wroberts-comscore
Copy link
Contributor Author

wroberts-comscore commented Jun 18, 2019 •
edited
Loading

Hi @zhouyx ... is there a way to send the pageview regardless of whether or not the {consentState} macro is resolved? I've attempted a couple of small things locally, (setting the macro into a separate variable, a few other triggers to send the pageview) but they all behave in the way you described above.

If there is a way to send the pageview ping even if it is not resolved, or if you had any other suggestions, please let me know. Otherwise, I have informed my team and we're reviewing with a product manager.

Thanks for taking the time to review!

  • edit: FWIW without <amp-consent> element on the page we are sending the pageview with a blank value which is more than sufficient for our needs. Thanks again..

@zhouyx
Copy link
Contributor

zhouyx commented Jun 18, 2019

Thanks for clarifying.
Currently we only support blocking CONSENT_STATE. Instead of including the value to the pageview request, could you create a new request and then combine them on server end?

@wroberts-comscore
Copy link
Contributor Author

@zhouyx so I signed the CLA.. can you confirm on your side?

thanks.

@wroberts-comscore
Copy link
Contributor Author

wroberts-comscore commented Jul 8, 2019 •
edited
Loading

@zhouyx @googlebot we have signed the CLA. Anything further needed on my side?

@zhouyx
Copy link
Contributor

zhouyx commented Jul 8, 2019

Hello @wroberts-comscore Looks like the @googlebot didn't pick up the CLA. Could you please let me know if you sign it as a company or as individual. And could you please also double check that the email you are using to commit match the email that you used to sign the CLA? Thank you very much.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Jul 8, 2019
@zhouyx
Copy link
Contributor

zhouyx commented Jul 8, 2019

Ha! CLA is all good now!

@zhouyx
Copy link
Contributor

zhouyx commented Jul 8, 2019

Let me know your decision on having the request blocked by consent state. And we can proceed with this PR. Thank you!

@wroberts-comscore
Copy link
Contributor Author

@zhouyx We should be good to go. We are fine with the consent state blocking the request at this time. Thanks.

@wroberts-comscore
Copy link
Contributor Author

@zhouyx Hey, wondering if there is an update here? Is there anything else required from my side?

@zhouyx
Copy link
Contributor

zhouyx commented Jul 15, 2019

Sorry about the delay here. The change looks goo to me. Could you please fix the unit test so that I can merge the PR. Thank you!

@wroberts-comscore
Copy link
Contributor Author

Adding the new label into the vendor-requests.json file to fix unit tests

@zhouyx zhouyx merged commit e35fb7d into ampproject:master Jul 16, 2019
@zhouyx
Copy link
Contributor

zhouyx commented Jul 16, 2019

Thank you @wroberts-comscore , all test passed. PR merged.

rindo pushed a commit to logly/amphtml that referenced this pull request Jul 24, 2019
@wroberts-comscore
✨Add Consent State Macro into Comscore's Vendor Script (ampproject#22641
fa863aa 
)

* Adding two new labels, cs_consent for the consent value and cs_ucfr for the translated value. Insufficient should return 0, sufficient should return 1, and anything else will return an empty string in cs_ucfr

* Removing comscore translated label for now, collecting consent state to determine if it is inline with our expectations, TAG-4790

* Adding the new parameter into the vendor-requests.json file
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
@wroberts-comscore @zhouyx
✨Add Consent State Macro into Comscore's Vendor Script (ampproject#22641
882da12 
)

* Adding two new labels, cs_consent for the consent value and cs_ucfr for the translated value. Insufficient should return 0, sufficient should return 1, and anything else will return an empty string in cs_ucfr

* Removing comscore translated label for now, collecting consent state to determine if it is inline with our expectations, TAG-4790

* Adding the new parameter into the vendor-requests.json file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhouyx zhouyx zhouyx approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wroberts-comscore @googlebot @torch2424 @zhouyx