Orbidder: bidfloor currency handling #2093
Conversation
SyntaxNode
changed the title
There was a problem hiding this comment.
Looks good to me. I believe getting rid of the conditional in line 86 makes for an easier read but I'm good either way and I'm approving anyways
84 func preprocessBidFloorCurrency(imp *openrtb2.Imp, reqInfo *adapters.ExtraRequestInfo) error {
85 // we expect every currency related data to be EUR
86 - if imp.BidFloorCur == "" {
87 - imp.BidFloorCur = "EUR"
88 - }
89 -
90 - if imp.BidFloor > 0 && strings.ToUpper(imp.BidFloorCur) != "EUR" {
+ if imp.BidFloor > 0 && (imp.BidFloorCur == "" || strings.ToUpper(imp.BidFloorCur) != "EUR") {
91 if convertedValue, err := reqInfo.ConvertCurrency(imp.BidFloor, imp.BidFloorCur, "EUR"); err != nil {
92 return err
93 } else {
94 imp.BidFloor = convertedValue
95 }
96 }
97 imp.BidFloorCur = "EUR"
98 return nil
99 }
adapters/orbidder/orbidder.go
Thanks @andreasmaurer0210 for addressing my feedback.
| return validImps, errs | ||
| } | ||
|
|
||
| func preprocessExtensions(imp *openrtb2.Imp) error { | ||
| var bidderExt adapters.ExtImpBidder | ||
| if err := json.Unmarshal(imp.Ext, &bidderExt); err != nil { |
There was a problem hiding this comment.
Are you just testing that unmarshalling imp.ext does not produce an error? Seems that you are throwing away the result.
There was a problem hiding this comment.
Yes, you are right. We only checking for errors in unmarshalling imp.ext. In the past we had some problems with invalid Extensions send to our adapter, so we find this check useful. We added some testcases for the preprocessing.
|
@andreasmaurer0210 Friendly ping to see if you have a chance to address the comment above |
@mansinahar Sry, for my late reply. I will take care of the comments. |
mansinahar
requested review from
mansinahar and
AlexBVolcy
and removed request for
guscarreon and
hhhjort
| CurrencyConversions: mockConversions, | ||
| } | ||
|
|
||
| err := preprocessBidFloorCurrency(&imp, &extraRequestInfo) |
There was a problem hiding this comment.
You need to call mockCurrencyConversion.AssertExpectations(t) to make sure that the calls that you're expecting to the currency converter implementation are called with the args you have specified in setMock in those test cases.
There was a problem hiding this comment.
Thank you for pointing this out to us. We added a call of AsserrExpectations in our test method. Always happy to learn new stuff :)
| BidFloorCur: "EUR", | ||
| }, | ||
| }, | ||
| "USD: bidfloor with currency": { |
There was a problem hiding this comment.
Can you please add another similar test case where the mock converter returns an error to make sure the error from the converter is properly propagated?
…sts, refactoring of test functions
|
Thanks @andreasmaurer0210 for addressing the comments. LGTM! |
* 'master' of https://github.com/wwwyyy/prebid-server: GDPR: pass geo based on special feature 1 opt-in (prebid#2122) Adyoulike: Fix adapter errors if no content (prebid#2134) Update adyoulike.yaml (prebid#2131) Add ORTB 2.4 schain support (prebid#2108) Stored response fetcher (with new func for stored resp) (prebid#2099) Adot: Add OpenRTB macros resolution (prebid#2118) Rubicon adapter: accept accountId, siteId, zoneId as strings (prebid#2110) Operaads: support multiformat request (prebid#2121) Update go-gdpr to v1.11.0 (prebid#2125) Adgeneration: Update request to include device and app related info in headers and query params (prebid#2109) Adnuntius: Read device information from ortb Request (prebid#2112) New Adapter: apacdex and Remove Adapter: valueimpression (prebid#2097) New Adapter: Compass (prebid#2102) Orbidder: bidfloor currency handling (prebid#2093)
The Orbidder Backend only supports EUR. This PR deals with Bid Floor Currency conversion into EUR as requested by mail from the Prebid Server committee.