-
Notifications
You must be signed in to change notification settings - Fork 181
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
DMX: Enforcing w and h in imp #1245
Conversation
bc57a15
to
5f66aa2
Compare
5f66aa2
to
661b1c1
Compare
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.
Pls rearrange methods in call order
return app == null | ||
? null | ||
: app.toBuilder() | ||
.id(StringUtils.isNotBlank(app.getId()) ? app.getId() : device.getIfa()) |
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.
Can device here be null?
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.
If device
is null, then validateRequest
method would throw an PreBidException
. I'll add unit test to cover this case
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.
As I see validateRequest will not throw ex with some userValues, and null device can be passed forward
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.
In real world, we expect that device being null
is kinda impossible because it holds the info about physical device through which user is interacting with website/app. Anyway, I'll add device == null
check to validateRequest
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.
As I know, we have stored requests and AMP, which as I know can not hold any user information
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.
Interesting. Seems we can't have null device because of Ortb2ImplicitParametersResolver.populateDevice()
661b1c1
to
02dc60a
Compare
02dc60a
to
307e144
Compare
I see. Completely agree. I've tried to rearrange them and keep the abstraction level |
307e144
to
8b2a46f
Compare
return app == null | ||
? null | ||
: app.toBuilder() | ||
.id(StringUtils.isNotBlank(app.getId()) ? app.getId() : device.getIfa()) |
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.
Interesting. Seems we can't have null device because of Ortb2ImplicitParametersResolver.populateDevice()
1b2c4f3
to
10f342d
Compare
10f342d
to
41f4dbc
Compare
prebid/prebid-server#1778