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

fix: user data setting method #3

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

naripok
Copy link

@naripok naripok commented May 18, 2023

Type of change

  • Bugfix

Description of change

Updates ortb2 user data setting method to conform the spec.

Other information

As requested here

CC @ydennisy

Comment on lines 100 to 119
// Is this correct?
const agUserData = [
{
id: AG_TCF_ID, // Not sure of this value
name: 'airgrid', // Not sure of this value
segment: [
{
id: 'perid', // Not sure of this value
name: 'perid', // Not sure of this value
value: audiences,
}
}
]
}
})
]
deepSetValue(agOrtb2, 'ortb2.user.data', agUserData);

const bidderConfig = Object.fromEntries(
bidders.map((bidder) => [bidder, agOrtb2])
)
mergeDeep(bidConfig?.ortb2Fragments?.bidder, bidderConfig)
Copy link
Author

Choose a reason for hiding this comment

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

Not really sure of the spec here, so I'm doing some guess-work.
As I understand, this includes exchange specific values, so we need to check with xandr guys if we're getting this right?
Do you happen to know anything about those values, @ydennisy?

Also, @dgirardi, do you think it is ok for us to just set the values in the agOrtb2 obj and merge it in the bidConfig.ortb2Fragments.bidder like so?
I saw it in some other module where they actually access previous data and merge it in the array prior to setting, but they are doing some different stuff there and I'm not sure it applies here.

Choose a reason for hiding this comment

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

this looks OK, except that there's an extra ortb2 - with your setup I believe line 98 should be deepSetValue(agOrtb2, 'user.keywords', agKeywords);

You can test this by looking at what gets into bid requests; if you configure your module and run an auction, pbjs.onEvent('bidRequested', (br) => console.log(br.ortb2)) should show your data for the bidders that are set up to get it.

Copy link
Author

Choose a reason for hiding this comment

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

@dgirardi, thanks for the directions!

Yesterday, I tried and tested both using ortb2.user.keywords and user.keywords for setting the ortb2 data, but I was unable to check in our integration example page that module was behaving correctly.
The data I got back from pbjs.onEvent('bidRequested', (br) => console.log(br.ortb2)) was the same as the one set in the pbjs.setBidderConfig method in that page, and I couldn't check that it was being mutated with the configs we pass in the bidConfig?.ortb2Fragments?.bidder.

Maybe I'm not getting something fully?
Do you happen to have any good example that I could use as reference on how to properly test the integration, specially in an automated manner?
We'd like to be really sure that the module is not broken as it is critical for us.

Also, sorry for being so asking, but maybe you do know about the validity of the data being passed to user.data with the segments there or could point me to some docs about it?
I also looked around and could only find one example of another module setting that data, it is not very complete, and when trying and looking with pbjs.onEvent, their data actually returns always empty.

Choose a reason for hiding this comment

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

your integration example does not set up params.bidders in the data provider config so it shorts out at line 91 in this file. With it, and the ortb2 change, it seems to work:

image

regarding the format, it looks right to me in the sense that it conforms to spec, but I know very little about how the taxonomies are managed. The wider group @ Prebid would be a better target for the question, probably easier to get people involved from the Prebid.js PR board. I'll say that if in practice your only target is appnexus, I think what you have here is more than enough (with the fixes mentioned above).

Choose a reason for hiding this comment

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

actually, I didn't pay enough attention - some data[].id and data[].id.segment[].value should be strings, so I'd encode the example above as:

{
  "id": "782",
  "name": "airgrid",
  "segment": [
     {
       "id": "perid",
       "name": "perid",
       "value": "sport",
     },
     {
       "id": "perid",
       "name": "perid",
       "value": "travel",
     }
  ]
}

Copy link
Author

Choose a reason for hiding this comment

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

Hey @dgirardi, thank you very much!
I got it to work adding the params.bidders. I've also fixed the segment to conform with the spec. Thank you for the catch!

@@ -59,6 +59,7 @@
apiKey: "key123",
accountId: "sdk",
publisherId: "pub123",
bidders: ["appnexus", "pubmatic"],
Copy link
Author

@naripok naripok May 22, 2023

Choose a reason for hiding this comment

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

@ydennisy this is needed so the data will be passed to the bidders.
I'm including this here so the example will be fully functional.
Please, let me know if you think I shouldn't.

Choose a reason for hiding this comment

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

this is fine; only the js cannot refer to specific bidders

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

Successfully merging this pull request may close these issues.

4 participants