REDTWOO-70: Implement Create Campaign REST Controller #42
Conversation
…enable the "PURCHASE" optimization goal for the ad campaign.
iamdharmesh
changed the title
There was a problem hiding this comment.
Thanks @iamdharmesh, left a few questions and suggestions. Can you review?
| */ | ||
| private function create_product_set() { | ||
| // Get the product set ID from the transients. | ||
| $product_set_id = Transients::get( TransientDefaults::PRODUCT_SET_ID ); |
There was a problem hiding this comment.
I'm not sure Transients are the best storage for this data as transients can by definition get dropped from the system depending on how cache is set up on a store.
Is the intention to ensure that if a product set has already been created that we don't create a new one? If so, saving to an option might be a better choice. Hard to say without knowing more context.
This probably applies to the above method as well.
There was a problem hiding this comment.
I'm not sure Transients are the best storage for this data as transients can by definition get dropped from the system depending on how cache is set up on a store.
Agree on this. In happy path scenario this storage won't be used. This will be only used in case create Ad Group or Create Ad fails and user retry the create campaign again. So, the intention here to reuse the initially created product set ID incase the create Ad Group or Create Ad fails and merchant retry to create it in certain time frame.
Initially, I thought to go with options but it comes with headache to discard it and there is no harm in create one more product set in case transient get cleared before it get used. So, I went with transient here. However, I’m open to discussing or switching to options if you think it’s a more suitable storage method.
There was a problem hiding this comment.
As long as you've thought through this limitation and think that transients are the right storage mechanism, I think that's ok. One thing to think about is how this REST API endpoint might get used apart from the initial use case of setting up an account for the first time.
For example, what happens when someone starts onboarding and then decides to disconnect and start over with a different account? At minimum, these transients should likely be keyed to an ads account ID or similar and make sure that they get cleared whenever the account is disconnected to avoid edge case errors.
There was a problem hiding this comment.
For example, what happens when someone starts onboarding and then decides to disconnect and start over with a different account? At minimum, these transients should likely be keyed to an ads account ID or similar and make sure that they get cleared whenever the account is disconnected to avoid edge case errors.
We are already clearing it when user disconnect or in case of successful campaign creation. But definitely agree on using ad account ID in key on top of that would be far better. Will update it.
There was a problem hiding this comment.
Updated in fd23b7d
|
@joemcgill @iamdharmesh don't merge this yet. I've gotten some more details and will add those changes here. |
3 participants
PR adds REST endpoint for the create ad campaign.
Closes https://linear.app/a8c/issue/REDTWOO-70/implement-the-budget-form-rest-controller
Note