-
Notifications
You must be signed in to change notification settings - Fork 108
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
Added card_uri and updated resource_property to match documentation #138
Conversation
…xcept for Lead Gen and App Download card
Going to make some more edits |
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.
Thanks for this, @carmenjyuen! Just a few changes, then we can merge it in.
twitter_ads/creative.py
Outdated
resource_property(Video, 'created_at', readonly=True, transform=TRANSFORM.TIME) | ||
resource_property(Video, 'deleted', readonly=True, transform=TRANSFORM.BOOL) | ||
resource_property(Video, 'description') | ||
resource_property(Video, 'duration', readonly=True) |
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 transform=TRANSFORM.INT
here
twitter_ads/creative.py
Outdated
resource_property(Video, 'ready_to_tweet', readonly=True, transform=TRANSFORM.BOOL) | ||
resource_property(Video, 'duration', readonly=True) | ||
resource_property(Video, 'reasons_not_servable', readonly=True) |
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 transform=TRANSFORM.LIST
here
@@ -104,17 +108,16 @@ class AccountMedia(Resource, Persistence): | |||
|
|||
# video properties |
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 update the comment here
twitter_ads/creative.py
Outdated
resource_property(Video, 'aspect_ratio', readonly=True) | ||
resource_property(Video, 'created_at', readonly=True, transform=TRANSFORM.TIME) | ||
resource_property(Video, 'deleted', readonly=True, transform=TRANSFORM.BOOL) | ||
resource_property(Video, 'description') |
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.
Since this is writable, let's move it down to that section
twitter_ads/creative.py
Outdated
@@ -104,17 +108,16 @@ class AccountMedia(Resource, Persistence): | |||
|
|||
# video properties | |||
# read-only | |||
resource_property(AccountMedia, 'created_at', readonly=True, transform=TRANSFORM.TIME) | |||
resource_property(AccountMedia, 'deleted', readonly=True, transform=TRANSFORM.BOOL) | |||
resource_property(AccountMedia, 'duration', readonly=True) |
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 sure this is available for this entity
@@ -127,17 +130,17 @@ class MediaCreative(Resource, Persistence): | |||
|
|||
# video properties |
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 update the comment here
twitter_ads/creative.py
Outdated
@@ -190,8 +200,6 @@ class VideoWebsiteCard(Resource, Persistence): | |||
resource_property(VideoWebsiteCard, 'video_poster_width', readonly=True) | |||
resource_property(VideoWebsiteCard, 'video_url', readonly=True) | |||
resource_property(VideoWebsiteCard, 'video_width', readonly=True) | |||
resource_property(VideoWebsiteCard, 'website_dest_url', readonly=True) |
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.
It looks like website_dest_url
and website_display_url
are returned in the response. The documentation just doesn't reflect that yet. Let's add these back in
resource_property(VideoAppDownloadCard, 'googleplay_app_id') | ||
resource_property(VideoAppDownloadCard, 'googleplay_deep_link') | ||
resource_property(VideoAppDownloadCard, 'app_cta') | ||
resource_property(VideoAppDownloadCard, 'image_media_id') |
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.
image_media_id
should be included
resource_property(ImageConversationCard, 'second_cta') | ||
resource_property(ImageConversationCard, 'second_cta_tweet') | ||
resource_property(ImageConversationCard, 'thank_you_text') | ||
resource_property(ImageConversationCard, 'thank_you_url') | ||
resource_property(ImageConversationCard, 'image_media_id') |
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.
Include image_media_id
Thank you for this contribution, @carmenjyuen! |
Except for Lead Gen and App Download card