-
Notifications
You must be signed in to change notification settings - Fork 21
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
Update Budget Setup Card. #2552
base: update/2535-consolidate-ad-creation-ccf-merged
Are you sure you want to change the base?
Update Budget Setup Card. #2552
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/2459-campaign-creation-flow #2552 +/- ##
=====================================================================
- Coverage 63.8% 63.5% -0.3%
=====================================================================
Files 326 329 +3
Lines 5089 5117 +28
Branches 1231 1238 +7
=====================================================================
+ Hits 3245 3249 +4
- Misses 1676 1695 +19
- Partials 168 173 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
This is looking really good, but issues I've noted a few issues where this could be improved.
@@ -266,16 +284,11 @@ test.describe( 'Complete your campaign', () => { | |||
textContent | |||
); | |||
|
|||
const responsePromise = |
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.
Why do we no longer need to wait for this response after the country setting is changed? It looks like when the country values change, the text that reads "Tip: Most merchants targeting similar countries set a daily budget of %d USD" does change, so we should probably ensure that is still being tested.
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.
@ankitrox I'm still unsure why we need to remove this. Could you clarify?
js/src/components/paid-ads/budget-section/budget-recommendation/index.js
Outdated
Show resolved
Hide resolved
Thank you @joemcgill for your feedback. I've addressed your comments and responded to one of your comments related to removing the response promise here. Please let me know if there's anything else you want me to look into. Over to you for another round of review. Thanks again! |
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.
@ankitrox I'm still seeing some unexpected behavior with these changes. If you go back to a previous step after after the budget recommendations have been displayed and then return to the last step, the budget recommendations are no longer showing. I've added a video to show what I'm seeing. Can you give this another look?
budget-card.mov
Could you also provide more context for this question about the e2e test changes?
@joemcgill The issue was happening because There was also an issue of amount getting changed to |
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 @ankitrox. This looks like it's working better now. I still have a question about one of the E2E changes that you made, which would be good to clarify, but I'm going to approve and send to @ankitguptaindia for QA.
@@ -266,16 +284,11 @@ test.describe( 'Complete your campaign', () => { | |||
textContent | |||
); | |||
|
|||
const responsePromise = |
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.
@ankitrox I'm still unsure why we need to remove this. Could you clarify?
Hi @joemcgill Apologies for missing the comment previously. I have responded to the query here |
QA/Test Report- ✅Testing Environment -
Test Results - Changes are working as expected. tested all related test scenarios and all are working well. Functional Demo / Screencast - Recording.812.mp4 |
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.
Left some suggestions, questions, and issues that need to be resolved.
In addition, if I understand correctly, post-onboarding can still select audience countries when creating a campaign via the /wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fdashboard&subpath=%2Fcampaigns%2Fcreate
path.
However, there are now two issues with this page:
- The recommended budget block is not shown
- There is no API triggered to get the recommended budget after changing the audience countries.
@@ -93,6 +93,8 @@ const BudgetSection = ( { formProps, disabled = false, children } ) => { | |||
<BudgetRecommendation | |||
countryCodes={ countryCodes } | |||
dailyAverageCost={ amount } | |||
currency={ currency } |
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.
BudgetRecommendation
component doesn't use the currency
prop.
@@ -11,6 +11,7 @@ import { Form } from '@woocommerce/components'; | |||
import useGoogleAdsAccount from '.~/hooks/useGoogleAdsAccount'; | |||
import useTargetAudienceFinalCountryCodes from '.~/hooks/useTargetAudienceFinalCountryCodes'; | |||
import useGoogleAdsAccountBillingStatus from '.~/hooks/useGoogleAdsAccountBillingStatus'; | |||
import useFetchBudgetRecommendationEffect from '.~/components/paid-ads/budget-section/budget-recommendation/useFetchBudgetRecommendationEffect'; |
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 I understand is that the same logic of setting the default budget will then be applied to all ad creation forms, so moving the useFetchBudgetRecommendationEffect
hook to the .~/hooks
directory would be more appropriate.
@@ -78,6 +96,14 @@ export default function PaidAdsSetupSections( { onStatesReceived } ) { | |||
const { hasGoogleAdsConnection } = useGoogleAdsAccount(); | |||
const { data: targetAudience } = useTargetAudienceFinalCountryCodes(); | |||
const { billingStatus } = useGoogleAdsAccountBillingStatus(); | |||
const { data: recommendationData } = | |||
useFetchBudgetRecommendationEffect( targetAudience ); |
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.
currentPaidAds = { | ||
...currentPaidAds, | ||
amount: sessionAmount || dailyBudget, | ||
recommendationData, |
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.
Wonder why recommendationData
needs to be a part of the paidAds
state?
@@ -143,6 +184,7 @@ export default function PaidAdsSetupSections( { onStatesReceived } ) { | |||
const initialValues = { | |||
countryCodes: paidAds.countryCodes, | |||
amount: paidAds.amount, | |||
recommendationData: paidAds.recommendationData, |
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.
The recommendationData
is not used as the form value, so passing it around this way is not ideal.
recommendations.filter( ( recommendation ) => | ||
countryCodes.includes( recommendation.country ) | ||
) |
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.
I'm curious about in which case it needs this filter?
/* | ||
* If a merchant selects more than one country, the budget recommendation | ||
* takes the highest country out from the selected countries. | ||
* | ||
* For example, a merchant selected Brunei (20 USD) and Croatia (15 USD), | ||
* then the budget recommendation should be (20 USD). | ||
*/ | ||
function getHighestBudget( recommendations ) { | ||
return recommendations.reduce( ( defender, challenger ) => { | ||
if ( challenger.daily_budget > defender.daily_budget ) { | ||
return challenger; | ||
} | ||
return defender; | ||
} ); | ||
} |
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.
This function is completely the same as:
google-listings-and-ads/js/src/components/paid-ads/budget-section/budget-recommendation/index.js
Lines 15 to 29 in 096c086
/* | |
* If a merchant selects more than one country, the budget recommendation | |
* takes the highest country out from the selected countries. | |
* | |
* For example, a merchant selected Brunei (20 USD) and Croatia (15 USD), | |
* then the budget recommendation should be (20 USD). | |
*/ | |
function getHighestBudget( recommendations ) { | |
return recommendations.reduce( ( defender, challenger ) => { | |
if ( challenger.daily_budget > defender.daily_budget ) { | |
return challenger; | |
} | |
return defender; | |
} ); | |
} |
Suggest considering whether it can be integrated into the useFetchBudgetRecommendationEffect
hook or extracted as a util to eliminate duplication.
if ( dailyBudget !== undefined ) { | ||
// If the amount is already set in session, use that one. | ||
const sessionData = clientSession.getCampaign(); | ||
const { amount: sessionAmount } = sessionData; | ||
|
||
setPaidAds( ( currentPaidAds ) => { | ||
currentPaidAds = { | ||
...currentPaidAds, | ||
amount: sessionAmount || dailyBudget, | ||
recommendationData, | ||
}; | ||
return resolveInitialPaidAds( currentPaidAds, targetAudience ); | ||
} ); | ||
} |
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.
If there is an amount restored from client session, the currentPaidAds.amount
is already that value when the first time running this useEffect, so it may not need to read from client session again.
google-listings-and-ads/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js
Lines 112 to 115 in 096c086
// Resolve the starting paid ads data with the campaign data stored in the client session. | |
const startingPaidAds = { | |
...defaultPaidAds, | |
...clientSession.getCampaign(), |
Additionally, moving this logic into the resolveInitialPaidAds
function would be better as it intends to resolve the initial values.
* Get budget recommendation tip section. | ||
* | ||
* @return {import('@playwright/test').Locator} The budget recommendation text row. |
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.
This JSDoc description is not quite consistent.
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.
@@ -55,6 +57,14 @@ export default function AdsCampaign( { | |||
'google-listings-and-ads' | |||
); | |||
|
|||
// Set the amount from client session if it is available. | |||
useEffect( () => { |
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.
@ankitrox Wondering if we can set the amount in the initialValues prop for the forms directly instead of setting the initial form value in the child components?
amount: 0, amount: 0,
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.
@ankitrox I see AdsCampaign
being used in 3 places:
js/src/pages/create-paid-ads-campaign/index.js
- the initial values are set viaCampaignAssetsForm
js/src/pages/edit-paid-ads-campaign/index.js
- the initial values are set viaCampaignAssetsForm
. Here we don't need to worry about using grabbingamount
fromlocalStorage
since it's for editing a campaign.js/src/setup-ads/ads-stepper/index.js
- TheAdsStepper
component is used bySetupAdsForm
which also makes use ofCampaignAssetsForm
to set the initial values.
So that leaves only js/src/pages/create-paid-ads-campaign/index.js
and js/src/setup-ads/ads-stepper/index.js
where we can potentially set the amount to the value from localStorage. Maybe we can even create a hook for it to reduce the code duplication.
Let me know what you think.
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.
@ankitrox I left some more comments. Can you kindly let me know about the suggested approach about setting the default value for amount via the initialValues
prop of CampaignAssetsForm
.
Also you need to check when there's no recommended budget to not display the recommendation. This is what I can see on my side while testing:
); | ||
|
||
const { country = '', daily_budget: dailyBudget } = getHighestBudget( | ||
budgetData?.recommendations || [] |
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.
@ankitrox We already have a check here https://github.com/woocommerce/google-listings-and-ads/pull/2552/files#diff-3bdecec9939ad884905f627b3905c8b138f73d843890bb6d41799694c57f2ab8R10 in case recommendations
is falsy. Shall we return an empty array in the getHighestBudget
function instead of null
?
> | ||
<CampaignPreviewCard /> | ||
</BudgetSection> | ||
{ ! loading && ( |
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.
@ankitrox This behaviour is different from what we currently have, i.e having a spinner and loading state while the data is being loaded. Is there a particular reason why we need to wait for the recommendations?
formProps={ formContext } | ||
disabled={ disabledBudgetSection } | ||
countryCodes={ formContext.values.countryCodes } | ||
country={ country } |
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.
@ankitrox We are passing those new props country
, dailyBudget
and isMultiple
to BudgetSection
. However those are being then passed to BudgetRecommendation
. Can't we get those values directly in BudgetRecommendation
?
<CampaignPreviewCard /> | ||
</BudgetSection> | ||
) } | ||
{ loading && ( |
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.
@ankitrox See my comment above.
return null; | ||
} | ||
const { | ||
isMultiple, |
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.
I see the countryCodes
prop was removed but it's still being passed to BudgetRecommendation
in js/src/components/paid-ads/budget-section/index.js
* @param {JSX.Element} [props.children] Extra content to be rendered under the card of budget inputs. | ||
*/ | ||
const BudgetSection = ( { | ||
formProps, | ||
country, |
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.
@ankitrox See my previous comment about passing those props.
const { country = '', daily_budget: dailyBudget } = getHighestBudget( | ||
budgetData?.recommendations || [] | ||
); | ||
const multipleRecommendations = budgetData?.recommendations.length > 1; |
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.
@ankitrox The code is duplicated here. Can we move it to a hook which can be reused elsewhere?
@asvinb I've created the new hook |
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.
@ankitrox I left some more comments. Can you check them out please?
Most importantly I think we can reduce a lot of the complexity around countryCodes
. Right now, we need to fetch the budget recommendation data once the country code changes and the country codes are tied to the form. In #2535 we are planning to remove countryCodes
altogether, instead relying on the country codes from useTargetAudienceFinalCountryCodes
. Here is what I suggest:
- Remove the Audience field everywhere in the current PR (as opposed to removing them in Consolidate the ad creation step in the Ads Setup flow with the one used in Onboarding #2535)
- Have the
initialValues
forCampaignAssetsForm
components set with the recommended budget or data from local storage.
The useBudgetRecommendationData
won't need to accept any parameters since it can get the list of targeted countries via the useTargetAudienceFinalCountryCodes
What do you think @joemcgill ?
useEffect( () => { | ||
if ( ! loading ) { | ||
const { | ||
country: budgetCountries = '', |
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.
@ankitrox This will cause an error in the app when getHighestBudget
returns null
as you have defined here: https://github.com/woocommerce/google-listings-and-ads/pull/2552/files#diff-3bdecec9939ad884905f627b3905c8b138f73d843890bb6d41799694c57f2ab8R10
useFetchBudgetRecommendationEffect( countryCodes ); | ||
|
||
const [ country, setCountry ] = useState( '' ); | ||
const [ dailyBudget, setDailyBudget ] = useState( 0 ); |
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.
@ankitrox Instead of defining all those state variables, can we not define only one state variable which is an object having all those properties?
|
||
setCountry( budgetCountries ); | ||
setDailyBudget( recommendedDailyBudget ); | ||
setMultipleRecommendations( data?.recommendations.length > 1 ); |
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.
@ankitrox Do we need to store that in a state variable? We can have a variable where data?.recommendations.length > 1
and return the variable.
const args = [ initialValues, values ].map( | ||
( { countryCodes, ...v } ) => { | ||
v.countrySet = new Set( countryCodes ); | ||
return v; | ||
} | ||
); | ||
|
||
// Set the amount in session storage. | ||
if ( isValid && _?.name === 'amount' ) { |
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.
@ankitrox Why not use values.amount
?
}; | ||
|
||
return ( | ||
<Form | ||
initialValues={ initialValues } | ||
onChange={ ( _, values, isValid ) => { | ||
setPaidAds( { ...paidAds, ...values, isValid } ); | ||
|
||
if ( isValid ) { | ||
clientSession.setCampaign( values ); |
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.
@ankitrox The campaign is already being updated in local storage here:
https://github.com/woocommerce/google-listings-and-ads/pull/2552/files#diff-6b9f6824ae1208e70b059bf81217131f2d1c42fad1664514c2280500e8e1a7b6R114
Changes proposed in this Pull Request:
Closes #2502 .
Replace this with a good description of your changes & reasoning.
Screenshots:
Detailed test instructions:
Daily average cost
input will have the pre-populated value with highest budget in the recommended budgets. For example:Most merchants targeting similar countries set a daily budget of 6 USD
(As next highest value is 6).Additional details:
Changelog entry