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 2.4.0 adapter updates #5421

Merged
merged 2 commits into from
Jul 8, 2020
Merged

ozone 2.4.0 adapter updates #5421

merged 2 commits into from
Jul 8, 2020

Conversation

afsheenb
Copy link
Contributor

@afsheenb afsheenb commented Jun 25, 2020

Type of change
• Bugfix
• Feature

Description of changes

• TCF 2.0 Support
• Video request updated to support oRTB standards
• Refactored outstream implementation
• Introduced additional custom key-value pairs for video to indicate if bid is for instream or outstream
• introduced additional key-value pair to support floor price controls.
• Deprecated key-values pairs the adapter previously was setting that are no longer required.
• Introduced a pageview ID to request payload.
• 'lotameData' now sent inside ext.ozone instead of imp.ext.ozone to reduce request payload length.

@lgtm-com
Copy link

lgtm-com bot commented Jun 25, 2020

This pull request introduces 1 alert when merging cd49812 into 9186d64 - view on LGTM.com

new alerts:

  • 1 for Useless conditional

@AskRupert-DM
Copy link
Contributor

hey @jsnellbaker - apologies for the random tag here, you helped in the past with a similar issue on a previous PR whereby my PR failed the circle CI tests due to an error unrelated to my adapter - I believe that's the case here - could you let me know please ?

@jsnellbaker
Copy link
Collaborator

The failure does appear to be another set of tests rather than the ozone adapter's. I have reran the job and it appears to have passed successfully.

@AskRupert-DM
Copy link
Contributor

Thanks for the help / looking into this @jsnellbaker - let us know if anything is needed with merging this.

Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

Looks good on first pass. Couple questions / comments to look at.

I plan on testing and doing a little more review later today / tomorrow.

ozoneRequest.regs = {};
ozoneRequest.regs.ext = {}; // setting default values in case there is no additional information, like for the Mirror
ozoneRequest.regs.ext.gdpr = bidderRequest.gdprConsent.gdprApplies ? 1 : 0;
let apiVersion = utils.deepAccess(bidderRequest.gdprConsent, 'apiVersion', '1');
Copy link
Collaborator

Choose a reason for hiding this comment

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

At this point your above if statement assured bidderRequest.gdprConsent so instead of using deepAccess with a single notation apiVersion, a simple:

let apiVersion = bidderRequest.gdprConsent.apiVersion || '1';

May be a little cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks have noted this to be updated/cleaned in our next update - assume it will be okay to leave as is for now (keen to get this version out there).

if (ozoneRequest.regs.ext.gdpr) {
ozoneRequest.user = ozoneRequest.user || {};
ozoneRequest.user.ext = {'consent': bidderRequest.gdprConsent.consentString};
} else {
utils.logInfo('OZONE: **** Failed to find required info for GDPR for request object, even though bidderRequest.gdprConsent is TRUE ****');
utils.logInfo('OZONE: **** Strange CMP info: bidderRequest.gdprConsent exists BUT bidderRequest.gdprConsent.gdprApplies is false. See bidderRequest logged above. ****');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not necessarily strange behavior.

By default gdprApplies will be false in the scenarios where the CMP did not yet respond or errored out for any reason.

So this could in fact be a very familiar case.

Not against logging it, but gdpr can be confusing with all the different use cases and wanted to point it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair - most users who will enable logging should be aware that it maybe flagged due to the CMP not responding or timing out but will look at maybe dropping/improving this log in the future.

@@ -234,38 +240,35 @@ export const spec = {
obj.ext.ozone.customData = [{'settings': {}, 'targeting': {'oztestmode': ozTestMode}}];
}
} else {
utils.logInfo('no ozTestMode ');
utils.logInfo('OZONE: no ozTestMode ');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a heads up, there have been discussions around the amount of logging bidAdapters are allowed to include. Obviously these are only fired if the user has debug on, but strings cannot be minified. So the prebid package becomes quite large.

For now, no changes are necessary as we have not come up with a plan to actually enforce a logging amount. But wanted to let you know!

Copy link
Contributor

Choose a reason for hiding this comment

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

Noted - we'll look at cleaning up / removing unnecessary logs in a future version or maybe introducing error/logging codes to reduce the length of the strings which should help with keeping string lengths to a minimum.

@AskRupert-DM
Copy link
Contributor

Thanks for the feedback @robertrmartinez - I've commented back on some of the points you mention and have noted them as items to address on our backlog.

Let us know if anything needs to be addressed immediately - but otherwise am keen to get this approved/merged if there's no major flags as publishers are awaiting for this to update their builds ahead of TCF 2.0.

@AskRupert-DM
Copy link
Contributor

Hey @robertrmartinez - hope all is well - just checking in to see if you were able to complete the tests you wanted to conduct/if there's anything else we can do to assist ?

@robertrmartinez
Copy link
Collaborator

Awesome test coverage guys! Love to see >90%.

Merging now

@robertrmartinez robertrmartinez self-requested a review July 8, 2020 20:40
@robertrmartinez robertrmartinez merged commit a7ec4d5 into prebid:master Jul 8, 2020
@AskRupert-DM
Copy link
Contributor

Thanks @robertrmartinez - noted the feedback for our next update.

sbrosinski added a commit to smaato/Prebid.js that referenced this pull request Jul 10, 2020
* 'master' of github.com:prebid/Prebid.js: (39 commits)
  Increment pre version
  Prebid 3.25.0 release
  Add lotame id system (prebid#5388)
  Change endpoint (prebid#5459)
  ozone 2.4.0 adapter updates (prebid#5421)
  Update AppNexus usersync script (prebid#5473)
  Eplanning fix: decode parameters (prebid#5448)
  Teads adapter : Support page referrer and network bandwidth (prebid#5430)
  Merge Valueimpression Bid Adapter to Quantumdex Bid Adapter (prebid#5405)
  Criteo - partially restore adapter before PR prebid#4518 following performance issues (prebid#5376)
  onetagBidAdapter: outstream support (prebid#5435)
  Vidazoo Adapter: Feature/bidder-version (prebid#5384)
  adform and adformOpenRTB bid adapters: Added support for userId modules (prebid#5425)
  proxistore bid adapter: delay request to server by 5 min if there were no bids (prebid#5379)
  Vidazoo Adapter: Feature/subdomain (prebid#5446)
  Inskin Bid adapter small changes (prebid#5373)
  Revert "add AMX adapter (prebid#5383)" (prebid#5455)
  ATS-change logError to logInfo type (prebid#5443)
  ATS-identityLinkId - add additional info logging events (prebid#5442)
  Update padsquad for meta.advertiserDomains (prebid#5439)
  ...
sbrosinski added a commit to smaato/Prebid.js that referenced this pull request Jul 10, 2020
* smaato-adapter: (40 commits)
  Smaato: Fix test data
  Increment pre version
  Prebid 3.25.0 release
  Add lotame id system (prebid#5388)
  Change endpoint (prebid#5459)
  ozone 2.4.0 adapter updates (prebid#5421)
  Update AppNexus usersync script (prebid#5473)
  Eplanning fix: decode parameters (prebid#5448)
  Teads adapter : Support page referrer and network bandwidth (prebid#5430)
  Merge Valueimpression Bid Adapter to Quantumdex Bid Adapter (prebid#5405)
  Criteo - partially restore adapter before PR prebid#4518 following performance issues (prebid#5376)
  onetagBidAdapter: outstream support (prebid#5435)
  Vidazoo Adapter: Feature/bidder-version (prebid#5384)
  adform and adformOpenRTB bid adapters: Added support for userId modules (prebid#5425)
  proxistore bid adapter: delay request to server by 5 min if there were no bids (prebid#5379)
  Vidazoo Adapter: Feature/subdomain (prebid#5446)
  Inskin Bid adapter small changes (prebid#5373)
  Revert "add AMX adapter (prebid#5383)" (prebid#5455)
  ATS-change logError to logInfo type (prebid#5443)
  ATS-identityLinkId - add additional info logging events (prebid#5442)
  ...
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.

None yet

4 participants