Colossus bid adapter #1472
Colossus bid adapter #1472
Conversation
|
Thank you for your contribution. We'll review this PR early next week. For now, could you please open a PR in https://github.com/prebid/prebid.github.io to add a doc for this new adapter. More information is found in our new adapter guide: https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html#document-your-adapter |
|
Waiting for merge prebid/prebid.github.io#2306 |
adapters/colossus/colossus.go
Outdated
| "github.com/prebid/prebid-server/openrtb_ext" | ||
| ) | ||
|
|
||
| // ColossusAdapter struct |
There was a problem hiding this comment.
This comment isn't very useful. Please consider either removing or expanding.
adapters/colossus/colossus_test.go
Outdated
| ) | ||
|
|
||
| func TestJsonSamples(t *testing.T) { | ||
| colossusAdapter := NewColossusBidder("http://colossusssp.com/?c=o&m=rtb") |
There was a problem hiding this comment.
Consider using an obviously fake url for testing.
| @@ -0,0 +1,39 @@ | |||
| { | |||
There was a problem hiding this comment.
Consider renaming the tests from ext-1 and ext-2 to more descriptive names like ext-emptystring and ext-emptyobject.
adapters/colossus/usersync_test.go
Outdated
| syncInfo, err := syncer.GetUsersyncInfo(privacy.Policies{ | ||
| GDPR: gdpr.Policy{ | ||
| Signal: "0", | ||
| Consent: "ANDFJDS", |
There was a problem hiding this comment.
Consider using an obviously fake consent, like A or anyConsent.
static/bidder-info/colossus.yaml
Outdated
| @@ -0,0 +1,11 @@ | |||
| maintainer: | |||
| email: "aigolkin1991@gmail.com" | |||
There was a problem hiding this comment.
This looks like a personal email address. Do you have a mailing list or work email from colossus to use instead?
| } | ||
|
|
||
| if response.StatusCode != http.StatusOK { | ||
| return nil, []error{&errortypes.BadServerResponse{ |
There was a problem hiding this comment.
Very nice test coverage. This looks to be the only excluded branch. Could you please add a test here to round out the lot?
|
All requested changes are done |
static/bidder-info/colossus.yaml
Outdated
| @@ -0,0 +1,12 @@ | |||
| maintainer: | |||
| email: "huddled.masses1650@gmail.com" | |||
There was a problem hiding this comment.
I'm curious why you are using a gmail account as a contact here instead of an email with @huddledmasses.com or @colossusssp.com.
There was a problem hiding this comment.
@SyntaxNode Because maintanace is beeing done by outsource, so for any questions related to adapters github register email address is used
There was a problem hiding this comment.
@HuddledMasses Gotcha. Our concern over the gmail account is that it does't have good optics as being officially provided by HuddledMasses and if the contractor changes in the future, this support email address may be forgotten about. It's important to us because we rely on these contacts long term for support.
Would you consider creating a @huddledmasses.com and forward it to the this gmail address? That leaves HuddledMasses in control of the contact flow and can change it in the future if needed.
There was a problem hiding this comment.
I've added support@huddledmasses.com email
|
|
||
| "expectedBidResponses": [ | ||
| { | ||
| "bids":[ |
There was a problem hiding this comment.
@SyntaxNode should this also have the top-level key"currency": "USD",? It looks like the JSON test infrastructure does not verify that bid response field.
There was a problem hiding this comment.
The adapters.NewBidderResponseWithBidsCapacity(1) will fill the currency field with USD. If there is any chance of bidding in some other currency, you should add some code to overwrite it. Would also be nice to test bidding in other currencies if that is a thing.
| "ifa": "sdjfksdf-dfsds-dsdg-dsgg" | ||
| } | ||
| }, | ||
| "httpCalls": [{ |
There was a problem hiding this comment.
Nitpick: can we fix the indentation in this section?
| }, | ||
| "device": {} | ||
| }, | ||
| "httpCalls": [{ |
There was a problem hiding this comment.
Nitpick: can we fix the indentation in this section?
exchange/adapter_map.go
Outdated
| @@ -117,7 +118,8 @@ func newAdapterMap(client *http.Client, cfg *config.Configuration, infos adapter | |||
| openrtb_ext.BidderAvocet: avocet.NewAvocetAdapter(cfg.Adapters[string(openrtb_ext.BidderAvocet)].Endpoint), | |||
| openrtb_ext.BidderBeachfront: beachfront.NewBeachfrontBidder(cfg.Adapters[string(openrtb_ext.BidderBeachfront)].Endpoint, cfg.Adapters[string(openrtb_ext.BidderBeachfront)].ExtraAdapterInfo), | |||
| openrtb_ext.BidderBeintoo: beintoo.NewBeintooBidder(cfg.Adapters[string(openrtb_ext.BidderBeintoo)].Endpoint), | |||
| openrtb_ext.BidderBrightroll: brightroll.NewBrightrollBidder(cfg.Adapters[string(openrtb_ext.BidderBrightroll)].Endpoint, cfg.Adapters[string(openrtb_ext.BidderBrightroll)].ExtraAdapterInfo), | |||
| openrtb_ext.BidderBrightroll: brightroll.NewBrightrollBidder(cfg.Adapters[string(openrtb_ext.BidderBrightroll)].Endpoint), | |||
There was a problem hiding this comment.
This looks like a merge conflict (and is causing the validation check errors). Brightroll recently changed their adapter. Please revert their adapter registration.
There was a problem hiding this comment.
I've reverted those commit, now it should be able to merge without conflict
There was a problem hiding this comment.
It actually looks like you reverted their entire adapter commit. I'll take responsibility for that as I see my message of "Please revert their adapter registration." may have been confusing. I intended to express that you should revert the change made in your merge, not their entire adatper.
To fix this from here, I think you'll need to revert the revert and ensure to take their change in this method. The end goal is for there to be no green or red lines in this PR with brightroll in them.
Merge upstream
This reverts commit 0eeee03.
|
@SyntaxNode Finaly, i think we have reached the goal |
Yay! It seems that you added in the recent account PRs now too. My gif-foo isn't strong enough to provide better guidance. It may be easier to start fresh from a new branch and copy/paste in the colossus adapter work? |
|
@SyntaxNode I've created new PR from branch updated to latest prebid-server master branch and with copied changes for colossus adapter #1495 . And will close this one |
New adapter