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

Display actual number of users coming from search in refactored "Unique Visitors (from Search)" widget #3064

Closed
felixarntz opened this issue Mar 31, 2021 · 11 comments
Assignees
Labels
Module: Analytics Google Analytics module related issues P0 High priority Type: Bug Something isn't working
Milestone

Comments

@felixarntz
Copy link
Member

felixarntz commented Mar 31, 2021

The refactored version of the "Unique Visitors from Search" widget currently uses "Unique Visitors" title, since the widget is (and has always been 🤦‍♂️ ) displaying the total number of users. This is actually wrong, and it should display the number of users that came from Organic Search.

This is a bug in the legacy widget primarily, but it should only be fixed in the refactored version, since that will roll out anyway.


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

Acceptance criteria

  • The Analytics DashboardUniqueVisitorsWidget (visible when enabling widgets.dashboard / widgets.pageDashboard feature flags) should be updated as follows:
    • It should be renamed to DashboardSearchVisitorsWidget throughout the codebase (also related references, e.g. Storybook name).
    • Its title should be changed to "Unique Visitors from Search".
    • It should display metrics for only users that came from Organic Search instead of all users (i.e. for dimensionName of ga:channelGrouping and dimensionValue of Organic Search).
    • Its source link should be updated accordingly:
      • It should link to a specific version of the acquisition-channels report.
      • The URL's _r.drilldown argument should include analytics.trafficChannel:Organic%20Search.
      • The URL should otherwise include the current date parameters and, if relevant, the analytics.pagePath for the current entity URL within _r.drilldown.
        • Multiple _r.drilldown parameters can be included by concatenating with a comma, e.g. _r.drilldown=analytics.trafficChannel:Organic%20Search,analytics.pagePath:~2F.
  • The legacy "Unique Visitors from Search" widget does not need to be updated, even though the same thing is broken there too.

Implementation Brief

Rename assets/js/modules/analytics/components/dashboard/DashboardUniqueVisitorsWidget.js to assets/js/modules/analytics/components/dashboard/DashboardSearchVisitorsWidget.js

Update references in:

  • stories/module-analytics-components.stories.js
  • assets/js/modules/analytics/index.js

Change title and label in

title={ __( 'Unique Visitors', 'google-site-kit' ) }
and in
to
Unique Visitors from Search

Update the commonArgs as both the regular report and sparkline will need to filter for users that only come from organic search,
in


add these after the date params:

dimensions: ['ga:channelGrouping']
dimensionFilters: { 'ga:channelGrouping': 'Organic Search'}

Also make sure that sparklineArgs also retains ga:date dimension

Update serviceURL from visitors-overview to acquisition-channels

serviceURL: store.getServiceReportURL( 'visitors-overview', {

Update _r.drilldown param

'_r.drilldown': url ? `analytics.pagePath:${ getURLPath( url ) }` : undefined,
as specified in AC. No need to urlencode params as there is a function that takes care of that (so it will do the " " -> %20 and "/" -> %2F -> ~2F ). For reference the function is located in /assets/js/modules/analytics/util/report-args.js and looking at the tests for it can give an idea of expected inputs and outputs.:
export const reportArgsToURLSegment = ( reportArgs ) => {

Test Coverage

  • Update backstop screenshots as the component will be updated, you may also need to update the storybook stories.

Visual Regression Changes

  • Labels will update on the widget, requiring a new screenshot.

QA Brief

  • Go to the Site Kit dashboard and find the 'Search Funnel' widget
  • The 'Unique Visitors' widget should now be renamed to 'Unique Visitors from Search'
  • It should also be displaying metrics only for users that came from Organic Search (instead of all users).
  • The source link to 'Analytics' under the sparkline should now point to acquisition-channels rather than visitors-overview.

Changelog entry

  • Update Unique Visitors widget to display the actual number of users coming from search
@felixarntz felixarntz added Type: Bug Something isn't working P0 High priority Module: Analytics Google Analytics module related issues labels Mar 31, 2021
@felixarntz felixarntz assigned felixarntz and unassigned felixarntz Mar 31, 2021
@ivankruchkoff ivankruchkoff self-assigned this Apr 6, 2021
@fhollis fhollis added this to the Sprint 47 milestone Apr 6, 2021
@ivankruchkoff ivankruchkoff removed their assignment Apr 6, 2021
@aaemnnosttv aaemnnosttv self-assigned this Apr 7, 2021
@aaemnnosttv
Copy link
Collaborator

Thanks @ivankruchkoff – a few things to address yet.

  • The IB does not currently address the change to displayed metric itself

    It should display metrics for only users that came from Organic Search instead of all users (i.e. for dimensionName of ga:channelGrouping and dimensionValue of Organic Search).

  • It's important to note that serviceReportURL argument values (e.g. analytics.trafficChannel:Organic%20Search) should not be URL encoded to avoid double encoding
    • This includes / -> ~2F for the page path which is also handled by the selector

@ivankruchkoff
Copy link
Contributor

Thanks @ivankruchkoff – a few things to address yet.

  • The IB does not currently address the change to displayed metric itself

    It should display metrics for only users that came from Organic Search instead of all users (i.e. for dimensionName of ga:channelGrouping and dimensionValue of Organic Search).

  • It's important to note that serviceReportURL argument values (e.g. analytics.trafficChannel:Organic%20Search) should not be URL encoded to avoid double encoding

    • This includes / -> ~2F for the page path which is also handled by the selector

@aaemnnosttv I've made an update, can you confirm that the common report args are the right place to add dimensionName and dimensionValue.

I also added another label that needs updating.

@aaemnnosttv
Copy link
Collaborator

@ivankruchkoff report selectors don't have arguments for dimensionName or dimensionValue. We need to pass these to the dimensions and dimensionFilters respectively, but it's not a 1-1 change. See the docs for these options here:

* @param {Array.<string>} [options.dimensions] Optional. List of dimensions to group results by. Default an empty array.
* @param {Object} [options.dimensionFilters] Optional. Map of dimension filters for filtering options on a dimension. Default an empty object.

I think you also worked on the backend for the dimensionFilters at one point too.

can you confirm that the common report args are the right place to add

Yes it looks like it since these args are shared between reports used for the main data (stat) and for the sparkline chart 👍

@ivankruchkoff
Copy link
Contributor

@ivankruchkoff report selectors don't have arguments for dimensionName or dimensionValue. We need to pass these to the dimensions and dimensionFilters respectively, but it's not a 1-1 change. See the docs for these options here:

* @param {Array.<string>} [options.dimensions] Optional. List of dimensions to group results by. Default an empty array.
* @param {Object} [options.dimensionFilters] Optional. Map of dimension filters for filtering options on a dimension. Default an empty object.

I think you also worked on the backend for the dimensionFilters at one point too.

can you confirm that the common report args are the right place to add

Yes it looks like it since these args are shared between reports used for the main data (stat) and for the sparkline chart 👍

Updated, and yes git blame certainly points at me :) 2421

@aaemnnosttv
Copy link
Collaborator

LGTM 😄

IB ✅

@eugene-manuilov eugene-manuilov removed their assignment Apr 16, 2021
@wpdarren wpdarren self-assigned this Apr 19, 2021
@wpdarren
Copy link
Collaborator

QA Update: Fail ❌

@johnPhillips three observations:

  1. the organic search in the pie chart is displaying 472 users, where the Unique Visitors from Search' data shows 551 unique visitors. Should these be the same? When I ran the report directly in Analytics, 551 was the result, but would like to check because I wonder if the difference will create confusion?

  2. When I click on the Source: Analytics link it takes me to the visitors-overview report in Analytics, but according to the QAB it should be acquisition-channels

  3. The default period for the report in Analytics is 7 days, but in Site Kit, the period was set to 28 days, is this correct?

image

Verified:

The 'Unique Visitors' widget is now be renamed to 'Unique Visitors from Search' - Screenshot

@wpdarren wpdarren assigned johnPhillips and unassigned wpdarren Apr 19, 2021
@johnPhillips
Copy link
Contributor

@wpdarren Thanks for the review.

Something funny might be going on here, I think...
This is my local environment - the figures match up:

Screenshot 2021-04-19 at 16 35 38

The source link under 'Unique visitors from search' points at acquisition-channels, and the duration on the report is 28 days 🙈

Is it possible to share a link to the site you used here?
Also, I wonder if it is worth checking whether there is some sort of cache issue. Perhaps the code on the site you are looking at needs a refresh? I don't know what your workflow is for QA but perhaps you could try clearing the browser cache, session storage etc.

If the code is up to date then this might require someone with a slightly better understanding than me about analytics reporting to have a little look as well. As far as I could see I implemented the IB correctly.
CC @ivankruchkoff I don't know if you can spot any issues with my PR - happy to address them if so 🙏

@wpdarren
Copy link
Collaborator

@johnPhillips let me go through my test site and make sure it is not a caching issue.

@wpdarren wpdarren assigned wpdarren and unassigned johnPhillips Apr 19, 2021
@wpdarren
Copy link
Collaborator

QA Update: Pass ✅

Verified:

  • The 'Unique Visitors' widget is now be renamed to 'Unique Visitors from Search'
  • The 'Unique Visitors from Search' is displaying metrics only for users that came from Organic Search.
  • The source link to 'Analytics' under the sparkline now points to acquisition-channels rather than visitors-overview.

image

@johnPhillips I figured out the issue with why the data was different (needed to set widgets.dashboard in the tester plugin on my test site)

@wpdarren wpdarren removed their assignment Apr 20, 2021
@felixarntz
Copy link
Member Author

felixarntz commented Apr 26, 2021

Approval ❌

There is a problem here related to the source link: It only leads to the filtered view including only "Organic Search" in the single URL view. On the overall dashboard, it still leads to the overall channels view without the "Organic Search" filter like it should.

This is because in https://github.com/google/site-kit-wp/pull/3142/files#diff-05c312086666c0cdc8e39ab72ba4f96e393a1448ace9e0593c2b8618e9c091edR105 the required filter parameter is only added if a single entity URL is present, but it needs to always be added.

@johnPhillips @eugene-manuilov @wpdarren

@felixarntz
Copy link
Member Author

With the additional fix merged, this works as expected now! ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Analytics Google Analytics module related issues P0 High priority Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

7 participants