-
Notifications
You must be signed in to change notification settings - Fork 114
Support for new cards endpoint #275
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
Conversation
| elif isinstance(value, list): | ||
| if not value: | ||
| continue | ||
| params[name] = ','.join(map(str, value)) |
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.
Before removing this, components is (incorrectly) represented as comma-separated objects wrapped in double quotes, example:
{'card_uri': 'card://1372017094963830793',
'components': "{'type': 'MEDIA', 'media_key': '13_1191948012077092867'},{'type': 'DETAILS', 'destination': {'type': 'WEBSITE', 'url': 'http://twitter.com/'}, 'title': 'Twitter Web'}",
'created_at': '2021-03-17T02:49:00Z',
'name': 'example',
'updated_at': '2021-03-17T02:49:00Z'}After removing this, it's represented properly
{'card_uri': 'card://1372017094963830793',
'components': [{'media_key': '13_1191948012077092867', 'type': 'MEDIA'},
{'destination': {'type': 'WEBSITE', 'url': 'http://twitter.com/'},
'title': 'Twitter Web',
'type': 'DETAILS'}],
'created_at': '2021-03-17T02:49:00Z',
'name': 'example',
'updated_at': '2021-03-17T02:49:00Z'}@twitterdev/ads-api-sdks let's talk through the implications of this change
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.
For peace of mind I would try running the example scripts that exercise batch and audiences, since Resource is used in many places I think the primary thing here is testing.
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.
For peace of mind I would run batch and audience related example scripts and make sure this doesn't unintentionally change behavior for a different resource type
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.
Ack! That's fair. Will do
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.
For peace of mind I would try running the example scripts that exercise batch and audiences, since Resource is used in many places I think the primary thing here is testing.
I manually tested batch TC from batch_request.py.
targeting_criterion_1 = TargetingCriteria(account)
targeting_criterion_1.line_item_id = 'itttn'
targeting_criterion_1.targeting_type = 'LOCATION'
targeting_criterion_1.targeting_value = '00a8b25e420adc94'
targeting_criterion_1.operator_type = 'EQ'
targeting_criterion_2 = TargetingCriteria(account)
targeting_criterion_2.line_item_id = 'itttn'
targeting_criterion_2.targeting_type = 'PHRASE_KEYWORD'
targeting_criterion_2.targeting_value = 'tech'
targeting_criterion_2.operator_type = 'EQ'
targeting_criteria_list = [targeting_criterion_1, targeting_criterion_2]
TargetingCriteria.batch_save(account, targeting_criteria_list)The targeting was successfully created.
I manually tested audiences using tailored_audience.py.
audience = TailoredAudience.create(account, 'test TA')
email_hash = hashlib.sha256("test-email@test.com").hexdigest()
user = [{
"operation_type": "Update",
"params": {
"users": [{
"email": [
email_hash
]
}]
}
}]
success_count, total_count = audience.users(user)This successfully added one user.
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.
LGTM
| raise AttributeError("'Card' object has no attribute 'reload'") | ||
|
|
||
| # card properties | ||
| # read-only |
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.
Not including id or card_type as these were deprecated in v9. That being said, I can add this in if we plan to roll this change out with v8 of the SDK.
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.
@tushdante what are your thoughts here?
jbabichjapan
left a comment
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.
Let's try to do a full test pass before merging this
Per: ``` twitter_ads/creative.py:626:1: E305 expected 2 blank lines after class or function definition, found 1 ```
This reverts commit e0aae96.
Adding support for the new Cards endpoints, announced here.
Usage example (create):