Events prebid bid extension was added #298
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
schernysh
suggested changes
Mar 5, 2019
| @@ -1053,13 +1063,24 @@ private BidResponse toBidResponseWithCacheInfo(List<BidderResponse> bidderRespon | |||
| .build(); | |||
| } | |||
|
|
|||
| private String getPublisherId(Site site, App app) { | |||
There was a problem hiding this comment.
There is already a method publisherId for doing exactly the same. Also is it possible to pass publisherId that is calculated in holdAuction method?
| @@ -93,6 +95,9 @@ | |||
| private static final String PREBID_EXT = "prebid"; | |||
| private static final String CACHE = "cache"; | |||
| private static final String DEFAULT_CURRENCY = "USD"; | |||
| private static final String EVENT_CALLBACK_URL_PATTERN = "%s/event?type=%s&bidid=%s&bidder=%s"; | |||
There was a problem hiding this comment.
What about extracting event construction logic (even though it's tiny) to a separate service?
|
|
||
| @Test | ||
| public void shouldNotAddExtPrebidEventsWhenAccountsEnabledDoesNotContainPublisherId() { | ||
| // given |
| @@ -135,3 +135,4 @@ analytics.log.enabled=true | |||
| gdpr.host-vendor-id=1 | |||
| gdpr.vendorlist.filesystem-cache-dir=src/test/resources/org/prebid/server/ApplicationTest/gdpr-vendorlist | |||
| gdpr.geolocation.maxmind.db-archive-path=/org/prebid/server/geolocation/GeoLite2-test.tar.gz | |||
| events.accounts-enabled=1001 | |||
There was a problem hiding this comment.
Let's improve one of the /openrtb2/auction and /openrtb2/amp test in AppliationTest to verify functionality implemented
schernysh
suggested changes
Mar 7, 2019
| @@ -173,14 +174,16 @@ public void setUp() { | |||
| metricsContext = MetricsContext.of(MetricName.openrtb2web); | |||
|
|
|||
| exchangeService = new ExchangeService(bidderCatalog, httpBidderRequester, responseBidValidator, cacheService, | |||
| bidResponsePostProcessor, currencyService, gdprService, metrics, clock, false, 0); | |||
| bidResponsePostProcessor, currencyService, gdprService, new EventsService(null, "http://external.org"), | |||
There was a problem hiding this comment.
Let's not use real EventsService in this class at all in favor of a mock
schernysh
approved these changes
Mar 7, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.