New Adapter: Mediasquare #3994
New Adapter: Mediasquare #3994
Conversation
…' into mediasquare_adapter
Code coverage summaryNote:
mediasquareRefer here for heat map coverage report |
Code coverage summaryNote:
mediasquareRefer here for heat map coverage report |
mediasquare-alexandre
changed the title
static/bidder-info/mediasquare.yaml
Outdated
| gvlVendorID: 791 | ||
| modifyingVastXmlAllowed: true | ||
| maintainer: | ||
| email: "[email protected]" |
There was a problem hiding this comment.
I sent email, please response.
There was a problem hiding this comment.
Unresolving this discussion to ask a question.
We generally prefer to have a corporate email, not personal.
Please refer to "medianet.yaml", "mediafuse.yaml", "appnexus.yaml".
Is this possible you can update it?
There was a problem hiding this comment.
Just found in the description: [email protected]
This will work better, please update, assuming this is a working email.
static/bidder-info/mediasquare.yaml
Outdated
| @@ -0,0 +1,12 @@ | |||
| endpoint: "http://pbs-front.mediasquare.fr/msq_prebid" | |||
There was a problem hiding this comment.
503 response, please fix
|
@mediasquare-alexandre Hi, I’m wondering why these tests were written in this way. In most adapters, we approach it by having a TestJsonSamples class, where we write JSON files to check our scenarios, like simple-banner.json, bad_media_type, etc. Additionally, we add a params_test class to verify validParams and invalidParams. |
Code coverage summaryNote:
mediasquareRefer here for heat map coverage report |
Hi Guys, honestly when I started doing my tests I remembered my PR 2 years ago here where most of my reviewers asked me to increase the code-coverage as much as possible. #2215 |
Code coverage summaryNote:
mediasquareRefer here for heat map coverage report |
Code coverage summaryNote:
mediasquareRefer here for heat map coverage report |
Code coverage summaryNote:
mediasquareRefer here for heat map coverage report |
|
Hi Guys, @przemkaczmarek @bsardo |
| } | ||
|
|
||
| msqParams := initMsqParams(request) | ||
| msqParams.Test = (request.Test == int8(1)) |
There was a problem hiding this comment.
I haven't seen this behavior before. Is this added because the msqParams.Test field is required by the MediaSquare backend to recognize the request as a test?
There was a problem hiding this comment.
Yes, it is.
|
@mediasquare-alexandre It's great that you decided to add the adapterstest.RunJSONBidderTest methods. |
| // MakeRequests : case request is empty. | ||
| resp, errs := bidder.MakeRequests(nil, nil) | ||
| expectingErrors := []error{errorWritter("<MakeRequests> request", nil, true)} | ||
| assert.Equal(t, []*adapters.RequestData(nil), resp, "resp, was supposed to be empty result.") | ||
| assert.Equal(t, expectingErrors, errs, "errs, was supposed to be :", expectingErrors) | ||
|
|
There was a problem hiding this comment.
This should be tested in JSON files.
There was a problem hiding this comment.
I initially would have if it was possible,
// MakeRequests : case request is empty.
resp, errs := bidder.MakeRequests(nil, nil)
Here is a screen-shot of the following test :
Cordially
There was a problem hiding this comment.
I have had, let this test be :
Tell me if you want me to take it back, it will reduce the coverage by 2% or so... But it is not critical.
There was a problem hiding this comment.
It is ok to have unit tests, however, all code coverage should be achieved using the JSON test framework via TestJsonSamples wherever possible.
Please see Adapter Code Tests.
|
@mediasquare-alexandre This still doesn't meet our standards, e.g., there's no params_test.go file where you would test validParams and invalidParams (for your required owner and code). |
Code coverage summaryNote:
mediasquareRefer here for heat map coverage report |
|
@przemkaczmarek I tried to use your naming for tests-files, but I'm not sure the names I put on are ok... The documentation only show that files have to be formated as json in supplemental & examplary, so it doesn't help me neither on this subject. |
|
Hello, |
| return requestData, errs | ||
| } | ||
|
|
||
| func (a *adapter) makeRequest(request *openrtb2.BidRequest, msqParams *msqParameters) (requestData *adapters.RequestData, err error) { |
There was a problem hiding this comment.
Why You have two makeRequest func()?
parsers.go structs.go utils.go these files should not exists.
please check one more time instruction how to build a adapter in Go.
https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html
There was a problem hiding this comment.
I guess you mean why we do have a makeRequest and a MakeRequests as we can't have twice a same function otherwise it would not compile ? If so, for the very same reasons other bidders do it (mgid, openx, taboola, improvedigital and many more), to split the MakeRequests func and make it easier to read. Is that an issue ? Is there any restriction in the amount of functions and their spelling ?
Can you elaborate on the the reason(s) why the files parsers.go, structs.go and utils.go should not exists ? If you referer to the file check list from https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html#file-checklist then my understanding is that this section is about the files that are mandatory not about the only files that we are allowed to create. For instance, appnexus adapter also has files iab_categories.go and models.go in its adapter. The reason why we have these files is to split business logic from the main mediasquare adapter file. If what you want to mean is that we should just merge the content of these files inside mediasquare.go please confirm, though it seems to us that it make this file less readable.
There was a problem hiding this comment.
I will confirm with the team, but I think it's totally ok to have extra files (within the adapter directory) to make code more readable and structured. This adapter accepts and returns it's own format, not OpenRTB format. Having all the structures and converters in the mediasquare.go will make it difficult to understand and navigate.
There was a problem hiding this comment.
Agreed. For simple adapters a single file should suffice. However, in this case, the adapter is rather complex so I do appreciate and prefer the code organization across multiple files.
adapters/mediasquare/structs.go
Outdated
| Gdpr struct { | ||
| ConsentRequired bool `json:"consent_required"` | ||
| ConsentString string `json:"consent_string"` | ||
| } `json:"gdpr"` |
There was a problem hiding this comment.
Looks like this structure is duplicated.
Function initMsqParams declares the same struct.
May you please create a type and reuse it?
adapters/mediasquare/mediasquare.go
Outdated
| var msqResp msqResponse | ||
| if err := jsonutil.Unmarshal(response.Body, &msqResp); err != nil { | ||
| errs = []error{&errortypes.BadServerResponse{ | ||
| Message: fmt.Sprintf("<MakeBids> Unexprected status code: %d. Bad server response: %s.", |
There was a problem hiding this comment.
Typo in UnexpRected
| }} | ||
| } | ||
| return bidderResponse, errs | ||
| } |
There was a problem hiding this comment.
Would you consider to clean up error handling using CheckResponseStatusCodeForErrors and IsResponseStatusCodeNoContent functions? Please refer to "app nexus.go"->MakeBids.
I also noticed an interesting thing. When I run the request to mediasquare and bids are not returned - you still return http 200 with error message. This is not typical behavior. If bids are not returned we usually get http 204. If this is something you cannot change, please consider to add a check for empty responses after jsonutil.Unmarshal(response.Body, &msqResp) so we can exit earlier.
adapters/mediasquare/structs.go
Outdated
| currentVideoBytes, _ := jsonutil.Marshal(currentImp.Video) | ||
| jsonutil.Unmarshal(currentVideoBytes, &video) | ||
| jsonutil.Unmarshal(currentImp.Video.Ext, &video) |
There was a problem hiding this comment.
Please handle errors for all Marshal and Unmarshal expressions.
In case of production errors it will help us to understand where they come from.
| return mediaTypeList["native"] | ||
| default: | ||
| return mediaTypeList["banner"] | ||
| } |
There was a problem hiding this comment.
Nitpick: remove mediaTypeList and return openrtb type:
// mType: Returns the openrtb2.MarkupType from an msqResponseBids.
func (msqBids *msqResponseBids) mType() openrtb2.MarkupType {
switch {
case msqBids.Video != nil:
return openrtb2.MarkupVideo
case msqBids.Native != nil:
return openrtb2.MarkupNative
default:
return openrtb2.MarkupBanner
}
}
adapters/mediasquare/utils.go
Outdated
| func intToPtrInt(i int) *int { | ||
| val := int(i) | ||
| return &val | ||
| } |
There was a problem hiding this comment.
Can be replaced with ptrutil.ToPtr( )
adapters/mediasquare/utils.go
Outdated
| } | ||
|
|
||
| // errorWritter: Returns a Custom error message. | ||
| func errorWritter(referer string, err error, isEmpty bool) error { |
There was a problem hiding this comment.
Typo: errorWritter -> single t
Hi @mediasquare-alexandre thanks for taking the initiative and moving to using the JSON test framework to achieve code coverage. This is required. |
| return requestData, errs | ||
| } | ||
|
|
||
| func (a *adapter) makeRequest(request *openrtb2.BidRequest, msqParams *msqParameters) (requestData *adapters.RequestData, err error) { |
There was a problem hiding this comment.
Agreed. For simple adapters a single file should suffice. However, in this case, the adapter is rather complex so I do appreciate and prefer the code organization across multiple files.
| // MakeRequests : case request is empty. | ||
| resp, errs := bidder.MakeRequests(nil, nil) | ||
| expectingErrors := []error{errorWritter("<MakeRequests> request", nil, true)} | ||
| assert.Equal(t, []*adapters.RequestData(nil), resp, "resp, was supposed to be empty result.") | ||
| assert.Equal(t, expectingErrors, errs, "errs, was supposed to be :", expectingErrors) | ||
|
|
There was a problem hiding this comment.
It is ok to have unit tests, however, all code coverage should be achieved using the JSON test framework via TestJsonSamples wherever possible.
Please see Adapter Code Tests.
static/bidder-info/mediasquare.yaml
Outdated
| gvlVendorID: 791 | ||
| modifyingVastXmlAllowed: true | ||
| maintainer: | ||
| email: "[email protected]" |
There was a problem hiding this comment.
Maintainer email addresses should not be a specific user account but rather a corporate account. Please provide another address, perhaps [email protected], as defined in the PR description.
Code coverage summaryNote:
mediasquareRefer here for heat map coverage report |
|
Hi @VeronikaSolovei9 @bsardo I do think I fixed all your tips. |
Summary
This PR introduces the Mediasquare bidder adapter for Prebid Server. It supports the following mediatypes:
Endpoint: "https://pbs-front.mediasquare.fr/msq_prebid"
Endpoint Compression: gzip
GVL Vendor ID: 791
Test Status
Unit tests: All tests passed.
Maintainer Contact
emails:
About us
https://www.mediasquare.fr/