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

Initial Consumable Bidder Adapter commit #2381

Merged

snapwich merged 4 commits into prebid:master from naffis:consumable-adapter

Apr 26, 2018

Contributor

naffis commented Apr 11, 2018

Type of change

[x ] New bidder adapter

Description of change

Adding Consumable's bidder adapter for Prebid.js.

test parameters for validating bids

{
  bidder: 'consumable',
  params: {
    placement: '1234567',
    network: '9599.1',
    unitId: '987654',
    unitName: 'unitname',
    zoneId: '9599.1',
  }
}

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

done
contact email of the adapter’s maintainer: naffis@consumable.com
official adapter submission

Other information

Link to the PR in the docs repo: Adding Consumable bidder dev docs prebid.github.io#703


          Initial Consumable Bidder Adapter commit

218076e

Member

mkendall07 commented Apr 11, 2018

This pull request introduces 1 alert when merging 218076e into cea9243 - view on lgtm.com

new alerts:

1 for Identical operands

Comment posted by lgtm.com


          fix tests and lint errors for Consumable bidder adapter and specs

5c2f90a

Member

mkendall07 commented Apr 11, 2018

This pull request introduces 1 alert when merging 5c2f90a into cea9243 - view on lgtm.com

new alerts:

1 for Identical operands

Comment posted by lgtm.com

snapwich assigned ndhimehta

ndhimehta assigned snapwich

mkendall07 unassigned ndhimehta

mkendall07 reviewed

View reviewed changes

modules/consumableBidAdapter.js Outdated

+                      let tagName = item.match(tagNameRegExp)[0];
+                      let url = item.match(srcRegExp)[2];
+                      if (tagName && tagName) {

Member

mkendall07 Apr 16, 2018

believe this should be if (tagName && url)

Contributor Author

naffis Apr 18, 2018

Thanks for catching this. Fixed.

snapwich requested changes

View reviewed changes

modules/consumableBidAdapter.js Outdated

+                buildRequests: function (bids) {
+                  return bids.map(bid => {
+                    return formatBidRequest(bid);
+                  });

Collaborator

snapwich Apr 16, 2018

I'd remove the extra function call, these three lines are equivalent to return bids.map(formatBidRequest);

Contributor Author

naffis Apr 18, 2018

Changed

modules/consumableBidAdapter.js Outdated

+                return pubapiTemplate({
+                  host: server,
+                  network: params.network,

Collaborator

snapwich Apr 16, 2018

shouldn't mutate the params object, just network: params.network || '10947.1' here should work. Same with bidRequest.network below. Might want to use a named constant rather than a magic number if doing more than once.

Contributor Author

naffis Apr 18, 2018

Thanks for the feedback. Now using a constant and no longer mutating the params object as suggested.

modules/consumableBidAdapter.js Outdated

+                  });
+                  return result.join('');
+                };
+              }

Collaborator

snapwich Apr 16, 2018

This is a lot of code for a simple templated function. May I suggest something like

const pubapiTemplate = ({host, network, placement, alias}) => `//${host}/pubapi/3.0/${network}/${placement}/0/0/ADTECH;v=2;cmd=bid;cors=yes;alias=${alias};misc=${new Date().getTime()}`

which could replace template, isInteger, and pubapiTemplate above.

Contributor Author

naffis Apr 18, 2018

You're right. Simplified based on your suggestions.

modules/consumableBidAdapter.js Outdated

+              }
+              function parsePixelItems(pixels) {
+                let itemsRegExp = /(img|iframe)[\s\S]*?src\s*=\s*("|')(.*?)\2/gi;

Collaborator

snapwich Apr 16, 2018

[\s\S] is all whitespace and all non-whitespace, which i'm pretty sure is equivalent to .

that's going to give you more than you're hoping for, like for the test string you have below <script>document.write('<img src="img.org"></iframe><iframe src="pixels1.org"></iframe>');</script> gives you iframe><iframe src="pixels1.org" as a match which is obviously not correct including the previous tag (and whatever might be between) and could cause errors.

Contributor Author

naffis Apr 18, 2018

The issue turned out to be matching on the tag name without the opening bracket. The regex should work now for the example given and other test cases.

modules/consumableBidAdapter.js Outdated

+                return pubapiTemplate({
+                  host: server,
+                  network: params.network,
+                  placement: parseInt(params.placement),

Collaborator

snapwich Apr 16, 2018

You should always specify the radix when using parseInt, or use parseFloat if applicable

Contributor Author

naffis Apr 18, 2018

Radix is now supplied.

modules/consumableBidAdapter.js Outdated

+                  host: server,
+                  network: params.network,
+                  placement: parseInt(params.placement),
+                  misc: new Date().getTime() // cache busting

Collaborator

snapwich Apr 16, 2018

this should just be handled by your templating function

Contributor Author

naffis Apr 18, 2018

Moved to the templating function.

modules/consumableBidAdapter.js Outdated

+                bidRequest = {
+                  url: _buildConsumableUrl(bid),
+                  method: 'GET',
+                  ttl: CONSUMABLE_TTL

Collaborator

snapwich Apr 16, 2018

there's no reason to return this for the bid request, the bidderFactory won't use it. I see you use it in interpretResponse but you can just use CONSUMABLE_TTL directly as it's a constant and you don't seem to be modifying it in the body of interpretResponse

Contributor Author

naffis Apr 18, 2018

Removed and using the constant directly as suggested.

modules/consumableBidAdapter.js Outdated

+                    'catch(e){continue;}' +
+                    '}}</script>';
+                },
+                _parseBidResponse: function (response, bidRequest) {

Collaborator

snapwich Apr 16, 2018

You shouldn't expose this, it's a private method. Also, using a private method (regular function scoped to this module, in this case) allows the minify to mangle the name resulting in smaller code.

Contributor Author

naffis Apr 18, 2018

Moved this method so it's no longer exposed.

modules/consumableBidAdapter.js Outdated

+                    }
+                  }
+                },
+                _formatPixels: function (pixels) {

Collaborator

snapwich Apr 16, 2018

This should not be exposed either. You can test it as part of interpretResponse

Contributor Author

naffis Apr 18, 2018

Moved this method so it's no longer exposed.

modules/consumableBidAdapter.js Outdated

+                _parseBidResponse: function (response, bidRequest) {
+                  let bidData;
+                  try {
+                    bidData = response.seatbid[0].bid[0];

Collaborator

snapwich Apr 16, 2018

If your endpoint is returning arrays (which it appears to be by this code) I suggest handling the responses as arrays w/ forEach or map (if you're returning the results directly) rather than picking off the first bid and bailing early if not present. interpretResponse allows you to return multiple bids. Even if you only want to return one bid for now, a forEach or map implementation will still behave correctly (as you have here) but will also be forward compatible should you decide to do multiple bids in the future.

Contributor Author

naffis Apr 18, 2018

Good suggestion. I'd like to wait on this to roll it into a larger refactor. It should work fine as-is as we're only concerned with the first bid in the array returned.


          Merge branch 'master' into consumable-adapter

5fd65d7

Member

mkendall07 commented Apr 18, 2018

This pull request introduces 1 alert when merging 5fd65d7 into 2e27a33 - view on lgtm.com

new alerts:

1 for Identical operands

Comment posted by lgtm.com


          changes based on reviews by @snapwich and @mkendall07

0adbd90

Contributor Author

naffis commented Apr 19, 2018 •

edited

Loading

Please let me know if everything looks ok now after the latest round of edits based on the reviews. We have a number of publishers who are eager for the adapter. Thank you.

Contributor Author

naffis commented Apr 24, 2018

Checking in again. Any updates on this?

mkendall07 approved these changes

View reviewed changes

snapwich approved these changes

View reviewed changes

Collaborator

snapwich left a comment

LGTM

snapwich merged commit bb9bc18 into prebid:master

naffis deleted the consumable-adapter branch

April 26, 2018 16:30

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

Labels

None yet