New Adapter : MediaGo #3705
New Adapter : MediaGo #3705
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code coverage summaryNote:
mediagoRefer here for heat map coverage report |
Code coverage summaryNote:
mediagoRefer here for heat map coverage report |
adapters/mediago/mediago.go
Outdated
| default: | ||
| var bidExt mediagoResponseBidExt | ||
| err := json.Unmarshal(bid.Ext, &bidExt) | ||
| if err == nil { | ||
| switch bidExt.MediaType { |
There was a problem hiding this comment.
@SylviaF any particular reason to fall back to bid.Ext? are there any scenarios where MType is not being set
There was a problem hiding this comment.
No, I will remove the logic related to bid.ext, but I will keep the code that retrieves the corresponding bid type through imp because our bidder has not yet fully returned mtype.
There was a problem hiding this comment.
@nicgallardo this is the PR, not sure if helpful
| - native | ||
| userSync: | ||
| redirect: | ||
| url: https://trace.mediago.io/ju/cs/prebid?gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&us_privacy={{.USPrivacy}}&redirect={{.RedirectURL}} |
There was a problem hiding this comment.
https://trace.mediago.io/ju/cs/prebid?gdpr=&gdpr_consent=&us_privacy=&redirect=https%3A%2F%2Fprebid.adnxs.com%2Fpbs%2Fv1%2Fsetuid%3Fbidder%3Dmediago%26gdpr%3D%26gdpr_consent%3D%26f%3Di%26uid%3D%24UID
redirect url is working
There was a problem hiding this comment.
Yes, if there's anything we need to change？
adapters/mediago/mediago.go
Outdated
|
|
||
| if response.StatusCode == http.StatusBadRequest { | ||
| return nil, []error{&errortypes.BadInput{ | ||
| Message: fmt.Sprintf("Unexpected status code: %d.", response.StatusCode), | ||
| }} | ||
| } | ||
|
|
||
| if response.StatusCode != http.StatusOK { | ||
| return nil, []error{&errortypes.BadServerResponse{ | ||
| Message: fmt.Sprintf("Unexpected status code: %d.", response.StatusCode), | ||
| }} |
There was a problem hiding this comment.
above code path is not covered. should add json tests for better coverage
adapters/mediago/mediago.go
Outdated
| case openrtb2.MarkupBanner: | ||
| return openrtb_ext.BidTypeBanner, nil | ||
| case openrtb2.MarkupAudio: | ||
| return openrtb_ext.BidTypeAudio, nil | ||
| case openrtb2.MarkupNative: | ||
| return openrtb_ext.BidTypeNative, nil | ||
| case openrtb2.MarkupVideo: | ||
| return openrtb_ext.BidTypeVideo, nil |
There was a problem hiding this comment.
above code path is not covered. should add json tests for better coverage
adapters/mediago/mediago.go
Outdated
| return mediaType, fmt.Errorf("unable to fetch mediaType in multi-format: %s", bid.ImpID) | ||
| } |
There was a problem hiding this comment.
above code path is not covered. should add json tests for better coverage
Code coverage summaryNote:
mediagoRefer here for heat map coverage report |
Code coverage summaryNote:
mediagoRefer here for heat map coverage report |
Code coverage summaryNote:
mediagoRefer here for heat map coverage report |
Code coverage summaryNote:
mediagoRefer here for heat map coverage report |
| default: | ||
| for _, imp := range imps { | ||
| if imp.ID == bid.ImpID { | ||
| if imp.Banner != nil { | ||
| return openrtb_ext.BidTypeBanner, nil | ||
| } | ||
| if imp.Native != nil { | ||
| return openrtb_ext.BidTypeNative, nil | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
any particular reason to fallback to this logic when no case for MType matches?
There was a problem hiding this comment.
@onkarvhanumante We retrieves the corresponding bid type through imp because our bidder api may has not yet fully returned mtype.
Since this is not an OpenRTB standard, we will implement special adaptation. However, to be safe, we would like to keep the fallback code.
There was a problem hiding this comment.
any particular reason to fallback to this logic when no case for MType matches?
Hi @onkarvhanumante chiming in here for my colleague @SylviaF . She replied to this question last week but somehow the reply was not seen. Essentially she's saying the reason why we map the MType field according to the bid type in imp is that we follow the standard OpenRTB specs, and MType is not included in oRTB specs. We will make modifications based on Prebid's requirements, however just in case, we want to keep this fallback code to make sure we can bid properly.
There was a problem hiding this comment.
@onkarvhanumante the Prebid Server committee discussed this thread the other day. We're ok with this fallback logic. Using the MType is preferable and there's no harm in having the fallback code. As I'm sure you're aware, a lot of other adapters only have the imp-based approach. I imagine you were just asking out of curiosity, but just in case, I want you to know that the committee is ok with this.
|
@onkarvhanumante @Sonali-More-Xandr @ChrisHuie Sorry to bother you all, but could you please let me know who will be able to review my code next? My boss keeps asking me why the code hasn't been merged yet. |
Yeah. @SylviaF trying to move this along. Can you please respond to Onkar's above question though about the fallback with no case matches. Think he is waiting on a response there for the review |
Hi @ChrisHuie , I have responded last week. But I found that even my colleage can't see my review. Very strange. |
|
Hi @onkarvhanumante @ChrisHuie, I think we may have some misunderstanding here. My colleague @SylviaF has been replying to all your questions but failed to submit so you never see her replying. Apologies for this as we're not familiar with the UI, and it's now addressed on our side and you can see her previous replies. Appreciate your support here. |
|
Hi @onkarvhanumante I work on the MediaGo team and wanted to check in to see what the status was for our review. Realize we had some miscommunication with the github UI but I think that's all been taken care of now. Is there any update on your end in regards to timing or do you need anything from us to proceed? Also, after your review, is the next step to get it reviewed by @Sonali-More-Xandr? I see 'All Checks have passed' so hoping that's good news! |
There was a problem hiding this comment.
Looks good to me. I have left two small nitpicks if you're able to address. Please also look out for an email to [email protected] to verify it's reachable.
|
Looking at the docs PR, I see that the @bretg for input on PBJS and PBS parameters. |
|
Good catch @SyntaxNode - I've asked for the relevant changes in the docs PR prebid/prebid.github.io#5395 |
Code coverage summaryNote:
mediagoRefer here for heat map coverage report |
2. remove the unsupported formats instead of leaving them as comments
Code coverage summaryNote:
mediagoRefer here for heat map coverage report |
@SyntaxNode @bretg Thank you for your suggestions. I have made all the corrections, including updating the parameter validity explanations in the GitHub.io documentation. |
* Add MediaGo bidder adapter * update getBidType * MediaGo bidder add test covarage and update some logic * Change the EP's domain macro replacement to let the Publisher modify the config. * change mediago docs * change mediago docs * 1. follow Go's comment convention 2. remove the unsupported formats instead of leaving them as comments
New Adapter : MediaGo
https://github.com/prebid/prebid.github.io/blob/master/dev-docs/bidders/mediago.md