Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 : MediaGo #3705
New Adapter : MediaGo #3705
Changes from 7 commits
fcc0e0f
34517f7
409161e
3a8f603
930ecf8
5e555e5
687cbbe
b682e0b
d776f1b
e325caa
b74708d
9809b43
d750484
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Based on this code block it seems like adapter code excepts all imps to have same bidder param values.
@SylviaF could you confirm if this is expected behaviour? If yes then this should be explicitly mentioned in bidder docs
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.
@onkarvhanumante Yes, this is expected behaviour. Which file do you mean by bidder doc?Can I submit the doc changes separately later? I'd like to get the code merged and released in new version in this week.
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.
@onkarvhanumante I have change static/bidder-params/mediago.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.
@onkarvhanumante looks like @SylviaF made the docs change updates you requested on this pr.
@SylviaF can you also update the below file in our docs repo to reflect that you are adding a server adapter -> https://github.com/prebid/prebid.github.io/blob/master/dev-docs/bidders/mediago.md
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.
@ChrisHuie I have request a PR to update the docs: prebid/prebid.github.io#5395
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.
@SylviaF could you link docs PR in this PR description
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.
@onkarvhanumante done.
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.
@onkarvhanumante I'm replying on behalf of @SylviaF : Yes, this is expected behavior. She has changed static/bidder-params/mediago.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.
@ChrisHuie replying on behalf of @SylviaF , she has requested a PR to update the docs: prebid/prebid.github.io#5395
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.
any particular reason to fallback to this logic when no case for MType matches?
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.