-
Notifications
You must be signed in to change notification settings - Fork 749
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
Improve Digital adapter: Add deal id and rewardred inventory logic #2439
Conversation
jbartek25
commented
Oct 27, 2022
- Handle Rewarded inventory flag
- Add dealid
- Handle Rewarded inventory flag - Add dealid
// BidExt This struct usage for parse line_item_id and buying_type from bid.ext | ||
type BidExt struct { | ||
Improvedigital struct { | ||
LineItemId int `json:"line_item_id"` |
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.
Nitpick: could you re-name this variable to be LineItemID
@@ -36,6 +54,15 @@ func (a *ImprovedigitalAdapter) MakeRequests(request *openrtb2.BidRequest, reqIn | |||
} | |||
|
|||
func (a *ImprovedigitalAdapter) makeRequest(request openrtb2.BidRequest, imp openrtb2.Imp) (*adapters.RequestData, error) { | |||
// Handle Rewarded Inventory | |||
extImp, err := getImpExtWithRewardedInventory(imp) |
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.
Nitpick: Could you re-name this to be impExt
type ImprovedigitalAdapter struct { | ||
endpoint string | ||
} | ||
|
||
// BidExt This struct usage for parse line_item_id and buying_type from bid.ext |
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.
Nitpick: Please rephrase this as BidExt represents Improved Digital bid extension with line item ID and buying type values
…pr-fix Deal id + rewarded inventory flag
Sorry for the mess. I thought this PR was already merged. Reopening. |
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.
Other than the minor comment below, LGTM
return nil, nil | ||
} | ||
|
||
if string(rewardedInventory) == stateRewardedInventoryEnable { |
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.
Minor comment: You can simplify this code a little by combining the two if statements as
if rewardedInventory, foundRewardedInventory := prebidMap[isRewardedInventory]; foundRewardedInventory && string(rewardedInventory) == stateRewardedInventoryEnable {
ext[isRewardedInventory] = json.RawMessage(`true`)
impExt, err := json.Marshal(ext)
.
.
}
return nil, nil
…rebid#2439) Co-authored-by: Samiul Amin <[email protected]>