-
Notifications
You must be signed in to change notification settings - Fork 748
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
New Adapter: RichAudience #2001
Conversation
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.
@richaudience Thank you for addressing my comments, I appreciate you cooperation and patience. I re-reviewed updates and added some more comments.
May I ask you to add a test where there are 2 or more imps in request? |
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.
I noticed you didn't comment a link to your docs PR? Do you mind posting a link to that and if you don't have a docs PR could you make one? Thanks!
Hi Alex, I send the pull request where you will see the documentation |
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.
I appreciate the extensive test coverage. The coded tests other than TestJsonSamples
and TestGetBuilder
are not necessary in our opinion. For code coverage, we only consider the json tests since we have extra protection and tests built into that framework. I consider TestResponseOK
particularly overkill, as one of the reasons we built the json test framework is to avoid this situation of walls of json hardcoded as strings in test code.
static/bidder-info/richaudience.yaml
Outdated
- banner | ||
userSync: | ||
iframe: | ||
url: "https://sync.richaudience.com/74889303289e27f327ad0c6de7be7264/?consentString={{.GDPRConsent}}&r={{.RedirectURL}}[PDID]" |
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.
What is the purpose of [PDID]
at the end of the url? This looks like it will cause the redirect url to be incorrectly formed.
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 next commit i have solved
For us this macro is correct, i will not change this
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.
This is not the correct way to specify the macro. It only works due to coincidental positioning of the fields of the default redirect url template . Please see: https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html#bidder-info
Please remove [PDID]
from the url and add it as a userMacro config:
url: "https://sync.richaudience.com/74889303289e27f327ad0c6de7be7264/?consentString={{.GDPRConsent}}&r={{.RedirectURL}}"
userMacro: "[PDID]"
Hi Thanks for your comments, I have push a new commit to apply the recommended changes Cheers |
Can you tell me why the compilation is failing in this point? In my tests it does not fail. |
That's a flaky test not related to your code. Don't worry about it. |
In documentation here https://github.com/prebid/prebid.github.io/blob/master/dev-docs/bidders/richaudience.md you mentioned: |
We are not video compatible for prebid server, but we will be soon. There is no need to generate confusion in the documentation. Cheers |
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.
LGTM. Thank you for addressing the comments
static/bidder-info/richaudience.yaml
Outdated
- banner | ||
userSync: | ||
iframe: | ||
url: "https://sync.richaudience.com/74889303289e27f327ad0c6de7be7264/?consentString={{.GDPRConsent}}&r={{.RedirectURL}}[PDID]" |
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.
This is not the correct way to specify the macro. It only works due to coincidental positioning of the fields of the default redirect url template . Please see: https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html#bidder-info
Please remove [PDID]
from the url and add it as a userMacro config:
url: "https://sync.richaudience.com/74889303289e27f327ad0c6de7be7264/?consentString={{.GDPRConsent}}&r={{.RedirectURL}}"
userMacro: "[PDID]"
everything is ok, what is missing so that you can upload the adapter? |
The changes look good. Please revisit my comments on the request data structure. You also need to fix the user macro configuration of the user sync in the bidder yaml file. |
Hi, I have modified the two requests you had, please check the code. Cheer |
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.
Thank you for your updates. The json test coverage looks good as does the code updates. I left a few nitpicks for misleading function names and a behavior change request for bid floor currency handling. The rest looks good to me.
I have applied the latest changes, please check. |
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.
Excellent. Thank you. The code looks good to me now. I have a few final comments after closely reviewing the tests.
adapters/richaudience/richaudiencetest/exemplary/single-banner-app.json
Outdated
Show resolved
Hide resolved
adapters/richaudience/richaudiencetest/exemplary/single-banner-deviceConfig.json
Show resolved
Hide resolved
adapters/richaudience/richaudiencetest/exemplary/single-banner-extUser.json
Show resolved
Hide resolved
adapters/richaudience/richaudiencetest/exemplary/single-banner-floorPrice.json
Show resolved
Hide resolved
adapters/richaudience/richaudiencetest/exemplary/single-banner-nosecure.json
Show resolved
Hide resolved
adapters/richaudience/richaudiencetest/exemplary/single-banner-sitePage.json
Show resolved
Hide resolved
adapters/richaudience/richaudiencetest/supplemental/pidNotORTB.json
Outdated
Show resolved
Hide resolved
adapters/richaudience/richaudiencetest/supplemental/requestWrongRequest.json
Outdated
Show resolved
Hide resolved
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.
LGTM
Co-authored-by: sgimenez <sergi.gimenez@richaudience.com>
Co-authored-by: sgimenez <sergi.gimenez@richaudience.com>
Co-authored-by: sgimenez <sergi.gimenez@richaudience.com>
New Adapter: RichAudience