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.

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

Add DOOH Support #2580

Merged
merged 14 commits into from
Oct 27, 2023
Merged

Add DOOH Support #2580

merged 14 commits into from
Oct 27, 2023

Conversation

AntoxaAntoxic
Copy link
Collaborator

No description provided.

@AntoxaAntoxic
Copy link
Collaborator Author

According to the prebid/prebid-server#2532

}

return bidRequest.toBuilder()
// User was already prepared above
.user(bidderPrivacyResult.getUser())
.device(bidderPrivacyResult.getDevice())
.imp(prepareImps(bidder, imps, bidRequest, useFirstPartyData, context.getAccount()))
.app(preparedApp)
.site(preparedApp == null ? preparedSite : null)
.app(preparedApp).dooh(preparedDooh).site(preparedSite)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Each field should be on new line

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh, the class will have more than 2000 lines of codes and checkstyle will fail, should we make the threshold lower?

return bidRequest.getApp() != null ? MetricName.openrtb2app : MetricName.openrtb2web;
if (bidRequest.getApp() != null) {
return MetricName.openrtb2app;
} else if (bidRequest.getDooh() != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Super small but we expect to have 99.99% requests of site and app types
So please make if(site) before if(dooh)
Also I think it would be great if we will throw some exception by default, and have explicit checks for each mediaType before this default statement

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

imho an exception here gonna be duplicate the request validator logic

CTMBNara
CTMBNara previously approved these changes Oct 4, 2023
And1sS
And1sS previously approved these changes Oct 11, 2023
checkstyle.xml Outdated Show resolved Hide resolved
@AntoxaAntoxic AntoxaAntoxic linked an issue Oct 27, 2023 that may be closed by this pull request
@AntoxaAntoxic AntoxaAntoxic self-assigned this Oct 27, 2023
@SerhiiNahornyi SerhiiNahornyi merged commit 4a5eddf into master Oct 27, 2023
2 checks passed
@SerhiiNahornyi SerhiiNahornyi deleted the add-dooh-support branch October 27, 2023 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port PR from PBS-Go: DOOH support
6 participants