Skip to content
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.

Sign up for GitHub

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

Bid floor #1085

Merged
merged 11 commits into from
Nov 13, 2019
Merged

Bid floor #1085

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3d4e5de
added tappx bidder doc
ah-tappx Jul 23, 2019
6157af3
tappx doc revision
ah-tappx Jul 23, 2019
e25ac1c
Merge branch 'master' of github.com:prebid/prebid-server
ah-tappx Sep 26, 2019
77def7d
adding bidFloor parameter to Tappx bidder
ah-tappx Oct 22, 2019
03a569b
Merge branch 'master' of github.com:prebid/prebid-server into bidFloor
ah-tappx Oct 22, 2019
564b838
missing comma
ah-tappx Oct 22, 2019
0df17f8
run go fmt
ah-tappx Oct 22, 2019
cf9def1
update tests
ah-tappx Oct 22, 2019
c0b3c56
missing updated test
ah-tappx Oct 22, 2019
1ca51f1
requested changes & added tests
ah-tappx Oct 29, 2019
2085aac
more requested changes
ah-tappx Nov 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions adapters/tappx/params_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@ package tappx

import (
"encoding/json"
"testing"

"github.com/prebid/prebid-server/openrtb_ext"
"testing"
)

func TestValidParams(t *testing.T) {
Expand Down Expand Up @@ -34,7 +33,8 @@ func TestInvalidParams(t *testing.T) {
}

var validParams = []string{
`{"tappxkey":"pub-12345-android-9876", "endpoint":"ZZ1INTERNALTEST149147915", "host":"test.tappx.com/"}`,
`{"tappxkey":"pub-12345-android-9876", "endpoint":"ZZ1INTERNALTEST149147915", "host":"test.tappx.com"}`,
`{"tappxkey":"pub-12345-android-9876", "endpoint":"ZZ1INTERNALTEST149147915", "host":"test.tappx.com", "bidfloor":0.5}`,
}

var invalidParams = []string{
Expand Down
42 changes: 29 additions & 13 deletions adapters/tappx/tappx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,14 @@ import (
"github.com/prebid/prebid-server/macros"
"github.com/prebid/prebid-server/openrtb_ext"
"net/http"
"net/url"
"strconv"
"text/template"
"time"
)

const TAPPX_BIDDER_VERSION = "1.0"
const TAPPX_BIDDER_VERSION = "1.1"
const TYPE_CNN = "prebid"

type TappxAdapter struct {
http *adapters.HTTPAdapter
Expand All @@ -35,12 +37,6 @@ func NewTappxBidder(client *http.Client, endpointTemplate string) *TappxAdapter
}
}

type tappxParams struct {
Host string `json:"host"`
TappxKey string `json:"tappxkey"`
Endpoint string `json:"endpoint"`
}

func (a *TappxAdapter) Name() string {
return "tappx"
}
Expand Down Expand Up @@ -78,6 +74,10 @@ func (a *TappxAdapter) MakeRequests(request *openrtb.BidRequest, reqInfo *adapte
return nil, []error{err}
}

if tappxExt.BidFloor > 0 {
request.Imp[0].BidFloor = tappxExt.BidFloor
}

reqJSON, err := json.Marshal(request)
if err != nil {
return nil, []error{&errortypes.BadInput{
Expand Down Expand Up @@ -124,9 +124,11 @@ func (a *TappxAdapter) buildEndpointURL(params *openrtb_ext.ExtImpTappx, test in
}
}

if reqKey == "" {
thisURI, err := url.Parse(host)

if err != nil {
return "", &errortypes.BadInput{
Message: "Tappx key undefined",
Message: "Malformed URL: check host parameter",
}
}

Expand All @@ -136,16 +138,30 @@ func (a *TappxAdapter) buildEndpointURL(params *openrtb_ext.ExtImpTappx, test in
}
}

thisURI := host + params.Endpoint + "?tappxkey=" + params.TappxKey
thisURI.Path += params.Endpoint

queryParams := url.Values{}

if reqKey == "" {
return "", &errortypes.BadInput{
Message: "Tappx key undefined",
}
}

queryParams.Add("tappxkey", params.TappxKey)

if test == 0 {
t := time.Now().UnixNano() / (int64(time.Millisecond) / int64(time.Nanosecond))
thisURI = thisURI + "&ts=" + strconv.Itoa(int(t))
queryParams.Add("ts", strconv.Itoa(int(t)))
}

thisURI = thisURI + "&v=" + TAPPX_BIDDER_VERSION
queryParams.Add("v", TAPPX_BIDDER_VERSION)

queryParams.Add("type_cnn", TYPE_CNN)

thisURI.RawQuery = queryParams.Encode()

return thisURI, nil
return thisURI.String(), nil
}

func (a *TappxAdapter) MakeBids(internalRequest *openrtb.BidRequest, externalRequest *adapters.RequestData, response *adapters.ResponseData) (*adapters.BidderResponse, []error) {
Expand Down
21 changes: 21 additions & 0 deletions adapters/tappx/tappx_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,31 @@ package tappx

import (
"github.com/prebid/prebid-server/adapters/adapterstest"
"github.com/prebid/prebid-server/openrtb_ext"
"github.com/stretchr/testify/assert"
"net/http"
"regexp"
"testing"
)

func TestJsonSamples(t *testing.T) {
adapterstest.RunJSONBidderTest(t, "tappxtest", NewTappxBidder(new(http.Client), "https://{{.Host}}"))
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to add some more test cases for the buildEndpointURL method like passing an invalid host which will lead to url.Parse(host) throwing an error, passing empty tappxkey, etc

Tip: You can look at table driven test cases to make it easier to add new test cases and reduce redundant code with multiple test cases. Ref: https://github.com/golang/go/wiki/TableDrivenTests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added test case for url.Parse(host) throwing error.
empty parameter tests should be already covered through JSONBidder tests
there are still some errors that aren't covered like resolving macros or JSON parsing errors, if those are necessary please send us a full list and we will try to write them all

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Yeah, that should cover most cases.

The only reason behind that suggestion was that JSONBidderTests are more testing the MakeRequest and MakeBids function and even though buildEndpointURL is used as part of the MakeRequest function, breaking up the unit tests by each function helps to get better coverage. For example, if we have good unit tests written for buildEndpointURL and all the other functions that MakeRequest and MakeBids call then we don't have to worry about the different places those functions can fail when writing unit tests for MakeRequest and MakeBids themselves. It just makes the tests more readable and manageable.

I understand that doing that in this PR can make it a little bloated but just something to consider for next time :)

func TestTsValue(t *testing.T) {
adapter := NewTappxBidder(new(http.Client), "https://{{.Host}}")
var test int
test = 0
var tappxExt openrtb_ext.ExtImpTappx
tappxExt.Host = "example.host.tappx.com"
tappxExt.Endpoint = "DUMMYENDPOINT"
tappxExt.TappxKey = "dummy-tappx-key"

url, err := adapter.buildEndpointURL(&tappxExt, test)

match, err := regexp.MatchString(`&ts=[0-9]{13}`, url)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say assert the url to the actual expected string rather than doing a regexp match

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex match was purposely added in order to validate the ts parameter. You can read more about this in one of the first requested changes in this pull request by @guscarreon .
The full URL cannot be asserted since ts value is dynamic. I will add the rest of the URL to the regexp match to make it more robust but I think this cannot be skipped.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! I missed the ts part before. Makes sense. Thanks for the clarification :)

if err != nil {
//something happened during regex validation
}
assert.True(t, match)
}
6 changes: 3 additions & 3 deletions adapters/tappx/tappxtest/exemplary/single-banner-impression.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"bidder": {
"tappxkey": "pub-12345-android-9876",
"endpoint": "PREBIDTEMPLATE",
"host": "test.tappx.com/"
"host": "test.tappx.com"
}
}
}
Expand All @@ -33,7 +33,7 @@
"httpCalls": [
{
"expectedRequest": {
"uri": "https://test.tappx.com/PREBIDTEMPLATE?tappxkey=pub-12345-android-9876&v=1.0",
"uri": "https://test.tappx.com/PREBIDTEMPLATE?tappxkey=pub-12345-android-9876&type_cnn=prebid&v=1.1",
"body": {
"id": "0000000000001",
"test": 1,
Expand All @@ -48,7 +48,7 @@
"bidder": {
"tappxkey": "pub-12345-android-9876",
"endpoint": "PREBIDTEMPLATE",
"host": "test.tappx.com/"
"host": "test.tappx.com"
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions adapters/tappx/tappxtest/exemplary/single-video-impression.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
"bidder": {
"tappxkey": "pub-12345-android-9876",
"endpoint": "PREBIDTEMPLATE",
"host": "test.tappx.com/"
"host": "test.tappx.com"
}
}
}
Expand All @@ -35,7 +35,7 @@
"httpCalls": [
{
"expectedRequest": {
"uri": "https://test.tappx.com/PREBIDTEMPLATE?tappxkey=pub-12345-android-9876&v=1.0",
"uri": "https://test.tappx.com/PREBIDTEMPLATE?tappxkey=pub-12345-android-9876&type_cnn=prebid&v=1.1",
"body": {
"id": "0000000000001",
"test": 1,
Expand All @@ -52,7 +52,7 @@
"ext": {
"bidder": {
"endpoint": "PREBIDTEMPLATE",
"host": "test.tappx.com/",
"host": "test.tappx.com",
"tappxkey": "pub-12345-android-9876"
}
}
Expand Down
2 changes: 1 addition & 1 deletion adapters/tappx/tappxtest/params/race/banner.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"tappxkey": "pub-12345-android-9876",
"endpoint": "PREBIDTEMPLATE",
"host": "test.tappx.com/"
"host": "test.tappx.com"
}
2 changes: 1 addition & 1 deletion adapters/tappx/tappxtest/params/race/video.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"tappxkey": "pub-12345-android-9876",
"endpoint": "PREBIDTEMPLATE",
"host": "test.tappx.com/"
"host": "test.tappx.com"
}
6 changes: 3 additions & 3 deletions adapters/tappx/tappxtest/supplemental/204status.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"bidder": {
"tappxkey": "pub-12345-android-9876",
"endpoint": "PREBIDTEMPLATE",
"host": "test.tappx.com/"
"host": "test.tappx.com"
}
}
}
Expand All @@ -30,7 +30,7 @@
"httpCalls": [
{
"expectedRequest": {
"uri": "https://test.tappx.com/PREBIDTEMPLATE?tappxkey=pub-12345-android-9876&v=1.0",
"uri": "https://test.tappx.com/PREBIDTEMPLATE?tappxkey=pub-12345-android-9876&type_cnn=prebid&v=1.1",
"body": {
"id": "0000000000001",
"test": 1,
Expand All @@ -45,7 +45,7 @@
"bidder": {
"tappxkey": "pub-12345-android-9876",
"endpoint": "PREBIDTEMPLATE",
"host": "test.tappx.com/"
"host": "test.tappx.com"
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion adapters/tappx/tappxtest/supplemental/banner-impression-noendpoint.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
"ext": {
"bidder": {
"tappxkey": "pub-12345-android-9876",
"host": "test.tappx.com/"
"host": "test.tappx.com"
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion adapters/tappx/tappxtest/supplemental/banner-impression-nokey.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
"ext": {
"bidder": {
"endpoint": "PREBIDTEMPLATE",
"host": "test.tappx.com/"
"host": "test.tappx.com"
}
}
}
Expand Down
119 changes: 119 additions & 0 deletions adapters/tappx/tappxtest/supplemental/bidfloor.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
{
"mockBidRequest": {
"id": "0000000000001",
"test": 1,
"imp": [
{
"id": "adunit-1",
"banner": {
"w": 300,
"h": 250
},
"ext": {
"bidder": {
"tappxkey": "pub-12345-android-9876",
"endpoint": "PREBIDTEMPLATE",
"host": "test.tappx.com",
"bidfloor": 1.5
}
}
}
],
"app": {
"id": "app_001",
"bundle": "com.rovio.angrybirds",
"publisher": {
"id": "2"
}
},
"user": {
"buyeruid": "A-38327932832"
}
},

"httpCalls": [
{
"expectedRequest": {
"uri": "https://test.tappx.com/PREBIDTEMPLATE?tappxkey=pub-12345-android-9876&type_cnn=prebid&v=1.1",
"body": {
"id": "0000000000001",
"test": 1,
"imp": [
{
"id": "adunit-1",
"banner": {
"w": 300,
"h": 250
},
"bidfloor": 1.5,
"ext": {
"bidder": {
"tappxkey": "pub-12345-android-9876",
"endpoint": "PREBIDTEMPLATE",
"host": "test.tappx.com",
"bidfloor": 1.5
}
}
}
],
"app": {
"bundle": "com.rovio.angrybirds",
"id": "app_001",
"publisher": {
"id": "2"
}
},
"user": {
"buyeruid": "A-38327932832"
}
}
},
"mockResponse": {
"status": 200,
"body": {
"id": "75472df2-1cb3-4f8e-9a28-10cb95fe05a4",
"seatbid": [{
"bid": [{
"id": "wehM-93KGr0_0_0",
"impid": "adunit-1",
"price": 0.5,
"cid": "3706",
"crid": "19005",
"adid": "19005",
"adm": "<!-- admarkup -->",
"cat": ["IAB2"],
"adomain": ["test.com"],
"h": 250,
"w": 300
}]
}],
"bidid": "wehM-93KGr0"
}
}
}
],

"expectedBidResponses": [
{
"currency": "USD",
"bids": [
{
"bid": {
"id": "wehM-93KGr0_0_0",
"impid": "adunit-1",
"price": 0.5,
"adm": "<!-- admarkup -->",
"adid": "19005",
"adomain": ["test.com"],
"cid": "3706",
"crid": "19005",
"w": 300,
"h": 250,
"cat": ["IAB2"]
},
"type": "banner"
}
]
}
]
}
Loading