-
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
Reload Adapter: New #3812
Reload Adapter: New #3812
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.
@mafernandez80 Needs some changes before we can merge your adapter. Please address comments
modules/reloadBidAdapter.js
Outdated
|
||
export const spec = { | ||
code: BIDDER_CODE, | ||
aliases: [], |
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.
Please remove aliases property if you are not creating any alias
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.
Fixed
modules/reloadBidAdapter.js
Outdated
export const spec = { | ||
code: BIDDER_CODE, | ||
aliases: [], | ||
supportedMediaTypes: [BANNER], |
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.
Banner is default format. Please remove supportedMediaTypes
property if you are only supporting banner
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.
Fixed
modules/reloadBidAdapter.js
Outdated
*/ | ||
buildRequests: function (validBidRequests, bidderRequest) { | ||
|
||
var vRequests = []; |
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.
Use let/const where applicable. We recommend using ES6. Please update at other places as well
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.
Fixed
modules/reloadBidAdapter.js
Outdated
* @param {ServerResponse[]} serverResponses List of server's responses. | ||
* @return {UserSync[]} The user syncs which should be dropped. | ||
*/ | ||
getUserSyncs: function (syncOptions, serverResponses) { |
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.
Please remove if you are not using getUserSyncs
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.
Fixed
modules/reloadBidAdapter.js
Outdated
* Register bidder specific code, which will execute if bidder timed out after an auction | ||
* @param {data} Containing timeout specific data | ||
*/ | ||
onTimeout: function (data) {}, |
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.
Please remove if you are not using onTimeout
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.
Fixed
modules/reloadBidAdapter.js
Outdated
* Register bidder specific code, which will execute if a bid from this bidder won the auction | ||
* @param {Bid} The bid that won the auction | ||
*/ | ||
onBidWon: function (bid) {}, |
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.
Please remove if you are not using onBidWon
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.
Fixed
modules/reloadBidAdapter.js
Outdated
*/ | ||
onBidWon: function (bid) {}, | ||
|
||
onSetTargeting: function (bid) {} |
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.
Please remove if you are not using onSetTargeting
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.
Fixed
* @param {json} args | ||
*/ | ||
|
||
function ReloadClientTool(args) { |
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 does this function do ?
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.
It's a class constructor. It is used to instantiate an interface class that we must use to have a single standard across all our adapters/drivers.
modules/reloadBidAdapter.js
Outdated
|
||
var that = 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.
Lot of unnecessary line breaks throughout the file. Please remove them
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.
Fixed
@@ -0,0 +1,300 @@ | |||
import { expect } from 'chai'; | |||
import { spec } from 'modules/reloadBidAdapter'; | |||
import { newBidder } from 'src/adapters/bidderFactory'; |
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.
Remove unused imports. Following are not used in your test file anywhere
import { newBidder } from 'src/adapters/bidderFactory';
import * as bidderFactory from 'src/adapters/bidderFactory';
import { deepClone } from 'src/utils';
import { config } from 'src/config';
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.
Fixed
@mafernandez80 Circleci failed due to lint errors. You can fix and check on local by running |
@jaiminpanchal27 I executed your command but lint ran ok. |
Yes, same happened for me too. Actually on running gulp lint, it autofixex the files. So if you now do git status, it will show you changes to commit. |
@jaiminpanchal27 I fixed all lint errors |
@jaiminpanchal27 I see that there is a new release but our adapter has not been included. Is there anything still to be fixed? We really need this ASAP to start our tests. Thnx! |
@mafernandez80 We release every Tuesday. It will be included in next release. I understand your urgency but one of my colleague had already started work on the release since morning. I will test your params on hello world page and let you know if anything more is needed. Code looks good. |
@mafernandez80 I am not getting valid bid back from your adapter. Here is the response from your endpoint. I used the params given in reloadBidAdapter.md file on Prebid hello_world page.
|
@jaiminpanchal27 I pasted test parameters when I created this pull 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.
LGTM.
Verified bids back with the params provided in PR description. Please update your test params in |
@jaiminpanchal27 I updated test params with PR test. |
* Reload Adapter: New * Reload Adapter - Spec * Reload Adapter and Spec - Changes according comments * Reload Adapter & Spec: lint errors fixed * Reload Adapter: Example updated
Type of change
Description of change
contact email of the adapter’s maintainer: prebid@reload.net
official adapter submission
DOC: A link to a PR #1304