2535: Consolidate Ad Creation #2623
Conversation
…onsolidate-ad-creation-ccf-merged
github-actions
bot
added
the
changelog: update
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/2459-campaign-creation-flow #2623 +/- ##
=====================================================================
- Coverage 62.6% 61.0% -1.6%
=====================================================================
Files 319 330 +11
Lines 5063 5183 +120
Branches 1232 1254 +22
=====================================================================
- Hits 3171 3162 -9
- Misses 1718 1832 +114
- Partials 174 189 +15
Flags with carried forward coverage won't be shown. Click here to find out more.
|
asvinb
changed the title
|
@joemcgill Can you kindly review PR please? |
There was a problem hiding this comment.
This is looking really good so far. I've left some inline feedback.
Additionally, I noticed that with these changes that when I create a new campaign from after editing an existing campaign that the initial value gets filled from the session storage value in gla-onboarding-paid-ads.
Path: page=wc-admin&path=%2Fgoogle%2Fdashboard&subpath=%2Fcampaigns%2Fcreate
This may already get handled in the new minimum budget work we have in progress, but ideally session storage would not be a factor when creating a campaign other than your first one.
| } | ||
|
|
||
| const description = | ||
| headerDescription || |
There was a problem hiding this comment.
headerDescription seems to be a required property, should it default to null or '' to allow it to be empty? What if someone intentionally passes a falsey value to that prop? Won't this fallback kick in? Similarly, we don't seem to handle any default fallback for the headerTitle prop here. Is that intentional?
There was a problem hiding this comment.
@joemcgill I removed the headerDescription prop and instead conditionally set the description based on the isOnboardingFlow prop. Because it's only during the onboarding flow that the description is different.
| } | ||
| /> | ||
|
|
||
| <FaqsSection /> |
There was a problem hiding this comment.
This FaqsSection component still doesn't include the improvements from ##2531. Can we cherry-pick those fixes into this branch as well and make sure this doesn't cause a regression when all is finalized?
There was a problem hiding this comment.
@joemcgill I noticed the changes were merged to the feature/2458-streamline-onboarding branch. I only merged the campaign creation one. If we want to have those changes, then it's better we merge the feature/2458-streamline-onboarding branch?
There was a problem hiding this comment.
The way I originally expected us to sequence this is for all of the 2458 work to be merged to develop and then pulled back into this branch to resolve merge conflicts before merging this work as well. However, we want to hold all of this work so it ships together. If we already had PRs set up for these feature branches, I think it would look like:
- feature/2458-streamline-onboarding > develop (not merged until everything is ready)
- feature/2590-consolidate-google-account-cards > feature/2458
- feature/2459-campaign-creation > feature/2458
- feature/2460-simplify-paid-ads-setup > feature/2459 (these are getting combined)
- feature/2460-google-ads-value-prop (post onboarding) > develop (refreshed after 2458 is merged)
So I think there are two ways of managing this, we could either merge feature/2458 into feature/2459 and to update/2535 now, or wait and merge feature/2458 into feature/2459 after the feature branch is ready and resolve merge conflicts at that point. Even so, since this moves and replaces some of the changed files, we'll have to be careful about applying those changes here during that merge. I think either strategy is fine.
There was a problem hiding this comment.
@joemcgill
The best way forward would be to merge #2458 into #2459 and then we merge everything in 2535.
I have a draft PR set up for it:
#2630
I have another PR ready to merge the updated 2459 into 2535 which we can review afterwards:
#2627
So right now, I think if we can review and merge #2630 and then the campaign creation branch would be the target branch for all future PRs.
Let me know what you think.
There was a problem hiding this comment.
This sounds right to me. To make things a bit clearer, I'll create draft PRs for the feature branches because 2590 and 2459 should both me opened against 2458, that way we'll be able to see the conflicts that will need to be resolved in an upstream branch.
There was a problem hiding this comment.
@asvinb I believe I've gotten all the feature branches updated and set up so that this should be ready to finish up. I've created the following draft PRs for the feature branches:
- Feature/2458 streamline onboarding #2631
- Feature/2459 campaign creation flow #2632
- Feature/2509 consolidate google account cards #2634
In doing so, I realized that we somehow had a duplicate feature branch for #2509 with a typo in the name (feature/2590... instead of feature/2509...) so I've updating all existing PRs to use the correct feature branch and deleted feature/2590...
I've merged all of the upstream changes from your merge conflict PRs here as well and fixed up an additional merge conflict that remained, so we should be in good shape now, but let me know if you have any issues.
| if ( ! hasFinishedResolution ) { | ||
| return <SpinnerCard />; | ||
| } | ||
|
|
||
| if ( billingStatus.status === APPROVED && ! showNotice ) { |
There was a problem hiding this comment.
Nit: This could be combined with the if below and use a tertiary to show the success component or null based on the value of showNotice, which could improve readability and make the relationship between these two logic branches more obvious. Not a blocker.
…nsolidate-ad-creation-ccf-onboarding-merged
|
@joemcgill I've made some updates where we are loading the amount from client session only during onboarding. Can you take a look and let me know what you think. |
…creation-ccf-onboarding-merged Merge onboarding improvements branch into consolidated ad
…onsolidate-ad-creation-ccf-merged
There was a problem hiding this comment.
I think these updates are looking pretty good and are testing well for me locally. Left one suggestion, but marking as approved and ready for another QA pass.
One note for QA that should be kept in mind is that when this PR is tested with billing not approved, you'll see the billing setup card on this form as well as a duplicate billing step when creating your first campaigns from the dashboard, until #2536 is completed.
| */ | ||
| export default function PaidAdsSetupSections( { | ||
| onStatesReceived, | ||
| countryCodes, | ||
| campaign, | ||
| loadCampaignFromClientSession, |
There was a problem hiding this comment.
I think this is a clever way of handling this. If adding a prop for controlling this ends up being undesirable, an alternate option might be to clear the clientSession data whenever the a campaign is created, and update this component to only save the clientSession data if isCreation is true, but I don't see this as a blocker.
QA/Test Report- ✅Testing Environment -
Test Results - Followed the testing instructions, acceptance criteria, and tested all possible ways to create, edit, and manage paid ad campaigns. All tests were passed when billing was not set, with billing set, and other use cases. Next Step- Ready to Code Review(Woo) |
Changes proposed in this Pull Request:
Closes #2535 .
Replace this with a good description of your changes & reasoning.
Detailed test instructions:
Additional details:
Changelog entry