Skip to content
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

Incorrect dimension filter applied in various components that uses adSourceName dimension #8658

Closed
3 tasks
kuasha420 opened this issue May 6, 2024 · 4 comments
Labels
Module: AdSense Google AdSense module related issues Module: Analytics Google Analytics module related issues P0 High priority Squad 1 (Team S) Issues for Squad 1 Type: Bug Something isn't working

Comments

@kuasha420
Copy link
Collaborator

kuasha420 commented May 6, 2024

Bug Description

In the following components, the dimensionFilters is applied in the report args incorrectly, resulting them to be no-op.

  • AnalyticsAndAdSenseAccountsDetectedAsLinkedOverlayNotification
  • DashboardTopEarningPagesWidgetGA4
  • TopEarningContentWidget

The dimension filters should be placed inside the dimensionFilters property in the report args object. Also for a little bit of simplification, the logic can be rewritten to be the following shorthand syntax, since the filter is a exact match string filter covered by Google\Site_Kit\Modules\Analytics_4\Report\Request::parse_dimension_filter:

		dimensionFilters: {
			adSourceName: `Google AdSense account (${ adSenseAccountID })`,
		},

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • Dimension filter for the Analytics reports in AnalyticsAndAdSenseAccountsDetectedAsLinkedOverlayNotification, DashboardTopEarningPagesWidgetGA4 and TopEarningContentWidget should be applied correctly.

Implementation Brief

  • Update the report args in assets/js/components/OverlayNotification/AnalyticsAndAdSenseAccountsDetectedAsLinkedOverlayNotification.js, assets/js/modules/adsense/components/dashboard/DashboardTopEarningPagesWidgetGA4.js and assets/js/modules/adsense/components/widgets/TopEarningContentWidget.js file:
    • Remove the filter property from the report options.
    • Add the dimensionFilters object to the report option, and add the adSourceName property with Google AdSense account (${ adSenseAccountID }) value inside the dimensionFilters object.

Test Coverage

  • Update all the relevant test and stories for the aforementioned components.

QA Brief

QAing the changes here specifically would require a site that would render the Top Earning Pages Widget and Top Earning Content Widget on the Site Kit dashboard, then testing the output before/after this change. That's probably not realistic, but checking for the filters being correct is enough really, so best to just test that the components are still rendering correctly without errors, and the new filters are working without issue.

  • Set up Site Kit on a site with existing Analytics and AdSense data.
  • Visit the main Site Kit dashboard.
  • Ensure the Top Earning Pages Widget and Top Earning Content Widget render without errors.

Changelog entry

  • Update reports that use the adSourceName dimension to use the correct dimension filter.
@kuasha420 kuasha420 added Type: Bug Something isn't working P0 High priority Module: Analytics Google Analytics module related issues Module: AdSense Google AdSense module related issues labels May 6, 2024
@kuasha420 kuasha420 assigned kuasha420 and unassigned kuasha420 May 6, 2024
@tofumatt tofumatt self-assigned this May 6, 2024
@tofumatt
Copy link
Collaborator

tofumatt commented May 6, 2024

IB ✅

@tofumatt tofumatt removed their assignment May 6, 2024
@benbowler benbowler assigned benbowler and unassigned benbowler May 9, 2024
@eclarke1 eclarke1 added the Squad 1 (Team S) Issues for Squad 1 label May 14, 2024
@tofumatt tofumatt self-assigned this May 20, 2024
@tofumatt tofumatt removed their assignment May 24, 2024
@benbowler benbowler assigned benbowler and unassigned benbowler May 28, 2024
@mohitwp mohitwp self-assigned this May 28, 2024
@mohitwp
Copy link
Collaborator

mohitwp commented May 29, 2024

QA Update ✅

  • Tested on dev environment.
  • Verified DashboardTopEarningPagesWidgetGA4 and TopEarningContentWidget on main dashboard.
  • Verified DashboardTopEarningPagesWidgetGA4 and TopEarningContentWidget on entity dashboard.
  • Verified both widgets are getting render without any error.
  • Verified both widgets content is same on latest and dev environment.
  • Verified all links under both widgets.

Develop :

Top Earning pages widget

image

Top Earning Content widget

image

Latest :

Top Earning pages widget

image

Top Earning Content widget

image

View Only Dash board

image

image

@mohitwp mohitwp removed their assignment May 29, 2024
@hussain-t
Copy link
Collaborator

@10upsimon, @tofumatt, as discussed on Slack, the changes in the associated PR have caused VRT failures. Therefore, I’m moving this ticket back in execution to update the VRT images accordingly.

@10upsimon 10upsimon removed their assignment May 30, 2024
@benbowler benbowler assigned benbowler and unassigned benbowler May 30, 2024
@techanvil techanvil self-assigned this May 30, 2024
@techanvil
Copy link
Collaborator

As this was previously in Approval and the only change in the followup was to update the VRT images, and with CI confirmed passing for the update, I'm moving this back to Approval.

@techanvil techanvil removed their assignment May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: AdSense Google AdSense module related issues Module: Analytics Google Analytics module related issues P0 High priority Squad 1 (Team S) Issues for Squad 1 Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

10 participants