-
Notifications
You must be signed in to change notification settings - Fork 17
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
base: master
Are you sure you want to change the base?
Ability to configure included and excluded geotargeting locations #118
Conversation
There was a problem hiding this 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
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:
|
There was a problem hiding this 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 :)
@@ -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' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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:
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.