-
Notifications
You must be signed in to change notification settings - Fork 769
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.
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
New Adapter: MadSense #4169
base: master
Are you sure you want to change the base?
New Adapter: MadSense #4169
Conversation
Code coverage summaryNote:
madsenseRefer here for heat map coverage report
|
@pm-jaydeep-mohite can you please review? |
@@ -0,0 +1,12 @@ | |||
endpoint: "https://ads.madsense.io/pbs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Endpoint is reachable
curl -i --location --request POST https://ads.madsense.io/pbs
HTTP/2 200
content-length: 0
return openrtb_ext.BidTypeBanner, nil | ||
case openrtb2.MarkupVideo: | ||
return openrtb_ext.BidTypeVideo, nil | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UT can be added to cover default case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added UT for default case
adapters/madsense/madsense.go
Outdated
bidErrors = append(bidErrors, err) | ||
continue | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Remove blank line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
adapters/madsense/utils.go
Outdated
"fmt" | ||
"github.com/prebid/openrtb/v20/openrtb2" | ||
"github.com/prebid/prebid-server/v3/adapters" | ||
"github.com/prebid/prebid-server/v3/errortypes" | ||
"github.com/prebid/prebid-server/v3/openrtb_ext" | ||
"github.com/prebid/prebid-server/v3/util/jsonutil" | ||
"net/http" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be re-ordered as
`
"fmt"
"net/http"
"github.com/prebid/openrtb/v20/openrtb2"
"github.com/prebid/prebid-server/v3/adapters"
"github.com/prebid/prebid-server/v3/errortypes"
"github.com/prebid/prebid-server/v3/openrtb_ext"
"github.com/prebid/prebid-server/v3/util/jsonutil"
`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-ordered
Code coverage summaryNote:
madsenseRefer here for heat map coverage report
|
@pm-jaydeep-mohite I addressed all comments, could you please review again? |
static/bidder-params/madsense.json
Outdated
"company_id": { | ||
"type": "string", | ||
"description": "An id used to identify madSense company" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking, how do you want to treat an empty string? If you require at least one character you should add a minLength
property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added minLength
property
adapters/madsense/params_test.go
Outdated
var validParams = []string{ | ||
`{"company_id": "9876543"}`, | ||
} | ||
|
||
var invalidParams = []string{ | ||
``, | ||
`null`, | ||
`true`, | ||
`5`, | ||
`4.2`, | ||
`[]`, | ||
`{}`, | ||
`{"companyId": "987654a"}`, | ||
`{"companyId": "98765432"}`, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test for the company_id
empty string case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
@@ -0,0 +1,12 @@ | |||
endpoint: "https://ads.madsense.io/pbs" | |||
maintainer: | |||
email: "[email protected]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sent an email to this address. Please respond to the email with a "received" message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sent
adapters/madsense/madsense_test.go
Outdated
|
||
func TestJsonSamples(t *testing.T) { | ||
bidder, buildErr := Builder(openrtb_ext.BidderMadSense, config.Adapter{ | ||
Endpoint: "https://ads.madsense.io/pbs"}, config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "2"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: we recommend using test urls for maintenance reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated test urls
|
||
func (a *adapter) MakeRequests(request *openrtb2.BidRequest, reqInfo *adapters.ExtraRequestInfo) (requests []*adapters.RequestData, errors []error) { | ||
reqs := make([]*adapters.RequestData, 0, len(request.Imp)) | ||
var errs []error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an issue here because you're mixing named returns with local variable declarations. I suggest deleting the named returns requests
and errors
and always using the local variables reqs
and errs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, I switched to only using local variables
} | ||
|
||
if len(request.Device.IPv6) > 0 { | ||
headers.Add("X-Forwarded-For", request.Device.IPv6) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add test coverage for this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
headers.Add("Origin", request.Site.Domain) | ||
} | ||
if request.Site.Ref != "" { | ||
headers.Set("Referer", request.Site.Ref) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add test coverage for this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
return nil, []error{&errortypes.BadServerResponse{ | ||
Message: "Bad Server Response", | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a supplemental test case where the response.Body
unmarshal fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested in new supplemental test bad-response-error
} | ||
|
||
if request.Test == 1 { | ||
ext.CompanyId = "test" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add test coverage for this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested in new supplemental test test-response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming this to bad-response-status-400.json
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed
Code coverage summaryNote:
madsenseRefer here for heat map coverage report
|
} | ||
|
||
request.Imp = imps | ||
body, err := json.Marshal(request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use jsonutil for marshaling
doc - prebid/prebid.github.io#5824