-
Notifications
You must be signed in to change notification settings - Fork 284
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
Analytics widgets not displaying in view-only dashboard without UA connected #6824
Comments
@nfmohit On the latest develop, in my testing, this behaviour happens even when UA is connected. This is occurring on the latest Screen.Recording.2023-04-06.at.23.34.14.movc.c. @aaemnnosttv @eclarke1 UPDATE: I can see you've set #6687 as a blocker to this issue. I think this should be the other way round as the errors within #6687 do not appear anymore (and hence cannot be fixed as such) since we are having different permission errors for analytics which are part of this issue. |
Thanks @jimmymadon – this one is ready to go if you'd like to write the IB for it. I've removed the blocker that was assigned here since it's in QA now anyways. |
In addition to the problem @nfmohit has identified in the Issue Description (conditionally adding analytics or analytics-4 within the widget registration params), we need to somehow fetch the new I'm not quite sure what the solution should be here. Do we want to somehow store the above setting as part of the "Sharing Settings" or just expose this individual setting on a separate rest endpoint? |
Thank you for sharing your findings, @jimmymadon. However, in my tests, it seems widgets do show up when UA is connected. Here is a screencast: 1.mp4However, I completely agree that it hasn't occurred as a result of #6745. I just discovered this issue while testing #6745. For now, I have removed #6687 as a blocker for this issue. However, the dependency can be adjusted after further testing. Thanks! |
@techanvil and I chatted about alternative approaches here, like one where we allowed "one of any of these modules" instead of "all of these modules". Unfortunately that wouldn't work for the So this approach will do the trick, and @techanvil pointed out it's straightforward to remove later as well. IB ✅ |
QA Update ❌
Issue 1 : Issue 2 : Top content over the last 28 days widget not loading for 2nd logged in admin and on view only dashboard. |
I opened a follow-up PR which should address some of the issues here. Regarding the gathering state vs zero state for a view-only user, this may be something that we won't be able to have exactly consistent because the gathering state uses data from the property itself which is not available in a shared context. Top content over the last 28 days is showing for me on both view-only and non-viewonly dashboards in GA4 mode. |
QA Update ✅
|
Bug Description
When the Analytics module is connected in Site Kit
with only GA4 without UA, we've encountered issues with Dashboard Sharing not working as expected. #6745 aims to solve the module sharing settings synchronisation for the module owners, but there's another issue where the Analytics widgets do not show up in the view-only dashboard at allif UA is not connected.Steps to reproduce
Screenshots
Additional Context
This happens because the widgets registered for Analytics are specified to the
analytics
module. Example:site-kit-wp/assets/js/modules/analytics/index.js
Lines 65 to 84 in ed6c981
Without UA connected, the
analytics
module doesn't come up as a shareable and a viewable module. We need to update themodules
array so that it conditionally setsanalytics
oranalytics-4
based on the current dashboard view.Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Implementation Brief
Allow the
dashboardView
setting to be retrieved in view-only mode.Update client-side settings to allow initial values to be passed in.
Within
assets/js/googlesitekit/data/create-settings-store.js
:createSettingsStore()
to support an additional optionalsettings
property passed inoptions
.settings
should be an object of setting keys to values, and it should be used as the value ofsettings
ininitialState
.Within
assets/js/googlesitekit/modules/create-module-store.js
:createModuleStore()
to allowsettings
to optionally be passed in, and pass this through tocreateSettingsStore()
.Add the current
dashboardView
setting to the_googlesitekitDashboardSharingData
global.Within
includes/Modules/Analytics.php
:googlesitekit_dashboard_sharing_data
.$this->authentication->is_authenticated()
- if it returns false, the mode is view-only.dashboardView
key to the data object with the value of the currentdashboardView
setting.Provide the
dashboardView
setting to client-side Analytics settings when in view-only mode.Within
assets/js/modules/analytics/datastore/base.js
:dashboardView
is defined on the global_googlesitekitDashboardSharingData
object.settings
object withdashboardView
as the only property to thecreateModuleStore()
call.Replace
GA4DashboardWidgetSwitcher
with an approach that allows the module list for a widget to be defined with the correct Analytics module.Allow widget definitions to include an optional
isActive()
callback which is passed a registryselect
function.Within
assets/js/googlesitekit/widgets/datastore/widgets.js
:registerWidget()
to allow an optionalisActive()
callback to be passed as part ofsettings
.getWidgets()
to usecreateRegistrySelector()
, and update the filtering of the widgets to include a check forwidget.isActive( select )
if theisActive()
callback is defined.Remove
GA4DashboardWidgetSwitcher
and instead create two definitions for each of the widgets, only one of which will be active based on the value ofisGA4DashboardView()
.For each widget that currently uses
GA4DashboardWidgetSwitcher
:registerWidget()
to directly use the UA version of the widget component instead of usingGA4DashboardWidgetSwitcher
.isActive( select )
callback to the definition that returns the result of! isGA4DashboardView()
.registerWidget()
call and modify it to make a GA4 version:GA4
to the widget slug.analytics
toanalytics-4
in the module list.isActive()
callback.Remove
GA4DashboardWidgetSwitcher
.Update GA4 widget shared module checks
Within each of the relevant GA4 widgets, update the call to
canViewSharedModule( 'analytics' )
to instead check theanalytics-4
module. These calls can be found in the following files:assets/js/modules/analytics/components/dashboard/DashboardAllTrafficWidgetGA4/index.js
assets/js/modules/search-console/components/dashboard/SearchFunnelWidgetGA4/index.js
assets/js/modules/search-console/components/dashboard/SearchFunnelWidgetGA4/Overview/index.js
Test Coverage
QA Brief
ga4Reporting
feature flag disabled.ga4Reporting
feature flag disabled, test the "Module Sharing Settings" for Analytics within the Dashboard Sharing Settings modal. Verify that the usual Universal Analytics widgets are rendered for both, the logged in admin as well as the view-only user with who the settings have been shared with (via their role).ga4Reporting
feature flag enabled, ensure the 'Dashboard View' setting toggle is "off" so that Universal Analytics data is still shown on the dashboard. For a new site, this should be the default case anyways. Verify the Universal Analytics widgets are rendered for the admin and view-only user as usual.Changelog entry
The text was updated successfully, but these errors were encountered: