Add improvedigitalBidAdapter #1381

agregorio-improve · 2017-07-14T09:01:04Z

Type of change

Description of change

test parameters for validating bids

{
  bidder: 'improvedigital',
  params: {
    placementId:1053688
  }
},
{
  bidder: 'improvedigital',
  params: {
    placementId:1053689,
    keyValues: {
      testKey: ["testValue"]
    }
},
{
  bidder: 'improvedigital',
  params: {
    placementId:1053687,
    size: {
      w:300,
      h:300
    }
}

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

contact email of the adapter’s maintainer: hb@improvedigital.com
official adapter submission

Other information

harpere

@agregorio-improve before reviewing the adapter code itself - it needs unit tests. You can see examples in test/spec/modules/

derdeka · 2017-08-01T08:53:14Z

The length of the bidder name "improvedigital" is to long when using enableSendAllBids() with dfp (hb_size_improvedigital, hb_adid_improvedigital, hb_pb_improvedigital). The limit of dfp's key length is 20. Please use a shorter name or register an alias.

derdeka · 2017-08-02T09:36:09Z

modules/improvedigitalBidAdapter.js

+};
+
+ImproveDigitalAdapter.createNew = function () {
+    return new BeachfrontAdapter();


Where is BeachfrontAdapter defined? This should be ImproveDigitalAdapter, right?

This has been fixed to ImproveDigitalAdapter.

harpere · 2017-08-03T17:58:29Z

modules/improvedigitalBidAdapter.js

+
+var ImproveDigitalAdapter = function ImproveDigitalAdapter() {
+  var IMPROVE_DIGITAL_BIDDER_CODE = "improvedigital";
+  var baseAdapter = Adapter.createNew(IMPROVE_DIGITAL_BIDDER_CODE);


Adapter.createNew no longer exists. this should be var baseAdapter = new Adapter(IMPROVE_DIGITAL_BIDDER_CODE);

This has now been fixed in the latest version.

harpere · 2017-08-03T20:11:55Z

modules/improvedigitalBidAdapter.js

+    getNormalizedBidRequest: baseAdapter.getNormalizedBidRequest,
+    callBids: baseAdapter.callBids
+  };
+};


this will need change to (see #1459)

return Object.assign(this, { LIB_VERSION: LIB_VERSION, idClient: baseAdapter.idClient, getNormalizedBidRequest: baseAdapter.getNormalizedBidRequest, callBids: baseAdapter.callBids });

This has also been fixed in the latest version.

agregorio-improve · 2017-08-07T08:56:43Z

I have now added unit tests and made fixes as per your comments.

I have not set up an alias because it seems that the latest implementation of enableSendAllBids seems to truncate the keys to 20 characters. Is aliasing still required?

agregorio-improve · 2017-08-14T12:37:31Z

Hello,

Can you please take a look at and approve the latest changes?

Best Regards,
Alfredo

muuki88

A few marks regarding style. Otherwise this LGTM

muuki88 · 2017-08-16T07:53:08Z

modules/improvedigitalBidAdapter.js

@@ -0,0 +1,408 @@
+var LIB_VERSION_GLOBAL = '3.0.2';


Why not const?

muuki88 · 2017-08-16T07:54:06Z

modules/improvedigitalBidAdapter.js

+var adaptermanager = require('src/adaptermanager');
+
+const ImproveDigitalAdapter = function () {
+  var IMPROVE_DIGITAL_BIDDER_CODE = 'improvedigital';


I'll use const where ever possible. And let instead of var for the rest.

The bidder code is also used in the createNew function. Maybe extracting this into a const would be a good idea.

muuki88 · 2017-08-16T07:55:17Z

modules/improvedigitalBidAdapter.js

+      singleRequestMode: false,
+      httpRequestType: this.idClient.CONSTANTS.HTTP_REQUEST_TYPE.GET,
+      callback: CALLBACK_FUNCTION,
+      secure: (loc.protocol === 'https:') ? 1 : 0,


Why not true and false instead of 1 and 0?

muuki88 · 2017-08-16T07:55:42Z

modules/improvedigitalBidAdapter.js

+    );
+
+    if (request.errors && request.errors.length > 0) {
+      console.log('ID WARNING 0x01');


Does prebid.js not have any logging facilities?

src/utils has logWarn and logError functions

Changed the logging to utils.logError.

0 and 1 instead of true or false is a throwback to our internal protocol

muuki88 · 2017-08-16T07:58:18Z

modules/improvedigitalBidAdapter.js

+
+    var baseUrl = (requestParameters.secure === 1 ? 'https' : 'http') + '://' + this.CONSTANTS.AD_SERVER_BASE_URL + '/';
+
+    var bidRequestObject;


why not initialize here directly?

agregorio-improve · 2017-08-20T19:36:23Z

All concerns have been addressed and automated checks are passing. Please approve the pull request.

muuki88

LGTM

agregorio-improve · 2017-08-21T12:29:56Z

Hi,

I don't see the approval. Merge is still blocked. Can you please take a look?

Cheers.
Alfredo

muuki88 · 2017-08-21T17:13:42Z

@agregorio-improve I'm not a maintainer or contributor, so I can just give my approval, but not actually merge. @dbemiller or @harpere can do this.

mkendall07 · 2017-08-21T20:52:00Z

@agregorio-improve
There needs to some updates on this before we can merge. Thanks

agregorio-improve · 2017-08-23T15:15:16Z

All comments have been addressed. Please approve this merge request.

jaiminpanchal27

LGTM

agregorio-improve · 2017-08-24T15:25:56Z

Looks like @harpere did not approve my changes despite the fact that I have addressed all the issues specified. Can someone please remove this block?

agregorio-improve · 2017-08-24T15:34:25Z

Thank you @muuki88 and @jaiminpanchal27 but it looks like @harpere is still blocking this. Anything you can do?

agregorio-improve · 2017-08-25T09:05:07Z

Hi,

Can someone unblock this please?

Regards,
Alfredo

Re-lM · 2017-08-28T10:14:46Z

Hi all,
What is the status of this one? Can it be merged?
Thanks!

mkendall07

Few comments to address then this is mergeable.

mkendall07 · 2017-08-28T21:39:28Z

modules/improvedigitalBidAdapter.js

+const LIB_VERSION_GLOBAL = '3.0.5';
+
+var CONSTANTS = require('src/constants');
+var utils = require('src/utils');


we prefer let and const over var

mkendall07 · 2017-08-28T21:40:02Z

modules/improvedigitalBidAdapter.js

+  let LIB_VERSION = LIB_VERSION_GLOBAL;
+
+  // Ad server needs to implement JSONP using this function as the callback
+  let CALLBACK_FUNCTION = '$$PREBID_GLOBAL$$' + '.improveDigitalResponse';


FYI we won't allow JSONP callbacks with prebid 1.0.

We support CORS and will update our adapter when required by prebid 1.0.

mkendall07 · 2017-08-28T21:42:48Z

modules/improvedigitalBidAdapter.js

+          if (bidObject.id === bidRequest.bidId) {
+            let bid;
+            if (!bidObject.price || bidObject.price === null) {
+              bid = bidfactory.createBid(CONSTANTS.STATUS.NO_BID, bidRequest);


repeated code blocks should go into a function

mkendall07 · 2017-08-28T21:44:12Z

modules/improvedigitalBidAdapter.js

+
+            let nurl = '';
+            if (bidObject.nurl && bidObject.nurl.length > 0) {
+              nurl = '<img src=\"' + bidObject.nurl + '\" width=\"0\" height=\"0\" style=\"display:none\">';


prefer template literals over string concatenation. (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals)

old

mkendall07 · 2017-08-28T21:50:05Z

@agregorio-improve @Re-lM
let a few comments.

…templates instead of string concatenation

agregorio-improve · 2017-08-30T17:03:51Z

All outstanding issues have been addressed. Please provide feedback and/or approve merge.

mkendall07 · 2017-08-30T17:09:24Z

modules/improvedigitalBidAdapter.js

              let syncInd = syncElement.replace(/\//g, '\\\/');
-              syncString += '<img src=\\\"' + syncInd + '\\\"\/>';
+              syncString = `${syncString}${(syncString === '') ? 'document.writeln(\"' : ''}<img src=\\\"${syncInd}\\\" style=\\\"display:none\\\"\/>`;


You don't need to escape " anymore

* Add improvedigitalBidAdapter * Add unit tests. Miscellaneous Fixes * Code review changes * Second level CR changes * Fix possible undefined error * Replace var with let and const. Move duplicate code to function. Use templates instead of string concatenation

* tag '0.28.0' of https://github.com/prebid/Prebid.js: (27 commits) Prebid 0.28.0 Release Revert "Upgrade sinon to 3.x (prebid#1491)" (prebid#1563) add () for correct order of operations in scaling increments for currency (prebid#1559) AppnexusAst adapter update: Added source and version to request payload (prebid#1555) remove unnecessary spread operator (prebid#1561) Adxcg adapter (prebid#1554) Upgrade sinon to 3.x (prebid#1491) Rename vastPayload to vastXml (prebid#1556) Single-size sizes array now can be taken, too (prebid#1535) Updated the istanbul-instrumenter-loader (prebid#1550) Add AerServ Adapter (prebid#1538) Fixed imports and made adform support aliasing (prebid#1518) Custom granularity fix (prebid#1546) Fix `documentation lint` issues (prebid#1544) Yieldbot adunit bidder params slot name usage fix (prebid#1394) Update serverbid adapter to use smartsync (prebid#1324) Add improvedigitalBidAdapter (prebid#1381) Fix prebid#1533 spring server typo (prebid#1542) userSync is off by default (prebid#1543) currency module (prebid#1374) ...

* Add improvedigitalBidAdapter * Add unit tests. Miscellaneous Fixes * Code review changes * Second level CR changes * Fix possible undefined error * Replace var with let and const. Move duplicate code to function. Use templates instead of string concatenation

….28.0 to aolgithub-master * commit '4d9ade3df767750743f8888ed9efd6c77f8d0050': (26 commits) Added changelog entry. Added new aol partners ids. Prebid 0.28.0 Release Revert "Upgrade sinon to 3.x (prebid#1491)" (prebid#1563) add () for correct order of operations in scaling increments for currency (prebid#1559) AppnexusAst adapter update: Added source and version to request payload (prebid#1555) remove unnecessary spread operator (prebid#1561) Adxcg adapter (prebid#1554) Upgrade sinon to 3.x (prebid#1491) Rename vastPayload to vastXml (prebid#1556) Single-size sizes array now can be taken, too (prebid#1535) Updated the istanbul-instrumenter-loader (prebid#1550) Add AerServ Adapter (prebid#1538) Fixed imports and made adform support aliasing (prebid#1518) Custom granularity fix (prebid#1546) Fix `documentation lint` issues (prebid#1544) Yieldbot adunit bidder params slot name usage fix (prebid#1394) Update serverbid adapter to use smartsync (prebid#1324) Add improvedigitalBidAdapter (prebid#1381) Fix prebid#1533 spring server typo (prebid#1542) ...

* Add improvedigitalBidAdapter * Add unit tests. Miscellaneous Fixes * Code review changes * Second level CR changes * Fix possible undefined error * Replace var with let and const. Move duplicate code to function. Use templates instead of string concatenation

Add improvedigitalBidAdapter

724f584

matthewlane assigned ndhimehta Jul 18, 2017

ndhimehta assigned harpere Jul 18, 2017

matthewlane added the needs review label Jul 18, 2017

harpere previously requested changes Jul 28, 2017

View reviewed changes

derdeka reviewed Aug 2, 2017

View reviewed changes

harpere reviewed Aug 3, 2017

View reviewed changes

agregorio-improve added 2 commits August 4, 2017 16:03

Merge remote-tracking branch 'upstream/master'

26d321b

Add unit tests. Miscellaneous Fixes

058e653

Merge remote-tracking branch 'upstream/master'

39d91b1

agregorio-improve closed this Aug 9, 2017

agregorio-improve reopened this Aug 9, 2017

muuki88 approved these changes Aug 16, 2017

View reviewed changes

dbemiller unassigned ndhimehta Aug 16, 2017

agregorio-improve added 2 commits August 18, 2017 15:58

Code review changes

230f4cb

Merge branch 'master' of https://github.com/ImproveDigital/Prebid.js

fc82279

agregorio-improve closed this Aug 18, 2017

agregorio-improve reopened this Aug 18, 2017

Merge remote-tracking branch 'staging/master'

7b7e6e9

agregorio-improve closed this Aug 18, 2017

agregorio-improve reopened this Aug 18, 2017

Merge remote-tracking branch 'upstream/master'

8fd5f8d

muuki88 approved these changes Aug 21, 2017

View reviewed changes

jaiminpanchal27 added needs update and removed needs review labels Aug 21, 2017

jaiminpanchal27 self-assigned this Aug 21, 2017

agregorio-improve added 3 commits August 23, 2017 15:39

Second level CR changes

0f2b077

Merge branch 'master' of https://github.com/ImproveDigital/Prebid.js

bdfd63d

Fix possible undefined error

f4394f7

jaiminpanchal27 approved these changes Aug 24, 2017

View reviewed changes

jaiminpanchal27 added LGTM and removed needs update labels Aug 24, 2017

muuki88 approved these changes Aug 24, 2017

View reviewed changes

mkendall07 requested changes Aug 28, 2017

View reviewed changes

Replace var with let and const. Move duplicate code to function. Use …

b3932ea

…templates instead of string concatenation

mkendall07 reviewed Aug 30, 2017

View reviewed changes

mkendall07 approved these changes Aug 30, 2017

View reviewed changes

mkendall07 merged commit 65b34c8 into prebid:master Aug 30, 2017


		var baseUrl = (requestParameters.secure === 1 ? 'https' : 'http') + '://' + this.CONSTANTS.AD_SERVER_BASE_URL + '/';

		var bidRequestObject;

Add improvedigitalBidAdapter #1381

Add improvedigitalBidAdapter #1381

Conversation

agregorio-improve commented Jul 14, 2017

Type of change

Description of change

Other information

harpere left a comment • edited Loading

Choose a reason for hiding this comment

derdeka commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agregorio-improve commented Aug 7, 2017

agregorio-improve commented Aug 14, 2017

muuki88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agregorio-improve commented Aug 20, 2017

muuki88 left a comment

Choose a reason for hiding this comment

agregorio-improve commented Aug 21, 2017

muuki88 commented Aug 21, 2017

mkendall07 commented Aug 21, 2017

agregorio-improve commented Aug 23, 2017

jaiminpanchal27 left a comment

Choose a reason for hiding this comment

agregorio-improve commented Aug 24, 2017

agregorio-improve commented Aug 24, 2017

agregorio-improve commented Aug 25, 2017

Re-lM commented Aug 28, 2017

mkendall07 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkendall07 commented Aug 28, 2017

agregorio-improve commented Aug 30, 2017

Choose a reason for hiding this comment

harpere left a comment •

edited

Loading