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

Adapter Name Case Insensitive: dealTier #3218

Merged
merged 8 commits into from
Nov 20, 2023
Merged

Conversation

guscarreon
Copy link
Contributor

Function ReadDealTiersFromImp unmarshals the Imp.Ext and creates a map of bidder names to deal tier info. This pull request normalizes the keys of this entries in this map in order to compare to the normalized bidder names and apply deal tiers if needed.

AlexBVolcy
AlexBVolcy previously approved these changes Oct 16, 2023
Comment on lines +42 to +44
if bidderNormalized, bidderFound := NormalizeBidderName(bidder); bidderFound {
dealTiers[bidderNormalized] = *param.DealTier
}
Copy link
Collaborator

@bsardo bsardo Oct 17, 2023

Choose a reason for hiding this comment

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

The deal tier map being created here from the original request has keys that are the normalized bidder names. This map is then used in applyDealSupport but that function does not appear to alter the bidder name in winningBidsByBidder topBidsPerImp prior to checking if the bidder has any deal tiers in the deal tier map. Is a change needed in applyDealSupport as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Modified.

Copy link
Contributor

@AlexBVolcy AlexBVolcy left a comment

Choose a reason for hiding this comment

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

This looks good, just one test coverage comment.


bidderNormalized, bidderFound := openrtb_ext.NormalizeBidderName(bidder.String())
if !bidderFound {
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

This line isn't covered by tests, do you think you can add a test where the bidder is not found from the NormalizeBidderName call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Also refactored TestApplyDealSupport a little bit.

Copy link
Contributor

@AlexBVolcy AlexBVolcy left a comment

Choose a reason for hiding this comment

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

LGTM

@bsardo bsardo assigned VeronikaSolovei9 and unassigned bsardo Nov 14, 2023
Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

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

Local testing looks good, approved

@VeronikaSolovei9
Copy link
Contributor

While testing this PR I found that deals don't work with stored bid responses. This happens because impressions with stored bid response are removed from bidRequest and func getDealTiers doesn't return impDealMap with data for this impression. I'm not sure if this is something that has to be fixed, just want to note about this behavior.

@bsardo bsardo merged commit 6869211 into master Nov 20, 2023
3 checks passed
svamiftah pushed a commit to sovrn/prebid-server that referenced this pull request Nov 21, 2023
SuprPhatAnon pushed a commit to GiftConnect/prebid-server that referenced this pull request Dec 7, 2023
@SyntaxNode SyntaxNode deleted the caseInsensitiveDealTier branch December 19, 2023 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants