agma: bugfixes #3495
agma: bugfixes #3495
Conversation
steffenmllr
changed the title
|
@AntoxaAntoxic Is there any chance this can be included in the next release? The bugs, especially the issue with the config, are preventing our members from using your port. If there is anything I can do, please let me know |
|
@Compile-Ninja When will be the next release? |
src/main/java/org/prebid/server/analytics/reporter/agma/AgmaAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/spring/config/AnalyticsConfiguration.java
Outdated
Show resolved
Hide resolved
|
@AntoxaAntoxic thanks for the review, hope I've addressed everything. If there is anything more I can do, please let me know |
| @@ -200,7 +206,9 @@ private static String getPublisherId(BidRequest bidRequest) { | |||
| return null; | |||
| } | |||
|
|
|||
| return publisherId; | |||
| return appSiteId != null | |||
| ? String.format("%s_%s", StringUtils.defaultString(publisherId, ""), appSiteId) | |||
There was a problem hiding this comment.
the second param is redundant, use just StringUtils.defaultString(publisherId)
also if it's a blank publisherId it will return _appSiteId value, imho just looks weird, but maybe it's fine for you
There was a problem hiding this comment.
_appSiteId is used when the publisherId is not set, this is not optimal, but 1:1 how the golang config works since some users don't configure the publishers in their system. (https://github.com/prebid/prebid-server-java/pull/3495/files#diff-9a3b09aae9225494e90113eb6184a4a888e0ed043127af1d267e95c85eea5bfaR481)
I've updated to StringUtils.defaultString(publisherId)
Thanks again for your patience!
| account -> { | ||
| final String publisherId = account.getPublisherId(); | ||
| final String siteAppId = account.getSiteAppId(); | ||
| return StringUtils.isNotBlank(siteAppId) | ||
| ? String.format("%s_%s", publisherId, siteAppId) | ||
| : publisherId; | ||
| }, |
There was a problem hiding this comment.
Please, extract to separate method.
| if (!toFlush.isEmpty()) { | ||
| sendEvents(toFlush); | ||
| } |
There was a problem hiding this comment.
Please, move this check to sendEvents method.
|
@And1sS Thanks for the view, please let me know if I need to change more |
🔧 Type of changes
✨ What's the context?
Thanks @AntoxaAntoxic for the port of our adapter! During testing we found 2 minor bugs in the implementation-
publisherId. original testThanks again for the port!