-
Notifications
You must be signed in to change notification settings - Fork 293
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
Ensure Dashboard Sharing works without UA #6745
Comments
I don't think adding another module to the module sharing options is the right choice from a product/UX perspective. It would be a bit confusing and mean:
I think instead we should adjust the logic here… probably what should happen is something similar to #6737 as mentioned in the issue preface. Partly this seems a bit complicated though, if the Additionally, we probably want to transfer settings/enabled flags/etc. between the two modules so they're in-sync whenever the "sharable" Analytics module changes. It would be bad UX to need to "re-share" an already-shared module because the "underlying module" changed. Does that all make sense or am I overcomplicating/misunderstanding something? 😅 |
@tofumatt This is exactly what @aaemnnosttv, @kuasha420, @marrrmarrr and I discussed yesterday on our AC sync. The conclusion was to have a two way sync of the sharing settings like you suggest and similar to the way we are planning to sync the "owner" in #6465. The only concern would be if this approach is "too hard" to implement as a first iteration before GA4 reporting launch. c.c. @nfmohit |
Thank you for your kind feedback, @tofumatt & @jimmymadon! I had some follow-up discussion about this with @aaemnnosttv and we decided it could be ideal to combine your ideas and show the appropriate module ( @tofumatt Over to you for another look, thank you! |
ACs here sound good. I was trying to think if we need any error handling/states/messaging for a case where the user of UA wouldn't have access to GA4… but I don't think that should be possible with how we have Analytics set up, so I think we're good 👍🏻 Moving to IB 👍🏻 |
Note to IB ReviewerAsked in #6687 (comment) to help clarify the relationship between these two issues. |
@nfmohit I think we could perhaps simplify this by applying the change in the reducer for the Thinking about this a bit more, one scenario we need to consider here are for folks that already have Analytics shared, in which case we can't rely entirely on changes to the sharing settings, we want it to work automatically for this case. This could be a one-time backend migration or perhaps something we bake into the backend sharing settings to populate it from GA settings if GA4 isn't present. What do you think? |
Agreed, I've updated this part of the IB accordingly.
Nice catch! I've included in the IB to return GA settings if GA4 isn't present in the backend. Let me know if this sounds good. Thank you, Evan! |
@nfmohit this would work, but we don't really need to modify that class to modify the value. This can be done just about anywhere using the |
Hi @jimmymadon! I was just checking this out and it doesn't seem to be working as expected. Please check the following screencast: I think this happens because this new check wasn't placed before the check if the module is site-kit-wp/assets/js/googlesitekit/modules/datastore/modules.js Lines 1363 to 1376 in 216103d
Instead of the above, I think it should be the following: return Object.keys( modules ).reduce( ( acc, slug ) => {
if (
slug === 'analytics' &&
select( MODULES_ANALYTICS ).isGA4DashboardView()
) {
return { 'analytics-4': modules[ 'analytics-4' ], ...acc };
}
if ( modules[ slug ].shareable && ! modules[ slug ].internal ) {
return { [ slug ]: modules[ slug ], ...acc };
}
return acc;
}, {} ); Let me know what you think. Thanks! (also added a comment in the relevant code review) CC: @techanvil |
Hi folks! I did some further testing after this was merged, and found out that Analytics widgets do not show up in the view-only dashboard if the Analytics module is connected with only GA4 without UA. Since this wasn't mentioned in the ACs and it is out of the scope for this issue, I've opened #6824 to address this. CC: @aaemnnosttv |
QA Update:
|
QA Update:
|
QA Update:
|
Feature Description
Since as a part of this epic, Site Kit will no longer require UA and would work with only GA4 connected, we need to ensure that Dashboard Sharing still works without UA and only GA4.
We're updating the module connection logic for GA4 in #6737 and according to those changes, the Analytics module doesn't show up as a
shareable
module anymore. We need to work around this in one of the two possible ways:internal
modules to be visible, based on the connection of each module.Analytics
module connection logic #6737, we can change the logic for Analytics being shareable similarly based on the availability of theAnalytics_4
module.Both approaches have their own challenges. Showing both modules might not be 100% acceptable from a product perspective, and the second approach might require some more undertaking regarding the permissions check for authentication.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
ga4Reporting
feature flag is enabled:Analytics_4
module should be displayed as a shareable module in the dashboard sharing settings modal.Analytics
andAnalytics_4
simultaneously,Analytics_4
should only be shown if theAnalytics
dashboardView
module setting isgoogle-analytics-4
, otherwise, theAnalytics
module should be shown.Analytics
andAnalytics_4
, similar to the one-way we're doing in Ensure GA4 widgets can display data on the Shared Dashboard. #6687.Implementation Brief
assets/js/modules/analytics-4/index.js
:registerModule()
function, add theIcon
property duplicating that ofassets/js/modules/analytics/index.js
.assets/js/googlesitekit/modules/datastore/modules.js
:getShareableModules()
selector should be updated to do the following:reduce
function, before returning anything yet, check ifmodules[ slug ]
isanalytics
. If so, check if the current dashboard view is GA4 (can be checked using theisGA4DashboardView()
selector from themodules/analytics
store). If that is the case, return theanalytics-4
module instead ofanalytics
, e.g.return { 'analytics-4': modules[ 'analytics-4' ], ...acc };
.assets/js/googlesitekit/modules/datastore/sharing-settings.js
:SET_SHARED_ROLES
andSET_SHARING_MANAGEMENT
such that if themoduleSlug
is eitheranalytics
oranalytics-4
, the updates would also be mirrored to the other. In short, whatever changes are applied toanalytics
should also apply toanalytics-4
and vice-versa.includes/Modules/Analytics_4.php
:register()
method, use the'option_' . Module_Sharing_Settings::OPTION
filter (documentation) to check if the sharing settings don't include theanalytics-4
module but do includeanalytics
. If so, it should include a copy of theanalytics
settings foranalytics-4
in the sharing settings array, and return the updated sharing settings array.Test Coverage
assets/js/googlesitekit/modules/datastore/modules.test.js
:getShareableModules()
to reflect the above changes.assets/js/googlesitekit/modules/datastore/sharing-settings.test.js
:setSharedRoles()
andsetSharingManagement()
actions to reflect the above changes.QA Brief
ga4Reporting
feature flag disabled, test the "Module Sharing Settings" for Analytics within the Dashboard Sharing Settings modal and verify that these settings are fetched, can be edited and saved correctly as before. There should be no change in its behaviour.ga4Reporting
feature flag enabled, ensure the 'Dashboard View' setting toggle is "off" so that Universal Analytics data is still show on the dashboard. For a new site, this should be the default case anyways. Repeat the first QAB point above and test the "Module Sharing Settings" for Analytics within the Dashboard Sharing Settings modal. This should still work as before. However, make a note of the settings that are saved for Analytics.Changelog entry
The text was updated successfully, but these errors were encountered: