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

Add getReport selector to modules/analytics store #1775

Closed
felixarntz opened this issue Jul 14, 2020 · 6 comments
Closed

Add getReport selector to modules/analytics store #1775

felixarntz opened this issue Jul 14, 2020 · 6 comments
Labels
P0 High priority Type: Enhancement Improvement of an existing feature
Milestone

Comments

@felixarntz
Copy link
Member

felixarntz commented Jul 14, 2020

There should be a getReport selector issuing an API request to our GET:modules/analytics/data/report datapoint, which relies on https://developers.google.com/analytics/devguides/reporting/core/v4/rest/v4/reports/batchGet.

While maintaining backward-compatibility, the datapoint should be adjusted to support a range of arguments closer to the original. At the same time, this should happen in a way where similar arguments are named the same way across different report datapoints (e.g. in Search Console or AdSense).

While not yet fully in its intended a state, a good reference here is the existing getReport selector of the modules/adsense store. The one here should be approached similarly.

While the PageSpeed Insights report endpoint is not time-based and generally simpler than the ones here, it's good to have a look at that respective selector implementation too.


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

Acceptance criteria

  • The modules/analytics JS store should expose a selector getReport( options = {} ).
    • Should use createFetchStore to issue an API request to the GET:modules/analytics/data/report datapoint.
    • Supported arguments (should all be documented on the selector):
      • startDate: Required, unless dateRange is provided. Start date to query report data for as YYYY-mm-dd.
      • endDate: Required, unless dateRange is provided. End date to query report data for as YYYY-mm-dd.
      • dateRange: Required, alternatively to startDate and endDate. A date range string such as 'last-28-days'.
      • compareDateRanges: Optional, only relevant with dateRange. Default false.
      • multiDateRange: Optional, only relevant with dateRange. Default false.
        • In the future, we will revisit date handling so that we'll do the mapping of start and end date parsing on the client and then use startDate and endDate. For now, our code should continue to use dateRange (and compareDateRanges and multiDateRange where needed). startDate and endDate however should already work today, to allow for flexible queries regardless of Site Kit limitations.
      • metrics: Required. List of metrics to query. See https://ga-dev-tools.appspot.com/dimensions-metrics-explorer/ for possible values. Alternatively to a string, each element can also be an object with 'expression' and 'alias' properties. At least one metric is required.
      • dimensions: Optional. List of dimensions to group results by. See https://ga-dev-tools.appspot.com/dimensions-metrics-explorer/ for possible values. Default empty array.
      • orderby: Optional. An order definition object, or a list of order definition objects, each one containing 'fieldName' and 'sortOrder'. 'sortOrder' must be either 'ASCENDING' or 'DESCENDING'. Default empty array.
      • url: Optional. URL to get a report for only this URL. Default empty string.
      • limit: Optional. Maximum number of entries to return. Default 1000.
  • The GET:modules/analytics/data/report datapoint should be modified to support arguments as described above, while maintaining full backward-compatibility. For example:
    • If startDate and endDate are provided, omit parse_date_range logic and related, and set them directly.
    • Support metrics items as string or object/associative array.
    • Support dimensions as either array or string (while only the array should be documented).
    • Support orderby to also be a single object.

Implementation Brief

  • Add a selector getReport( options = {} ) to a new file assets/js/modules/analytics/datastore/report.js.
    • Use createFetchStore to issue an API request to the GET:modules/analytics/data/report datapoint.
    • Supported arguments (should all be documented on the selector):
      • startDate: Required, unless dateRange is provided. Start date to query report data for as YYYY-mm-dd.
      • endDate: Required, unless dateRange is provided. End date to query report data for as YYYY-mm-dd.
      • dateRange: Required, alternatively to startDate and endDate. A date range string such as 'last-28-days'.
      • compareDateRanges: Optional, only relevant with dateRange. Default false.
      • multiDateRange: Optional, only relevant with dateRange. Default false.
      • metrics: Required. List of metrics to query. See https://ga-dev-tools.appspot.com/dimensions-metrics-explorer/ for possible values. Alternatively to a string, each element can also be an object with 'expression' and 'alias' properties. At least one metric is required.
      • dimensions: Optional. List of dimensions to group results by. See https://ga-dev-tools.appspot.com/dimensions-metrics-explorer/ for possible values. Default empty array.
      • orderby: Optional. An order definition object, or a list of order definition objects, each one containing 'fieldName' and 'sortOrder'. 'sortOrder' must be either 'ASCENDING' or 'DESCENDING'. Default empty array.
      • url: Optional. URL to get a report for only this URL. Default empty string.
      • limit: Optional. Maximum number of entries to return. Default 1000.
  • Add support to the GET:modules/analytics/data/report datapoint in PHP to support arguments as described above, while maintaining backward-compatibility for older arguments. For example:
    • If startDate and endDate are provided, omit parse_date_range logic and related, and set them directly.
    • Support metrics items as string or object/associative array.
    • Support dimensions as either array or string (while only the array should be documented).
    • Support orderby to also be a single object.

QA Brief

Changelog entry

  • Add getReport( options ) selector to modules/analytics store for querying Analytics reports.
@felixarntz felixarntz added P0 High priority Type: Enhancement Improvement of an existing feature Next Up labels Jul 14, 2020
@felixarntz felixarntz assigned felixarntz and unassigned felixarntz Jul 14, 2020
@tofumatt tofumatt self-assigned this Jul 14, 2020
@tofumatt
Copy link
Collaborator

I was unclear on what multiDateRange and compareDateRanges should be doing, so I searched the code base for both. In the case of multiDateRange, I can only find it being used in a single place:

with the relevant PHP code here:
if ( ! empty( $data['multiDateRange'] ) ) {
. The implication in the AC seems to be that startDate and endDate don't require either params, and also that startDate and endDate are the end-goal of date code. Given the ACs also mandate they work out-of-the-box and there's only one place to change the approach used, I propose we scrap the older arguments and just use startDate and endDate. It's much easier to understand, easier to implement, and code in the UI can sort out things like last-28-days to create the correct startDate and endDate values.

I wondered about accepting plain date objects rather than formatted YYYY-mm-dd strings, which might be nicer, and we can just parse the date out of them ourselves rather than forcing the developer to do so every time. Thoughts? (This could apply to #1774 and #1176 as well.)

Unless I'm missing something, I think doing that should simplify this issue, reduce technical debt, and reduce code complexity.

@felixarntz
Copy link
Member Author

@tofumatt See my comment on #1774, we should keep these legacy parameters around for now since implementing on the client is not trivial and should happen separately. I'm open to discussing whether we should not document them, but we should implement support for them per the ACs. It shouldn't be complex to implement because the complex parts are all already there - I agree the legacy args are complex to understand, but we'll still need them for now.

@felixarntz felixarntz assigned tofumatt and unassigned felixarntz Jul 14, 2020
@tofumatt
Copy link
Collaborator

Sounds good, I've updated the IB to be more in-line with #1774 then, and changed the startDate and endDate to be strings for now as that IB was already accepted. It's not a big deal, but if we want to use Date objects there in the future should be straightforward to change. 🙂

@tofumatt tofumatt assigned felixarntz and unassigned tofumatt Jul 15, 2020
@felixarntz
Copy link
Member Author

IB ✅

@tofumatt
Copy link
Collaborator

Code review approved; just assigning here to merge the code. 😄

@cole10up
Copy link

Tested

Installed latest SK release candidate, setup and ran a series of regression tests around Analytics.

Installed and checked functionality:
image
image
image
image

Setup a new property:
image
image
Setup a new view:
image
Disconnected the module:
image

No issues discovered

Passed QA ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants