Implement RON support and Bidder Code Params #52

pbrisson · 2018-04-10T18:46:06Z

Fixes #16 - used suggested parameter DFP_ALLOW_NO_INVENTORY_TARGETING when no DFP_TARGETED_PLACEMENT_NAMES is provided.

I also needed Bidders' Params to be used when sending all bids to the ad server was required (see: http://prebid.org/dev-docs/bidders.html and http://prebid.org/adops/send-all-bids-adops.html), so I added the PREBID_BIDDER_PARAMS parameter for that purpose. It's important to note that when that parameter is used, hb_bidder is not matched against Bidder Code.

I also put the base to retrieve ad units when required, although it was finally not used (preferred to extract root ad unit id directly from network). But that could eventually be used to fix #17 !

…l existing ad units.

…h, in order to prevent timeouts from DFP.

pbrisson · 2018-04-11T18:27:45Z

Just added another option so it's possible to make line item/creative associations in batch. Was using 450 line items and 10 creatives, which ended up requiring 4500 associations - this unfortunately times out on DFP API.

Named the option "DFP_ASSOCIATIONS_BATCH".

… recommendations.

kmjennison

Thank you for contributing this PR! Overall, it looks great, and I expect it'll helpful to a lot of people. The requested changes are mostly just related to code cleanliness and test coverage. LMK if you won't have the time to address them.

kmjennison · 2018-04-20T20:36:30Z

dfp/associate_line_items_and_creatives.py

+import settings
+import time
+import suds
+from pprint import pprint


Remove unused pprint

kmjennison · 2018-04-20T20:37:31Z

README.md

@@ -80,11 +80,14 @@ Setting | Description | Default
 `DFP_USE_EXISTING_ORDER_IF_EXISTS` | Whether we should modify an existing order if one already exists with name `DFP_ORDER_NAME` | `False`
 `DFP_NUM_CREATIVES_PER_LINE_ITEM` | The number of duplicate creatives to attach to each line item. Due to [DFP limitations](https://support.google.com/dfp_sb/answer/82245?hl=en), this should be equal to or greater than the number of ad units you serve on a given page. | the length of setting `DFP_TARGETED_PLACEMENT_NAMES`
 `DFP_CURRENCY_CODE` | The currency to use in line items. | `'USD'`
+`DFP_ALLOW_NO_INVENTORY_TARGETING` | If no placement should be used, for example for a run of network. If True, DFP_TARGETED_PLACEMENT_NAMES still need to be set to an empty array. | `False`
+`DFP_ASSOCIATIONS_BATCH` | Determine number of line item/creative associations to be created in one batch. | the number of line items to be created multiplied by `DFP_NUM_CREATIVES_PER_LINE_ITEM`


This doesn't need to be a setting. We can simply always use batch updates.

kmjennison · 2018-04-20T20:39:53Z

README.md

@@ -80,11 +80,14 @@ Setting | Description | Default
 `DFP_USE_EXISTING_ORDER_IF_EXISTS` | Whether we should modify an existing order if one already exists with name `DFP_ORDER_NAME` | `False`
 `DFP_NUM_CREATIVES_PER_LINE_ITEM` | The number of duplicate creatives to attach to each line item. Due to [DFP limitations](https://support.google.com/dfp_sb/answer/82245?hl=en), this should be equal to or greater than the number of ad units you serve on a given page. | the length of setting `DFP_TARGETED_PLACEMENT_NAMES`
 `DFP_CURRENCY_CODE` | The currency to use in line items. | `'USD'`
+`DFP_ALLOW_NO_INVENTORY_TARGETING` | If no placement should be used, for example for a run of network. If True, DFP_TARGETED_PLACEMENT_NAMES still need to be set to an empty array. | `False`
+`DFP_ASSOCIATIONS_BATCH` | Determine number of line item/creative associations to be created in one batch. | the number of line items to be created multiplied by `DFP_NUM_CREATIVES_PER_LINE_ITEM`
+`PREBID_BIDDER_PARAMS` | Whether DFP targeting keys should be created following Bidders' Params structure. This is used when it's required to send all bids to the ad server. | `False`


What do you think about renaming this setting to PREBID_SEND_ALL_BIDS? I think that's a little clearer.

kmjennison · 2018-04-20T20:40:50Z

README.md


 ## Limitations

-* Currently, the names of the bidder code targeting key (`hb_bidder`) and price bucket targeting key (`hb_pb`) are not customizable. The `hb_bidder` targeting key is currently required (see [#18](../../issues/18))


We should keep the "and price bucket targeting key (hb_pb) are not customizable" because this PR still doesn't allow naming hb_pb to anything arbitrary.

kmjennison · 2018-04-20T20:41:56Z

dfp/associate_line_items_and_creatives.py

-      u'Created {0} line item <> creative associations.'.format(len(licas)))
+  associations_batch = getattr(settings, 'DFP_ASSOCIATIONS_BATCH', None)
+
+  if not associations_batch is None:


As noted above, let's just always use batch uploads here. Do you know if DFP has any batch size limits? I didn't see any. Hardcoding something like 20 or 50 items seems reasonable (whatever you found works for you).

kmjennison · 2018-04-20T20:54:27Z

settings.py

+# Optional
+# Determine if line items and creative should be associated in batch.
+# Useful to avoid timeouts if many of them have to be created.
+# DFP_ASSOCIATIONS_BATCH = 50


As noted above, let's not add this setting.

kmjennison · 2018-04-20T21:02:05Z

tasks/add_new_prebid_partner.py

+  # In the case where no inventory is being used, make sure at least one 
+  # creative is created
+  if not num_creatives > 0:
+    num_creatives = 1


Let's actually raise an exception (earlier in the settings validation where we first call getattr) and force the user to set a number in this case. That's better than having their ads not show because they have too few creatives (see #13 (comment)).

kmjennison · 2018-04-20T21:02:33Z

tasks/add_new_prebid_partner.py

+  additional_keys = ''
+
+  if bidder_params is True:
+    hb_pb_key += '_' + bidder_code


As noted elsewhere, please move this logic into a standalone module and reuse it elsewhere.

kmjennison · 2018-04-20T21:05:20Z

tests/test_dfp_create_line_items.py

                'operator': 'IS',
-                'valueIds': [111111],
+                'valueIds': [222222],
                'xsi_type': 'CustomCriteria'
              }
            ],


Add another test for run-of-network settings.

kmjennison · 2018-04-20T21:11:59Z

tasks/add_new_prebid_partner.py

      prices_summary=prices_summary,
      bidder_code=bidder_code,
      placements=placements,
      sizes=sizes,
      name_start_format=color.BOLD,
      format_end=color.END,
      value_start_format=color.BLUE,
+      additional_keys=additional_keys
    ))

  ok = input('Is this correct? (y/n)\n')


Could you add a couple tests to test_add_new_prebid_partner.py to test the validations of the new settings?

betancourtl · 2019-01-17T15:44:33Z

Would love to see this merged. I would be willing to make the changes but python is not my main language.

Whats holding this back? general cleanup and tests?

pbrisson · 2019-01-17T15:57:40Z

Apologies - just really busy with work and most of changes requests are, as @kmjennison mentions, for code cleanliness and tests coverage. I'm still using changes suggested in this PR in a production environment, almost daily. Might proceed as some point, but my schedule is just too hectic for now...

betancourtl · 2019-01-17T18:00:08Z

Alright well, thx for adding this functionality.

pbrisson added 15 commits April 5, 2018 16:01

Added basic support to retrieve ad units.

e9c3f4a

Completed targeting of root ad unit when no placement is provided.

d89af9d

Changes to the settings file.

dabc750

Merge branch 'ron_support'

590a8bb

Completed implementation of Prebid params.

346b834

Fixed tests or DFP create creatives.

6ebd50d

Use effectiveRootAdUnitd from current network instead of filtering al…

eb8e763

…l existing ad units.

Completed support for RON.

be9cc2e

Fixed tests.

24a71d0

Removed unused line from settings.py.

70a277d

Fixed typo.

3dcfa0a

Changes to readme. Removed reference to ad units from settings.

7fe03f9

More changes to readme since kmjennison#16 has been fixed.

2efc445

Restored proper default value in order to fix tests.

3aac015

Added an option to make line items and creatives associations in batc…

b5f0be8

…h, in order to prevent timeouts from DFP.

pbrisson added 2 commits April 11, 2018 17:03

Manage CommonError.CONCURRENT_MODIFICATION when it occurs, per Google…

132e32e

… recommendations.

Added exception handling for common errors.

3840021

kmjennison requested changes Apr 20, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement RON support and Bidder Code Params #52

Implement RON support and Bidder Code Params #52

pbrisson commented Apr 10, 2018

pbrisson commented Apr 11, 2018

kmjennison left a comment

kmjennison Apr 20, 2018

kmjennison Apr 20, 2018

kmjennison Apr 20, 2018

kmjennison Apr 20, 2018

kmjennison Apr 20, 2018

kmjennison Apr 20, 2018

kmjennison Apr 20, 2018

kmjennison Apr 20, 2018

kmjennison Apr 20, 2018

kmjennison Apr 20, 2018

betancourtl commented Jan 17, 2019

pbrisson commented Jan 17, 2019

betancourtl commented Jan 17, 2019


		## Limitations

		* Currently, the names of the bidder code targeting key (`hb_bidder`) and price bucket targeting key (`hb_pb`) are not customizable. The `hb_bidder` targeting key is currently required (see [#18](../../issues/18))

Implement RON support and Bidder Code Params #52

Are you sure you want to change the base?

Implement RON support and Bidder Code Params #52

Conversation

pbrisson commented Apr 10, 2018

pbrisson commented Apr 11, 2018

kmjennison 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

betancourtl commented Jan 17, 2019

pbrisson commented Jan 17, 2019

betancourtl commented Jan 17, 2019