Skip to content
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 adfinity adapter #4591

Merged
merged 7 commits into from
Jan 9, 2020
Merged

New adfinity adapter #4591

merged 7 commits into from
Jan 9, 2020

Conversation

adfinity-prebid
Copy link
Contributor

Type of change

  • New bidder adapter
  • test parameters for validating bids
{
  bidder: 'afinity',
                        params: {
                            placement_id: 0,
                            traffic: 'banner'
                        }
}

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @adfinity-prebid

Thanks for submitting this adapter. Overall everything seems okay, there are a few points that need to be addressed.

  • It seems that the response (for the test banner at least) was not gzip encoded. Can you please make this update?
  • Are there any sample test params for video and native? If you're going to support these formats, I'd like to test them out through the adapter to ensure the end-to-end workflow is working as expected.

Thanks!

}
},
bids: [{
bidder: 'afinity',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small typo in the bidder name here. Should be adfinity as per the BIDDER_CODE in the .js file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added test params for video and native into .md file. For testing just change traffic type.
About gzip, is this mandatory in current release?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the test params, I'll take a look.

The compression of the response is required; this can be done with gzip or other similar settings in the content-encoding param (such as deflate or compress, if those are more preferable).

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @adfinity-prebid

I ran some tests on the video and native params and have some additional feedback. Can you take a look when you get the chance?

}
},
{
bidder: 'afinity',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the typo cropped up again. :) Can you update these two references?

}
},
{
bidder: 'afinity',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

Comment on lines 29 to 42
{
bidder: 'afinity',
params: {
placement_id: 0,
traffic: 'video'
}
},
{
bidder: 'afinity',
params: {
placement_id: 0,
traffic: 'native'
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you restructure these test params into separate adUnits with their own mediaTypes? I assume you're supporting instream video only? I can't really confirm.

The same applies to native, in terms of which assets you support.

Comment on lines +98 to +101
let resItem = serverResponse[i];
if (isBidResponseValid(resItem)) {
response.push(resItem);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This current code doesn't really support native bids, at least based on the test native params that were provided.

When creating a native bid for Prebid, there are certain fields that need to be defined to properly identify all the normal native assets (which are used in the rendering process). Can you update the logic here to add this support?

The following page provides some additional information if needed:
http://prebid.org/dev-docs/bidder-adaptor.html#supporting-native

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, help me to understend, what prebid adapter shoukd get in response from SSP, currently, using test params we return such object
{ mediaType: 'native', native:{ clickUrl: "", title: 'Test', image: { url: '', width: 226, height: 158 }, impressionTrackers: [] }, ttl: 120, cpm: 0.4, requestId: placement['bidId'], creativeId: '2', netRevenue: true, currency: 'USD', dealId: '1' }
This response contains minimal data to render native ad: clickUrl, impTrackers, image and title. This should be enough to test integration. To apply assets, our users stores them into ssp user UI

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So on the whole, that list you provided should be sufficient in the minimal terms. If you're not going to add the code in the adapter to set the values for the bidResponse, you'll have to ensure any other potential native assets that may be used are setup properly in the SSP response that your endpoint returns.

If the values aren't formatted correctly, there's a chance that Prebid will reject that native bid as not being setup properly. Please let me know if that makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Building response to Prebid adapter on backend side i'm using construction to set prebid assets
like this:
if (asset.data) { switch (asset.data.type) { case 1: hbResponse.sponsoredBy = asset.data.value; break; case 2: hbResponse.body = asset.data.value; break; case 3: hbResponse.rating = asset.data.value; break; case 4: hbResponse.likes = asset.data.value; break; case 5: hbResponse.downloads = asset.data.value; break; case 6: hbResponse.price = asset.data.value; break; case 7: hbResponse.salePrice = asset.data.value; break; case 8: hbResponse.phone = asset.data.value; break; case 9: hbResponse.address = asset.data.value; break; case 10: hbResponse.body2 = asset.data.value; break; case 11: hbResponse.displayUrl = asset.data.value; break; case 12: hbResponse.cta = asset.data.value; break; default: hbResponse.body = asset.data.value; } } else if (asset.img) { switch (asset.img.type) { case 2: hbResponse.icon = { url: asset.img.url, width: asset.img.w, height: asset.img.h }; break; case 3: hbResponse.image = { url: asset.img.url, width: asset.img.w, height: asset.img.h }; break; default: hbResponse.image = { url: asset.img.url, width: asset.img.w, height: asset.img.h }; } }

@jsnellbaker
Copy link
Collaborator

Hi @adfinity-prebid
When you get the chance, can you take a look at the unit tests and ensure they're setup correctly? There's one that's failing for me atm when I try to run the tests when I checkout this PR locally.

Below is a copy of the error for reference:

AdfinityAdapter
    buildRequests
      ✗ Returns valid data if array of bids is valid
	expected false to be a number or a date
	Error
	   at AssertionError (webpack:///node_modules/assertion-error/index.js:74 <- test/test_index.js:40669:7)
	   at Anonymous function (webpack:///node_modules/chai/lib/chai/core/assertions.js:1584 <- test/test_index.js:50965:7)
	   at methodWrapper (webpack:///node_modules/chai/lib/chai/utils/addMethod.js:57 <- test/test_index.js:48481:5)
	   at Anonymous function (webpack:///test/spec/modules/adfinityBidAdapter_spec.js:81:6 <- test/test_index.js:70440:7)

@adfinity-prebid
Copy link
Contributor Author

Thanks, i've fixed this one. For soe reason, whe i'm running gulp test localy - outpu show only this
`[10:31:45] Using gulpfile /commonuser/Documents/ADFINITYPrebid/gulpfile.js
[10:31:45] Starting 'test'...
[10:31:45] Starting 'clean'...
[10:31:45] Finished 'clean' after 42 ms
[10:31:45] Starting 'lint'...
[10:32:06]
/commonuser/Documents/ADFINITYPrebid/modules/criteoBidAdapter.js
8:1 error import "criteo-direct-rsa-validate/build/verify" cannot be resolved prebid/validate-imports

✖ 1 problem (1 error, 0 warnings)

[10:32:06] 'lint' errored after 21 s
[10:32:06] ESLintError in plugin "gulp-eslint"
Message:
Failed with 1 error
Details:
domainEmitter: [object Object]
domain: [object Object]
domainThrown: false

[10:32:06] 'test' errored after 21 s`

@jsnellbaker
Copy link
Collaborator

@adfinity-prebid You may want to try and reinstall the npm packages for your local copy of prebid.js. It looks like the package is missing. Can you give this a try?

@adfinity-prebid
Copy link
Contributor Author

Yep, this worked. For now output looks good
[10:14:53] Using gulpfile ~/Documents/ADFINITYPrebid/gulpfile.js
[10:14:53] Starting 'test'...
[10:14:53] Starting 'clean'...
[10:14:53] Finished 'clean' after 35 ms
[10:14:53] Starting 'lint'...
[10:15:13] Finished 'lint' after 20 s
[10:15:13] Starting 'test'...

START:
ℹ 「wdm」:
ℹ 「wdm」: Compiled successfully.
ℹ 「wdm」: Compiling...
ℹ 「wdm」:
ℹ 「wdm」: Compiled successfully.
09 01 2020 10:15:49.408:INFO [karma-server]: Karma v3.1.4 server started at http://0.0.0.0:9876/
09 01 2020 10:15:49.433:INFO [launcher]: Launching browsers ChromeHeadless with concurrency unlimited
09 01 2020 10:15:49.463:INFO [launcher]: Starting browser ChromeHeadless
09 01 2020 10:15:51.248:INFO [HeadlessChrome 79.0.3945 (Linux 0.0.0)]: Connected on socket vtgGq_ORparbtM5pAAAA with id 38590072
09 01 2020 10:15:52.526:WARN [web-server]: 404: /vuble_renderer.js
09 01 2020 10:15:54.662:WARN [web-server]: 404: /nurl&s=0.66
09 01 2020 10:15:54.663:WARN [web-server]: 404: /burl&s=0.66
09 01 2020 10:15:55.291:WARN [web-server]: 404: /scriptUrl.com
LOG: 'entrance', '15:55'

Finished in 5.805 secs / 3.195 secs @ 10:15:58 GMT+0200 (Eastern European Standard Time)

SUMMARY:
✔ 3768 tests completed
ℹ 2 tests skipped
[10:15:58] Finished 'test' after 45 s
[10:15:58] Finished 'test' after 1.08 min

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adfinity-prebid
Thanks for the update. LGTM

@jsnellbaker jsnellbaker merged commit f7f79c3 into prebid:master Jan 9, 2020
redaguermas added a commit to redaguermas/Prebid.js that referenced this pull request Jan 9, 2020
…idVersion1.2.0

* 'master' of https://github.com/prebid/Prebid.js:
  New adfinity adapter (prebid#4591)
  add segmento bid adapter (prebid#4660)
  ozone 2.2.0 adapter updates (prebid#4676)
audiencerun pushed a commit to audiencerun/Prebid.js that referenced this pull request Jan 20, 2020
* New adfinity adapter

* Update adfinityBidAdapter.md

* add test params

* native response validattion

* fix secure param

Co-authored-by: Check your git settings! <chris@chris-laptop>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants