-
Notifications
You must be signed in to change notification settings - Fork 762
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
AdView: support multi imps request & formattype tag for bid response #3355
Conversation
AdviewOpen
commented
Dec 15, 2023
- support multi imps from request.
- add formattype tag for bid response, use this to handle resp bid-type.
2.json format tidy up.
(2)add test codes for currency.
(2)test script re-fix.
2.add formattype tag for bid response, use this to handle resp bid-type.
adapters/adview/adview.go
Outdated
// setting "banner" as the default bid type | ||
bidType := openrtb_ext.BidTypeBanner | ||
if bidExt != nil { | ||
switch bidExt.BidType { |
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.
Please consider implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.
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.
Sorry, do you mean in line 203, bidType set to banner default type is not suitable, so I should return error if no matched bidtype, right?
Or you mean I should set Mtype such as "seatBid.Bid[i].MType = openrtb2.MarkupBanner" in MakeBids()?
Thank you.
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.
@Sonali-More-Xandr So, you mean is I can use the following codes to get the bid type is enough?
func getMediaTypeForBid(bid openrtb2.Bid) (openrtb_ext.BidType, error) {
switch bid.MType {
case openrtb2.MarkupBanner:
return openrtb_ext.BidTypeBanner, nil
case openrtb2.MarkupVideo:
return openrtb_ext.BidTypeVideo, nil
case openrtb2.MarkupNative:
return openrtb_ext.BidTypeNative, nil
default:
return "", fmt.Errorf("Unable to fetch mediaType in: %s", bid.ImpID)
}
}
Code coverage summaryNote:
adviewRefer here for heat map coverage report
|
Code coverage summaryNote:
adviewRefer here for heat map coverage report
|
Based on reviews, and I got the confirmation that adview server already support mtype in response, after test, i change getMediaTypeForBid(), pls review it. |
adapters/adview/adview.go
Outdated
return "", fmt.Errorf("Unable to fetch mediaType in multi-format: %s", bid.ImpID) | ||
} |
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.
@AdviewOpen should also add received mType
in error case
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.
Ok, add it.
Code coverage summaryNote:
adviewRefer here for heat map coverage report
|
2.remove unused codes.
Code coverage summaryNote:
adviewRefer here for heat map coverage report
|
Code coverage summaryNote:
adviewRefer here for heat map coverage report
|
Hi, I add mtype in error output & removed no used codes. |
adapters/adview/adview.go
Outdated
return openrtb_ext.BidTypeBanner, nil | ||
case openrtb2.MarkupVideo: | ||
return openrtb_ext.BidTypeVideo, nil | ||
case openrtb2.MarkupAudio: |
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.
as per adview
bidder-info yml, audio media type is not supported. should remove audio case
prebid-server/static/bidder-info/adview.yaml
Lines 9 to 12 in cd98554
mediaTypes: | |
- banner | |
- native | |
- video |
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.
Thanks for the reminder, fixed.
Code coverage summaryNote:
adviewRefer here for heat map coverage report
|
Code coverage summaryNote:
adviewRefer here for heat map coverage report
|
Remove audio type cause not supporting now. thx. |
@AdviewOpen I didn't see a docs PR; can you open one please? Your documentation currently states that your adapter doesn’t support multi impression and can not perform impression splitting, so only the first impression will be delivered. |
We have update the doc PR #5126, Thanks again. |