Create property and measurement ID from SetupBanner #5717
Conversation
|
Size Change: +1.07 kB (0%) Total Size: 1.5 MB
ℹ️ View Unchanged
|
This existed in
I wasn't able to make it show up in my local setup normally as well, so I thought it must be something wrong on my end. In my local setup, both Thank you, @tofumatt! |
There was a problem hiding this comment.
Hey @nfmohit, thanks for the comment. Looking at the issue, indeed, that's largely what this issue is about: adding the "Create property" functionality when there is no property (your scenario when you have no propertyID/webStreamID.
I've left a comment and asked for clarification from @techanvil, but you'll need to add that feature into this PR to get things working.
Let me know if you want a hand with that, we can talk it through either here or on a call if that helps 🙂
| const handleSubmitChanges = useCallback( async () => { | ||
| const { error } = await submitChanges(); | ||
|
|
||
| if ( error ) { | ||
| setErrorNotice( error ); | ||
| } | ||
| }, [ submitChanges ] ); |
There was a problem hiding this comment.
This is the code you added as an action, but submitChanges on its own won't do anything if there are no changes made, which is the case here when there is no GA4 properties setup and the user doesn't have a propertyID/webDataStreamID like you mentioned.
From the Design Doc and Figma Design, it doesn't look like there's any specific UI for this, so the "Create Property" should do whatever the Settings page in Analytics does to create a new property. That means clicking it, UX-wise, should prompt the user for extra permissions and create a GA4 property by redirecting them to Analytics I think.
Whatever the Analytics Settings component already does can be used here I think.
Maybe @techanvil can confirm since he wrote up the Design Doc and has more context, but that should do the trick.
There was a problem hiding this comment.
Hi @tofumatt & @nfmohit - it looks like an implementation detail that is mentioned in the Feature Description has been missed along the way here. This does correlate with what the PropertySelect component is already doing:
For the "no existing GA4 property" variants, i.e when the CTA is Create property, ensure the GA4
propertyIDis set toPROPERTY_CREATEprior to callingsubmitChanges.
Dispatching selectProperty( PROPERTY_CREATE ) will ensure the execution path to create the property will be taken within submitChanges.
site-kit-wp/assets/js/modules/analytics-4/datastore/settings.js
Lines 57 to 61 in 7168169
Make sure to only dispatch this action for the "no existing GA4 property" variants:
I'd refactor it slightly so it's:
} else {
selectProperty( PROPERTY_CREATE );
if ( existingTag ) {
// ...
} else {
// ...
}
}One more aspect to be aware of is that the case where the edit scope is not available is handled in #5278. So this issue only really needs to handle the case where the permission already exists.
In terms of UX the expectation is for the property creation to happen in the background while the spinner is active, it's not necessary to redirect to Analytics for this, although of course a redirect to the OAuth flow will be necessary to grant permissions but as mentioned this will be handled via #5278.
There was a problem hiding this comment.
Thank you, @tofumatt and @techanvil! I have updated this PR to set the GA4 propertyID to PROPERTY_CREATE for no existing GA4 property. The spinner now shows up to create a measurement ID, but shows an OAuth flow or a permissions error, which I believe will be handled in #5278.
There was a problem hiding this comment.
Nice one @nfmohit. In terms of exercising the happy path - I would have thought you'd be able to run through the OAuth flow, grant the permission, and then reload Site Kit with the permissions now granted, at which point you could successfully run through the "Create property" flow...
There was a problem hiding this comment.
All looks good, but one UX question here after a property is successfully created:
CleanShot.2022-08-31.at.21.58.30.mp4
Should we change the text of the "Cancel" button to "Close" or "OK, got it!" once the property has been created? Given the user can't actually update the property once it's made, I wonder if it's even better not to show the select? Here's what happens if I try to create another new property and select it:
CleanShot.2022-08-31.at.22.01.33.mp4
It doesn't allow me to select a property and since there isn't a save button doesn't allow me to update it anyway.
Maybe this is all out-of-scope for this particular issue? If this will be covered by #5621 and other issues then this looks good-to-go mostly, but the current UX feels a bit incomplete and I want to make sure it's alright…
If you think it's good let me know and maybe I'll get @techanvil to do a merge review since he's more familiar 🙂
|
Thank you so much for pointing that out, @tofumatt! In the screen where the user gets to select a property (i.e. a measurement ID has been created and is available), I believe we want to show a CTA there that says Do you agree that would cover it? I can surely go ahead and add it in this PR, but that would be a little duplicate/similarily between both PRs. I'm open to your and @techanvil's suggestions. Thank you! |
There was a problem hiding this comment.
#5282 is about continuing the flow if OAuth permissions aren't set, but there's nothing there about a final "connect" step/button.
The issue I see here is that the existing implementation does more than "create" the property, it actually selects it and saves it in the settings. This can be seen in my previous video, but here's a more clear one where GA4 settings aren't set, and then clicking on the "Create property" button actually saves the setting:
CleanShot.2022-09-01.at.13.37.45.mp4
It seems like that property shouldn't be saved to settings until we hook up the "connect" button, which I think should be a part of this issue as I don't see it mentioned elsewhere.
What I expect to happen is that after clicking "Create property" the user is sent to a screen that looks like this:
Before then, the property would be created but it would NOT be saved in the WordPress/Site Kit settings. The user should need to click "Connect" to actually complete the process. Right now, it happens as soon as they click "Create property".
|
Thank you so much for clearing this, @tofumatt! I did some more thorough reading and observation of the Design Docs and the Figma designs, and I think if there are no existing GA4 properties available and the user creates a new property, we shouldn't display the property select and directly take them to the Thank you! |
Hi @nfmohit, @tofumatt - it's a fair question, but as Nahid has pointed out, in this case the intention is to click on "Create property" and then go directly to the Success banner on success. It's more analogous to the Setup screen where selecting "Set up a new property" and clicking "Configure Analytics" will go straight through to the Congrats banner without showing the new property in the dropdown. setup-new-property.mp4 |
|
Thank you for the confirmation, @techanvil! I have updated the PR to go to the SuccessBanner after a successful submission. |
| import { getBannerDismissalExpiryTime } from '../../../utils/banner-dismissal-expiry'; | ||
| const { useSelect } = Data; | ||
| const { useDispatch, useSelect } = Data; | ||
|
|
||
| export default function SetupBanner( { onCTAClick } ) { |
There was a problem hiding this comment.
I'd suggest renaming onCTAClick to something that better represents what it's used for in the component - how about onSubmitSuccess ?
I've updated it, thank you @techanvil! |
|
Hi @nfmohit, apologies as I had thought this PR was good to go. However I subsequently realised the case where an existing property does exist also needs to be covered (see #5277 (comment)). In order to try to progress this issue today I merged Secondly, please can you verify the GA4 Property Select dropdown and "Connect" button work as expected in the "existing property" flow, i.e. it correctly connects the selected GA4 component, creating it as necessary? I think it should do but if not please can you make whatever change is needed to make this use case work too. With this working, please can you also update the QAB to include the "existing property" flow? |
| export default function SetupBanner( { onSubmitSuccess } ) { | ||
| const [ errorNotice, setErrorNotice ] = useState( null ); | ||
|
|
||
| const ga4MeasurementID = useSelect( ( select ) => |
There was a problem hiding this comment.
As discussed I am not sure this is needed now...
There was a problem hiding this comment.
I've removed this, thank you!
| ctaLink={ onCTAClick ? '#' : null } | ||
| onCTAClick={ onCTAClick } | ||
| ctaComponent={ | ||
| ! ga4MeasurementID && ( |
There was a problem hiding this comment.
As discussed, I am not so sure this condition is required, it seems we should always be rendering the CTA here...
There was a problem hiding this comment.
I initially thought we might need a different functionality for the Connect action instead of submitChanges. I've removed this, thank you!
techanvil
force-pushed
the
new/#5277-setupbanner-create-id
branch
from
c6db855 to
551ae68
Compare
|
I've addressed the suggestions. Thank you, @techanvil! |
Summary
Addresses issue:
Relevant technical choices
This PR adds the property and measurement ID creation mechanism in SetupBanner of GA4 Activation Banner.
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist