-
Notifications
You must be signed in to change notification settings - Fork 750
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
Equativ: SmartAdserver alias with update to use mtype #4045
Conversation
Code coverage summaryNote:
smartadserverRefer here for heat map coverage report
|
Code coverage summaryNote:
smartadserverRefer here for heat map coverage report
|
Code coverage summaryNote:
smartadserverRefer here for heat map coverage report
|
Code coverage summaryNote:
smartadserverRefer here for heat map coverage report
|
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.
Looks good. Does this take care of prebid server v2 and v3?
Not sure if the Prebid team is still publishing updates on PBS V2 🤔 |
Code coverage summaryNote:
smartadserverRefer here for heat map coverage report
|
👍 |
Hello @przemkaczmarek or @bsardo could we have a review on this ? I don't know the process on your end ? |
|
||
func TestGetBidTypeFromMarkupType_WhenBanner_ShouldReturnBanner(t *testing.T) { | ||
mediaType := getBidTypeFromMarkupType(openrtb2.MarkupBanner) | ||
|
||
assert.Equal(t, openrtb_ext.BidTypeBanner, mediaType) | ||
} | ||
|
||
func TestGetBidTypeFromMarkupType_WhenVideo_ShouldReturnVideo(t *testing.T) { | ||
mediaType := getBidTypeFromMarkupType(openrtb2.MarkupVideo) | ||
|
||
assert.Equal(t, openrtb_ext.BidTypeVideo, mediaType) | ||
} | ||
|
||
func TestGetBidTypeFromMarkupType_WhenAudio_ShouldReturnAudio(t *testing.T) { | ||
mediaType := getBidTypeFromMarkupType(openrtb2.MarkupAudio) | ||
|
||
assert.Equal(t, openrtb_ext.BidTypeAudio, mediaType) | ||
} | ||
|
||
func TestGetBidTypeFromMarkupType_WhenNative_ShouldReturnNative(t *testing.T) { | ||
mediaType := getBidTypeFromMarkupType(openrtb2.MarkupNative) | ||
|
||
assert.Equal(t, openrtb_ext.BidTypeNative, mediaType) | ||
} |
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.
Why did You add those test?
correctly is to test this through jsons
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.
I removed it. It's indeed already tested by jsons
Code coverage summaryNote:
smartadserverRefer here for heat map coverage report
|
Hello @SyntaxNode or @bsardo is it possible to have a second review on this please 🙏 |
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 @rneuplanche, is this a rebranding? I see that you're adding Equativ as an alias of smartadserver. Are you planning to transition your customers from smartadserver to equativ including asking them to use the equativ bidder code? I see that the support email address for smartadserver has been an equativ domain email address for over a year so it appears there has been a relationship here for quite some time.
site: | ||
mediaTypes: | ||
- banner | ||
- video | ||
- native | ||
- audio | ||
multiformatSupport: "will-bid-on-any" |
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 remove as this is not a supported YAML field in PBS-Go.
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.
Is there a way to mention that we support multi format in the doc ? And is there an official documentation on supported fields ? I couldn't found it
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.
You can declare multi format support in the docs. See how other bidders have done it: https://github.com/search?q=repo%3Aprebid%2Fprebid.github.io%20multiformat_supported&type=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.
Should this test be named multi-format-multi-imp-type.json
? Your imps are both banner and 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.
Renamed 👍
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.
Given that multi-banner-multi-imp-types.json
actually contains imps that are of both banner and video, do you need this test?
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.
You're right, I changed it for a native/audio test as audio was not well covered
Code coverage summaryNote:
smartadserverRefer here for heat map coverage report
|
Hello @bsardo, it has been nearly two years since we rebranded from Smartadserver to Equativ. While our company remains unchanged, we are actively working to phase out references to our previous name. As part of this effort, we have introduced Equativ as an alias for Smartadserver, marking the first step in a gradual migration process aimed at aligning all of our branding and communications with our new identity. |
Thanks for your approvals @przemkaczmarek & @bsardo 🙏 |
In order to support correctly multi impression type bid request, we need to change the way we compute the BidType in the Equativ adapter