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

Jump to bottom

Bid floor #1085

Merged
merged 11 commits into from
Nov 13, 2019
Merged

Bid floor #1085

Show file tree
Hide file tree
Changes from all 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
68 changes: 37 additions & 31 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 @@ -26,7 +28,7 @@ func NewTappxBidder(client *http.Client, endpointTemplate string) *TappxAdapter
a := &adapters.HTTPAdapter{Client: client}
template, err := template.New("endpointTemplate").Parse(endpointTemplate)
if err != nil {
glog.Fatal("Unable to parse endpoint url template")
glog.Fatal("Unable to parse endpoint url template: " + err.Error())
return nil
}
return &TappxAdapter{
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
}

ah-tappx marked this conversation as resolved.
Show resolved Hide resolved
reqJSON, err := json.Marshal(request)
if err != nil {
return nil, []error{&errortypes.BadInput{
Expand All @@ -98,54 +98,60 @@ func (a *TappxAdapter) MakeRequests(request *openrtb.BidRequest, reqInfo *adapte

// Builds enpoint url based on adapter-specific pub settings from imp.ext
func (a *TappxAdapter) buildEndpointURL(params *openrtb_ext.ExtImpTappx, test int) (string, error) {
reqHost, reqKey, reqEndpoint := "", "", ""
if params.Host != "" {
reqHost = params.Host
}
if params.Endpoint != "" {
reqEndpoint = params.Endpoint
}
if params.TappxKey != "" {
reqKey = params.TappxKey
}

if reqHost == "" {
if params.Host == "" {
return "", &errortypes.BadInput{
Message: "Tappx host undefined",
}
}

endpointParams := macros.EndpointTemplateParams{Host: reqHost}
host, err := macros.ResolveMacros(a.endpointTemplate, endpointParams)

if err != nil {
if params.Endpoint == "" {
return "", &errortypes.BadInput{
Message: "Unable to parse endpoint url template",
Message: "Tappx endpoint undefined",
}
}

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

if reqEndpoint == "" {
endpointParams := macros.EndpointTemplateParams{Host: params.Host}
host, err := macros.ResolveMacros(a.endpointTemplate, endpointParams)

if err != nil {
return "", &errortypes.BadInput{
Message: "Tappx endpoint undefined",
Message: "Unable to parse endpoint url template: " + err.Error(),
}
}

thisURI, err := url.Parse(host)

if err != nil {
return "", &errortypes.BadInput{
Message: "Malformed URL: " + err.Error(),
}
}

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

queryParams := url.Values{}

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()

ah-tappx marked this conversation as resolved.
Show resolved Hide resolved
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
22 changes: 22 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,32 @@ 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(`https://example\.host\.tappx\.com/DUMMYENDPOINT\?tappxkey=dummy-tappx-key&ts=[0-9]{13}&type_cnn=prebid&v=1\.1`, url)
if err != nil {
t.Errorf("Error while running regex validation: %s", err.Error())
return
}
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
39 changes: 39 additions & 0 deletions adapters/tappx/tappxtest/supplemental/banner-impression-badhost.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
{
"mockBidRequest": {
"id": "0000000000001",
"imp": [
{
"id": "adunit-1",
"banner": {
"w": 300,
"h": 250
},
"ext": {
"bidder": {
"host": "example.ho%st.tappx.com",
"endpoint": "PREBIDTEMPLATE",
"tappxkey": "pub-12345-android-9876"
}
}
}
],
"app": {
"id": "app_001",
"bundle": "com.rovio.angrybirds",
"publisher": {
"id": "2"
}
},
"user": {
"buyeruid": "A-38327932832"
}
},

"expectedMakeRequestsErrors": [
{
"value": "Malformed URL: parse https://example.ho%st.tappx.com: invalid URL escape \"%st\"",
"comparison": "literal"
}
]

}
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
Loading