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

Yahoo ConnectId Module : backend sync updates. #10590

Merged
merged 2 commits into from
Oct 27, 2023

Conversation

radubarbos
Copy link
Contributor

Type of change

  • Bugfix

  • Feature

  • New bidder adapter

  • Code style update (formatting, local variables)

  • Refactoring (no functional changes, no api changes)

  • Build related changes

  • CI related changes

  • Does this change affect user-facing APIs or examples documented on http://prebid.org?

  • [ x] Other

Description of change

Other information

The back-end user id syncs are done more each time for yahoo sources traffic and the id validity can be determited by a TTL value the back-end provides in hours.

@ChrisHuie ChrisHuie changed the title Yahoo ConnectId - backend sync updates. Yahoo ConnectId Module : backend sync updates. Oct 10, 2023
@@ -104,9 +114,11 @@ function syncLocalStorageToCookie() {
}

function isStale(storedIdData) {
if (isPlainObject(storedIdData) && storedIdData.lastSynced &&
(storedIdData.lastSynced + VALID_ID_DURATION) <= Date.now()) {
if (isOAndOTraffic()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this feels like a lot of code to determine if your id is stale and needs to be re-synced. Why not just look for something in the environment only your O+o sets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll ask around but where a user is redirected from yahoo O&O page to partner pages there is nothing in the environment that is different and can be used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe you should set something instead of burdening the rest of the internet with this bulk

Copy link
Collaborator

Choose a reason for hiding this comment

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

or you could have your own properties fork your own adapter

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @patmmccann,

Trust me when I say that both @radubarbos and I raised questions about this method. This is actually not for our properties, it is for third party properties that are clicked through to from our properties, hence the referrer check. This change request came straight from the identity team.

We will go back to that team one more time to see if there is another way that can work. Will get back to you asap.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was told that currently there is no custom traffic contract between yahoo and third party partners like query params or other information, there is only what the HTTP provides (referrer). If this was for our own pages we could have used something in the environment but on the third party pages we can't.

@@ -127,6 +139,17 @@ function getSiteHostname() {
return pageInfo.hostname;
}

function isOAndOTraffic() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

not covered in tests

Copy link
Contributor Author

@radubarbos radubarbos Oct 13, 2023

Choose a reason for hiding this comment

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

it is covered by this test

it('returns an object with the stored ID from cookies and syncs because is O&O traffic', () => {
          const last2Days = Date.now() - (60 * 60 * 24 * 1000 * 2);
          const last21Days = Date.now() - (60 * 60 * 24 * 1000 * 21);
          const ttl = 60 * 60 * 24 * 1000 * 3;
          const cookieData = {connectId: 'foo', he: HASHED_EMAIL, lastSynced: last2Days, puid: '9', lastUsed: last21Days, ttl};
          getCookieStub.withArgs(STORAGE_KEY).returns(JSON.stringify(cookieData));
          const getRefererInfoStub = sinon.stub(refererDetection, 'getRefererInfo');
          getRefererInfoStub.returns({
            ref: 'https://dev.fc.yahoo.com?test'
          });```

Copy link
Collaborator

@patmmccann patmmccann left a comment

Choose a reason for hiding this comment

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

This adapter needlessly has large blocks of text to identify yahoo sites and behave differently on them. This is less than ideal, a single toggle could be set on these sites or these sites could run a private fork of this adapter

@patmmccann patmmccann removed the request for review from jlukas79 October 16, 2023 14:12
@patmmccann patmmccann self-assigned this Oct 16, 2023
@patmmccann patmmccann merged commit f40a324 into prebid:master Oct 27, 2023
4 checks passed
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.

5 participants