-
Notifications
You must be signed in to change notification settings - Fork 769
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
Media type price granularity #2721
Changes from all commits
a96265f
61783cf
082ce54
0340ae9
59459f7
833547a
f19e70d
c28cf36
69dca07
cd013d8
f949e12
c0a3217
74af7d1
6183f53
ad16925
4ca20a4
23feb90
1cb8277
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1584,29 +1584,50 @@ func validateTargeting(t *openrtb_ext.ExtRequestTargeting) error { | |
} | ||
|
||
if t.PriceGranularity != nil { | ||
pg := t.PriceGranularity | ||
if err := validatePriceGranularity(t.PriceGranularity); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
if pg.Precision == nil { | ||
return errors.New("Price granularity error: precision is required") | ||
} else if *pg.Precision < 0 { | ||
return errors.New("Price granularity error: precision must be non-negative") | ||
} else if *pg.Precision > openrtb_ext.MaxDecimalFigures { | ||
return fmt.Errorf("Price granularity error: precision of more than %d significant figures is not supported", openrtb_ext.MaxDecimalFigures) | ||
if t.MediaTypePriceGranularity.Video != nil { | ||
if err := validatePriceGranularity(t.MediaTypePriceGranularity.Video); err != nil { | ||
return err | ||
} | ||
} | ||
if t.MediaTypePriceGranularity.Banner != nil { | ||
if err := validatePriceGranularity(t.MediaTypePriceGranularity.Banner); err != nil { | ||
return err | ||
} | ||
} | ||
if t.MediaTypePriceGranularity.Native != nil { | ||
if err := validatePriceGranularity(t.MediaTypePriceGranularity.Native); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
var prevMax float64 = 0 | ||
for _, gr := range pg.Ranges { | ||
if gr.Max <= prevMax { | ||
return errors.New(`Price granularity error: range list must be ordered with increasing "max"`) | ||
} | ||
return nil | ||
} | ||
|
||
if gr.Increment <= 0.0 { | ||
return errors.New("Price granularity error: increment must be a nonzero positive number") | ||
} | ||
prevMax = gr.Max | ||
} | ||
func validatePriceGranularity(pg *openrtb_ext.PriceGranularity) error { | ||
if pg.Precision == nil { | ||
return errors.New("Price granularity error: precision is required") | ||
} else if *pg.Precision < 0 { | ||
return errors.New("Price granularity error: precision must be non-negative") | ||
} else if *pg.Precision > openrtb_ext.MaxDecimalFigures { | ||
return fmt.Errorf("Price granularity error: precision of more than %d significant figures is not supported", openrtb_ext.MaxDecimalFigures) | ||
} | ||
|
||
var prevMax float64 = 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a Go preference for how this should be initialized? e.g: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code was moved without modifications from |
||
for _, gr := range pg.Ranges { | ||
if gr.Max <= prevMax { | ||
return errors.New(`Price granularity error: range list must be ordered with increasing "max"`) | ||
} | ||
|
||
if gr.Increment <= 0.0 { | ||
return errors.New("Price granularity error: increment must be a nonzero positive number") | ||
} | ||
prevMax = gr.Max | ||
} | ||
return nil | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1863,60 +1863,172 @@ func TestValidateTargeting(t *testing.T) { | |
expectedError: nil, | ||
}, | ||
{ | ||
name: "price granularity empty", | ||
name: "price granularity ranges out of order", | ||
givenTargeting: &openrtb_ext.ExtRequestTargeting{ | ||
IncludeWinners: ptrutil.ToPtr(true), | ||
PriceGranularity: &openrtb_ext.PriceGranularity{}, | ||
IncludeWinners: ptrutil.ToPtr(true), | ||
PriceGranularity: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(2), | ||
Ranges: []openrtb_ext.GranularityRange{ | ||
{Min: 1.0, Max: 2.0, Increment: 0.2}, | ||
{Min: 0.0, Max: 1.0, Increment: 0.5}, | ||
}, | ||
}, | ||
}, | ||
expectedError: errors.New("Price granularity error: precision is required"), | ||
expectedError: errors.New(`Price granularity error: range list must be ordered with increasing "max"`), | ||
}, | ||
{ | ||
name: "price granularity negative", | ||
name: "media type price granularity video correct", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that we have separated out price granularity validation in a separate method, you think we can add another unit test specifically for logic in This allows detailed test cases to be separated out from higher level function test cases. Plus, the test cases in this test have become too large and this will help keep them to a manageable number as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! Some tests are gone due to duplicated tests with |
||
givenTargeting: &openrtb_ext.ExtRequestTargeting{ | ||
IncludeWinners: ptrutil.ToPtr(true), | ||
PriceGranularity: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(-1), | ||
MediaTypePriceGranularity: openrtb_ext.MediaTypePriceGranularity{ | ||
Video: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(2), | ||
Ranges: []openrtb_ext.GranularityRange{ | ||
{Min: 0.0, Max: 10.0, Increment: 1}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
expectedError: errors.New("Price granularity error: precision must be non-negative"), | ||
expectedError: nil, | ||
}, | ||
{ | ||
name: "price granularity greater than max", | ||
name: "media type price granularity banner correct", | ||
givenTargeting: &openrtb_ext.ExtRequestTargeting{ | ||
IncludeWinners: ptrutil.ToPtr(true), | ||
PriceGranularity: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(openrtb_ext.MaxDecimalFigures + 1), | ||
MediaTypePriceGranularity: openrtb_ext.MediaTypePriceGranularity{ | ||
Banner: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(2), | ||
Ranges: []openrtb_ext.GranularityRange{ | ||
{Min: 0.0, Max: 10.0, Increment: 1}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
expectedError: errors.New("Price granularity error: precision of more than 15 significant figures is not supported"), | ||
expectedError: nil, | ||
}, | ||
{ | ||
name: "price granularity ranges out of order", | ||
name: "media type price granularity native correct", | ||
givenTargeting: &openrtb_ext.ExtRequestTargeting{ | ||
IncludeWinners: ptrutil.ToPtr(true), | ||
PriceGranularity: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(2), | ||
Ranges: []openrtb_ext.GranularityRange{ | ||
{Min: 1.0, Max: 2.0, Increment: 0.2}, | ||
{Min: 0.0, Max: 1.0, Increment: 0.5}, | ||
MediaTypePriceGranularity: openrtb_ext.MediaTypePriceGranularity{ | ||
Native: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(2), | ||
Ranges: []openrtb_ext.GranularityRange{ | ||
{Min: 0.0, Max: 20.0, Increment: 1}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
expectedError: errors.New(`Price granularity error: range list must be ordered with increasing "max"`), | ||
expectedError: nil, | ||
}, | ||
{ | ||
name: "price granularity negative increment", | ||
name: "media type price granularity video and banner correct", | ||
givenTargeting: &openrtb_ext.ExtRequestTargeting{ | ||
IncludeWinners: ptrutil.ToPtr(true), | ||
PriceGranularity: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(2), | ||
Ranges: []openrtb_ext.GranularityRange{ | ||
{Min: 0.0, Max: 1.0, Increment: -0.1}, | ||
MediaTypePriceGranularity: openrtb_ext.MediaTypePriceGranularity{ | ||
Banner: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(2), | ||
Ranges: []openrtb_ext.GranularityRange{ | ||
{Min: 0.0, Max: 10.0, Increment: 1}, | ||
}, | ||
}, | ||
Video: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(2), | ||
Ranges: []openrtb_ext.GranularityRange{ | ||
{Min: 0.0, Max: 10.0, Increment: 1}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
expectedError: nil, | ||
}, | ||
{ | ||
name: "media type price granularity video incorrect", | ||
givenTargeting: &openrtb_ext.ExtRequestTargeting{ | ||
IncludeWinners: ptrutil.ToPtr(true), | ||
MediaTypePriceGranularity: openrtb_ext.MediaTypePriceGranularity{ | ||
Video: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(2), | ||
Ranges: []openrtb_ext.GranularityRange{ | ||
{Min: 0.0, Max: 10.0, Increment: -1}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
expectedError: errors.New("Price granularity error: increment must be a nonzero positive number"), | ||
}, | ||
{ | ||
name: "media type price granularity banner incorrect", | ||
givenTargeting: &openrtb_ext.ExtRequestTargeting{ | ||
IncludeWinners: ptrutil.ToPtr(true), | ||
MediaTypePriceGranularity: openrtb_ext.MediaTypePriceGranularity{ | ||
Banner: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(2), | ||
Ranges: []openrtb_ext.GranularityRange{ | ||
{Min: 0.0, Max: 0.0, Increment: 1}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
expectedError: errors.New("Price granularity error: range list must be ordered with increasing \"max\""), | ||
}, | ||
{ | ||
name: "media type price granularity native incorrect", | ||
givenTargeting: &openrtb_ext.ExtRequestTargeting{ | ||
IncludeWinners: ptrutil.ToPtr(true), | ||
MediaTypePriceGranularity: openrtb_ext.MediaTypePriceGranularity{ | ||
Native: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(2), | ||
Ranges: []openrtb_ext.GranularityRange{ | ||
{Min: 0.0, Max: 0.0, Increment: 1}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
expectedError: errors.New("Price granularity error: range list must be ordered with increasing \"max\""), | ||
}, | ||
{ | ||
name: "media type price granularity video correct and banner incorrect", | ||
givenTargeting: &openrtb_ext.ExtRequestTargeting{ | ||
IncludeWinners: ptrutil.ToPtr(true), | ||
MediaTypePriceGranularity: openrtb_ext.MediaTypePriceGranularity{ | ||
Banner: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(2), | ||
Ranges: []openrtb_ext.GranularityRange{ | ||
{Min: 0.0, Max: 10.0, Increment: -1}, | ||
}, | ||
}, | ||
Video: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(2), | ||
Ranges: []openrtb_ext.GranularityRange{ | ||
{Min: 0.0, Max: 0.0, Increment: 1}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
expectedError: errors.New("Price granularity error: range list must be ordered with increasing \"max\""), | ||
}, | ||
{ | ||
name: "media type price granularity native incorrect and banner correct", | ||
givenTargeting: &openrtb_ext.ExtRequestTargeting{ | ||
IncludeWinners: ptrutil.ToPtr(true), | ||
MediaTypePriceGranularity: openrtb_ext.MediaTypePriceGranularity{ | ||
Native: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(2), | ||
Ranges: []openrtb_ext.GranularityRange{ | ||
{Min: 0.0, Max: 10.0, Increment: -1}, | ||
}, | ||
}, | ||
Video: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(2), | ||
Ranges: []openrtb_ext.GranularityRange{ | ||
{Min: 0.0, Max: 0.0, Increment: 1}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
expectedError: errors.New("Price granularity error: range list must be ordered with increasing \"max\""), | ||
}, | ||
} | ||
|
||
for _, tc := range testCases { | ||
|
@@ -1926,6 +2038,80 @@ func TestValidateTargeting(t *testing.T) { | |
} | ||
} | ||
|
||
func TestValidatePriceGranularity(t *testing.T) { | ||
testCases := []struct { | ||
description string | ||
givenPriceGranularity *openrtb_ext.PriceGranularity | ||
expectedError error | ||
}{ | ||
{ | ||
description: "Precision is nil", | ||
givenPriceGranularity: &openrtb_ext.PriceGranularity{ | ||
Precision: nil, | ||
}, | ||
expectedError: errors.New("Price granularity error: precision is required"), | ||
}, | ||
{ | ||
description: "Precision is negative", | ||
givenPriceGranularity: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(-1), | ||
}, | ||
expectedError: errors.New("Price granularity error: precision must be non-negative"), | ||
}, | ||
{ | ||
description: "Precision is too big", | ||
givenPriceGranularity: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(20), | ||
}, | ||
expectedError: errors.New("Price granularity error: precision of more than 15 significant figures is not supported"), | ||
}, | ||
{ | ||
description: "price granularity ranges out of order", | ||
givenPriceGranularity: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(2), | ||
Ranges: []openrtb_ext.GranularityRange{ | ||
{Min: 1.0, Max: 2.0, Increment: 0.2}, | ||
{Min: 0.0, Max: 1.0, Increment: 0.5}, | ||
}, | ||
}, | ||
expectedError: errors.New(`Price granularity error: range list must be ordered with increasing "max"`), | ||
}, | ||
{ | ||
description: "price granularity negative increment", | ||
givenPriceGranularity: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(2), | ||
Ranges: []openrtb_ext.GranularityRange{ | ||
{Min: 0.0, Max: 1.0, Increment: -0.1}, | ||
}, | ||
}, | ||
expectedError: errors.New("Price granularity error: increment must be a nonzero positive number"), | ||
}, | ||
{ | ||
description: "price granularity correct", | ||
givenPriceGranularity: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(2), | ||
Ranges: []openrtb_ext.GranularityRange{ | ||
{Min: 0.0, Max: 10.0, Increment: 1}, | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for separating the test cases out. This looks great. Can you please just add one more test case where a valid precision value is present but ranges is empty? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added |
||
}, | ||
expectedError: nil, | ||
}, | ||
{ | ||
description: "price granularity with correct precision and ranges not specified", | ||
givenPriceGranularity: &openrtb_ext.PriceGranularity{ | ||
Precision: ptrutil.ToPtr(2), | ||
}, | ||
expectedError: nil, | ||
}, | ||
} | ||
|
||
for _, tc := range testCases { | ||
t.Run(tc.description, func(t *testing.T) { | ||
assert.Equal(t, tc.expectedError, validatePriceGranularity(tc.givenPriceGranularity)) | ||
}) | ||
} | ||
} | ||
|
||
func TestValidateOrFillChannel(t *testing.T) { | ||
testCases := []struct { | ||
description string | ||
|
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.
Will we ever get to a situation where this will be
nil
? Validate is called afterSetDefaults
and as part of set defaults we ensure that precision is set to the default value if not specified. See hereThere 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.
Discussed offline. No changes needed.