Skip to content
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: VisibleMeasures #2560

Merged
merged 4 commits into from
Feb 24, 2023
Merged

Conversation

VisibleMeasuresHB
Copy link
Contributor

@SyntaxNode
Copy link
Contributor

Please merge in the latest changes from the master branch to resolve the Trivy code scanning failure.

@VisibleMeasuresHB
Copy link
Contributor Author

@SyntaxNode last changes from master was merged.

@onkarvhanumante onkarvhanumante self-assigned this Feb 6, 2023
Comment on lines 18 to 19
url: "https://cs.visiblemeasures.com/pserver?gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&us_privacy={{.USPrivacy}}&redir={{.RedirectURL}}"
userMacro: "[UID]"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It fixed, should works now

@Sonali-More-Xandr Sonali-More-Xandr self-assigned this Feb 9, 2023
Comment on lines +143 to +153
if index, found := impMap[impID]; found {
if index.Banner != nil {
return openrtb_ext.BidTypeBanner, nil
}
if index.Video != nil {
return openrtb_ext.BidTypeVideo, nil
}
if index.Native != nil {
return openrtb_ext.BidTypeNative, nil
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an anti-pattern. Also, this for loop code assumes that impression could either be Banner or Video and not both. But single impression can have both banner and video in the request. In that case, this code will just assumes that it is banner.
Ideally, you should set response bid type based on bidder server response.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our case we don't support multi format, and we are also satisfied with the banner in that case.
Сould you please share what it should look like correctly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since multi format bid is not supported so currently made changes should be enough. Also as an fyi, openrtb2.BidResponse has MType field which can be set in bidder server response and then used in MakeBids to determine impression type

refer https://github.com/prebid/openrtb/blob/main/openrtb2/bid.go#L322-L334

	// Attribute:
	//   mtype
	// Type:
	//   integer
	// Description:
	//   Type of the creative markup so that it can properly be
	//   associated with the right sub-object of the BidRequest.Imp.
	//   Values:
	//   1 = Banner
	//   2 = Video
	//   3 = Audio
	//   4 = Native
	MType MarkupType `json:"mtype,omitempty"`

Copy link
Contributor

@Sonali-More-Xandr Sonali-More-Xandr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! GTG after one more approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants