New Adapter: Bidtheatre #4069
New Adapter: Bidtheatre #4069
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
adapters/bidtheatre/bidtheatre.go
Outdated
| var bidExt openrtb_ext.ExtBid | ||
| err := jsonutil.Unmarshal(bid.Ext, &bidExt) | ||
| if err == nil && bidExt.Prebid != nil { | ||
| return openrtb_ext.ParseBidType(string(bidExt.Prebid.Type)) |
There was a problem hiding this comment.
Consider this as a suggestion. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, recommends implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.
Code coverage summaryNote:
bidtheatreRefer here for heat map coverage report |
|
@Rothalack can you please review? |
|
@pm-nikhil-vaidya can you please review? |
|
Sure @bsardo |
|
I have no additional feedback for this PR. Looks good to me. You'll probably want to flip pbs support to true on the docs whenever this goes out. |
|
Thank you all for your feedback, we will review your comments in detail and address them shortly. |
adapters/bidtheatre/bidtheatre.go
Outdated
| return "", &errortypes.BadServerResponse{ | ||
| Message: fmt.Sprintf("Failed to parse impression \"%s\" mediatype", bid.ImpID), | ||
| } |
There was a problem hiding this comment.
Please add test coverage for these lines by adding a supplemental JSON test that contains a malformed bid.ext.
There was a problem hiding this comment.
Not sure if this were your polite way of saying these lines can't be reached or not.. because I seem to be unable to create a test that triggers them =)
The code comes from your references implementation but would you prefer I remove them?
| func getMediaTypeForBid(bid openrtb2.Bid) (openrtb_ext.BidType, error) { | ||
| var bidExt openrtb_ext.ExtBid | ||
| if err := jsonutil.Unmarshal(bid.Ext, &bidExt); err == nil && bidExt.Prebid != nil { | ||
| return openrtb_ext.ParseBidType(string(bidExt.Prebid.Type)) |
There was a problem hiding this comment.
Consider this as a suggestion. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, recommends implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.
Code coverage summaryNote:
bidtheatreRefer here for heat map coverage report |
| ] | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
what happend if we add muli-format, multi-imps ?
please add multi-format and multi-imp
There was a problem hiding this comment.
The mock impression already is multi-format, you want me to add mock impression requests that's multi-imp?
|
Hi @andreasgreen, since the review process is underway, instead of rebasing and force pushing, please push new commits when making changes or merge with master to resolve conflicts if they arise. This will expedite the process as it makes it much easier and less time consuming for reviewers since we can just check the delta. When your PR is merged, all commits will be squashed. Thanks! |
|
@bsardo , sorry, I thought you required the PR to be one-commit at all times, I will push the individual commits going forward. |
👍 No worries! |
|
@andreasgreen just want to point out that you'll need to run |
Docs: prebid/prebid.github.io#5723
Prebid.js: prebid/Prebid.js#12485