-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
PBJS-ORTB conversion library #8738
Conversation
fef5f18
to
ce7d2d9
Compare
This pull request introduces 2 alerts when merging f6ed3122eb0f5f97ab9cb6317b9b9bfbc88c6b5d into 90d1a18 - view on LGTM.com new alerts:
|
f6ed312
to
df34289
Compare
Fixed Regarding |
@bretg, I don't know what that refers to - I don't see granularity in PBS requests generated from master. Is this a regression or a new requirement? What should be in that field exactly? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been using it while working in my adapter, and in general there are no stoppers. I left some comments regarding "how it works", speaking of how i understood it, and also some questions to the author.
const impContext = Object.assign({bidderRequest, reqContext: ctx.req}, defaultContext, context); | ||
const result = buildImp(bidRequest, impContext); | ||
if (result != null) { | ||
if (result.hasOwnProperty('id')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mainly cosmetic. Instead of "if (...) else ..." you can go with:
if (!result.hasOwnProperty('id')) {
logError('Converted ORTB imp does not specify an id, ignoring bid request', bidRequest, result);
return;
}
// logic for old true case...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from my end. Great work here!
* Automatically resolve libraries and their dependencies * Move AnalyticsAdapter under libraries * PBJS-ORTB conversion library * Update ortbConverter README * Improvements on request generation * refactor openxOrtbBidAdapter * Update README * Filter out imps that have no id * Fix nativeMapper logic * Prebid core: simplify native ORTB logic * Remove nativeMapper; do not mix ORTB request and response * fix lint * set bidRequest.nativeParams after ortb -> legacy conversion * Fix native trackers for native ortb responses (including Prebid Server) * Clean up native processors; add s2sConfig.ortbNative * Make ext.prebid.floors.enabled respect first party data * do not set placement for oustream video * Improvements * convert improvedigitalBidAdapter * Update openxOrtbBidAdapter * Improve video validation logic * Move skip validation back to improvedigital * Update README * Override (not merge) context.nativeRequest * Update improvedigital adapter * Address PR feedback on improvedigital * Fix tmax to s2sConfig.timeout for PBS adapter * PBS adapter: set source.ext.schain with the most commonly used schain * Remove dead code * Use auctionId for source.tid * Update package-lock * Set source.tid to auctionId * Update README * Update README * Update README
* Automatically resolve libraries and their dependencies * Move AnalyticsAdapter under libraries * PBJS-ORTB conversion library * Update ortbConverter README * Improvements on request generation * refactor openxOrtbBidAdapter * Update README * Filter out imps that have no id * Fix nativeMapper logic * Prebid core: simplify native ORTB logic * Remove nativeMapper; do not mix ORTB request and response * fix lint * set bidRequest.nativeParams after ortb -> legacy conversion * Fix native trackers for native ortb responses (including Prebid Server) * Clean up native processors; add s2sConfig.ortbNative * Make ext.prebid.floors.enabled respect first party data * do not set placement for oustream video * Improvements * convert improvedigitalBidAdapter * Update openxOrtbBidAdapter * Improve video validation logic * Move skip validation back to improvedigital * Update README * Override (not merge) context.nativeRequest * Update improvedigital adapter * Address PR feedback on improvedigital * Fix tmax to s2sConfig.timeout for PBS adapter * PBS adapter: set source.ext.schain with the most commonly used schain * Remove dead code * Use auctionId for source.tid * Update package-lock * Set source.tid to auctionId * Update README * Update README * Update README
Type of change
Description of change
This is an initial version of the pbjs-ortb conversion library proposal, an attempt to unify all ORTB-related request generation and response parsing.
The following adapters have been refactored to use the new library:
Other information
Closes #8562
Closes #8665 (missing: documentation on new
s2sConfig.ortbNative
configuration)Library documentation is included in
libraries/ortbConverter/README.md