-
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
Precisonat Bid Adapter : initial release #12208
Conversation
… into master_native
docs -> prebid/prebid.github.io#5582 |
const imp = { | ||
id: slot.bidId, | ||
native: mapNative(slot), | ||
bidfloor: slot.params.bidFloor, |
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.
Sorry, not acceptable. You must first read the bid floor from the standard location as described in https://docs.prebid.org/dev-docs/bidder-adaptor.html . Specifically, bidRequest.getFloor()
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.
@bretg Thanks for the updates. I will change bid floor read by using bidRequest.getFloor()
id: slot.bidId, | ||
native: mapNative(slot), | ||
bidfloor: slot.params.bidFloor, | ||
bidfloorcur: slot.params.currency |
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.
Likewise, floor currency needs to come from the floor system so publishers don't have to specify it multiple times for different bidders
@@ -11,23 +12,7 @@ export const buildRequests = (endpoint) => (validBidRequests = [], bidderRequest | |||
// bidRequest: bidderRequest, | |||
id: validBidRequests[0].auctionId, | |||
cur: validBidRequests[0].params.currency || ['USD'], |
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.
You need to first read currency out of config.getConfig(‘currency.adServerCurrency’). Your local params can be an alternate source.
| Name | Scope | Description | Example | | ||
| :------------ | :------- | :------------------------ | :------------------- | | ||
| `publisherId` | required (for prebid.js) | partner ID provided by preciso | PreTest_0001 | ||
| `region` | required (for prebid.js) | 3 letter country code | "USA" | |
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.
Region can't be required. It's difficult for global publishers to set this kind of metadata on their cached pages around the globe. You should be sniffing the IP address to get a sense for where in the world this user is.
|
||
const BIDDER_CODE = 'precisonat'; | ||
export const storage = getStorageManager({ moduleType: MODULE_TYPE_UID, moduleName: BIDDER_CODE }); | ||
const SUPPORTED_MEDIA_TYPES = [NATIVE]; |
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.
@PrecisoSRL This seems like a lot of duplicative code when comparing to your existing adapter, were there technical reasons why you couldn't add this new Media type and the supporting code there?
gvlid: GVLID, | ||
|
||
isBidRequestValid: (bid) => { | ||
logInfo('TESTETSTETSTETSTETST'); |
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.
can be removed
if (Object.is(precisonatId, 'NA') || Object.is(precisonatId, null) || Object.is(precisonatId, undefined)) { | ||
if (!bid.precisoBid) { | ||
precisoBid = false; | ||
const puid = pid(sharedId); |
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.
@bretg this line is calling an endpoint using sharedId for an id match, I'm concerned this is outside the realm of responsibility of what is expected from this function in a bid adapter. It's currently occurring in their other adapter as well and seems the bid request won't be built if there is no match. Curious what you think of this behavior?
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.
@ncolletti - as long as the adapter is using the storagemanager to retrieve sharedId it seems ok to me.
precisoBid = false; | ||
const puid = pid(sharedId); | ||
if (!Object.is(puid, null) && !Object.is(puid, undefined)) { | ||
storage.setDataInLocalStorage('_pre|id', puid); |
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.
@PrecisoSRL This should check localStorageIsEnabled
const native = JSON.parse(adm).native; | ||
const result = { | ||
clickUrl: encodeURI(native.link.url), | ||
// clickUrl: encodeURI(native.link.clicktrackers[0]), |
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.
there are a number of unnecessary comments throughout the PR, could you address?
} | ||
|
||
function interpretNativeAd(adm) { | ||
const native = JSON.parse(adm).native; |
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.
please put this JSON parse in a try/catch block and check the native
property
function interpretNativeAd(adm) { | ||
const native = JSON.parse(adm).native; | ||
const result = { | ||
clickUrl: encodeURI(native.link.url), |
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.
can you use deepAccess
for the values in this result object?
@@ -86,6 +72,7 @@ export function interpretResponse(serverResponse) { | |||
}) | |||
}) | |||
}) | |||
logInfo('bid respone::' + JSON.stringify(bidsValue)) |
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.
typo
Hi @bretg @ncolletti, Thank you for the PR review and your valuable suggestions. To avoid code duplication, we have included native request and response handling in our existing bid adapter instead of creating a new one. I’ve implemented all the changes you suggested in the existing bid adapter as well. I’ve created another PR with those changes, and I will close this PR. you please take a look at the new PR when you have time? Thank you! |
@PrecisoSRL - I skimmed the other PR for the bid parameters issues noted here: floors, region, currency. Seems fine. I'm not going to "approve" the PR because I did not do a full review, but thanks for the work here. |
closing with new pr |
Type of change
Bugfix
Feature
New bidder adapter
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
Other information