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

Adding Orbitsoft adapter #1378

Merged
merged 13 commits into from
Sep 15, 2017
Merged

Adding Orbitsoft adapter #1378

merged 13 commits into from
Sep 15, 2017

Conversation

Indra-sensei
Copy link
Contributor

@Indra-sensei Indra-sensei commented Jul 13, 2017

Type of change

  • New bidder adapter

Description of change

New Orbitsoft bidder

  • test parameters for validating bids
{
  bidder: 'orbitsoft',
  params: {
    placementId: '142',
    requestUrl: '//adserver.com/ads/show/hb' // Url to perform search request
  }
}

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

Other information

let bids = params.bids || [];

if (bids.length === 0) {
return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this conditional is kind of pointless. if bids.length === 0 then the for loop below won't iterate and the function will return anyways

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

// @if NODE_ENV='debug'
utils.logMessage('No param requestUrl');
// @endif
return null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If there are errors creating the bid request, you should return a NO_BID to prebid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed (on line 47)

cpm: 0
};

pbjs._bidsRequested.push(bidderRequest);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't need to mess with Prebid's private vars. This has the ability to affect other tests, plus it's a private var so it could possible it could change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this is done by push method, which not has affect other tests, and this is exactly what is recommended by the official document at http://prebid.org/dev-docs/testing-prebid.html
And this technique is used for most other adapter tests for prebid.js

@mkendall07
Copy link
Member

@snapwich what's the status on this?

callBids: baseAdapter.callBids,
setBidderCode: baseAdapter.setBidderCode,
buildJPTCall: buildJPTCall
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

In order for your adapter to be alias-able the .constructor reference on your adapter instances needs to point to the correct constructor function; by returning a new object that reference will be incorrect.

You can solve this with:

return Obect.assign(this, {
     callBids: baseAdapter.callBids,
     setBidderCode: baseAdapter.setBidderCode,
     buildJPTCall: buildJPTCall
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@snapwich Fixed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, you need to modify it to be the way I described (return this). You can look at another adapter if you wish to get an example, but the modifications you made look like the old alias pattern. createNew is no longer needed, but the constructor functions must return a reference to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@snapwich Sorry for this. I made changes.

};

adaptermanager.registerBidAdapter(new OrbitsoftAdapter(), ORBITSOFT_BIDDERCODE);
adaptermanager.aliasBidAdapter('orbitsoft', 'orbitadserving');
Copy link
Member

Choose a reason for hiding this comment

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

why so many aliases?

Copy link
Contributor Author

@Indra-sensei Indra-sensei Sep 15, 2017

Choose a reason for hiding this comment

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

@mkendall07 We rebranded recently from Orbitscripts with Orbit Ad Serving product to Orbitsoft.
If too much, I can delete.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is a bit confusing to have so many. Typically if the code changes we put the alias in to help with transition but since this is a new adapter I don't think you need them. You should use your current preferred name only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mkendall07 It is fair remark. Deleted it.

for (let i = 0; i < bids.length; i++) {
let bidRequest = bids[i];
let callbackId = bidRequest.bidId;
let jptCall = buildJPTCall(bidRequest, callbackId);
Copy link
Member

Choose a reason for hiding this comment

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

I see you are hitting the AppNexus endpoint. Is there a reason you cannot use an AppNexus alias instead of creating your own adapter?

Copy link
Contributor Author

@Indra-sensei Indra-sensei Sep 15, 2017

Choose a reason for hiding this comment

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

@mkendall07 AppNexus has built-in enpoint URL for bid requesting. Our adapter sends bid request to various endpoint URL where customers install our product. Endpoint URL is defined by the requestUrl parameter and different for diffrent customers.
We also have additional options for requesting bids and ads.

Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

LGTM

@mkendall07 mkendall07 merged commit d05af09 into prebid:master Sep 15, 2017
outoftime pushed a commit to Genius/Prebid.js that referenced this pull request Sep 18, 2017
…built

* 'master' of https://github.com/prebid/Prebid.js: (46 commits)
  Serverbid alias (prebid#1560)
  Add user sync to process for approving adapter PRs (prebid#1457)
  fix travis build (prebid#1595)
  Rubicon project improvement/user sync (prebid#1549)
  Adding Orbitsoft adapter (prebid#1378)
  Fix renderer test for new validation rule (prebid#1592)
  Allow SET_TARGETING to be used in AnalyticsAdapter (prebid#1577)
  Add support for video stream context (prebid#1483)
  Invalidate bid if matching bid request not found (prebid#1575)
  allow adapters to set default adserverTargeting for specific bid (prebid#1568)
  Custom granularity precision should honor 0 if it is passed in closes prebid#1479 (prebid#1591)
  BaseAdapter for the Prebid 0.x -> 1.x transition  (prebid#1494)
  Add a version to the Criteo adapter (prebid#1573)
  Allow bundling from node.js or with new gulp task bundle-to-stdout  (prebid#1570)
  Add url.parse option to not decode the whole URL (prebid#1480)
  Tremor Video Bid Adapter (prebid#1552)
  Yieldmo bid adapter (prebid#1415)
  Switch `gulp docs` to build its output using documentation.js (prebid#1545)
  Increment pre version
  Prebid 0.28.0 Release
  ...
jbAdyoulike pushed a commit to jbAdyoulike/Prebid.js that referenced this pull request Sep 21, 2017
* Adding Orbitsoft module

* Adding Orbitsoft module (corrected)

* Adding Orbitsoft module (correction of remarks)

* Adding Orbitsoft module (correction of remarks)

* Adding Orbitsoft module (correction to alias-able)

* Adding Orbitsoft module (correction to alias-able)

* Adding Orbitsoft module (correction to alias-able)

* Adding Orbitsoft module (correction to alias-able)

* Adding Orbitsoft module (correction to new constructor)

* Adding Orbitsoft module (delete unnecessary aliases)

* Adding Orbitsoft module (delete unnecessary aliases)
outoftime pushed a commit to Genius/Prebid.js that referenced this pull request Sep 21, 2017
* tag '0.29.0' of https://github.com/prebid/Prebid.js: (29 commits)
  Prebid 0.29.0 Release
  Fix for not syncing bidders.  (prebid#1598)
  fix amp example pages (prebid#1597)
  closes prebid#1298 (prebid#1583)
  Fixed the broken tests. (prebid#1602)
  Rubicon Bidder Factory (prebid#1587)
  Trustx adapter (prebid#1488)
  Add nurl to markup (prebid#1601)
  Pass bidRequest to createBid (prebid#1600)
  Add Kumma adapter (prebid#1512)
  Serverbid alias (prebid#1560)
  Add user sync to process for approving adapter PRs (prebid#1457)
  fix travis build (prebid#1595)
  Rubicon project improvement/user sync (prebid#1549)
  Adding Orbitsoft adapter (prebid#1378)
  Fix renderer test for new validation rule (prebid#1592)
  Allow SET_TARGETING to be used in AnalyticsAdapter (prebid#1577)
  Add support for video stream context (prebid#1483)
  Invalidate bid if matching bid request not found (prebid#1575)
  allow adapters to set default adserverTargeting for specific bid (prebid#1568)
  ...
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
* Adding Orbitsoft module

* Adding Orbitsoft module (corrected)

* Adding Orbitsoft module (correction of remarks)

* Adding Orbitsoft module (correction of remarks)

* Adding Orbitsoft module (correction to alias-able)

* Adding Orbitsoft module (correction to alias-able)

* Adding Orbitsoft module (correction to alias-able)

* Adding Orbitsoft module (correction to alias-able)

* Adding Orbitsoft module (correction to new constructor)

* Adding Orbitsoft module (delete unnecessary aliases)

* Adding Orbitsoft module (delete unnecessary aliases)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants