AdSmartx Bid Adapter : New Bidder Adapter#14559
Conversation
Update Master Branch From Upstream Master
…s-adapter-smart-exchange Rm 1476 prebid js adapter smart exchange
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4fe9eae4c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Adds a new AdSmartX bidder adapter and introduces shared ORTB conversion/response/sync utilities, also refactoring the existing RiseMediaTech adapter to use those utilities.
Changes:
- Added
adsmartxbidder adapter (module + in-repo markdown doc). - Added shared helper library
libraries/adsmartxUtils/bidderUtils.js(converter, request builder, response interpreter, user sync builder). - Updated/added unit tests for
adsmartxand adjusted one RiseMediaTech response expectation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
modules/adsmartxBidAdapter.js |
New AdSmartX bidder adapter wired to shared bidderUtils helpers. |
modules/adsmartxBidAdapter.md |
New adapter documentation (currently missing bid params table). |
libraries/adsmartxUtils/bidderUtils.js |
New shared ORTB converter/request/response/sync helpers used by adapters. |
modules/risemediatechBidAdapter.js |
Refactored to use adsmartxUtils helpers; behavior changes in consent mapping and user sync return. |
test/spec/modules/adsmartxBidAdapter_spec.js |
New AdSmartX adapter tests (currently contains JS syntax errors). |
test/spec/libraries/adsmartxUtils/bidderUtils_spec.js |
New unit tests for shared bidderUtils helpers. |
test/spec/modules/risemediatechBidAdapter_spec.js |
Adjusted expectation for unknown mtype to default to banner. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…-adapter-smart-exchange
…s-adapter-smart-exchange RM-1476 : Handled prebid js PR review comments
|
Whoa there partner! This project is migrating to typescript. Consider changing the new JS files to TS, with well-defined types for what interacts with the prebid public API (for example: bid params and configuration). Thanks!
|
|
Hi @jsnellbaker & @ncolletti , |
Coverage Report for CI Build 24393716418Coverage increased (+0.01%) to 96.354%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats💛 - Coveralls |
|
@pritishmd-talentica I checked over the new adapter and it seems ok. I ran a test for it using the hello_world page and it returned a bid. However when I double-checked the risemediatech bid adapter (since it was also updated), I saw the ad server request failed. Per the browser's console tab, the prebid debugger indicated the request timed out after 1000ms - so there was no response at all. Below are the request details. Can you please take a look? url: payload: |
|
@jsnellbaker We have discontinued the old adapter and we have no plans going forward to support or use it. |
|
@jsnellbaker if everything looks fine, can you approve |
|
My concern is that some publishers will just auto-update and things will break for them. This PR (as is) would be included in a minor release of Prebid.js, so there's some level of expectation that adapters would continue to work if they were to do a minor release update. I'm not sure about this approach of just letting it be potentially broken. @patmmccann could you help take a look at the above? |
|
is this pr actively breaking risemediatech? it works in master? |
|
@patmmccann , Risemediatech Adapter is not being used by any publishers as of now. Moreover, the ad server endpoint has been discontinued and hence even in Master branch, it will not work. CC : @jsnellbaker |
|
@jsnellbaker @patmmccann Can you please review and approve |
If this is indeed dead can you have the adapter reflect that? It should simply print out a warning that it is dead and not try and make bid requests |
| * @param {Object} bid - The bid request object. | ||
| * @returns {boolean} True if the bid request is valid. | ||
| */ | ||
| const isBidRequestValid = (bid) => { |
There was a problem hiding this comment.
i guess maybe if you just always return false and a logWarn here you will be good?
There was a problem hiding this comment.
I have done the needful. Please check
| request.cur = [DEFAULT_CURRENCY]; | ||
| request.tmax = bidderRequest.timeout; | ||
| request.test = bidderRequest.test || 0; | ||
| const isBidRequestValid = validateBidRequest; |
There was a problem hiding this comment.
you said this is always false in the comments?
Update forked master
|
Whoa there partner! This project is migrating to typescript. Consider changing the new JS files to TS, with well-defined types for what interacts with the prebid public API (for example: bid params and configuration). Thanks!
|
…bid adapter is deprecated
…s-adapter-smart-exchange RM-1476 : Commented that risemediatech adapter is deprecated
|
Whoa there partner! This project is migrating to typescript. Consider changing the new JS files to TS, with well-defined types for what interacts with the prebid public API (for example: bid params and configuration). Thanks!
|
…s-adapter-smart-exchange RM-1476 : Updated unit tests
|
Whoa there partner! This project is migrating to typescript. Consider changing the new JS files to TS, with well-defined types for what interacts with the prebid public API (for example: bid params and configuration). Thanks!
|
Type of change
Bugfix
Feature
[ x] New bidder adapter
[ x] Updated bidder adapter RiseMediaTech Bidder Adapter
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Does this change affect user-facing APIs or examples documented on http://prebid.org?
Other
Description of change
Added a bidder adapter for AdSmartX
Maintainer : prebid@aidigital.com
Sample Ad Unit : Banner
Sample Ad Unit : Video
Other information
Documentation PR:
prebid/prebid.github.io#6467