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

Ability to configure included and excluded geotargeting locations #118

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

cameron-brown-future
Copy link

We needed the ability to configure geo targeting when generating our line items, and I noticed there is no way to do that currently, so I gave it a shot. Hopefully this will help, it's probably not a perfect solution.

Example config:

geographies:
  include:
    - United States
  exclude:
    - Florida

I had to mess with the AppOperations logic a bit, since there doesn't seem to be an API for getting geos by name. It may be a good idea to separate this more query-based logic to a base class if there are other gaps in the API that need to be filled this way.

Copy link
Collaborator

@dshore dshore left a comment

Choose a reason for hiding this comment

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

Great contribution! To merge this it would be good to document the usage in the config template (conf.d/line_item_manger.yml) and to include testing the new use cases and provide test coverage on the new code

@dshore
Copy link
Collaborator

dshore commented Oct 21, 2022

Also note one way to support custom additions to the line item template is to use the '--template' command to specify an alternate template. I have used it in the past to include for example this:

...
targeting:
  geoTargeting:
    targetedLocations:
      - id: 2840
        displayName: "United States"
...

Copy link
Collaborator

@dshore dshore left a comment

Choose a reason for hiding this comment

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

After tests and uses are included in template a more detailed review can be done :)

line_item_manager/conf.d/line_item_template.yml Outdated Show resolved Hide resolved
@@ -189,6 +189,43 @@ def __init__(self, *args, name: str=None, _type: str='PREDEFINED', **kwargs):
kwargs['type'] = _type
super().__init__(*args, **kwargs)

class Geography(AppOperations):
service = 'PublisherQueryLanguageService'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This service is inconsistent with the class name, seems this should be a generic PQL class and supporting methods, so things like 'Geo_Target' are provided on instantiation. Possible a new 'Geo_Target' class could use the PQL as a parent class.

Choose a reason for hiding this comment

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

Yeah, as mentioned in the top:

I had to mess with the AppOperations logic a bit, since there doesn't seem to be an API for getting geos by name. It may be a good idea to separate this more query-based logic to a base class if there are other gaps in the API that need to be filled this way.

But I'm not sure getting this right is within my capabilities

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants