-
Notifications
You must be signed in to change notification settings - Fork 748
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
Adnuntius: Add Price Type #3084
Conversation
…cify in your bidderExt
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report
|
adapters/adnuntius/adnuntius.go
Outdated
adDomain := []string{} | ||
for _, url := range ad.DestinationUrls { | ||
domainArray := strings.Split(url, "/") | ||
domain := strings.Replace(domainArray[2], "www.", "", -1) | ||
adDomain = append(adDomain, domain) | ||
} | ||
|
||
// strings.ToLower(str) |
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.
nitpick: should remove comment
openrtb_ext/imp_adnuntius.go
Outdated
@@ -5,4 +5,5 @@ type ImpExtAdnunitus struct { | |||
Network string `json:"network"` | |||
NoCookies bool `json:"noCookies"` | |||
MaxDeals int `json:"maxDeals"` | |||
PriceType string `json:"priceType,omitempty"` |
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.
should also update
https://github.com/prebid/prebid-server/blob/71f76eb39e23d778e7ff3356711004775851e56e/static/bidder-params/adnuntius.json bidder params to include priceType
as optional param
@mikael-lundin bidder params is also missing entry for maxDeals
. Requesting to update bidder param json to include description for priceType
and maxDeals
openrtb_ext/imp_adnuntius.go
Outdated
@@ -5,4 +5,5 @@ type ImpExtAdnunitus struct { | |||
Network string `json:"network"` | |||
NoCookies bool `json:"noCookies"` | |||
MaxDeals int `json:"maxDeals"` | |||
PriceType string `json:"priceType,omitempty"` |
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.
update params tests to add valid and invalid tests for PriceType
and maxDeals
Thanks for the feedback, I've changed it and pushed to this PR. |
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report
|
@mikael-lundin should create docs PR to include details for PriceType and maxDeals |
adapters/adnuntius/adnuntius.go
Outdated
Currency string | ||
} | ||
GrossBid struct { | ||
Amount float64 | ||
Currency string |
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.
@mikael-lundin could you help to understand how values are set to NetBid.Currency
and GrossBid.Currency
. If these vars are not in use then can should remove them
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.
They are always returned from the ad-server so they will always be included in the response.
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.
But yeah you are right they can be removed. :)
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report
|
adapters/adnuntius/adnuntius.go
Outdated
@@ -344,6 +350,18 @@ func generateAdResponse(ad Ad, impId string, html string, request *openrtb2.BidR | |||
}} | |||
} | |||
|
|||
price := ad.Bid.Amount | |||
priceType, _, _, _ := jsonparser.Get(imp.Ext, "bidder", "priceType") |
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 do not ignore errors.
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 changed the way I fetched the value. because the way I did it above, it wouldn't allow for the key to be omitted it seemed. Hope my new way of doing it is good.
adapters/adnuntius/adnuntius.go
Outdated
priceType, _, _, _ := jsonparser.Get(imp.Ext, "bidder", "priceType") | ||
|
||
if string(priceType) != "" { | ||
if strings.ToLower(string(priceType)) == "net" { |
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 prefer to use strings.EqualFold
for a case insensitive comparison.
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.
Thanks, this is changed.
I'm waiting for this PR to go through before adding the prebid server things: Is it ok to have one name for prebid client and another for prebid server? it seems when I looked into the documentation that server prefered the name "priceType" |
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report
|
assuming prebid.js is being referred as prebid client. It would be better if both prebid js and prebid server use same name for new bidder info param. |
Good to know, I will make change the name in prebid server as well. |
@mikael-lundin any updates to move this PR forward |
Yes, will be done today! sorry for the wait. |
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report
|
static/bidder-params/adnuntius.json
Outdated
"priceType": { | ||
"type": "string", | ||
"description": "Allows you to specify Net or Gross bids." |
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.
should update priceType
to bidType
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.
My bad .:/
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report
|
This PR will allow Prebid server to request a price type. The ad will be returned with either net, or gross bid depending on what you specify.