Skip to content

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 :)

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.

2 participants