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.

Sign up for GitHub

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

Remove hard coded targeting keys #923

Merged
merged 4 commits into from
Jun 10, 2019
+52 −59
Merged

Remove hard coded targeting keys #923

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9d146ba
Remove hard coded targeting keys
mansinahar May 29, 2019
7245c50
Remove leftover comment about targeting constants
mansinahar May 31, 2019
8499c00
Replace Id with ID in targeting keys variable names
mansinahar Jun 6, 2019
f44be2a
Unexport mistakenly exported hbCacheID variable
mansinahar Jun 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 14 additions & 21 deletions endpoints/auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,6 @@ type bidResult struct {

const defaultPriceGranularity = "med"

// Constant keys for ad server targeting for responses to Prebid Mobile
const hbpbConstantKey = "hb_pb"
const hbBidderConstantKey = "hb_bidder"
const hbCacheIdConstantKey = "hb_cache_id"
const hbDealIdConstantKey = "hb_deal"
const hbSizeConstantKey = "hb_size"

func min(x, y int) int {
if x < y {
return x
Expand Down Expand Up @@ -445,16 +438,16 @@ func sortBidsAddKeywordsMobile(bids pbs.PBSBidSlice, pbs_req *pbs.PBSRequest, pr
hbSize = width + "x" + height
}

hbPbBidderKey := hbpbConstantKey + "_" + bid.BidderCode
hbBidderBidderKey := hbBidderConstantKey + "_" + bid.BidderCode
hbCacheIdBidderKey := hbCacheIdConstantKey + "_" + bid.BidderCode
hbDealIdBidderKey := hbDealIdConstantKey + "_" + bid.BidderCode
hbSizeBidderKey := hbSizeConstantKey + "_" + bid.BidderCode
hbPbBidderKey := string(openrtb_ext.HbpbConstantKey) + "_" + bid.BidderCode
hbBidderBidderKey := string(openrtb_ext.HbBidderConstantKey) + "_" + bid.BidderCode
hbCacheIDBidderKey := string(openrtb_ext.HbCacheKey) + "_" + bid.BidderCode
hbDealIDBidderKey := string(openrtb_ext.HbDealIdConstantKey) + "_" + bid.BidderCode
guscarreon marked this conversation as resolved.
Show resolved Hide resolved
hbSizeBidderKey := string(openrtb_ext.HbSizeConstantKey) + "_" + bid.BidderCode
if pbs_req.MaxKeyLength != 0 {
hbPbBidderKey = hbPbBidderKey[:min(len(hbPbBidderKey), int(pbs_req.MaxKeyLength))]
hbBidderBidderKey = hbBidderBidderKey[:min(len(hbBidderBidderKey), int(pbs_req.MaxKeyLength))]
hbCacheIdBidderKey = hbCacheIdBidderKey[:min(len(hbCacheIdBidderKey), int(pbs_req.MaxKeyLength))]
hbDealIdBidderKey = hbDealIdBidderKey[:min(len(hbDealIdBidderKey), int(pbs_req.MaxKeyLength))]
hbCacheIDBidderKey = hbCacheIDBidderKey[:min(len(hbCacheIDBidderKey), int(pbs_req.MaxKeyLength))]
hbDealIDBidderKey = hbDealIDBidderKey[:min(len(hbDealIDBidderKey), int(pbs_req.MaxKeyLength))]
hbSizeBidderKey = hbSizeBidderKey[:min(len(hbSizeBidderKey), int(pbs_req.MaxKeyLength))]
}

Expand All @@ -466,24 +459,24 @@ func sortBidsAddKeywordsMobile(bids pbs.PBSBidSlice, pbs_req *pbs.PBSRequest, pr

kvs[hbPbBidderKey] = roundedCpm
kvs[hbBidderBidderKey] = bid.BidderCode
kvs[hbCacheIdBidderKey] = bid.CacheID
kvs[hbCacheIDBidderKey] = bid.CacheID

if hbSize != "" {
kvs[hbSizeBidderKey] = hbSize
}
if bid.DealId != "" {
kvs[hbDealIdBidderKey] = bid.DealId
kvs[hbDealIDBidderKey] = bid.DealId
}
// For the top bid, we want to add the following additional keys
if i == 0 {
kvs[hbpbConstantKey] = roundedCpm
kvs[hbBidderConstantKey] = bid.BidderCode
kvs[hbCacheIdConstantKey] = bid.CacheID
kvs[string(openrtb_ext.HbpbConstantKey)] = roundedCpm
kvs[string(openrtb_ext.HbBidderConstantKey)] = bid.BidderCode
kvs[string(openrtb_ext.HbCacheKey)] = bid.CacheID
if bid.DealId != "" {
kvs[hbDealIdConstantKey] = bid.DealId
kvs[string(openrtb_ext.HbDealIdConstantKey)] = bid.DealId
}
if hbSize != "" {
kvs[hbSizeConstantKey] = hbSize
kvs[string(openrtb_ext.HbSizeConstantKey)] = hbSize
}
}
}
Expand Down
54 changes: 27 additions & 27 deletions endpoints/auction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,56 +148,56 @@ func TestSortBidsAndAddKeywordsForMobile(t *testing.T) {
t.Error("Ad server targeting should not be nil")
}
if bid.BidderCode == "audienceNetwork" {
if bid.AdServerTargeting["hb_size"] != "300x250" {
t.Error("hb_size key was not parsed correctly")
if bid.AdServerTargeting[string(openrtb_ext.HbSizeConstantKey)] != "300x250" {
t.Error(string(openrtb_ext.HbSizeConstantKey) + " key was not parsed correctly")
}
if bid.AdServerTargeting["hb_pb"] != "2.00" {
t.Error("hb_pb key was not parsed correctly ", bid.AdServerTargeting["hb_pb"])
if bid.AdServerTargeting[string(openrtb_ext.HbpbConstantKey)] != "2.00" {
t.Error(string(openrtb_ext.HbpbConstantKey)+" key was not parsed correctly ", bid.AdServerTargeting[string(openrtb_ext.HbpbConstantKey)])
}

if bid.AdServerTargeting["hb_cache_id"] != "test_cache_id1" {
t.Error("hb_cache_id key was not parsed correctly")
if bid.AdServerTargeting[string(openrtb_ext.HbCacheKey)] != "test_cache_id1" {
t.Error(string(openrtb_ext.HbCacheKey) + " key was not parsed correctly")
}
if bid.AdServerTargeting["hb_bidder"] != "audienceNetwork" {
t.Error("hb_bidder key was not parsed correctly")
if bid.AdServerTargeting[string(openrtb_ext.HbBidderConstantKey)] != "audienceNetwork" {
t.Error(string(openrtb_ext.HbBidderConstantKey) + " key was not parsed correctly")
}
if bid.AdServerTargeting["hb_deal"] != "2345" {
t.Error("hb_deal_id key was not parsed correctly ")
if bid.AdServerTargeting[string(openrtb_ext.HbDealIdConstantKey)] != "2345" {
t.Error(string(openrtb_ext.HbDealIdConstantKey) + " key was not parsed correctly ")
}
}
if bid.BidderCode == "appnexus" {
if bid.AdServerTargeting["hb_size_appnexus"] != "320x50" {
t.Error("hb_size key for appnexus bidder was not parsed correctly")
if bid.AdServerTargeting[string(openrtb_ext.HbSizeConstantKey)+"_appnexus"] != "320x50" {
t.Error(string(openrtb_ext.HbSizeConstantKey) + " key for appnexus bidder was not parsed correctly")
}
if bid.AdServerTargeting["hb_cache_id_appnexus"] != "test_cache_id2" {
t.Error("hb_cache_id key for appnexus bidder was not parsed correctly")
if bid.AdServerTargeting[string(openrtb_ext.HbCacheKey)+"_appnexus"] != "test_cache_id2" {
t.Error(string(openrtb_ext.HbCacheKey) + " key for appnexus bidder was not parsed correctly")
}
if bid.AdServerTargeting["hb_bidder_appnexus"] != "appnexus" {
t.Error("hb_bidder key for appnexus bidder was not parsed correctly")
if bid.AdServerTargeting[string(openrtb_ext.HbBidderConstantKey)+"_appnexus"] != "appnexus" {
t.Error(string(openrtb_ext.HbBidderConstantKey) + " key for appnexus bidder was not parsed correctly")
}
if bid.AdServerTargeting["hb_pb_appnexus"] != "1.00" {
t.Error("hb_pb key for appnexus bidder was not parsed correctly")
if bid.AdServerTargeting[string(openrtb_ext.HbpbConstantKey)+"_appnexus"] != "1.00" {
t.Error(string(openrtb_ext.HbpbConstantKey) + " key for appnexus bidder was not parsed correctly")
}
if bid.AdServerTargeting["hb_pb"] != "" {
t.Error("hb_pb key was parsed for two bidders")
if bid.AdServerTargeting[string(openrtb_ext.HbpbConstantKey)] != "" {
t.Error(string(openrtb_ext.HbpbConstantKey) + " key was parsed for two bidders")
}
if bid.AdServerTargeting["hb_deal_appnexus"] != "1234" {
t.Errorf("hb_deal_id_appnexus was not parsed correctly %v", bid.AdServerTargeting["hb_deal_id_appnexus"])
if bid.AdServerTargeting[string(openrtb_ext.HbDealIdConstantKey)+"_appnexus"] != "1234" {
t.Errorf(string(openrtb_ext.HbDealIdConstantKey)+"_appnexus was not parsed correctly %v", bid.AdServerTargeting[string(openrtb_ext.HbDealIdConstantKey)+"_appnexus"])
}
}
if bid.BidderCode == "rubicon" {
if bid.BidderCode == string(openrtb_ext.BidderRubicon) {
if bid.AdServerTargeting["rpfl_1001"] != "15_tier0100" {
t.Error("custom ad_server_targeting KVPs from adapter were not preserved")
}
}
if bid.BidderCode == "nosizebidder" {
if _, exists := bid.AdServerTargeting["hb_size_nosizebidder"]; exists {
t.Error("hb_size key for nosize bidder was not parsed correctly", bid.AdServerTargeting)
if _, exists := bid.AdServerTargeting[string(openrtb_ext.HbSizeConstantKey)+"_nosizebidder"]; exists {
t.Error(string(openrtb_ext.HbSizeConstantKey)+" key for nosize bidder was not parsed correctly", bid.AdServerTargeting)
}
}
if bid.BidderCode == "nodeal" {
if _, exists := bid.AdServerTargeting["hb_deal_nodeal"]; exists {
t.Error("hb_deal_id key for nodeal bidder was not parsed correctly")
if _, exists := bid.AdServerTargeting[string(openrtb_ext.HbDealIdConstantKey)+"_nodeal"]; exists {
t.Error(string(openrtb_ext.HbDealIdConstantKey) + " key for nodeal bidder was not parsed correctly")
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions endpoints/openrtb2/video_auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,9 +375,9 @@ func buildVideoResponse(bidresponse *openrtb.BidResponse, podErrors []PodError)
podId, _ := strconv.ParseInt(podNum, 0, 64)

videoTargeting := openrtb_ext.VideoTargeting{
Hb_pb: tempRespBidExt.Prebid.Targeting[string(openrtb_ext.HbpbConstantKey)],
Hb_pb_cat_dur: tempRespBidExt.Prebid.Targeting[string(openrtb_ext.HbCategoryDurationKey)],
Hb_cache_id: tempRespBidExt.Prebid.Targeting[string(openrtb_ext.HbVastCacheKey)],
HbPb: tempRespBidExt.Prebid.Targeting[string(openrtb_ext.HbpbConstantKey)],
HbPbCatDur: tempRespBidExt.Prebid.Targeting[string(openrtb_ext.HbCategoryDurationKey)],
HbCacheId: tempRespBidExt.Prebid.Targeting[string(openrtb_ext.HbVastCacheKey)],
}

adPod := findAdPod(podId, adPods)
Expand Down
6 changes: 3 additions & 3 deletions endpoints/openrtb2/video_auction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func TestVideoEndpointImpressionsNumber(t *testing.T) {
assert.Len(t, resp.AdPods[3].Targeting, 1, "Incorrect Targeting data in response")
assert.Len(t, resp.AdPods[4].Targeting, 3, "Incorrect Targeting data in response")

assert.Equal(t, resp.AdPods[4].Targeting[0].Hb_pb_cat_dur, "20.00_395_30s", "Incorrect number of Ad Pods in response")
assert.Equal(t, resp.AdPods[4].Targeting[0].HbPbCatDur, "20.00_395_30s", "Incorrect number of Ad Pods in response")

}

Expand Down Expand Up @@ -398,8 +398,8 @@ func TestVideoBuildVideoResponseMissedCacheForOneBid(t *testing.T) {
assert.NoError(t, err, "Should be no error")
assert.Len(t, bidRespVideo.AdPods, 1, "AdPods length should be 1")
assert.Len(t, bidRespVideo.AdPods[0].Targeting, 2, "AdPod Targeting length should be 2")
assert.Equal(t, "17.00_123_30s", bidRespVideo.AdPods[0].Targeting[0].Hb_pb_cat_dur, "AdPod Targeting first element hb_pb_cat_dur should be 17.00_123_30s")
assert.Equal(t, "17.00_456_30s", bidRespVideo.AdPods[0].Targeting[1].Hb_pb_cat_dur, "AdPod Targeting first element hb_pb_cat_dur should be 17.00_456_30s")
assert.Equal(t, "17.00_123_30s", bidRespVideo.AdPods[0].Targeting[0].HbPbCatDur, "AdPod Targeting first element hb_pb_cat_dur should be 17.00_123_30s")
assert.Equal(t, "17.00_456_30s", bidRespVideo.AdPods[0].Targeting[1].HbPbCatDur, "AdPod Targeting first element hb_pb_cat_dur should be 17.00_456_30s")
}

func TestVideoBuildVideoResponseMissedCacheForAllBids(t *testing.T) {
Expand Down
6 changes: 3 additions & 3 deletions openrtb_ext/bid_response_video.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ type AdPod struct {
}

type VideoTargeting struct {
Hb_pb string `json:"hb_pb"`
Hb_pb_cat_dur string `json:"hb_pb_cat_dur"`
Hb_cache_id string `json:"hb_cache_id"`
HbPb string `json:"hb_pb"`
HbPbCatDur string `json:"hb_pb_cat_dur"`
HbCacheId string `json:"hb_cache_id"`
Copy link
Contributor

Choose a reason for hiding this comment

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

In view of the earlier discussion of GoLint, why HbCacheId instead of HbCacheID?

Related to this, I see other "Id" instances in the above code (minimally, HbDealIdConstantKey). Are they outside the scope of what's being addressed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should be HbCacheID. My bad!

And about the "Id" instances, there are a lot of violations of those and other GoLint issues in our code which we should address. We will probably not be able to address all of them in one PR but I agree, refactoring those targeting keys in openrtb_ext/bid.go should be minimal and can probably be done as part of this PR. Thanks for the catch Joe. I will update the PR

}