New Adapter: Nativo #3790
New Adapter: Nativo #3790
Conversation
Code coverage summaryNote:
nativoRefer here for heat map coverage report |
|
@onkarvhanumante Is there any ETA for reviewing this PR? |
|
@rafataveira PR tests are failing due to file miss format. Requesting to format files |
|
@rafataveira Does this bidder supports |
| @@ -0,0 +1,19 @@ | |||
| endpoint: "https://exchange.postrelease.com/esi?ntv_epid=7" | |||
| maintainer: | |||
| email: "[email protected]" | |||
There was a problem hiding this comment.
I've sent an email to this address to confirm it is correct. Please respond to the email with a "received" message.
There was a problem hiding this comment.
It is correct. I've just replied to the email.
static/bidder-params/nativo.json
Outdated
| "title": "Nativo Adapter Params", | ||
| "description": "A schema which validates params accepted by the Nativo adapter", | ||
| "type": "object", | ||
|
|
There was a problem hiding this comment.
NIP, please delete white line
adapters/nativo/nativo_test.go
Outdated
|
|
||
| func TestBidderNativo(t *testing.T) { | ||
| bidder, buildErr := Builder(openrtb_ext.BidderNativo, config.Adapter{ | ||
| Endpoint: "https://foo.io/?src=prebid"}, |
There was a problem hiding this comment.
why is it "https://foo.io/?src=prebid" ?
It should be endpoint from yaml
| @@ -0,0 +1,19 @@ | |||
| endpoint: "https://exchange.postrelease.com/esi?ntv_epid=7" | |||
There was a problem hiding this comment.
"esi?ntv_epid=7" does not look fine
There was a problem hiding this comment.
this is correct, that's the url to our exchange server, in this case, prebid server.
|
@onkarvhanumante I will review all comments and make the changes accordingly. |
Code coverage summaryNote:
nativoRefer here for heat map coverage report |
Code coverage summaryNote:
nativoRefer here for heat map coverage report |
Code coverage summaryNote:
nativoRefer here for heat map coverage report |
Code coverage summaryNote:
nativoRefer here for heat map coverage report |
|
@bsardo The merge issue and test failures are fixed. Could you please go ahead and merge it? |
|
Hi @rafataveira, now I see the redirect working but the user id macro Your server appears to just be echoing whatever I send as |
|
@bsardo could you please recheck it? We pushed a fixed this morning to solve this issue. |
There was a problem hiding this comment.
@rafataveira your cookie sync looks good now and the adapter looks good overall. I just took over review responsibilities from Onkar as he is currently unavailable. I left just a few more comments. Please take a look, update and ping me when ready so I can prioritize re-review. Thanks for your patience.
| userSync: | ||
| redirect: | ||
| url: https://jadserve.postrelease.com/suid/101787?gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&us_privacy={{.USPrivacy}}&ntv_gpp_consent={{.GPP}}&ntv_r={{.RedirectURL}} | ||
| userMacro: NTV_USER_ID |
There was a problem hiding this comment.
Confirmed redirect user sync is working. Observed 302 to PBS setuid endpoint with a user ID which is successfully added to cookie.
adapters/nativo/nativo.go
Outdated
| return bidResponse, errs | ||
| } | ||
|
|
||
| func getMediaTypeForImp(bid openrtb2.Bid, imps []openrtb2.Imp) (openrtb_ext.BidType, error) { |
There was a problem hiding this comment.
Nitpick: I suggest just passing in what you need so instead of copying the bid object just pass in bidImpID string as the first parameter.
adapters/nativo/nativo.go
Outdated
| } | ||
| } | ||
| } | ||
| return "", fmt.Errorf("Unrecognized impression type in response from nativo: %s", bid.ImpID) |
There was a problem hiding this comment.
Can you add a supplemental JSON test where the bid returns an imp ID that is different from what is in the request so that this code path is executed?
There was a problem hiding this comment.
You can delete this test. It is exactly the same as 204-response-from-nativo.json.
adapters/nativo/nativo_test.go
Outdated
|
|
||
| func TestBidderNativo(t *testing.T) { | ||
| bidder, buildErr := Builder(openrtb_ext.BidderNativo, config.Adapter{ | ||
| Endpoint: "https://exchange.postrelease.com/esi?ntv_epid=7"}, |
There was a problem hiding this comment.
I suggest using a fake URL here and in all of your JSON test files for maintenance purposes.
| "h": 50 | ||
| } | ||
| ], | ||
| "type": "banner", |
There was a problem hiding this comment.
I see this field seatbid[i].type is in all of your JSON tests. This is not a valid ORTB bid response field so I suggest removing it as it is misleading. There is a field seatbid[i].bid[j].mtype which can be used to indicate the media type and this is strongly recommended instead of deriving the media type from impression IDs via your function getMediaTypeForImp but that is up to you if you want to make use of it at this time.
At a minimum, please remove this "type" field from all of your JSON test mock responses.
Code coverage summaryNote:
nativoRefer here for heat map coverage report |
Code coverage summaryNote:
nativoRefer here for heat map coverage report |
|
@bsardo all the comments have been addressed. could you please validate? |
|
@bsardo Following up with this. Are you able to review it again and merge it? Thank you. |
|
@przemkaczmarek please give this another look given recent changes. |
|
Thank you @bsardo . Quick question, when and what version will Prebid Server with our adapter be available for publishers? |
No description provided.