-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Teqblaze Library: Add ORTB2 device data to request payload #12073
Teqblaze Library: Add ORTB2 device data to request payload #12073
Conversation
Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:
Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀 |
Fields such as |
Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:
Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀 |
Hi @MaksymTeqBlaze! Thanks for your comment. The oRTB While it is definitely safer to assume that each adapter should decide for itself whether to pass this data - it is quite tedious to add the same code to each individual adapter, also would result in a lot of code duplication, thus it is much easier to add on the common library level and should benefit every adapter. The major bidder adapters s.a. If you still think this is harmful to individual adapters - perhaps we could consider adding a common function on the library level and some light-weight mechanism for the dependent adapters to opt-in this behavior of passing the full |
Hi @MaksymTeqBlaze! What are you thoughts on the above? Thx |
As fyi, both of you are in the prebid slack |
Thanks Patrick, just contacted @MaksymTeqBlaze on Slack. Maksym, please check my message, thanks. |
Yeah, we can add device to request |
Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:
Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀 |
@MaksymTeqBlaze thanks for confirming that this is ok with you guys. The test failing is unrelated to this PR. We thus removed Draft status and set as Ready for review. @patmmccann thanks for stimulating the communication on this, what would be the best process to get this PR approved and merged? |
@justadreamer pubrise test failure sure looks related to me |
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.
Test failures do appear related to me
Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:
Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀 |
Hi @patmmccann, It turns out I missed some errors in the tests because new bidders were switched to Teqblaze, and I wasn't aware of them. Thanks for the heads up! I've fixed the tests, and the CI/CD is passing. I think it can be merged now. Thanks! |
Type of change
Description of change
This PR enhances bid requests formed within Teqblaze library by incorporating device data from the global ORTB2 object.
The device object has previously been populated by simplistic parsers, if at all, and was inaccurate as a result. Prebid now benefits from RTD modules such as 51Degrees that enrich all the device object fields including Apple iPhone model category and device ID. The PR enables users Teqblaze-dependent bid adapters to benefit from the device object.
Affected bid adapters:
Other information
cc: @MaksymTeqBlaze, @teqblaze, @qt-io