New Adapter: bidglass by dliebner · Pull Request #3861 · prebid/Prebid.js

dliebner · 2019-05-27T23:53:43Z

Type of change

Bugfix
Feature
[x ] New 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 the bidglass adapter + tests

test parameters for validating bids

{
  bidder: 'bidglass',
  params: {
    'adUnitId': '-1'
  }
}

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

dliebner@gmail.com
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

bretg · 2019-05-28T18:20:34Z

Docs PR prebid/prebid.github.io#1328

sumit116

Hi,
Thanks for submitting your adaptor. I tested your adaptor on hello-world page and found functionality working fine. But you haven't added any definition to functions like getUserSyncs(), onTimeout(), onBidWon() and onSetTargeting(). Would you want to add something to them, may be just a log.

Also, the unit test coverage is slightly below the required criteria of 80% (its 79.xx %), can you add a few more tests for the same.

Can you please make the minor changes I have mentioned, and let me know if you have any questions.

Thanks.

sumit116 · 2019-06-12T12:20:57Z

modules/bidglassBidAdapter.js

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


the formal params are missing here. Can you please add some code, may be just a log in function definition.

Thanks for taking the time to review our adapter! We've made the updates as requested. One question - is there a gulp (or other) command to check the coverage for an individual bidder's unit tests?

A note regarding the empty function definitions - we don't plan on using getUserSyncs, onTimeout, onBidWon, or onSetTargeting to any practical effect in the near future.

Thanks for the update. You can use gulp test-coverage, and then gulp view-coverage to view unit test-coverage of any adaptor file. The view command will open coverage on http://localhost:1999/, where you can navigate to your file.

- Added formal params to getUserSyncs function definition - getUserSyncs now always returns an array - Improved unit test coverage

sumit116 · 2019-06-13T09:11:55Z

Tested the adaptor on hello_world page and all unit tests have passed. Test coverage of the adaptor is 80%. The changes are working well against all parameters listed in https://github.com/prebid/Prebid.js/blob/master/PR_REVIEW.md, hence approving the PR.

jaiminpanchal27

@dliebner Please see comments.

jaiminpanchal27 · 2019-06-13T17:26:02Z

modules/bidglassBidAdapter.js

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


getUserSyncs, onTimeout, onBidWon and onSetTargeting are all optional. You can remove from the spec if you do not plan to use these features

Got it, thanks.

jaiminpanchal27 · 2019-06-13T17:27:19Z

modules/bidglassBidAdapter.js

+    */
+
+    let imps = [];
+    let getReferer = function() {


We already pass this information in bidderRequest. Check http://prebid.org/dev-docs/bidder-adaptor.html#referrers

Our rules for determining and reporting referer and origins vary slightly from prebid's built-in methods. We'd prefer to use our own methods for consistency's sake across our platform.

jaiminpanchal27 · 2019-06-13T17:32:37Z

modules/bidglassBidAdapter.js

+      bidResponses.push({
+        requestId: bid.requestId,
+        cpm: parseFloat(bid.cpm),
+        width: parseInt(bid.width),


parseInt takes two parameters string and radix. I would suggest to add radix as it does not default to 10 and may return unexpected results.
More info here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt and interesting article on parseInt https://medium.com/dailyjs/parseint-mystery-7c4368ef7b21

Thanks, good call.

jaiminpanchal27 · 2019-06-13T17:32:47Z

modules/bidglassBidAdapter.js

+   * @return {Bid[]} An array of bids which were nested inside the server.
+   */
+  interpretResponse: function(serverResponse) {
+    // const serverBody  = serverResponse.body;


remove dead code

Will do. Commit pending.

- Removed unused methods: getUserSyncs, onTimeout, onBidWon, onSetTargeting - Removed getUserSyncs unit test - Removed "dead code" - Removed some unnecessary comments - Fixed usage of parseInt

dliebner · 2019-06-25T17:33:57Z

Hey, is there anything else you need from us here?

* Added bidglass adapter + test * PR Review Updates: - Added formal params to getUserSyncs function definition - getUserSyncs now always returns an array - Improved unit test coverage * PR Review Updates: - Removed unused methods: getUserSyncs, onTimeout, onBidWon, onSetTargeting - Removed getUserSyncs unit test - Removed "dead code" - Removed some unnecessary comments - Fixed usage of parseInt

Added bidglass adapter + test

d241df7

jaiminpanchal27 assigned sumit116 Jun 3, 2019

sumit116 suggested changes Jun 12, 2019

View reviewed changes

sumit116 added the needs update label Jun 12, 2019

PR Review Updates:

ca16df0

- Added formal params to getUserSyncs function definition - getUserSyncs now always returns an array - Improved unit test coverage

sumit116 approved these changes Jun 13, 2019

View reviewed changes

sumit116 added LGTM and removed needs update labels Jun 13, 2019

sumit116 requested review from jaiminpanchal27 and sumit116 and removed request for sumit116 June 13, 2019 09:12

jaiminpanchal27 requested changes Jun 13, 2019

View reviewed changes

PR Review Updates:

c890091

- Removed unused methods: getUserSyncs, onTimeout, onBidWon, onSetTargeting - Removed getUserSyncs unit test - Removed "dead code" - Removed some unnecessary comments - Fixed usage of parseInt

jaiminpanchal27 approved these changes Jun 25, 2019

View reviewed changes

jaiminpanchal27 merged commit 9d2f06c into prebid:master Jun 25, 2019

Conversation

dliebner commented May 27, 2019

Type of change

Description of change

Other information

Uh oh!

bretg commented May 28, 2019

Uh oh!

sumit116 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sumit116 commented Jun 13, 2019

Uh oh!

jaiminpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dliebner commented Jun 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

sumit116 left a comment •

edited

Loading