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

Axis Bid Adapter : initial release #9684

Merged
merged 4 commits into from
Sep 14, 2023
Merged

Conversation

PyjamaWarrior
Copy link
Contributor

@ChrisHuie ChrisHuie changed the title New Adapter: Axis Axis Bid Adapter : initial release Mar 21, 2023
@ChrisHuie ChrisHuie requested a review from Rothalack March 21, 2023 19:52
@Rothalack
Copy link
Collaborator

Hey @PyjamaWarrior I think your code here is good. I'm just concerned with your endpoint. When I test locally I am getting the error Prebid axis ERROR: Server call for axis failed: 0. Continuing without bids. Are you able to test with the integration tests on your end?

I believe this is because my local test is on http, not https, not certain though. I am also seeing 1000+ms response time from your server as well. Is this intended?

@Rothalack
Copy link
Collaborator

Hey @PyjamaWarrior , just wanted to check in and see if you had any questions or anything. Are you still working on things on your end?

coppa: config.getConfig('coppa') === true ? 1 : 0,
ccpa: bidderRequest.uspConsent || undefined,
gdpr: bidderRequest.gdprConsent || undefined,
tmax: config.getConfig('bidderTimeout')
Copy link
Collaborator

Choose a reason for hiding this comment

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

please prefer the request object to config

Copy link
Collaborator

Choose a reason for hiding this comment

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

@PyjamaWarrior here is an example

tmax: timeout

host,
page,
placements,
coppa: config.getConfig('coppa') === true ? 1 : 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

please prefer the request object to config

Copy link
Collaborator

Choose a reason for hiding this comment

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

@PyjamaWarrior here is an example

const coppa = deepAccess(bid, `ortb2.regs.coppa`)

@PyjamaWarrior
Copy link
Contributor Author

@Rothalack Can you check again, please? Now everything seems to be working fine.

@Rothalack
Copy link
Collaborator

I'll have a look today. In the meantime, please try to address Pat's feedback above. I should have time to test things in an hour or two.

@Rothalack
Copy link
Collaborator

@PyjamaWarrior I got a chance to test, everything looks good and is working. I still think you should address Pat's feedback about using the request instead of config above.

@patmmccann patmmccann assigned patmmccann and unassigned Rothalack Aug 9, 2023
@patmmccann
Copy link
Collaborator

Thanks so much @Rothalack ! I'll leave assigned to me to get this last change over the finish line.

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.

Please change these two locations to look at the request object instead of the configuration.

host,
page,
placements,
coppa: config.getConfig('coppa') === true ? 1 : 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@PyjamaWarrior here is an example

const coppa = deepAccess(bid, `ortb2.regs.coppa`)

coppa: config.getConfig('coppa') === true ? 1 : 0,
ccpa: bidderRequest.uspConsent || undefined,
gdpr: bidderRequest.gdprConsent || undefined,
tmax: config.getConfig('bidderTimeout')
Copy link
Collaborator

Choose a reason for hiding this comment

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

@PyjamaWarrior here is an example

tmax: timeout

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.

We don't allow parameters that are just standard request fields

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.

Two changes are currently required:

  1. iabCat: ['IAB1-1', 'IAB3-1', 'IAB4-3'] is not an allowed parameter, please read this from ortb2.site.cat or pagecat. Publishers don't enjoy setting the same field in specific ways for each adapter. Feel free to include instructions on how publishers can set this parameter or link to https://docs.prebid.org/features/firstPartyData.html

  2. You are failing your own test according to the last circleci run

@PyjamaWarrior
Copy link
Contributor Author

@patmmccann I've fixed tests and iabCat. But i want to ask you about ortb2.site.cat. As you can see, i got this data from bidderRequest as you suggested and its same for all adUnits. Maybe its better to use ortb2Imp from validBidRequests to have categories for every adUnit separately? What do you think about that?

@patmmccann
Copy link
Collaborator

IAB hangs it on the site object, but we allow it to change per request. see Supplying Auction-Specific Data

@patmmccann patmmccann merged commit 5fba72d into prebid:master Sep 14, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants