New Adapter: AdUp Tech #4076
New Adapter: AdUp Tech #4076
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| func getBidType(markupType openrtb2.MarkupType) (openrtb_ext.BidType, error) { | ||
| switch markupType { |
There was a problem hiding this comment.
Consider this as a suggestion. The current implementation follows an anti-pattern, assumes that if there is a multi-format request, the media type defaults to openrtb_ext.BidTypeBanner. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, we strongly recommend implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.
| } | ||
|
|
||
| func getBidType(markupType openrtb2.MarkupType) (openrtb_ext.BidType, error) { | ||
| switch markupType { |
There was a problem hiding this comment.
Consider this as a suggestion. The current implementation follows an anti-pattern, assumes that if there is a multi-format request, the media type defaults to openrtb_ext.BidTypeNative. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, we strongly recommend implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.
Code coverage summaryNote:
aduptechRefer here for heat map coverage report |
danygielow
force-pushed
the
feat/add-aduptech-adapter
branch
from
74112fe to
8e94b4b
Compare
| } | ||
|
|
||
| func getBidType(markupType openrtb2.MarkupType) (openrtb_ext.BidType, error) { | ||
| switch markupType { |
There was a problem hiding this comment.
Consider this as a suggestion. The current implementation follows an anti-pattern, assumes that if there is a multi-format request, the media type defaults to openrtb_ext.BidTypeBanner. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, we strongly recommend implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.
| } | ||
|
|
||
| func getBidType(markupType openrtb2.MarkupType) (openrtb_ext.BidType, error) { | ||
| switch markupType { |
There was a problem hiding this comment.
Consider this as a suggestion. The current implementation follows an anti-pattern, assumes that if there is a multi-format request, the media type defaults to openrtb_ext.BidTypeNative. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, we strongly recommend implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.
There was a problem hiding this comment.
What should be changed here? I assume this check is wrong, because we do exactly what the comment suggests.
There was a problem hiding this comment.
💯 this suggestion (x3?) is not very helpful
There was a problem hiding this comment.
I'm not sure why this is triggering but you can ignore it.
Code coverage summaryNote:
aduptechRefer here for heat map coverage report |
|
@scr-oath can you please review? |
|
@pm-isha-bharti can you please review? |
adapters/aduptech/aduptech.go
Outdated
| func Builder(bidderName openrtb_ext.BidderName, config config.Adapter, server config.Server) (adapters.Bidder, error) { | ||
| bidder := &adapter{ | ||
| endpoint: config.Endpoint, | ||
| target_currency: "EUR", |
There was a problem hiding this comment.
question: Can this go into ExtraAdapterInfo (either directly or as json that's parsed or inspected maybe with gjson.Get) rather than being hard coded?
There was a problem hiding this comment.
Changed to use ExtraAdapterInfo.
adapters/aduptech/aduptech.go
Outdated
|
|
||
| convertedValue, err := a.convertCurrency(imp.BidFloor, imp.BidFloorCur, reqInfo) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
suggestion: This return doesn't seem right - as it should be []error - perhaps, if convertCurrency returns a []error, it should be named errs rather than err which suggests a single error as the type.
There was a problem hiding this comment.
convertCurrency doesn't have to return []error. Refactored to just return a single error.
adapters/aduptech/aduptech.go
Outdated
| } | ||
|
|
||
| requestData := &adapters.RequestData{ | ||
| Method: "POST", |
There was a problem hiding this comment.
| Method: "POST", | |
| Method: http.MethodPost, |
adapters/aduptech/aduptech.go
Outdated
|
|
||
| // try again by first converting to USD | ||
| // then convert to target_currency | ||
| convertedValue, err = reqInfo.ConvertCurrency(value, cur, "USD") |
There was a problem hiding this comment.
observation we should have constants or an enum of valid currencies… you can ignore for this PR but just making the note; filed #4134
adapters/aduptech/aduptech.go
Outdated
| if responseData.StatusCode == http.StatusNoContent { | ||
| return nil, nil | ||
| } | ||
|
|
||
| if responseData.StatusCode == http.StatusBadRequest { | ||
| err := &errortypes.BadInput{ | ||
| Message: "Unexpected status code: 400. Run with request.debug = 1 for more info.", | ||
| } | ||
| return nil, []error{err} | ||
| } | ||
|
|
||
| if responseData.StatusCode != http.StatusOK { | ||
| err := &errortypes.BadServerResponse{ | ||
| Message: fmt.Sprintf("Unexpected status code: %d. Run with request.debug = 1 for more info.", responseData.StatusCode), | ||
| } | ||
| return nil, []error{err} | ||
| } |
There was a problem hiding this comment.
question: would a switch on the statusCode (with empty for ok and default clause for this everything-else case) be more readable?
There was a problem hiding this comment.
Refactored into a switch
adapters/aduptech/aduptech.go
Outdated
| for _, seatBid := range response.SeatBid { | ||
| for i, bid := range seatBid.Bid { | ||
| bidType, err := getBidType(bid.MType) | ||
| if err != nil { | ||
| errs = append(errs, err) | ||
| continue | ||
| } | ||
|
|
||
| bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{ | ||
| Bid: &seatBid.Bid[i], | ||
| BidType: bidType, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion: I don't like making copies of the seatbid and bid in a loop
| for _, seatBid := range response.SeatBid { | |
| for i, bid := range seatBid.Bid { | |
| bidType, err := getBidType(bid.MType) | |
| if err != nil { | |
| errs = append(errs, err) | |
| continue | |
| } | |
| bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{ | |
| Bid: &seatBid.Bid[i], | |
| BidType: bidType, | |
| }) | |
| } | |
| } | |
| for i := range response.SeatBid { | |
| seatBid = &response.SeatBid[i] | |
| for j := range seatBid.Bid { | |
| bid := &seatBid.Bid[j] | |
| bidType, err := getBidType(bid.MType) | |
| if err != nil { | |
| errs = append(errs, err) | |
| continue | |
| } | |
| bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{ | |
| Bid: bid, | |
| BidType: bidType, | |
| }) | |
| } | |
| } |
There was a problem hiding this comment.
Used your suggestion
| } | ||
|
|
||
| func getBidType(markupType openrtb2.MarkupType) (openrtb_ext.BidType, error) { | ||
| switch markupType { |
There was a problem hiding this comment.
💯 this suggestion (x3?) is not very helpful
adapters/aduptech/aduptech_test.go
Outdated
| if buildErr != nil { | ||
| t.Fatalf("Builder returned unexpected error %v", buildErr) | ||
| } |
There was a problem hiding this comment.
| if buildErr != nil { | |
| t.Fatalf("Builder returned unexpected error %v", buildErr) | |
| } | |
| if buildErr != nil { | |
| assert.NoError(t, buildErr, "Builder returned unexpected error") | |
| } |
There was a problem hiding this comment.
Ah, good to know. This is our first time working in Go.
There was a problem hiding this comment.
suggestion: Change assert to require
For what it's worth, I realize you were calling fatal before so I realize you need the require.NoError - assertions from the assert module continue but the test fails whereas those from require halt immediately. This is kind of like google's gtest with its two variants for each assertion so when you can continue you report more/all things that fail but when you can't, you don't.
adapters/aduptech/params_test.go
Outdated
| if err := validator.Validate(openrtb_ext.BidderAdUpTech, json.RawMessage(invalidParam)); err == nil { | ||
| t.Errorf("Schema allowed unexpected params: %s", invalidParam) | ||
| } |
There was a problem hiding this comment.
| if err := validator.Validate(openrtb_ext.BidderAdUpTech, json.RawMessage(invalidParam)); err == nil { | |
| t.Errorf("Schema allowed unexpected params: %s", invalidParam) | |
| } | |
| assert.NoErrorf(t, validator.Validate(openrtb_ext.BidderAdUpTech, json.RawMessage(invalidParam)), "Schema allowed unexpected params: %s", invalidParam) |
There was a problem hiding this comment.
using assert.Errorf here, but otherwise used the suggestion.
derfryday
force-pushed
the
feat/add-aduptech-adapter
branch
from
8e94b4b to
2ad858d
Compare
| } | ||
|
|
||
| func getBidType(markupType openrtb2.MarkupType) (openrtb_ext.BidType, error) { | ||
| switch markupType { |
There was a problem hiding this comment.
Consider this as a suggestion. The current implementation follows an anti-pattern, assumes that if there is a multi-format request, the media type defaults to openrtb_ext.BidTypeBanner. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, we strongly recommend implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.
| } | ||
|
|
||
| func getBidType(markupType openrtb2.MarkupType) (openrtb_ext.BidType, error) { | ||
| switch markupType { |
There was a problem hiding this comment.
Consider this as a suggestion. The current implementation follows an anti-pattern, assumes that if there is a multi-format request, the media type defaults to openrtb_ext.BidTypeNative. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, we strongly recommend implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.
Code coverage summaryNote:
aduptechRefer here for heat map coverage report |
| - banner | ||
| - native | ||
| userSync: | ||
| redirect: |
There was a problem hiding this comment.
@danygielow The usersync URL returned by the /cookie_sync for aduptech is currently throwing 404. Can you please check if the URLs added for userSync are correct?
There was a problem hiding this comment.
Fixed sync Endpoint.
There was a problem hiding this comment.
Verified redirect works with id set in cookie:
| func (a *adapter) MakeBids(request *openrtb2.BidRequest, requestData *adapters.RequestData, responseData *adapters.ResponseData) (*adapters.BidderResponse, []error) { | ||
| switch responseData.StatusCode { | ||
| case http.StatusNoContent: | ||
| return nil, nil |
There was a problem hiding this comment.
Please add a supplemental JSON test where your mock response returns a 204 status code.
|
|
||
| var response openrtb2.BidResponse | ||
| if err := jsonutil.Unmarshal(responseData.Body, &response); err != nil { | ||
| return nil, []error{err} |
There was a problem hiding this comment.
Please add a supplemental JSON test where your mock response returns a bid response that fails to unmarshal successfully. This can be achieved by setting one of the fields to an incorrect data type.
adapters/aduptech/aduptech.go
Outdated
| switch responseData.StatusCode { | ||
| case http.StatusNoContent: | ||
| return nil, nil | ||
| case http.StatusBadRequest: |
There was a problem hiding this comment.
Please add a supplemental JSON test where your mock response returns a 400 status code.
| func (a *adapter) convertCurrency(value float64, cur string, reqInfo *adapters.ExtraRequestInfo) (float64, error) { | ||
| convertedValue, err := reqInfo.ConvertCurrency(value, cur, a.extraInfo.TargetCurrency) | ||
|
|
||
| if err != nil { |
There was a problem hiding this comment.
Please add supplemental JSONs for currency conversion failures
adapters/aduptech/aduptech_test.go
Outdated
| bidder, buildErr := Builder(openrtb_ext.BidderAdUpTech, config.Adapter{ | ||
| Endpoint: "https://example.com/rtb/bid", ExtraAdapterInfo: "{\"target_currency\": \"EUR\"}"}, config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "2"}) | ||
|
|
||
| assert.NoError(t, buildErr, "Builder returned unexpected error") |
There was a problem hiding this comment.
| assert.NoError(t, buildErr, "Builder returned unexpected error") | |
| require.NoError(t, buildErr, "Builder returned unexpected error") |
adapters/aduptech/aduptech_test.go
Outdated
| package aduptech | ||
|
|
||
| import ( | ||
| "github.com/stretchr/testify/assert" |
There was a problem hiding this comment.
| "github.com/stretchr/testify/assert" | |
| "github.com/stretchr/testify/assert" | |
| "github.com/stretchr/testify/require" |
adapters/aduptech/params_test.go
Outdated
| } | ||
|
|
||
| for _, validParam := range validParams { | ||
| assert.NoErrorf(t, validator.Validate(openrtb_ext.BidderAdUpTech, json.RawMessage(validParam)), "Schema rejected Aduptech params: %s", validParam) |
There was a problem hiding this comment.
FWIW, assert seems correct here as you want all failing assertions reported
|
Hi @danygielow, @derfryday, now that the review process has begun, can you please push new commits when making changes instead of rebasing and force pushing? It will save reviewers a lot of time when performing delta reviews. All of the commits will be squashed when we merge. Thanks! |
adapters/aduptech/aduptech.go
Outdated
| func Builder(bidderName openrtb_ext.BidderName, config config.Adapter, server config.Server) (adapters.Bidder, error) { | ||
| var extraInfo ExtraInfo | ||
| if err := jsonutil.Unmarshal([]byte(config.ExtraAdapterInfo), &extraInfo); err != nil { | ||
| return nil, fmt.Errorf("invalid extra info: %v", err) |
There was a problem hiding this comment.
suggestion: Use %w instead - see https://pkg.go.dev/fmt#Errorf
| return nil, fmt.Errorf("invalid extra info: %v", err) | |
| return nil, fmt.Errorf("invalid extra info: %w", err) |
| return nil, []error{err} | ||
| } | ||
|
|
||
| imp.BidFloorCur = a.extraInfo.TargetCurrency |
There was a problem hiding this comment.
bug?: There seems like a bug in here somewhere… the convertCurrency has a path that can handle errors and, instead, convert to "USD" - how is that choice made here if you always set the BidFloorCur to a.extraInfo.TargetCurrency - should you pass in a pointer to the extraInfo and set the chosen currency in the convertCurrency method maybe? or return the choice back and set to that? Style is your choice, but I think that's an unhandled case and might deserve some unit tests to prove it works as intended.
There was a problem hiding this comment.
Hi, we try to be ready for the eventuality of conversion rates to EUR not being there for every currency.
The process is:
- first try to convert directly to EUR
- if there is an error, check if it is a conversion rate error
- if it is, try converting to USD first and then from USD to EUR
- So if convertCurrency is successful, then the BidFloorCur is EUR.
There are already special currency conversion JSON tests. See exemplary/simple-banner-convert-currency.json and exemplary/simple-banner-convert-currency-intermediate.json.
There was a problem hiding this comment.
oh… so it won't convert to USD and return no error - it would try to go from USD to EUR and return that result - therefore, the err != nil would save setting blindly to TargetCurrency even if USD were chosen; thanks
adapters/aduptech/aduptech.go
Outdated
|
|
||
| if err != nil { | ||
| var convErr currency.ConversionNotFoundError | ||
| if errors.As(err, &convErr) { |
There was a problem hiding this comment.
nitpick/suggestion: In go, there's a pattern that you try to take the "terminal" case first - return/break/panic/exit so that you don't need an else clause - it saves "cognitive overload" because you can just reason that everything after is the "happy path" (not having your brain full with remembering what the condition was when you get to the else way down the screen or having to scroll back up to understand it) - and also saves precious indentation 😄
(so make this !errors.As and pull the else clause into the block and remove the curly braces {} (keeping and unindenting this code))
There was a problem hiding this comment.
Changed, to have the terminal case first.
Code coverage summaryNote:
aduptechRefer here for heat map coverage report |
adapters/aduptech/aduptech_test.go
Outdated
| bidder, buildErr := Builder(openrtb_ext.BidderAdUpTech, config.Adapter{ | ||
| Endpoint: "https://example.com/rtb/bid", ExtraAdapterInfo: "{\"target_currency\": \"EUR\"}"}, config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "2"}) |
There was a problem hiding this comment.
suggestion: json can be easier to read/maintain if using the backtick string literal as long as the data doesn't contain backtick, which should be uncommon. (See https://go.dev/ref/spec#String_literals). Also… a few newlines couldn't hurt here to line up the fields so you can tell what goes with what at a glance:
| bidder, buildErr := Builder(openrtb_ext.BidderAdUpTech, config.Adapter{ | |
| Endpoint: "https://example.com/rtb/bid", ExtraAdapterInfo: "{\"target_currency\": \"EUR\"}"}, config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "2"}) | |
| bidder, buildErr := Builder( | |
| openrtb_ext.BidderAdUpTech, | |
| config.Adapter{ | |
| Endpoint: "https://example.com/rtb/bid", | |
| ExtraAdapterInfo: `{"target_currency": "EUR"}`, | |
| }, | |
| config.Server{ | |
| ExternalUrl: "http://hosturl.com", | |
| GvlID: 1, | |
| DataCenter: "2", | |
| }, | |
| ) |
There was a problem hiding this comment.
There are other places where this pattern can improve readability; I've only exemplified this one.
There was a problem hiding this comment.
OK, sorry, i thought gofmt takes care of everything. I didn't even try to format anything.
I come from Python where black or now ruff-format will also take care of line breaks and indentations in appropriate places to improve readability. And they reference gofmt as an inspiration.
Code coverage summaryNote:
aduptechRefer here for heat map coverage report |
|
@scr-oath The checks seem to have problems. Can you please take a look. |
Hmm… I still see checks running and none of the completed ones have failed -
|
adapters/aduptech/aduptech.go
Outdated
| } | ||
|
|
||
| if extraInfo.TargetCurrency == "" { | ||
| return nil, fmt.Errorf("invalid extra info: TargetCurrency is empty, pls check") |
There was a problem hiding this comment.
nitpick (not-blocking) while a little pedantic, I prefer errors.New for errors that are just string literals.
| return nil, fmt.Errorf("invalid extra info: TargetCurrency is empty, pls check") | |
| return nil, errors.New("invalid extra info: TargetCurrency is empty, pls check") |
There was a problem hiding this comment.
Good catch, agreed @scr-oath.
There was a problem hiding this comment.
makes sense, changed it
openrtb_ext/imp_aduptech.go
Outdated
| Publisher string `json:"publisher"` | ||
| Placement string `json:"placement"` | ||
| Query string `json:"query"` | ||
| AdTest string `json:"adtest"` |
There was a problem hiding this comment.
I understand that this data structure is not being used but please make it match what you've defined in your bidder params JSON file so that it contains all of the parameters you've defined there and have corresponding Go types. In that case you should add a bool debug field and adtest should be bool.
| @@ -0,0 +1,27 @@ | |||
| endpoint: "https://rtb.d.adup-tech.com/rtb/bid" | |||
There was a problem hiding this comment.
Confirmed endpoint is reachable:
curl -i --location --request POST https://rtb.d.adup-tech.com/rtb/bid
HTTP/2 200
date: Tue, 21 Jan 2025 17:14:24 GMT
content-type: application/json
content-length: 87
server: nginx
{"error":"Could not determine network. Publisher is missing in request parameters. {}"}
| - CHE | ||
| - GBR | ||
| maintainer: | ||
| email: "[email protected]" |
There was a problem hiding this comment.
We've sent an email to this address. Please respond with a message of "Received" to confirm it is active.
| - GBR | ||
| maintainer: | ||
| email: "[email protected]" | ||
| gvlVendorID: 647 |
There was a problem hiding this comment.
GVL Vendor ID looks correct:
curl https://vendor-list.consensu.org/v3/vendor-list.json | jq '.vendors."647"'
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 656k 100 656k 0 0 4138k 0 --:--:-- --:--:-- --:--:-- 4156k
{
"id": 647,
"name": "Axel Springer Teaser Ad GmbH",
"purposes": [
1,
3,
4,
5,
6
],
"legIntPurposes": [
2,
7,
10,
11
],
"flexiblePurposes": [
2,
7,
10,
11
],
"specialPurposes": [
1,
2,
3
],
"features": [
3
],
"specialFeatures": [],
"cookieMaxAgeSeconds": 31536000,
"usesCookies": true,
"cookieRefresh": true,
"urls": [
{
"langId": "en",
"privacy": "https://www.adup-tech.com/en/privacy-policy/",
"legIntClaim": "https://www.adup-tech.com/en/privacy-policy/#c4489"
},
{
"langId": "de",
"privacy": "https://www.adup-tech.com/rechtliches/datenschutz/",
"legIntClaim": "https://www.adup-tech.com/rechtliches/datenschutz/#c4488"
}
],
"usesNonCookieAccess": true,
"dataRetention": {
"stdRetention": 365,
"purposes": {},
"specialPurposes": {}
},
"dataDeclaration": [
1,
2,
3,
6,
8,
10,
11
],
"deviceStorageDisclosureUrl": "https://s.d.adup-tech.com/gdpr/deviceStorage.json"
}
| iframe: | ||
| url: "https://rtb.d.adup-tech.com/service/sync?iframe=1&gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&us_privacy={{.USPrivacy}}&redirect={{.RedirectURL}}" | ||
| userMacro: "[UID]" |
There was a problem hiding this comment.
Verified iframe works with id set in cookie:
|
|
||
| var validParams = []string{ | ||
| `{ "publisher": "123456789", "placement": "234567890" }`, | ||
| `{ "publisher": "123456789", "placement": "234567890", "query": "test" }`, |
There was a problem hiding this comment.
According to your bidder params JSON file, your string params publisher, placement and query are considered valid if they are specified as empty strings. Please add a test case for this.
There was a problem hiding this comment.
added minLength to publisher and placement, added more params tests
| if !errors.As(err, &convErr) { | ||
| return 0, err | ||
| } |
There was a problem hiding this comment.
Please add a supplemental JSON test case where imp[].bidfloorcur is invalid to cover this block.
adapters/aduptech/aduptech.go
Outdated
| if err != nil { | ||
| return 0, err | ||
| } |
There was a problem hiding this comment.
Please add a supplemental JSON test case where ext.prebid.currency.rates contains an invalid USD conversion to cover this block:
e.g.
"USD": {
"INVALID": 2.0
}
There was a problem hiding this comment.
The currency rates are already validated before the json test runs.
See: https://github.com/prebid/prebid-server/blob/master/adapters/adapterstest/test_json.go#L149
There was a problem hiding this comment.
Ah yes I understand but I'm just concerned with testing your adapter. We can force an intermediate currency conversion failure so that we achieve code coverage of this error block, though my original suggested approach was incorrect. Instead you can use this test:
currency-conversion-intermediate-failure.json
There was a problem hiding this comment.
OK, added that test. Thank you
adapters/aduptech/aduptech.go
Outdated
| switch responseData.StatusCode { | ||
| case http.StatusNoContent: | ||
| return nil, nil | ||
| case http.StatusBadRequest: | ||
| return nil, []error{&errortypes.BadInput{ | ||
| Message: "Unexpected status code: 400. Run with request.debug = 1 for more info.", | ||
| }} | ||
| case http.StatusOK: | ||
| // we don't need to do anything here and this is just so we can use the default case for the unexpected status code | ||
| break | ||
| default: | ||
| return nil, []error{&errortypes.BadServerResponse{ | ||
| Message: fmt.Sprintf("Unexpected status code: %d.", responseData.StatusCode), | ||
| }} | ||
|
|
||
| } |
There was a problem hiding this comment.
Please use the adapter utility functions IsResponseStatusCodeNoContent and CheckResponseStatusCodeForErrors found in adapters/response.go for processing the http status code.
There was a problem hiding this comment.
now using the utility functions
| } | ||
|
|
||
| func getBidType(markupType openrtb2.MarkupType) (openrtb_ext.BidType, error) { | ||
| switch markupType { |
There was a problem hiding this comment.
I'm not sure why this is triggering but you can ignore it.
…StatusCodeForErrors to check response status codes
…int to publisher and placement param, added params tests
Code coverage summaryNote:
aduptechRefer here for heat map coverage report |
Code coverage summaryNote:
aduptechRefer here for heat map coverage report |
|
|
||
| if extraInfo.TargetCurrency == "" { | ||
| return nil, errors.New("invalid extra info: TargetCurrency is empty, pls check") | ||
| } |
There was a problem hiding this comment.
To make sure the AdUp Tech adapter is configured with a known currency string, can we further the validation by checking whether or not the config value represents a valid three-digit currency code? The ParseISO(string) function from the already imported currency library could come in handy:
27 func Builder(bidderName openrtb_ext.BidderName, config config.Adapter, server config.Server) (adapters.Bidder, error) {
28 var extraInfo ExtraInfo
29 if err := jsonutil.Unmarshal([]byte(config.ExtraAdapterInfo), &extraInfo); err != nil {
30 return nil, fmt.Errorf("invalid extra info: %w", err)
31 }
32
33 if extraInfo.TargetCurrency == "" {
34 return nil, errors.New("invalid extra info: TargetCurrency is empty, pls check")
35 }
36
+ targetCurrency, err := currency.ParseISO(extraInfo.TargetCurrency)
+ if err != nil {
+ return nil, fmt.Errorf("invalid TargetCurrency: %s", extraInfo.TargetCurrency)
+ }
+ extraInfo.TargetCurrency = targetCurrency
+
37 bidder := &adapter{
38 endpoint: config.Endpoint,
39 extraInfo: extraInfo,
40 }
41
42 return bidder, nil
43 }
adapters/aduptech/aduptech.go
There was a problem hiding this comment.
Hi, it is not the same currency library, and ParseISO also returns a currency.Unit.
So I adapted your suggestion and added a corresponding test.
Code coverage summaryNote:
aduptechRefer here for heat map coverage report |
| "expectedBidResponses": [], | ||
| "expectedMakeRequestsErrors": [ | ||
| { | ||
| "value": "Currency conversion rate not found: 'USD' => 'EUR'", |
There was a problem hiding this comment.
Nitpick: I don't oppose this error message if you find it useful, but wouldn't "Converted currency of bid floor from 'AUD' to 'USD' but there was an error converting from 'USD' to 'EUR': Currency conversion rate not found: 'USD' => 'EUR'" be more accurate? We could tweak the convertCurrency function to get a more descriptive message:
87 func (a *adapter) convertCurrency(value float64, cur string, reqInfo *adapters.ExtraRequestInfo) (float64, error) {
88 convertedValue, err := reqInfo.ConvertCurrency(value, cur, a.extraInfo.TargetCurrency)
89
90 if err != nil {
91 var convErr prebidcurrency.ConversionNotFoundError
92 if !errors.As(err, &convErr) {
93 return 0, err
94 }
95
96 // try again by first converting to USD
97 // then convert to target_currency
98 convertedValue, err = reqInfo.ConvertCurrency(value, cur, "USD")
99
100 if err != nil {
101 - return 0, err
+ return 0, fmt.Errorf("Currency conversion rate not found from '%s' to '%s'. Error converting from '%s' to 'USD': %w", cur, a.extraInfo.TargetCurrency, cur, err)
102 }
adapters/aduptech/aduptech.go
There was a problem hiding this comment.
I agree, I changed the error message.
| "expectedBidResponses": [], | ||
| "expectedMakeRequestsErrors": [ | ||
| { | ||
| "value": "Currency conversion rate not found: 'AUD' => 'USD'", |
There was a problem hiding this comment.
Nitpick: I don't oppose this error message if you find it useful, but wouldn't "Currency conversion rate not found from 'AUD' to 'EUR'. Error converting from 'AUD' to 'USD': Currency conversion rate not found: 'AUD' => 'USD'" be more accurate? We could tweak the convertCurrency function to get a more descriptive message:
104 convertedValue, err = reqInfo.ConvertCurrency(convertedValue, "USD", a.extraInfo.TargetCurrency)
105
106 if err != nil {
107 - return 0, err
+ return 0, fmt.Errorf("Currency conversion rate not found from '%s' to '%s'. Error converting from '%s' to 'USD': %w", cur, a.extraInfo.TargetCurrency, cur, err)
108 }
109 }
110 return convertedValue, nil
111 }
112
adapters/aduptech/aduptech.go
There was a problem hiding this comment.
I agree, I changed the error message.
Code coverage summaryNote:
aduptechRefer here for heat map coverage report |
Hello,
We as AdUp Technology want to add our own Prebid Server Adapter.
We already have a PrebidJS Adapter.
Docs: prebid/prebid.github.io#5728