Trustx adapter #1488

PWyrembak · 2017-08-15T13:33:12Z

Type of change

Description of change

Adding an Trustx adapter.
Added module, test spec and integration example for Trustx Bid Adapter in hello_world.html.

test parameters for validating bids

{
    bidder: 'trustx',
    params: {
        uid: 42
    }
},
{
    bidder: 'trustx',
    params: {
        uid: 43
    }
},
{
    bidder: 'trustx',
    params: {
        uid: 44
    }
},
{
    bidder: 'trustx',
    params: {
        uid: 45
    }
}

You can setup globalPrebidTrustxPriceType to use net price type:

window.globalPrebidTrustxPriceType = 'net'

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

paul@trustx.org
official adapter submission

PWyrembak · 2017-08-16T13:06:37Z

Hello,
Could you please comment on the reasons of trustx-adaptor pull request being rejected?

mkendall07 · 2017-08-16T13:59:40Z

this hasn't been reviewed yet, but the build is failing. Run gulp test locally to see and fix the error.

PWyrembak · 2017-08-16T16:11:55Z

We run gulp test and it looks like the build failing not really due to adaptor code malfunctioning:

HeadlessChrome 0.0.0 (Linux 0.0.0) "before each" hook FAILED
Error: Uncaught SyntaxError: Unexpected token < (http://localhost:9876/context.html:1)
HeadlessChrome 0.0.0 (Linux 0.0.0): Executed 772 of 1164 (1 FAILED) (skipped 1) (0 secs / 2.66 secs)
HeadlessChrome 0.0.0 (Linux 0.0.0) "before each" hook FAILED
Error: Uncaught SyntaxError: Unexpected token < (http://localhost:9876/context.html:1)
e 0.0.0 (Linux 0.0.0): Executed 772 of 1164 (1 FAILED) (skipped 1) (25.213 secs / 2.66 secs)
[13:37:40] 'test-coverage' errored after 2.08 min
[13:37:40] Error: Karma tests failed with exit code 1
at /home/travis/build/prebid/Prebid.js/gulpfile.js:94:12
at removeAllListeners (/home/travis/build/prebid/Prebid.js/node_modules/karma/lib/server.js:380:7)
at Server. (/home/travis/build/prebid/Prebid.js/node_modules/karma/lib/server.js:391:9)
at Server.g (events.js:291:16)
at emitNone (events.js:91:20)
at Server.emit (events.js:185:7)
at emitCloseNT (net.js:1544:8)
at _combinedTickCallback (internal/process/next_tick.js:71:11)
at process._tickCallback (internal/process/next_tick.js:98:9)

Could you please check?

matthewlane

Thanks for the PR. The Unexpected token < error can be ignored, that's an unrelated failure and after rerunning your tests they are passing. I was also able to validate bid responses. A few changes requested below

matthewlane · 2017-09-05T20:23:13Z

integrationExamples/gpt/hello_world.html

@@ -22,11 +22,29 @@

        // Replace this object to test a new Adapter!
        bids: [{
-          bidder: 'appnexus',
+          bidder: 'trustx',


This page is just for testing, changes to this file aren't required in PRs so the changes here can be dropped

matthewlane · 2017-09-05T20:25:47Z

modules/trustxBidAdapter.js

+    }
+  }
+
+  function tryToSendStat(startTime, timeout, auids, isTimeouted, error) {


We don't permit bid adapters to collect information about the auction mechanics, this function should be dropped. If you'd like to collect this info please create an analytics adapter

This makes sense. I would like to note that this function doesn't collect any information on the 'auction mechanics' itself- it sends TrustX header bidder information on the amount of header bids which have timeouted. Do you still think we should remove this function from the adapter code? Would an analytics adapter provide such information to the header bidder?

Thanks for clarifying. Yes, the function should remain removed as adapters should only make the minimum amount of network calls necessary for the sake of performance. Analytic adapters do have access to the BID_TIMEOUT event

Thanks for the details. As I can see you have removed the 'trytosend' stat function. Do I understand correct that you don't need us to make any changes to the TrustX adapter code for now?

Correct, no further changes requested from me. Next a second reviewer will be assigned per our PR process and they may have additional feedback for you before merging

matthewlane · 2017-09-05T20:29:13Z

modules/trustxBidAdapter.js

+    var active = true;
+    var handler = function() {
+      worker.called = true;
+      $$PREBID_GLOBAL$$.offEvent('bidTimeout', handler);


$$PREBID_GLOBAL$$.offEvent and $$PREBID_GLOBAL$$.onEvent are meant for publisher use and shouldn't be needed within bidder adapters

Could you please let us know what we should replace $$PREBID_GLOBAL$$.offEvent and $$PREBID_GLOBAL$$.onEvent with?

mkendall07 · 2017-09-13T16:31:40Z

modules/trustxBidAdapter.js

@@ -0,0 +1,167 @@
+var utils = require('src/utils.js');


prefer const or let

mkendall07 · 2017-09-13T16:32:19Z

modules/trustxBidAdapter.js

+  function _forEachPlacement(error, bid, placementCode) {
+    var bidObject;
+    if (error) {
+      bidObject = bidfactory.createBid(2);


use constant value here (STATUS.GOOD, STATUS. NO_BID)

mkendall07 · 2017-09-13T16:33:33Z

modules/trustxBidAdapter.js

+    if (error) {
+      bidObject = bidfactory.createBid(2);
+    } else {
+      bidObject = bidfactory.createBid(1);


pass through the bid request object as a 2nd argument to createBid. See example here: https://github.com/prebid/Prebid.js/blob/master/modules/appnexusAstBidAdapter.js#L206

thanks for the update. do we need to apply these changes to the code by ourselves?

Yes please make the update so we can merge your adapter. Thanks.

PWyrembak · 2017-09-19T16:03:40Z

Hello,
Could you please review the fixes?
Thanks

mkendall07 · 2017-09-19T16:40:30Z

This is merged into master. Please submit a PR to the docs repo to add a file for your adapter to the bidders directory so your adapter's params will appear on the bidders page. Thank you for contributing

* Add trustx adapter and tests for it * update integration example * Update trustx adapter * Post-review fixes of Trustx adapter

* 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) ...

PWyrembak · 2017-09-27T12:22:14Z

We've submitted the PR to the docs repo: prebid/prebid.github.io#378.
Pending approval.

PWyrembak · 2017-10-10T14:16:07Z

Hey team,
we would like to make a simple adjustment to our TrustX adapter currently available in the repo. Can we expect you reviewing it and merging soon?

* Add trustx adapter and tests for it * update integration example * Update trustx adapter * Post-review fixes of Trustx adapter

PWyrembak added 3 commits August 15, 2017 15:43

Add trustx adapter and tests for it

031fa56

update integration example

319cadd

Merge remote-tracking branch 'upstream/master'

0e877f9

mkendall07 requested a review from protonate August 24, 2017 13:00

mkendall07 assigned protonate Aug 24, 2017

matthewlane added the needs review label Aug 28, 2017

matthewlane requested review from matthewlane and removed request for protonate September 5, 2017 16:42

matthewlane assigned matthewlane and unassigned protonate Sep 5, 2017

matthewlane requested changes Sep 5, 2017

View reviewed changes

matthewlane added needs update and removed needs review labels Sep 5, 2017

PWyrembak added 2 commits September 8, 2017 16:46

Merge remote-tracking branch 'upstream/master'

0425234

Update trustx adapter

69ab1f4

matthewlane added LGTM and removed needs update labels Sep 12, 2017

matthewlane requested a review from jaiminpanchal27 September 12, 2017 22:25

matthewlane assigned jaiminpanchal27 Sep 12, 2017

mkendall07 requested changes Sep 13, 2017

View reviewed changes

mkendall07 added needs update and removed LGTM labels Sep 13, 2017

PWyrembak added 2 commits September 19, 2017 18:45

Merge remote-tracking branch 'upstream/master'

817f2fa

Post-review fixes of Trustx adapter

ef50012

mkendall07 approved these changes Sep 19, 2017

View reviewed changes

mkendall07 merged commit ea97915 into prebid:master Sep 19, 2017

jbAdyoulike pushed a commit to jbAdyoulike/Prebid.js that referenced this pull request Sep 21, 2017

Trustx adapter (prebid#1488)

b73259e

* Add trustx adapter and tests for it * update integration example * Update trustx adapter * Post-review fixes of Trustx adapter

dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018

Trustx adapter (prebid#1488)

05f8dde

* Add trustx adapter and tests for it * update integration example * Update trustx adapter * Post-review fixes of Trustx adapter

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trustx adapter #1488

Trustx adapter #1488

PWyrembak commented Aug 15, 2017

PWyrembak commented Aug 16, 2017

mkendall07 commented Aug 16, 2017

PWyrembak commented Aug 16, 2017

matthewlane left a comment

matthewlane Sep 5, 2017

matthewlane Sep 5, 2017

PWyrembak Sep 11, 2017

PWyrembak Sep 11, 2017

matthewlane Sep 11, 2017

PWyrembak Sep 12, 2017

matthewlane Sep 12, 2017

matthewlane Sep 5, 2017

PWyrembak Sep 11, 2017

PWyrembak Sep 11, 2017

mkendall07 Sep 13, 2017

mkendall07 Sep 13, 2017 •

edited

Loading

mkendall07 Sep 13, 2017

PWyrembak Sep 18, 2017

mkendall07 Sep 18, 2017

PWyrembak commented Sep 19, 2017

mkendall07 commented Sep 19, 2017

PWyrembak commented Sep 27, 2017

PWyrembak commented Oct 10, 2017

Trustx adapter #1488

Trustx adapter #1488

Conversation

PWyrembak commented Aug 15, 2017

Type of change

Description of change

PWyrembak commented Aug 16, 2017

mkendall07 commented Aug 16, 2017

PWyrembak commented Aug 16, 2017

matthewlane 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

mkendall07 Sep 13, 2017 • edited Loading

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

PWyrembak commented Sep 19, 2017

mkendall07 commented Sep 19, 2017

PWyrembak commented Sep 27, 2017

PWyrembak commented Oct 10, 2017

mkendall07 Sep 13, 2017 •

edited

Loading