Targeting updates #1689

jaiminpanchal27 · 2017-10-13T15:28:57Z

Type of change

Feature
Refactoring (no functional changes, no api changes)

Description of change

This PR covers targeting changes for Prebid 1.0 mentioned over here
#1574 (comment)

Also,

removed usage of $$PREBID_GLOBAL$$ from setTargetingForAst function. convert targeting module to factory pattern #1606 was closed because of mutable state.
refactored setTargeting function mainly used for dfp targeting.

…ng-updates # Conflicts: # src/adaptermanager.js # test/spec/unit/core/adapterManager_spec.js

mkendall07

Overall a step in the right direction, but please refactor into more readable code. I still need to test for completeness but please refactor first.

mkendall07 · 2017-10-24T17:20:50Z

src/targeting.js

+      .reduce(function(acc, bid, index, arr) {
+        Object.keys(bid.adserverTargeting)
+          .filter(key => standardKeys.indexOf(key) === -1)
+          .forEach(function(key) {


use arrow functions for consistency.

mkendall07 · 2017-10-24T17:30:35Z

src/targeting.js

    let standardKeys = getStandardKeys();

    return auctionManager.getBidsReceived()
-      .filter(adUnitsFilter.bind(this, adUnitCodes))
+      .filter(bid => adUnitCodes.includes(bid.adUnitCode))


This entire code block is really unmanageable. Part of the reason for re-writing this module was to get better readability. I can't really tell what's going on here, and if there is a bug I'd never catch it from reading the code. It's very difficult to debug as well at runtime.

My suggestion would be to break each piece into separate functions that do one thing and give them meaningful names so that when they are composed the code is easier to understand.

snapwich

This code still looks pretty hairy to me. I think it's a lot of the big chunks of logic wrapped up in array chaining. I'm guessing it's not very performant because of all the chaining/iteration as well.

snapwich · 2017-11-07T23:25:09Z

src/config.js

@@ -14,7 +14,7 @@ const DEFAULT_DEBUG = false;
 const DEFAULT_BIDDER_TIMEOUT = 3000;
 const DEFAULT_PUBLISHER_DOMAIN = window.location.origin;
 const DEFAULT_COOKIESYNC_DELAY = 100;
-const DEFAULT_ENABLE_SEND_ALL_BIDS = false;
+const DEFAULT_ENABLE_SEND_ALL_BIDS = true;


Why is this changing? It's kind of a big change, isn't it?

#1574 (comment)

mkendall07 · 2017-11-14T17:13:21Z

src/targeting.js

@@ -49,25 +49,41 @@ export function newTargeting(auctionManager) {
        });
      });
    });
+


can you add some comments here as to the structure of the objects you are working with?

I moved that part of code to separate function. Its purpose it flatten targeting array so that it can be easily iterated when used in setTargetingForGPT function.

Have also added sample input and result and jsdoc params to help.

mkendall07 · 2017-11-14T17:13:47Z

src/targeting.js

+      accumulator[key] = Object.assign({}, accumulator[key], targeting[key]);
+      return accumulator;
+    }, {});
+
    return targeting;
  };

  targeting.setTargeting = function(targetingConfig) {


this should be setTargetingForDFP.

After discussion it was decided to name this function setTargetingForGPT to make it consistent with public api $$PREBID_GLOBAL$$.setTargetingForGPTAsync

mkendall07 · 2017-11-14T17:14:59Z

src/targeting.js

-              });
-          }));
-    });
+      Object.keys(targetingConfig).filter(targetId => targetId === slot.getSlotElementId() || targetId === slot.getAdUnitPath())


combine this check into a function please and replace the other occurrence of it.

mkendall07 · 2017-11-14T17:15:29Z

src/targeting.js

@@ -135,7 +151,9 @@ export function newTargeting(auctionManager) {
            typeof winner.sendStandardTargeting === 'undefined' ||
            winner.sendStandardTargeting ||
            standardKeys.indexOf(key) === -1)
-          .map(key => ({ [key.substring(0, 20)]: [winner.adserverTargeting[key]] }))
+          .map(key => ({
+            [(key === 'hb_deal') ? `${key}_${winner.bidderCode}`.substring(0, 20) : key.substring(0, 20)]: [winner.adserverTargeting[key]]


make 20 a const.

mkendall07 · 2017-11-14T17:15:56Z

src/targeting.js

   */
-  function getAlwaysUseBidTargeting(adUnitCodes) {
+  function mergeAdServerTargeting(acc, bid, index, arr) {


add jsdoc params please.

mkendall07 · 2017-11-14T17:16:16Z

src/targeting.js

-
-              return { [key.substring(0, 20)]: [bid.adserverTargeting[key]] };
-            }).filter(key => key) // remove empty elements
+            [key.substring(0, 20)]: [bid.adserverTargeting[key]]


use const for 20.

mkendall07 · 2017-11-14T17:16:30Z

src/targeting.js

+  /**
+   * Get custom targeting keys for bids`.
+   */
+  function getCustomBidTargeting(adUnitCodes) {


jsdoc params

mkendall07 · 2017-11-14T17:17:00Z

test/fixtures/fixtures.js

-    [
-      'foobar',
-      '300x250'
+      ['0x0', '300x250', '300x600']


why is 0x0 being set?

This is getting set because of a scenario like this #1574 (comment)

In fixture
https://github.com/prebid/Prebid.js/blob/master/test/fixtures/fixtures.js#L319
https://github.com/prebid/Prebid.js/blob/master/test/fixtures/fixtures.js#L348

{ 'bidderCode': 'triplelift', 'adUnitCode': '/19968336/header-bid-tag-0', 'adserverTargeting': { 'foobar': '0x0' } }, { 'bidderCode': 'appnexus', 'adUnitCode': '/19968336/header-bid-tag-0', 'adserverTargeting': { 'foobar': '300x250' } }

…ng-updates # Conflicts: # src/utils.js

jaiminpanchal27 · 2017-11-16T15:49:25Z

@mkendall07 Updated code

…ng-updates # Conflicts: # src/secureCreatives.js # test/spec/unit/pbjs_api_spec.js

…ng-updates

snapwich

LGTM

* Concurrent auctions (#1593) * Adding timestamp function to utils * Auction manager (WIP) * WIP * Updated targeting with auction instances and added auction status * Updated appnexus and appnexusAst adapter * Added done callback * Removing bidmanager functions * Removing deprecated public api functions * remove bidmanager file * Revert "remove bidmanager file" This reverts commit 616409e. * Updated appnexus and rubicon adapters to call done callback after all responses are back * Added todo * Fixing circular dependency * filter s2s requests * Emit auction end Add getBidsRequested function * remove unused stuff from adapters * cleanup and fixes * Unit test fixes Added exclude to karma temporarily to support unit tests * added setTimeout for each auction instance * added constants closer to code * binded auction object inside done method * Adding auctionmanager_spec tests and fixes * Remove breakpoint * Moving auction to separate file and unit tests * Merged manually * Merged config api changes * Passing adunits, adunitscodes via constructor and fixing unit tests * added ajax with timeout * convert targeting module to factory pattern * Auction code refactored and unit tests * Updated bidderfactory for 1.0 changes and added unit tests * Renamed placementCode to adUnitCode * Refactor auction and update unit tests * JSDoc for auction and auctionManager * Unit test fixes after rebase * Manual merge new updates to bidmanager and its spec file into auction * Update appnexusAst spec file * Refactor * Prebid 1.0 does not require no_bid to be added * removed side effect by cloning objects * bug fix to set status on already used bid * Removing all !1.0 complaiant adapters * Adding aliases to appnexusAst adapter * Removed bidmanager from currency * Add ignore-loader to handle .md files (#1646) * Auction module refactor (#1644) * Auction moduel refactor * remove comment and global cbtimeout * Config api updates (#1633) * Add timeoutBuffer, s2sconfig, bidderSequence to config * Removing deprecated function and its unit test * fixed linting errors * Adapter/rubicon bid adapter markup (#1674) * initial take on rubiconBidAdapter markup * formatting * formatting, cont. * formatting, cont. * Prebid 1.0 Fix issue with video bid validation (#1680) * Fix issue with video bid validation * Modified tests to stub `auctionManager.getBidsRequested` instead of `getBidRequest` * Move stub to beforeEach hook * Fix lint errors * Add bidRequests param to bid validation * Auction module updated to support currency/hook (#1733) * added hook module to prebid core that allows extension of arbitrary functions * remove unused dependency tiny-queue * change PluginFunction to HookedFunction * more hook documentation fixes * Auction module updated to support currency/hook * remove unused dependency tiny-queue * change PluginFunction to HookedFunction * more hook documentation fixes * WIP * allow context for hooked functions * added tests for context * remove withContext, just use bind * fix in hooks so asyncSeries keeps proper bound context * Unit test fixes * Updated bid validation function * Fixed video unit test * updated sizeMapping to use sizeConfig and support labels (#1772) * updated sizeMapping to use sizeConfig and support labels * added new tests for labels and sizes w/ sizeConfig when making auction * made some names clearer and added type to labels for sizeMapping * make error message more descriptive in adaptermanager * remove extra line in adpatermanager * update package.json with correct version. (#1813) * Unit test fixes for IE 10 and other old browsers (#1810) * Added fix for location.origin * Fixed test case failing in IE and Safari browsers * Added utils.getOrigin method * Updated renderer to use hooks * Rename appnexusAst adapter to appnexus adapter (#1848) * Renamed appnexusAst adapter to appnexus * Updated unit test to use appnexus bidderCode * Remove completed todo comment * fixed safeframe for 1.0 (#1834) * Emit array of objects from BID_TIMEOUT event (#1824) * Emit array of objects from BID_TIMEOUT event * requestId is now auctionId * Use v4 UUID to match previous requestId format * Move function * Move public winningBids to auction (#1828) * Move public winningBids to auction * removed _winningBids * removed _winningBids * bugfix: return only new bids * Updates for PubWise Prebid 1.0 Support (#1847) * Updates for Prebid 1.0 Support * Updates for Bug Fixes and a small refactor * update JSDoc comment. Remove trailing space (#1872) tests were failing due to no trailing space eslint rule. * Prebid 1.0 adxcg analytics adapter fix for bidtimeout event (#1871) * adxcg analytics adapter for 1.0 bidtimeout event * update tests for adxcg analytics 1.0 * Prebid 1.0 prebid server (#1846) * fix adaptermanager s2sTest unit tests * fix s2s log message * remove errant comment * fixed log statement * removed seemingly unnecessary call to transformHeightWidth(adUnit); * removed legacy sizeMapping code block * initial refactor of prebidServerBidAdapter working w/o tests (cherry picked from commit 2b843d0) * add transformSizes back for prebidServer adUnits to fix request * fixed adapterManager_spec tests * added prebidServerBidAdapter tests for 1.0 * fixed lint errors * make sure addBidResponse and doneCb are stubbed for s2s calls * s2s requests now firing BID_REQUESTED event * fixed commented tests and other minor fixes * update defaults in prebidServerBidAdapter and fix doBidderSync bug * add new API for setting defaults in config for modules * Targeting updates (#1689) * Cherry pick alias bidder * Cherry pick alias bidder * Cherry pick alias bidder * Updated test case to not import adapter * targeting updates * targeting functions refactoring * Refactored functions * more refactor of function * added jsdoc and some more refactor * check bid expiry and filtering used bids * make sure we have the right version * Update version to fix invalid semver * video bug fix (#1906) * video bug fix added required params to prebidServerAdapter * Emitted auction_end and updated ttl for prebidServer * fix logging of server adapters * Manually merge bidManager code to auction module (#1905) * Unit test fix in IE for adxcgAnalytics Adapter (#1929) * IE bug fix * get only unique bidders * added log message for xhr timeout (#1928) * remove polyfill.js and remove global polyfills (#1918) * remove polyfill.js and remove global polyfills * make sure find, findIndex, and includes use core-js in tests * switch from virtual function bind core-js to explicit usage. * remove transform-function-bind babel plugin * Replace usePrebidCache with cache:url and remove default (#1904) * replace usePrebidCache with video:cacheUrl and remove default * remove newConfig in dfpAdServerVideo_spec that isn't doing anything * change video.cacheUrl to cache.url * update to cache.url in auction and remove from adapter * pulsepointLiteBidAdapter renamed to pulsepointBidAdapter (#1931) * ET-1691: Pulsepoint Analytics adapter for Prebid. (#1) * ET-1691: Adding pulsepoint analytics and tests for pulsepoint adapter * ET-1691: Adding pulsepoint analytics and tests for pulsepoint adapter * ET-1691: cleanup * ET-1691: minor * ET-1691: revert package.json change * Adding bidRequest to bidFactory.createBid method as per #509 * ET-1765: Adding support for additional params in PulsePoint adapter (#2) * ET-1850: Fixing #866 * Minor fix * Adding mandatory parameters to Bid * Pulsepoint adapter changes * readme updates * add 'x_source.tid' to rubicon requests (#1933) * Bring in pre-1.0 native-image asset adapter change (#1934) * 33Across Adapter: Removed the usage of utils library (#1917) * Removed the usage of utils library to get bidder requests during userSync since this info will now be passed into the method * Fixed extra space which cause lint to fail * Implemented user sync per code review feedback in #1917 * Minor feedback changes * Re-instated lint check for all files which was accidentally commited after testing * Prebid 1.0 server cache (#1935) * add cache stuff to video for prebid server * add description for cacheMarkup

jaiminpanchal27 added 7 commits October 9, 2017 23:27

Cherry pick alias bidder

6ae1236

Cherry pick alias bidder

f391438

Cherry pick alias bidder

81eb1d4

Updated test case to not import adapter

c26fd41

targeting updates

fa8295d

targeting functions refactoring

760a14e

Merge branch 'prebid-1.0' of github.com:prebid/Prebid.js into targeti…

c5533ad

…ng-updates # Conflicts: # src/adaptermanager.js # test/spec/unit/core/adapterManager_spec.js

jaiminpanchal27 added the needs review label Oct 13, 2017

jaiminpanchal27 requested a review from mkendall07 October 16, 2017 17:44

jaiminpanchal27 assigned mkendall07 Oct 16, 2017

jaiminpanchal27 added the Prebid 1.0 label Oct 18, 2017

mkendall07 requested changes Oct 24, 2017

View reviewed changes

mkendall07 added needs update and removed needs review labels Oct 25, 2017

Refactored functions

37612c0

ndhimehta assigned snapwich Nov 7, 2017

snapwich reviewed Nov 7, 2017

View reviewed changes

more refactor of function

73d5a9f

mkendall07 requested changes Nov 14, 2017

View reviewed changes

ndhimehta added ready in progress and removed ready labels Nov 15, 2017

jaiminpanchal27 added 2 commits November 16, 2017 10:34

added jsdoc and some more refactor

37f6292

Merge branch 'prebid-1.0' of github.com:prebid/Prebid.js into targeti…

55b5583

…ng-updates # Conflicts: # src/utils.js

jaiminpanchal27 added 3 commits November 21, 2017 11:07

Merge branch 'prebid-1.0' of github.com:prebid/Prebid.js into targeti…

e3f9b0b

…ng-updates # Conflicts: # src/secureCreatives.js # test/spec/unit/pbjs_api_spec.js

Merge branch 'prebid-1.0' of github.com:prebid/Prebid.js into targeti…

ab46a3e

…ng-updates

check bid expiry and filtering used bids

3f4531e

jaiminpanchal27 mentioned this pull request Nov 28, 2017

No Bid Responses in 1.0 Branch #1890

Closed

mkendall07 approved these changes Nov 29, 2017

View reviewed changes

snapwich approved these changes Nov 29, 2017

View reviewed changes

mkendall07 merged commit 11ae6bf into prebid-1.0 Nov 29, 2017

matthewlane deleted the targeting-updates branch December 4, 2017 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Targeting updates #1689

Targeting updates #1689

jaiminpanchal27 commented Oct 13, 2017

mkendall07 left a comment

mkendall07 Oct 24, 2017

mkendall07 Oct 24, 2017

snapwich left a comment

snapwich Nov 7, 2017

jaiminpanchal27 Nov 7, 2017

mkendall07 Nov 14, 2017

jaiminpanchal27 Nov 16, 2017

mkendall07 Nov 14, 2017

jaiminpanchal27 Nov 16, 2017

mkendall07 Nov 14, 2017

mkendall07 Nov 14, 2017

mkendall07 Nov 14, 2017

mkendall07 Nov 14, 2017

mkendall07 Nov 14, 2017

mkendall07 Nov 14, 2017

jaiminpanchal27 Nov 16, 2017

jaiminpanchal27 commented Nov 16, 2017

snapwich left a comment

@@ @@ -49,25 +49,41 @@ export function newTargeting(auctionManager) { @@
                       });
                     });
                   });

Targeting updates #1689

Targeting updates #1689

Conversation

jaiminpanchal27 commented Oct 13, 2017

Type of change

Description of change

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

snapwich 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaiminpanchal27 commented Nov 16, 2017

snapwich left a comment

Choose a reason for hiding this comment