Add bucksense Adapter #3785

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.

Sign up for GitHub

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

Jump to bottom

Merged

jsnellbaker merged 5 commits into prebid:master from bkse-stefanodechicchis:master

May 7, 2019

Contributor

bkse-stefanodechicchis commented Apr 26, 2019

Type of change

[X ] New bidder adapter

Description of change

This is first release for Bucksense Adapter

test parameters for validating bids

{
  bidder: 'bucksense',
  params: {
    placementId : 1000
  }
}

Be sure to test the integration with your adserver using the Hello World sample page.

contact email of the adapter’s maintainer ([email protected])
[ X] official adapter submission

For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:

A link to a PR on the docs repo at https://github.com/prebid/prebid.github.io/

Other information

stefanodechicchis added 2 commits

April 26, 2019 13:51


          Add bucksense Adapter

9421e12


          fixed first revision for new Bucksense adapter approval

317d082

https://circleci.com/gh/prebid/Prebid.js/2365?

Collaborator

bretg commented Apr 29, 2019

docs PR prebid/prebid.github.io#1284

bretg requested a review from jsnellbaker

April 29, 2019 17:03

bretg assigned jsnellbaker

jsnellbaker requested changes

View reviewed changes

Collaborator

jsnellbaker left a comment

Hi @bkse-stefanodechicchis

Thanks for submitting this adapter. I took a look through the changes, and there are a few things that need to be addressed. Please take at the notes below when you have the chance and let me know if you have any questions. Thanks!

General point:

The code coverage for the unit tests only reach 56.45%. For adapters, we'd want this to be at least 80% coverage. Can you add some more unit tests to fill the gaps? You can use the gulp test-coverage and gulp view-coverage in your prebid project to review the missing areas. Alternatively - you could remove some the empty functions from your adapter; this could help reduce the coverage gap (see comments below for the specific areas related to this point).

modules/bucksenseBidAdapter.js Outdated

		@@ -0,0 +1,171 @@
		import { registerBidder } from 'src/adapters/bidderFactory';
		import { BANNER } from 'src/mediaTypes';

Collaborator

jsnellbaker Apr 29, 2019

In lieu of this PR #3435, any imported files within a modules file needs to use the relative style path.

Can you please update these imports accordingly?

Contributor Author

bkse-stefanodechicchis Apr 30, 2019

fixed with new Push

modules/bucksenseBidAdapter.js Outdated

+                 * @return boolean True if this is a valid bid, and false otherwise.
+                */
+                isBidRequestValid: function (bid) {
+                  if (isNaN(bid)) {

Collaborator

jsnellbaker Apr 29, 2019

The bid param is an object at this stage of the code, so this if statement check will always return true and cause the bidRequest for your bidder to be considered invalid.

Can you review this logic and adjust it accordingly?

Contributor Author

bkse-stefanodechicchis Apr 30, 2019

fixed with new Push

modules/bucksenseBidAdapter.js Outdated

+                      data: {
+                        'pub_id': location.host,
+                        'pl_id': bid.params.placementId,
+                        'sys_href': encodeURI(location.href),

Collaborator

jsnellbaker Apr 29, 2019

If you haven't already looked at it, there are a set of data points you can use in your adapter to get information on the webpage loading the adapter. Please see the link below and consider using them if they would fit your needs:
http://prebid.org/dev-docs/bidder-adaptor.html#referrers

Contributor Author

bkse-stefanodechicchis Apr 30, 2019

Thanks for point to this but in this release we would like to keep using the location object if it is not a problem

modules/bucksenseBidAdapter.js Outdated

+                * @param {ServerResponse[]} serverResponses List of server's responses.
+                * @return {UserSync[]} The user syncs which should be dropped.
+                */
+                getUserSyncs: function(syncOptions, serverResponses) {

Collaborator

jsnellbaker Apr 29, 2019

Is this code still a work-in-progress?

If not and you're not planning to support userSyncs (at least at this time) - you can just remove this function entirely from your adapter.

Contributor Author

bkse-stefanodechicchis Apr 30, 2019

function not needed on this release, we removed.

modules/bucksenseBidAdapter.js Outdated

+                 * @param ...
+                 * @return ...
+                */
+                onTimeout: function(timeoutData) {

Collaborator

jsnellbaker Apr 29, 2019

Similar to the getUserSyncs function - you can remove this function if you're not planning to use it.

Contributor Author

bkse-stefanodechicchis Apr 30, 2019

function not needed on this release, we removed.

modules/bucksenseBidAdapter.js Outdated

+                 * @param {object} bid The bid to validate.
+                 * @return ...
+                */
+                onBidWon: function(bid) {

Collaborator

jsnellbaker Apr 29, 2019

Similar to the getUserSyncs function - you can remove this function if you're not planning to use it.

Contributor Author

bkse-stefanodechicchis Apr 30, 2019

function not needed on this release, we removed.

jsnellbaker reviewed

View reviewed changes

modules/bucksenseBidAdapter.js Outdated

+              const WHOIS = 'BKSHBID-008';
+              const BIDDER_CODE = 'bucksense';
+              const URL = 'https://prebid.bksn.se:445/prebid';

Collaborator

jsnellbaker Apr 29, 2019

Is this the correct endpoint URL for your adapter?

When I tried to test the adapter, the POST requests to this URL were resulting in a 301 Moved Permanently response.

Contributor Author

bkse-stefanodechicchis Apr 30, 2019

Yes request is doing a 301 but then got redirect to the script that return the BID. I have tested the prebid on the Hello World example. Let me know if you do not see the response at all from your test.

jsnellbaker added needs unit tests needs update not receiving bids labels

stefanodechicchis added 2 commits

April 30, 2019 13:49


          fixed second revision for new Bucksense adapter approval from jsnellb…

fc1d4e4

…aker

prebid#3785


          fixed third revision for new Bucksense adapter approval.

48e4ccb

prebid#3785

Collaborator

jsnellbaker commented May 1, 2019

Hi @bkse-stefanodechicchis, Thanks for making the updates.

I retested the adapter and was now able to see the bid return and render successfully.

However, when I reran the coverage tests - the adapter is still below the ideal; it's at 72% currently.
Can you put together some additional unit tests to fill in the gaps?

Also can you update the various console.log statements in the adapter file to use theutils.logInfo() function instead? This function would categorize/brand the messages in the console that they're part of Prebid and would be more ideal to use.

We should be good to merge once these last few items are addressed. Thanks.

jsnellbaker removed the not receiving bids label

Contributor Author

bkse-stefanodechicchis commented May 3, 2019

I'm struggling to increase the percentage of testing.

My specs file already test all the 4 functions that are in the Adapter, so I thought I would add more scenarios to check in each function, but if II added some more and I don't see the % going up, so I think I'm in the wrong direction.
I appreciate any suggestions/help on what I'm doing wrong
Thanks

Collaborator

jsnellbaker commented May 6, 2019

Hi @bkse-stefanodechicchis

Just to clarify, you are looking at your specific adapter's coverage rate correct? It's inside the modules sub-folder in that initial screen. See below for example:

From what I can see (currently with what has been committed), the outstanding areas all relate to the bksdebug param.

If you setup a pair of tests where that field is enabled (one for requestBids and one for interpretResponse) and swap out the console.log with utils.logInfo - you could create a stub of the utils.logInfo function and run your checks against that stub (ie check for the called property is true for example). These new tests should help fill the gap.


          fixed 4th revision from jsnellbaker

236bd35

- removed console.log and using utils.logInfo();
- removed bksdebug params;

pending issue:
- gulp test-coverage still under 80%

Contributor Author

bkse-stefanodechicchis commented May 6, 2019

I just commit a new version, I removed the console.log and use utils.logInfo.
I'm not able to reach the 80% coverage I also removed the bksdebug param and added some more use case but the % just not moving up.

I look at the file from the gulp view-coverage

and I'm try to figure out why some IF or ELSE get a flag

Collaborator

jsnellbaker commented May 7, 2019

Hi @bkse-stefanodechicchis I checked out your updates to the adapter test cases. I'm seeing your adapter coverage at 94% now (see below):

Are you not seeing the same thing? If you're happy with how your adapter is handling certain scenarios (this is in reference to your last comment on the if/else statements in the interpretResponse), we should be okay to merge the PR.

Please let me know when you have the chance.

Contributor Author

bkse-stefanodechicchis commented May 7, 2019

Hi, I get confuse I was looking at column "Branches" and the % was low there.
Anyway we are good in our side, so ready for the merge from our point of view.
Thanks

jsnellbaker approved these changes

View reviewed changes

jsnellbaker added LGTM and removed needs unit tests needs update labels

jsnellbaker merged commit 10ec9a5 into prebid:master

jacekburys-quantcast pushed a commit to jacekburys-quantcast/Prebid.js that referenced this pull request


          Add bucksense Adapter (prebid#3785)

2fdfb51

* Add bucksense Adapter

* fixed first revision for new Bucksense adapter approval
https://circleci.com/gh/prebid/Prebid.js/2365?

* fixed second revision for new Bucksense adapter approval from jsnellbaker
prebid#3785

* fixed third revision for new Bucksense adapter approval.
prebid#3785

* fixed 4th revision from jsnellbaker
- removed console.log and using utils.logInfo();
- removed bksdebug params;

pending issue:
- gulp test-coverage still under 80%

VideoReach pushed a commit to VideoReach/Prebid.js that referenced this pull request


          Add bucksense Adapter (prebid#3785)

0f95574

* Add bucksense Adapter

* fixed first revision for new Bucksense adapter approval
https://circleci.com/gh/prebid/Prebid.js/2365?

* fixed second revision for new Bucksense adapter approval from jsnellbaker
prebid#3785

* fixed third revision for new Bucksense adapter approval.
prebid#3785

* fixed 4th revision from jsnellbaker
- removed console.log and using utils.logInfo();
- removed bksdebug params;

pending issue:
- gulp test-coverage still under 80%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM