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

Always return cookie from origin if exist #26130

Merged
merged 1 commit into from
Jan 7, 2020

Conversation

zhouyx
Copy link
Contributor

@zhouyx zhouyx commented Dec 20, 2019

Please see #25525 (comment) for context.

Closes #25525

@zikas Could you please confirm that this is the desired behavior. That the CLIENT_ID(scope, opt_userNotificationId, opt_cookieName) always return the value from opt_cookieName even if there's also value stored in scope (or if there's a new value generated for scope)? Thank you!

@zhouyx zhouyx requested review from zikas and micajuine-ho and removed request for jridgewell December 20, 2019 23:47
@zhouyx
Copy link
Contributor Author

zhouyx commented Dec 20, 2019

cc @lannka as well

@zhouyx zhouyx closed this Jan 3, 2020
@zhouyx zhouyx reopened this Jan 3, 2020
@mattwomple
Copy link
Member

I commented here that this is a reasonable approach to resolving #25525. Has there been any more discussion about accepting this change? And should it be accompanied by a unit test?

@zhouyx
Copy link
Contributor Author

zhouyx commented Jan 6, 2020

@rcebulko Could you please help with the owner bot here? Is it picking up the change from #26193? Thanks

Copy link
Contributor

@rcebulko rcebulko left a comment

Choose a reason for hiding this comment

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

Bumping owners bot

@rcebulko
Copy link
Contributor

rcebulko commented Jan 7, 2020

@rcebulko Could you please help with the owner bot here? Is it picking up the change from #26193? Thanks

It always uses master to determine ownership, but it does not watch the repository for owners changes and try to update every pending PR when one is updated. In the future, you can bump the owners bot by leaving a "Comment" review (or any other review), or by pushing a new or ammended commit.

@zhouyx
Copy link
Contributor Author

zhouyx commented Jan 7, 2020

Talked to @zikas offline, and we agreed this is the desired behavior. Merging the PR.

@zhouyx zhouyx merged commit 7657651 into ampproject:master Jan 7, 2020
@zhouyx zhouyx deleted the cid-scope-order branch January 7, 2020 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

amp-analytics reports wrong cid when multiple tags present
6 participants