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

Criteo: Add pubid/uid fields in the configuration #4132

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Criteo: Add pubid/uid fields in the configuration #4132

wants to merge 1 commit into from
+25 −2

Conversation

vraybaud
Copy link
Contributor

@vraybaud vraybaud commented Jan 7, 2025

No description provided.

@bsardo bsardo changed the title Criteo bid adapter: add pubid/uid fields in the configuration Criteo: Add pubid/uid fields in the configuration Jan 7, 2025
@bsardo bsardo added the adapter label Jan 7, 2025
@bsardo bsardo self-assigned this Jan 20, 2025
VeronikaSolovei9
VeronikaSolovei9 reviewed Jan 21, 2025
View reviewed changes
static/bidder-params/criteo.json
Comment on lines +27 to +35
"pubid": {
"type": "string",
"description": "Impression's publisher ID.",
"minLength": 1
},
"uid": {
"type": "integer",
"description": "Impression's ad unit id.",
"minimum": 0
Copy link
Contributor

Choose a reason for hiding this comment

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

This will make more sense if you keep it in sync with ExtImpCriteo, even if you don't use it in code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, please update openrtb_ext/imp_criteo.go so that all fields listed in this file appear in ExtImpCriteo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review, I have updated the pull request.

bsardo
bsardo reviewed Jan 21, 2025
View reviewed changes
static/bidder-params/criteo.json
Comment on lines +27 to +35
"pubid": {
"type": "string",
"description": "Impression's publisher ID.",
"minLength": 1
},
"uid": {
"type": "integer",
"description": "Impression's ad unit id.",
"minimum": 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, please update openrtb_ext/imp_criteo.go so that all fields listed in this file appear in ExtImpCriteo.

static/bidder-params/criteo.json
@@ -23,6 +23,16 @@
"type": "integer",
"description": "Impression's network ID, preferred.",
"minimum": 0
},
"pubid": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add test cases to your params_test.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review, I have added test cases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@vraybaud can you add the following invalid test cases as well?

  • {"zoneid": 0, "networkid": 0, "pubid": ""}
  • {"zoneid": 0, "networkid": 0, "pubid": null}
  • {"zoneid": 0, "networkid": 0, "uid": null}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done !

@vraybaud vraybaud force-pushed the add-pubid-uid-criteo branch from 54528d3 to 6ec495c Compare January 22, 2025 09:19
@github-actions GitHub Actions
Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 6ec495c

criteo

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/criteo/criteo.go:43:	Builder				100.0%
github.com/prebid/prebid-server/v3/adapters/criteo/criteo.go:51:	MakeRequests			80.0%
github.com/prebid/prebid-server/v3/adapters/criteo/criteo.go:67:	MakeBids			90.9%
github.com/prebid/prebid-server/v3/adapters/criteo/criteo.go:117:	ParseFledgeAuctionConfigs	100.0%
github.com/prebid/prebid-server/v3/adapters/criteo/criteo.go:142:	getBidMeta			100.0%
total:									(statements)			93.2%

VeronikaSolovei9
VeronikaSolovei9 reviewed Jan 23, 2025
View reviewed changes
adapters/criteo/params_test.go
`{"zoneid": 123456, "networkid": 78910, "uid": 100, "pubid": "testpubid"}`,
`{"networkId": 78910, "pubid": "testpubid"}`,
`{"networkid": 78910, "uid": 100}`,
`{"networkid": 78910, "uid": 100, "pubid": "testpubid"}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: please add a test with invalid params too.

@vraybaud vraybaud force-pushed the add-pubid-uid-criteo branch from 6ec495c to 6dba06f Compare January 23, 2025 08:56
@github-actions GitHub Actions
Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 6dba06f

criteo

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/criteo/criteo.go:43:	Builder				100.0%
github.com/prebid/prebid-server/v3/adapters/criteo/criteo.go:51:	MakeRequests			80.0%
github.com/prebid/prebid-server/v3/adapters/criteo/criteo.go:67:	MakeBids			90.9%
github.com/prebid/prebid-server/v3/adapters/criteo/criteo.go:117:	ParseFledgeAuctionConfigs	100.0%
github.com/prebid/prebid-server/v3/adapters/criteo/criteo.go:142:	getBidMeta			100.0%
total:									(statements)			93.2%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VeronikaSolovei9 VeronikaSolovei9 VeronikaSolovei9 left review comments

@bsardo bsardo bsardo left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees

@bsardo bsardo

Labels
adapter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vraybaud @bsardo @VeronikaSolovei9