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

Jump to bottom

33Across: Enable Support for SRA requests #2079

Merged
merged 34 commits into from Nov 18, 2021
Merged

33Across: Enable Support for SRA requests #2079

merged 34 commits into from Nov 18, 2021

Conversation

curlyblueeagle
Copy link
Contributor

In this PR we introduce SRA and group together Ad Units that have the same parameters under the same Bid Request in order to minimize http calls to our end point.

curlyblueeagle and others added 30 commits October 28, 2020 18:08
@curlyblueeagle
add video support
cfb4cea
@curlyblueeagle
renamed folder for tests per code review from Prebid
b37978e
@curlyblueeagle
updated path to renamed test folder
e248833
@curlyblueeagle
Added more unit tests
820fb30
@curlyblueeagle
Merge pull request #1 from 33Across/add_test_coverage
193c42f 
Added more unit tests
@curlyblueeagle
Added a newline between structs
67e04cd
@curlyblueeagle
breaking up multi-imps
8725d45
@curlyblueeagle
Added tests for multi-imp
88dfe5e
@curlyblueeagle
Adding caller info
a29a0f7
@curlyblueeagle
We do not support app
0b02576
@curlyblueeagle
fixed typo in debug messages
662c02d
@curlyblueeagle
removed confusing comment
9a6a4e0
@curlyblueeagle
removed outdated comments
a14bb1b
@curlyblueeagle
Merge pull request #2 from 33Across/handle_multi-imp_requests
8f35484 
Handle multi imp requests
@curlyblueeagle
Merge branch 'master' of https://github.com/prebid/prebid-server
5f20987
@curlyblueeagle
Code review feedback
7ac08dd
@curlyblueeagle
Refactored creating request ext
79bf68b
@curlyblueeagle
More optimizations based on code review
29f98fd
@curlyblueeagle
Fixed comments
3827313
@curlyblueeagle
made better comments
171a13d
@curlyblueeagle
fixed endpoint to s2s
bc77e9a
@curlyblueeagle
Merge pull request #3 from 33Across/s2s_endpoint_fix
a0dd8b5 
fixed endpoint to s2s
@curlyblueeagle
Merge branch 'master' of https://github.com/prebid/prebid-server
11103a3
@curlyblueeagle
Merge branch 'master' of https://github.com/33Across/prebid-server
242c792
@curlyblueeagle
Merge branch 'master' of https://github.com/prebid/prebid-server
3a9fd07
@curlyblueeagle
SRA in make requests
8f002cb
@curlyblueeagle
fixed tests to include SRA support
64e3946
@curlyblueeagle
added more test cases for grouping imps
a9dafaf
@curlyblueeagle
update user sync url
eaa6479
@curlyblueeagle
testing groups by prod as well
41acf9b
AlexBVolcy
AlexBVolcy reviewed Nov 11, 2021
View reviewed changes
Copy link
Contributor

@AlexBVolcy AlexBVolcy left a comment

Choose a reason for hiding this comment

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

The code to update to support SRA looks good, but I want to bring to your attention that TestJsonSamples is failing with a test case from this path, 33acrosstest/exemplary/multi-imp-banner.json. Could you address this fail case?

@curlyblueeagle
Copy link
Contributor Author

The code to update to support SRA looks good, but I want to bring to your attention that TestJsonSamples is failing with a test case from this path, 33acrosstest/exemplary/multi-imp-banner.json. Could you address this fail case?

@AlexBVolcy tests seem to be running fine on my local and in our org's pipeline. Also it looks like the order of http reqs made of the Ad Requests matters in tests, so multiple requests would result in expectedRequest to not always have the same order and therefore the tests to flake. Do you have any suggestions on how I can check for the presence of requests in an agnostic manner?

@AlexBVolcy
Copy link
Contributor

@curlyblueeagle I'll have an answer for what to do about this tomorrow!

@AlexBVolcy
Copy link
Contributor

AlexBVolcy commented Nov 16, 2021 •
edited

@curlyblueeagle So after some investigating, the reason for this test failure is on our end, due to a shortcoming around the JSON Test Framework with not supporting independent assertions.

So while we work on updating this framework, we'll need you to remove the test that is failing from this PR. We'll verify locally that this test is working properly, and we can approve this PR.

Once we update the test framework, you can then open a new PR to add this test back in.

Let me know if you have any other questions regarding this!

@guscarreon
Copy link
Contributor

@curlyblueeagle pull request #2087 is meant to correct the false negatives our testing framework throws under certain MakeRequests() outputs. As soon as this newly posted PR gets approved and merged, your changes will pass validation (tested your changes locally with the test framework fix). Will let you know as soon as your fix gets merged

@curlyblueeagle
Copy link
Contributor Author

@AlexBVolcy @guscarreon sorry I just removed the test that triggered the fail. Should I be waiting for the testing framework PR to be merged instead?

@guscarreon
Copy link
Contributor

@AlexBVolcy @guscarreon sorry I just removed the test that triggered the fail. Should I be waiting for the testing framework PR to be merged instead?

No @curlyblueeagle, we'll add your test case to #2087 instead. Is that ok?

@guscarreon
Copy link
Contributor

@curlyblueeagle We've added your test case to #2087, it currently fails because this PR has not been merged yet. @AlexBVolcy and @VeronikaSolovei9 will continue to review your PR.

@mansinahar
Copy link
Contributor

@curlyblueeagle Can you please review the test added to #2087 for 33across to test this behavior and make sure it looks good?

@curlyblueeagle
Copy link
Contributor Author

@mansinahar reviewed and looks good

@mansinahar mansinahar merged commit 5e2d890 into prebid:master Nov 18, 2021
wwwyyy pushed a commit to wwwyyy/prebid-server that referenced this pull request Dec 20, 2021
Merge branch 'master' of https://github.com/wwwyyy/prebid-server
f1615d3 
* 'master' of https://github.com/wwwyyy/prebid-server: (23 commits)
  Add GPID Reserved Bidder Name (prebid#2107)
  Marsmedia adapter - param zoneId can get string and int (prebid#2105)
  Algorix: add Server Region Support (prebid#2096)
  New Adapter: Bizzclick (prebid#2056)
  Collect Prometheus Go process metrics (prebid#2101)
  Added channel support (prebid#2037)
  Rubicon: update fpd fields resolving (prebid#2091)
  Beachfront - Currency conversion (prebid#2078)
  New Adapter: Groupm (Alias Of PubMatic) (prebid#2042)
  Adform adapter lacked gross/net parameter support (prebid#2084)
  add iframe sync for openx (prebid#2094)
  changes to the correct user sync url (prebid#2092)
  Smaato: Add instl to tests (prebid#2089)
  Correct MakeRequests() output assertion in adapter JSON tests (prebid#2087)
  Adview: adapter currency fix. (prebid#2060)
  New Adapter: Coinzilla (prebid#2074)
  Syncer Level ExternalURL Override (prebid#2072)
  33Across: Enable Support for SRA requests (prebid#2079)
  Currency conversion on adapter JSON tests (prebid#2071)
  New Adapter: VideoByte (prebid#2058)
  ...
jorgeluisrocha pushed a commit to jwplayer/prebid-server that referenced this pull request Sep 28, 2022
@curlyblueeagle @jorgeluisrocha
33Across: Enable Support for SRA requests (prebid#2079)
7a4ddb0
shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022
@curlyblueeagle @shunj-nb
33Across: Enable Support for SRA requests (prebid#2079)
2212512
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VeronikaSolovei9 VeronikaSolovei9 VeronikaSolovei9 approved these changes

@AlexBVolcy AlexBVolcy AlexBVolcy approved these changes

Assignees

@VeronikaSolovei9 VeronikaSolovei9

@AlexBVolcy AlexBVolcy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@curlyblueeagle @AlexBVolcy @guscarreon @mansinahar @VeronikaSolovei9