Request scope only if it's needed #9840
Conversation
|
Build files for 9a03b2a have been deleted. |
|
Size Change: +1.3 kB (+0.06%) Total Size: 2.03 MB
ℹ️ View Unchanged
|
…if audiences need to be created before optionally requesting the edit scope.
…tom dimension exists.
There was a problem hiding this comment.
Thanks @techanvil – I have a few questions and minor feedback to address.
The main question I have is: how much simpler would this be if we let the scope be requested in response to the request made which requires it rather than attempting to determine if it's needed beforehand?
| invalidateResolution( 'getQueuedNotifications', [ | ||
| viewContext, | ||
| NOTIFICATION_GROUPS.DEFAULT, | ||
| ] ); |
There was a problem hiding this comment.
Why invalidate the default queue? I'd expect this should invalidate the setup CTA resolution, but I'm not entirely sure why it would invalidate this for either.
There was a problem hiding this comment.
I believe this was introduced as part of the banner notification refactor as a way to show the CTA immediately. We do have an issue in IB to address this in a more elegant way: #9453
There was a problem hiding this comment.
Yes; what I mean is that this invalidates the default group, but the CTA is registered with other setup CTAs in their respective group:
site-kit-wp/assets/js/modules/analytics-4/index.js
Lines 730 to 734 in 9a03b2a
Edit: actually, this is about showing the setup success banner right away – which is in the default group – so that makes sense. I missed that detail in your response initially 👍
| return { error: syncError }; | ||
| } | ||
| const availableAudiences = | ||
| select( MODULES_ANALYTICS_4 ).getAvailableAudiences(); |
There was a problem hiding this comment.
Should this not be resolve-selected instead?
There was a problem hiding this comment.
No, it's just a module setting and it'll be set and resolved by the time we get here as we're always syncing the audiences beforehand (the sync is moved to maybeEnableAudienceGroup() but the current order of events with regard to the sync is retained).
| */ | ||
| *enableAudienceGroupMain( failedSiteKitAudienceSlugs ) { | ||
| *getSelectionFromExistingAudiences() { |
There was a problem hiding this comment.
Let's avoid introducing actions that start with get* (other than resolvers). Maybe determine would be a better verb in this case? I'm open to other ideas.
Looking at the description, I'd also prefer to avoid retrieve* actions (I see we already have one) as it's easy to confuse with the common receive* convention we have.
Finally, it seems a bit unintuitive that this gets "selected audiences" but returns "configured audiences". I'd simplify it to just "audiences".
There was a problem hiding this comment.
Let's avoid introducing actions that start with
get*(other than resolvers). Maybedeterminewould be a better verb in this case? I'm open to other ideas.Looking at the description, I'd also prefer to avoid
retrieve*actions (I see we already have one) as it's easy to confuse with the commonreceive*convention we have.
Fair point on get* and retrieve* actions. Actually, taking another look at these, I've realised that they shouldn't really be actions - they can simply be helper functions. They were only introduced when refactoring the actions that call them, and were added as more actions but this was unnecessary; there's no intention for them to be used as actions in the wider codebase. They are returning values so the get* prefix is appropriate when they are helper functions. I've refactored actions to helpers accordingly.
Finally, it seems a bit unintuitive that this gets "selected audiences" but returns "configured audiences". I'd simplify it to just "audiences".
I take your point on the selected/configured audiences ambiguity, but just stripping it to "audiences" removes the specificity. I've changed it to use the term "configured audiences", please see what you think.
| * | ||
| * @return {Object} Object with `needsScope` or `error`. | ||
| */ | ||
| *needsAnalytics4EditScope() { |
There was a problem hiding this comment.
This also has a name closer to what I'd expect to be a selector. Actions should be named using verbs.
There was a problem hiding this comment.
Good point, I've renamed it to determineNeedForAnalytics4EditScope(). Happy to change it to something else if you have a better suggestion!
| setValues( AUDIENCE_SEGMENTATION_SETUP_FORM, { | ||
| autoSubmit: false, | ||
| } ); |
There was a problem hiding this comment.
Can we move this to be set as soon as the autosubmit is invoked from the effect? This is how we do it in many other places and avoids potentially duplicate invocations in some cases.
There was a problem hiding this comment.
I'm hesitant to do this, as it will be changing the current logic which has otherwise been retained despite the refactor.
It's worth noting that the autoSubmit value is used in a number of conditions that relate to showing the Setup CTA banner and the OAuth error modal. It's also set back to false in a number of places. Changing the timing of resetting autoSubmit could easily introduce a bug that would be hard to spot.
I'd rather not risk introducing a bug here and would prefer to leave this as it is. If we do want to revisit the autoSubmit logic, I think this would be best tackled in another issue. Please let me know if you'd like me to create one.
Usage, for reference:
Set to true:
site-kit-wp/assets/js/modules/analytics-4/hooks/useEnableAudienceGroup.js
Lines 123 to 132 in 9a03b2a
Use in a condition:
site-kit-wp/assets/js/modules/analytics-4/hooks/useEnableAudienceGroup.js
Lines 193 to 195 in 9a03b2a
Set to false:
site-kit-wp/assets/js/modules/analytics-4/hooks/useEnableAudienceGroup.js
Lines 105 to 107 in 9a03b2a
There was a problem hiding this comment.
Thanks for the comprehensive response. We can leave it for now, but I think it's worth revisiting to apply consistently and in a way where it isn't possible to auto-submit consecutively more than once (a bug we've had before elsewhere). IMO, it should be disabled in the effect before it invokes the submit action, e.g.
site-kit-wp/assets/js/modules/analytics-4/hooks/useEnableAudienceGroup.js
Lines 193 to 195 in 9a03b2a
Perhaps we should encapsulate this behavior with some kind of core infra to avoid re-implementing it a little differently in each scenario.
| const { error, needsScope, failedSiteKitAudienceSlugs } = | ||
| await maybeEnableAudienceGroup(); |
There was a problem hiding this comment.
What happens if this error is a permission scope error? It seems like we may be making a significant effort to avoid letting this happen naturally.
There was a problem hiding this comment.
It looks like, as things stand, we'd try to show two modals: the Audience Segmentation generic error modal, and the Additional Permissions Required modal.
As I've mentioned in my other comment, we have gone to a significant effort to avoid the default behaviour, but this was by design from the outset. It's also worth noting that much of the error handling is there to support the AS error modals, so it's not the case that it's all there to accommodate the suppression of the Additional Permissions Required modal.
…rather than an action. Also rename it to `getConfiguredAudiencesFromExistingAudiences().
…ather than an action. Also renamed it to `getInitialConfiguredAudiences()`.
… navigating to OAuth.
|
Hey @aaemnnosttv! Thanks for the feedback on this one. To address this question:
The problem with taking the suggested approach would be that it would be at odds with the agreed UX design. We made a clear decision not to show the Additional Permissions Required modal, and instead to simply require the edit scope by default when actioning the setup CTA. This is complimentary to the design where we have the new error modal displayed for a OAuth error, as well as for permission errors that may still occur in an edge case, and the catch all / generic error modal. If we simply let the missing scope be determined by the request being made, AFAIK the only way to automatically handle this is to let the Additional Permissions Required modal be shown. We could do this but it would certainly be reversing a decision that's been made and so we should check in with Mariya and/or Sigal before considering this option. I actually think that showing the Additional Permissions Required modal as well as potentially showing the error modal would not be a great UX, and could itself introduce a bit of additional complexity. We'd need to refactor the current logic, taking care to cover all the various happy and unhappy paths, and making sure it all works with the automatic missing scope handling and the existing modal error handling. This might not be so simple, as we'd want to ensure the Additional Permissions Required wouldn't show up at the same time as another error modal. We could end up with something simpler at the end of it, but it's definitely going to add a lot of additional complexity to get us to that point from where we are now. Plus, we'd be showing the extra modal so our simpler code would be at the expense of a more complicated UX which is what we wanted to avoid from the outset. My opinion is that we should not go down this path, what we've got works according to the design and might not actually be much more complicated than it would be if we did allow the scope to be requested in response to a given request. |
|
Hey @techanvil - a quick response to your thoughts here and then we should be able to wrap this one up :)
Yeah, I understand it was an intentional decision, and I'm not suggesting we change that. I didn't mean to say that we'd necessarily show the permissions modal, but let the error response handle the need on demand, at which point I think we could follow the We can discuss this more outside of this issue, as it is out of scope here anyways. |
Summary
Addresses issue:
Relevant technical choices
[Updated by @techanvil] This PR required a substantial deviation from the IB, because the IB assumes that we'll always want to create the new/returning audiences if they don't exist, whereas in fact we need to first check to see if the selection can be populated from the existing audiences (including user defined and defaults) and only if we don't have an initial selection will we want to create the new/returning audiences.
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