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

Adding translatecategories flag to includebrandcategory #1098

Merged
merged 3 commits into from
Nov 11, 2019
Merged

Adding translatecategories flag to includebrandcategory #1098

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3df0227
Making IAB category translation optional with translatecategories boo…
Oct 29, 2019
a23dfb2
Updating exchange unit tests to remove extra bids
Oct 29, 2019
ce9c17a
Updates from code review comments
Nov 5, 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
7 changes: 4 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 @@ -508,9 +508,10 @@ func createBidExtension(videoRequest *openrtb_ext.BidRequestVideo) ([]byte, erro
var inclBrandCat *openrtb_ext.ExtIncludeBrandCategory
if videoRequest.IncludeBrandCategory != nil {
inclBrandCat = &openrtb_ext.ExtIncludeBrandCategory{
PrimaryAdServer: videoRequest.IncludeBrandCategory.PrimaryAdserver,
Publisher: videoRequest.IncludeBrandCategory.Publisher,
WithCategory: true,
PrimaryAdServer: videoRequest.IncludeBrandCategory.PrimaryAdserver,
Publisher: videoRequest.IncludeBrandCategory.Publisher,
WithCategory: true,
TranslateCategories: videoRequest.IncludeBrandCategory.TranslateCategories,
}
} else {
inclBrandCat = &openrtb_ext.ExtIncludeBrandCategory{
Expand Down
12 changes: 8 additions & 4 deletions endpoints/openrtb2/video_auction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,10 +106,12 @@ func TestCreateBidExtension(t *testing.T) {
Increment: 0.1,
})

translateCat := true
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming to translateCategories to match the naming of video_auction.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to translateCategories

videoRequest := openrtb_ext.BidRequestVideo{
IncludeBrandCategory: &openrtb_ext.IncludeBrandCategory{
PrimaryAdserver: 1,
Publisher: "",
PrimaryAdserver: 1,
Publisher: "",
TranslateCategories: &translateCat,
},
PodConfig: openrtb_ext.PodConfig{
DurationRangeSec: durationRange,
Expand Down Expand Up @@ -138,10 +140,12 @@ func TestCreateBidExtensionExactDurTrueNoPriceRange(t *testing.T) {
durationRange = append(durationRange, 15)
durationRange = append(durationRange, 30)

translateCat := false
videoRequest := openrtb_ext.BidRequestVideo{
IncludeBrandCategory: &openrtb_ext.IncludeBrandCategory{
PrimaryAdserver: 1,
Publisher: "",
PrimaryAdserver: 1,
Publisher: "",
TranslateCategories: &translateCat,
},
PodConfig: openrtb_ext.PodConfig{
DurationRangeSec: durationRange,
Expand Down
38 changes: 25 additions & 13 deletions exchange/exchange.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,14 +355,22 @@ func applyCategoryMapping(ctx context.Context, requestExt openrtb_ext.ExtRequest
var primaryAdServer string
var publisher string
var err error
var translateCategories = true

if includeBrandCategory && brandCatExt.WithCategory {
//if ext.prebid.targeting.includebrandcategory present but primaryadserver/publisher not present then error out the request right away.
primaryAdServer, err = getPrimaryAdServer(brandCatExt.PrimaryAdServer) //1-Freewheel 2-DFP
if err != nil {
return res, seatBids, err
//if TranslateCategories is nil, default category translation to enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

You might be fine without this comment, especially since the the following code line doesn't set the default to enabled... that happens a few lines above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, this isn't the right place for this comment. I removed it as translateCategories is declared as being true above, so the comment is probably not necessary.

if brandCatExt.TranslateCategories != nil {
translateCategories = *brandCatExt.TranslateCategories
}
//if translateCategories is set to false, ignore checking primaryAdServer and publisher
if translateCategories {
//if ext.prebid.targeting.includebrandcategory present but primaryadserver/publisher not present then error out the request right away.
primaryAdServer, err = getPrimaryAdServer(brandCatExt.PrimaryAdServer) //1-Freewheel 2-DFP
if err != nil {
return res, seatBids, err
}
publisher = brandCatExt.Publisher
}
publisher = brandCatExt.Publisher
}

seatBidsToRemove := make([]openrtb_ext.BidderName, 0)
Expand All @@ -387,15 +395,19 @@ func applyCategoryMapping(ctx context.Context, requestExt openrtb_ext.ExtRequest
bidsToRemove = append(bidsToRemove, bidInd)
continue
}
//if unique IAB category is present then translate it to the adserver category based on mapping file
category, err = categoriesFetcher.FetchCategories(ctx, primaryAdServer, publisher, bidIabCat[0])
if err != nil || category == "" {
//TODO: add metrics
//if mapping required but no mapping file is found then discard the bid
bidsToRemove = append(bidsToRemove, bidInd)
continue
if translateCategories {
//if unique IAB category is present then translate it to the adserver category based on mapping file
category, err = categoriesFetcher.FetchCategories(ctx, primaryAdServer, publisher, bidIabCat[0])
if err != nil || category == "" {
//TODO: add metrics
//if mapping required but no mapping file is found then discard the bid
bidsToRemove = append(bidsToRemove, bidInd)
continue
}
} else {
//category translation is disabled, continue with IAB category
category = bidIabCat[0]
}

}

// TODO: consider should we remove bids with zero duration here?
Expand Down
157 changes: 156 additions & 1 deletion exchange/exchange_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,8 @@ func newExtRequest() openrtb_ext.ExtRequest {
},
}

brandCat := openrtb_ext.ExtIncludeBrandCategory{PrimaryAdServer: 1, WithCategory: true}
translateCat := true
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming to translateCategories to match the naming of exchange.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to translateCategories

brandCat := openrtb_ext.ExtIncludeBrandCategory{PrimaryAdServer: 1, WithCategory: true, TranslateCategories: &translateCat}

reqExt := openrtb_ext.ExtRequestTargeting{
PriceGranularity: priceGran,
Expand Down Expand Up @@ -568,6 +569,61 @@ func newExtRequestNoBrandCat() openrtb_ext.ExtRequest {
}
}

func newExtRequestTranslateCatNil() openrtb_ext.ExtRequest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this helper method below the TestCategoryMappingTranslateCategoriesNil (the first place it's called) for better readability. Consider accepting a TranslateCategories argument so you don't need two helper methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this function to be below the first test that uses it. Also updated it to take a translateCategories argument so that there is only one helper function for both tests.

priceGran := openrtb_ext.PriceGranularity{
Precision: 2,
Ranges: []openrtb_ext.GranularityRange{
{
Min: 0.0,
Max: 20.0,
Increment: 2.0,
},
},
}

brandCat := openrtb_ext.ExtIncludeBrandCategory{WithCategory: true, PrimaryAdServer: 1}

reqExt := openrtb_ext.ExtRequestTargeting{
PriceGranularity: priceGran,
IncludeWinners: true,
IncludeBrandCategory: &brandCat,
}

return openrtb_ext.ExtRequest{
Prebid: openrtb_ext.ExtRequestPrebid{
Targeting: &reqExt,
},
}
}

func newExtRequestTranslateCatFalse() openrtb_ext.ExtRequest {
priceGran := openrtb_ext.PriceGranularity{
Precision: 2,
Ranges: []openrtb_ext.GranularityRange{
{
Min: 0.0,
Max: 20.0,
Increment: 2.0,
},
},
}

translateCat := false
brandCat := openrtb_ext.ExtIncludeBrandCategory{WithCategory: true, TranslateCategories: &translateCat}

reqExt := openrtb_ext.ExtRequestTargeting{
PriceGranularity: priceGran,
IncludeWinners: true,
IncludeBrandCategory: &brandCat,
}

return openrtb_ext.ExtRequest{
Prebid: openrtb_ext.ExtRequestPrebid{
Targeting: &reqExt,
},
}
}

func TestCategoryMapping(t *testing.T) {

categoriesFetcher, error := newCategoryFetcher("./test/category-mapping")
Expand Down Expand Up @@ -676,6 +732,105 @@ func TestCategoryMappingNoIncludeBrandCategory(t *testing.T) {
assert.Equal(t, 4, len(bidCategory), "Bidders category mapping doesn't match")
}

func TestCategoryMappingTranslateCategoriesNil(t *testing.T) {

categoriesFetcher, error := newCategoryFetcher("./test/category-mapping")
if error != nil {
t.Errorf("Failed to create a category Fetcher: %v", error)
}

requestExt := newExtRequestTranslateCatNil()

targData := &targetData{
priceGranularity: requestExt.Prebid.Targeting.PriceGranularity,
includeWinners: true,
}

requestExt.Prebid.Targeting.DurationRangeSec = []int{15, 30, 50}

adapterBids := make(map[openrtb_ext.BidderName]*pbsOrtbSeatBid)

cats1 := []string{"IAB1-3"}
cats2 := []string{"IAB1-4"}
cats3 := []string{"IAB1-1000"}
bid1 := openrtb.Bid{ID: "bid_id1", ImpID: "imp_id1", Price: 10.0000, Cat: cats1, W: 1, H: 1}
bid2 := openrtb.Bid{ID: "bid_id2", ImpID: "imp_id2", Price: 20.0000, Cat: cats2, W: 1, H: 1}
bid3 := openrtb.Bid{ID: "bid_id3", ImpID: "imp_id3", Price: 30.0000, Cat: cats3, W: 1, H: 1}

bid1_1 := pbsOrtbBid{&bid1, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}}
bid1_2 := pbsOrtbBid{&bid2, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 40}}
bid1_3 := pbsOrtbBid{&bid3, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}}

innerBids := []*pbsOrtbBid{
&bid1_1,
&bid1_2,
&bid1_3,
}

seatBid := pbsOrtbSeatBid{innerBids, "USD", nil, nil}
bidderName1 := openrtb_ext.BidderName("appnexus")

adapterBids[bidderName1] = &seatBid

bidCategory, adapterBids, err := applyCategoryMapping(nil, requestExt, adapterBids, categoriesFetcher, targData)

assert.Equal(t, nil, err, "Category mapping error should be empty")
assert.Equal(t, "10.00_Electronics_30s", bidCategory["bid_id1"], "Category mapping doesn't match")
assert.Equal(t, "20.00_Sports_50s", bidCategory["bid_id2"], "Category mapping doesn't match")
assert.Equal(t, 2, len(adapterBids[bidderName1].bids), "Bidders number doesn't match")
assert.Equal(t, 2, len(bidCategory), "Bidders category mapping doesn't match")
}

func TestCategoryMappingTranslateCategoriesFalse(t *testing.T) {

categoriesFetcher, error := newCategoryFetcher("./test/category-mapping")
if error != nil {
t.Errorf("Failed to create a category Fetcher: %v", error)
}

requestExt := newExtRequestTranslateCatFalse()

targData := &targetData{
priceGranularity: requestExt.Prebid.Targeting.PriceGranularity,
includeWinners: true,
}

requestExt.Prebid.Targeting.DurationRangeSec = []int{15, 30, 50}

adapterBids := make(map[openrtb_ext.BidderName]*pbsOrtbSeatBid)

cats1 := []string{"IAB1-3"}
cats2 := []string{"IAB1-4"}
cats3 := []string{"IAB1-1000"}
bid1 := openrtb.Bid{ID: "bid_id1", ImpID: "imp_id1", Price: 10.0000, Cat: cats1, W: 1, H: 1}
bid2 := openrtb.Bid{ID: "bid_id2", ImpID: "imp_id2", Price: 20.0000, Cat: cats2, W: 1, H: 1}
bid3 := openrtb.Bid{ID: "bid_id3", ImpID: "imp_id3", Price: 30.0000, Cat: cats3, W: 1, H: 1}

bid1_1 := pbsOrtbBid{&bid1, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}}
bid1_2 := pbsOrtbBid{&bid2, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 40}}
bid1_3 := pbsOrtbBid{&bid3, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}}

innerBids := []*pbsOrtbBid{
&bid1_1,
&bid1_2,
&bid1_3,
}

seatBid := pbsOrtbSeatBid{innerBids, "USD", nil, nil}
bidderName1 := openrtb_ext.BidderName("appnexus")

adapterBids[bidderName1] = &seatBid

bidCategory, adapterBids, err := applyCategoryMapping(nil, requestExt, adapterBids, categoriesFetcher, targData)

assert.Equal(t, nil, err, "Category mapping error should be empty")
assert.Equal(t, "10.00_IAB1-3_30s", bidCategory["bid_id1"], "Category should not be translated")
assert.Equal(t, "20.00_IAB1-4_50s", bidCategory["bid_id2"], "Category should not be translated")
assert.Equal(t, "20.00_IAB1-1000_30s", bidCategory["bid_id3"], "Bid should not be rejected")
assert.Equal(t, 3, len(adapterBids[bidderName1].bids), "Bidders number doesn't match")
assert.Equal(t, 3, len(bidCategory), "Bidders category mapping doesn't match")
}

func TestCategoryDedupe(t *testing.T) {

categoriesFetcher, error := newCategoryFetcher("./test/category-mapping")
Expand Down
8 changes: 8 additions & 0 deletions openrtb_ext/bid_request_video.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,14 @@ type IncludeBrandCategory struct {
// string; optional
// Identifier for the Publisher
Publisher string `json:"publisher"`

// Attribute:
// translatecategories
// Type:
// *bool; optional
// Description:
// Indicates if IAB categories should be translated to adserver category
TranslateCategories *bool `json:"translatecategories,omitempty"`
}

type Cacheconfig struct {
Expand Down
7 changes: 4 additions & 3 deletions openrtb_ext/request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,10 @@ type ExtRequestTargeting struct {
}

type ExtIncludeBrandCategory struct {
PrimaryAdServer int `json:"primaryadserver"`
Publisher string `json:"publisher"`
WithCategory bool `json:"withcategory"`
PrimaryAdServer int `json:"primaryadserver"`
Publisher string `json:"publisher"`
WithCategory bool `json:"withcategory"`
TranslateCategories *bool `json:"translatecategories,omitempty"`
}

// Make an unmarshaller that will set a default PriceGranularity
Expand Down