Skip to content

RP adapter: use video placement parameter to set size ID #1607

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

Merged
merged 4 commits into from
Dec 10, 2020
Merged

RP adapter: use video placement parameter to set size ID #1607

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dcdf776
RP adapter: use video placement parameter to set size ID
Dec 3, 2020
4ee8062
Fixes after review
Dec 7, 2020
b7d1840
Fixes after review
Dec 9, 2020
083059c
Fixes after review
Dec 9, 2020
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
22 changes: 20 additions & 2 deletions adapters/rubicon/rubicon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,8 @@ func (a *RubiconAdapter) Call(ctx context.Context, req *pbs.PBSRequest, bidder *
rubiReq.Device = &deviceCopy

if thisImp.Video != nil {
videoExt := rubiconVideoExt{Skip: params.Video.Skip, SkipDelay: params.Video.SkipDelay, RP: rubiconVideoExtRP{SizeID: params.Video.VideoSizeID}}
sizeId := resolveVideoSizeId(thisImp.Video.Placement, thisImp.Instl)
videoExt := rubiconVideoExt{Skip: params.Video.Skip, SkipDelay: params.Video.SkipDelay, RP: rubiconVideoExtRP{SizeID: sizeId}}
thisImp.Video.Ext, err = json.Marshal(&videoExt)
} else {
primarySizeID, altSizeIDs, err := parseRubiconSizes(unit.Sizes)
Expand Down Expand Up @@ -601,6 +602,22 @@ func (a *RubiconAdapter) Call(ctx context.Context, req *pbs.PBSRequest, bidder *
return bids, nil
}

func resolveVideoSizeId(placement openrtb.VideoPlacementType, instl int8) (sizeID int) {
if placement != 0 {
if placement == 1 {
return 201
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add JSON test cases to cover this part of the code as well where the SizeID is determined based on imp.Video.Placement

}
if placement == 3 {
return 203
}
}

if instl == 1 {
return 202
}
return 0
Copy link
Contributor

Choose a reason for hiding this comment

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

In case an Imp element comes in still written in the "the old" way without instl and a placement and with a valid rubiconExt.Video.VideoSizeID, do we want to use the latter? In other words, if resolveVideoSizeId returns zero, do we want to try rubiconExt.Video.VideoSizeID, set the size to zero, or return an error?

}

func appendTrackerToUrl(uri string, tracker string) (res string) {
// Append integration method. Adapter init happens once
urlObject, err := url.Parse(uri)
Expand Down Expand Up @@ -785,7 +802,8 @@ func (a *RubiconAdapter) MakeRequests(request *openrtb.BidRequest, reqInfo *adap
}

videoCopy := *thisImp.Video
videoExt := rubiconVideoExt{Skip: rubiconExt.Video.Skip, SkipDelay: rubiconExt.Video.SkipDelay, VideoType: videoType, RP: rubiconVideoExtRP{SizeID: rubiconExt.Video.VideoSizeID}}
sizeId := resolveVideoSizeId(thisImp.Video.Placement, thisImp.Instl)
videoExt := rubiconVideoExt{Skip: rubiconExt.Video.Skip, SkipDelay: rubiconExt.Video.SkipDelay, VideoType: videoType, RP: rubiconVideoExtRP{SizeID: sizeId}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that the rubiconExt.Video.VideoSizeID field will no longer be used here nor elsewhere, should we remove the check on lines 791 to 796 above?

790         if isVideo {
791 -           if rubiconExt.Video.VideoSizeID == 0 {
792 -               errs = append(errs, &errortypes.BadInput{
793 -                   Message: fmt.Sprintf("imp[%d].ext.bidder.rubicon.video.size_id must be defined for video impression", i),
794 -               })
795 -               continue
796 -           }
797
798             // if imp.ext.is_rewarded_inventory = 1, set imp.video.ext.videotype = "rewarded"
799             var videoType = ""
800             if bidderExt.Prebid != nil && bidderExt.Prebid.IsRewardedInventory == 1 {
801                 videoType = "rewarded"
802             }
803
804             videoCopy := *thisImp.Video
805             sizeId := resolveVideoSizeId(thisImp.Video.Placement, thisImp.Instl)
806             videoExt := rubiconVideoExt{Skip: rubiconExt.Video.Skip, SkipDelay: rubiconExt.Video.SkipDelay, VideoType: videoType, RP: rubiconVideoExtRP{SizeID: sizeId}}
807             videoCopy.Ext, err = json.Marshal(&videoExt)
adapters/rubicon/rubicon.go

If future imps come with an empty VideSizeID (JSON field size_id) field, we might risk wrongfully discarding impressions.

videoCopy.Ext, err = json.Marshal(&videoExt)
thisImp.Video = &videoCopy
thisImp.Banner = nil
Expand Down
36 changes: 36 additions & 0 deletions adapters/rubicon/rubicon_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ type rubiAppendTrackerUrlTestScenario struct {
expected string
}

type rubiResolveVideoSizeIdTestScenario struct {
placement openrtb.VideoPlacementType
instl int8
expected int
}

type rubiTagInfo struct {
code string
zoneID int
Expand Down Expand Up @@ -524,6 +530,36 @@ func TestAppendTracker(t *testing.T) {
}
}

func TestResolveVideoSizeId(t *testing.T) {
testScenarios := []rubiResolveVideoSizeIdTestScenario{
Copy link
Contributor

Choose a reason for hiding this comment

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

You can define the test struct right here as:

testScenarios := []struct {
	placement openrtb.VideoPlacementType
	instl     int8
	expected  int
} {
	{
		placement: 1,
		instl:     1,
		expected:  201,
	},
	{
		placement: 3,
		instl:     1,
		expected:  203,
	},
}

{
placement: 1,
instl: 1,
expected: 201,
},
{
placement: 3,
instl: 1,
expected: 203,
},
{
placement: 4,
instl: 1,
expected: 202,
},
{
placement: 4,
instl: 3,
expected: 0,
},
}

for _, scenario := range testScenarios {
res := resolveVideoSizeId(scenario.placement, scenario.instl)
assert.Equal(t, scenario.expected, res)
}
}

func TestNoContentResponse(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusNoContent)
Expand Down