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

Ozone Bid Adapter: add support for GPP Module & updates #11648

Merged
merged 4 commits into from
Jun 19, 2024

Conversation

AskRupert-DM
Copy link
Contributor

Type of change

  • Bugfix

  • Feature

  • New bidder adapter

  • [x ] Updated 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?

  • Other

Description of change

  • GPP Module Support
  • Allowed for configVar to be used/defined to dictate the number of impression objects a single request can carry.

Other information

@AskRupert-DM
Copy link
Contributor Author

Hi @patmmccann - I've tried to resubmit my PR a couple of times - the error looks to be with the circle CI tests as opposed to the code submitted - I think I can see other PRs also with a similar issue/error preventing the checks from successfully passing.

@ChrisHuie ChrisHuie changed the title Ozone Bid Adapter: Support GPP Module & Minor Adapter Functionality Updates Ozone Bid Adapter: add support for GPP Module & updates Jun 3, 2024
@ChrisHuie ChrisHuie requested a review from ncolletti June 3, 2024 13:00
@patmmccann
Copy link
Collaborator

@AskRupert-DM looks like none of the tests ran ```Chrome 125.0.0.0 (Windows 10): Executed 0 of 0 SUCCESS (0 secs / 0 secs)
TOTAL: 0 SUCCESS`

@AskRupert-DM
Copy link
Contributor Author

Thanks can see the checks have run - will review your comments and push some updates accordingly/feedback once I've reviewed @patmmccann

modules/ozoneBidAdapter.js Outdated Show resolved Hide resolved
@AskRupert-DM
Copy link
Contributor Author

think majority of your feedback has been addressed/handled in the latest submission which also seems to have passed testing - hopefully good to go now @patmmccann - lmk your thoughts when you get a minute!

@AskRupert-DM
Copy link
Contributor Author

hey @patmmccann have responded to the outstanding comments - the adapter passed our internal QA and all the automated tests here. The remainder feedback you had provided did not require any actions from me. There was a comment that we have removed in our latest submission and the feedback around pclmt I don't think requires any changes from me. If this is defined on page - prebid core adds the array object to the bid-request unless you are saying that's not the case ?

@patmmccann patmmccann removed the request for review from ncolletti June 17, 2024 12:28
@patmmccann patmmccann assigned patmmccann and unassigned ncolletti Jun 17, 2024
@patmmccann
Copy link
Collaborator

@AskRupert-DM looks good, please resolve the conflict

this.tryGetPubCidFromOldLocation(ret, bidRequest); // legacy
return ret;
}
let keymap = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a very large block of code to add. Is it adding value to the publisher?

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 keymap is adding a lot of bulk

@AskRupert-DM AskRupert-DM reopened this Jun 18, 2024
fix to address plcmt feedback
@AskRupert-DM
Copy link
Contributor Author

@patmmccann have updated - hopefully we can get this merged into the next release!

@patmmccann patmmccann merged commit eacd24e into prebid:master Jun 19, 2024
5 checks passed
@patmmccann
Copy link
Collaborator

Thanks! Merged in

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.

None yet

4 participants