-
Notifications
You must be signed in to change notification settings - Fork 0
REDTWOO-87: Adjust campaign settings #55
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
|
Note: |
joemcgill
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.
@iamdharmesh I left some questions about targeting. Otherwise, I think this is looking ok.
| $targeting_type = $campaign_data['targeting_type'] ?? ''; | ||
|
|
||
| $shopping_targeting = array( | ||
| 'targeting_type' => $targeting_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.
What happens if $targeting_type is '' above? Is that ok?
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.
Ahh, I just ignored this as we are passing hardcoded value in the function call. Updated this to use PROSPECTING as a default now.
These errors are related to the way this dependency is currently set up. This package has since been moved and the path can be updated similar to how it was done in this PR. |
joemcgill
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.
Pre approved assuming CI passes.
QA Status: Need Feedback/Fixes
|
QA Status: Approved ✅
New setup and new ads accountRecording.1573.mp4Existing Setup with Existing Ads account:Recording.1571.mp4With a Specific Demographic country:exisiting.setup.with.specific.demographic.area.mp4Next Step: Ready to merge 🚀 |

Changes proposed in this Pull Request:
PR Updates the campaign options for Campaign, Ad group and Ad as suggested in issue here
Closes https://linear.app/a8c/issue/REDTWOO-87/adjust-campaign-settings-for-launch-of-feature
Steps to test the changes in this Pull Request:
Changelog entry
N/A