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 for GA4 #6173

Closed
aaemnnosttv opened this issue Nov 18, 2022 · 10 comments
Closed

Add getReport selector for GA4 #6173

aaemnnosttv opened this issue Nov 18, 2022 · 10 comments
Labels
Exp: SP P0 High priority QA: Eng Requires specialized QA by an engineer Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Nov 18, 2022

Feature Description

This issue is for adding the primary selector to the GA4 data store to retrieve GA4 reporting data from the backend.


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

Acceptance criteria

  • The modules/analytics-4 data store should receive a getReport( options ) selector
  • The selector should be fulfilled by the GET:report datapoint for analytics-4 if no report data has been received yet for the given report options

Implementation Brief

  • Make a copy of the assets/js/modules/analytics/datastore/report.js file in assets/js/modules/analytics-4/datastore.
  • In assets/js/modules/analytics-4/datastore/report.js:
    • Remove the definitions of the getPageTitles, isGatheringData, and hasZeroData selectors, for now.
    • Replace all instances of analytics with analytics-4.
    • Replace all instances of MODULES_ANALYTICS with MODULES_ANALYTICS_4.
    • In the getReport resolver, remove the code execution after fetchGetReport is called.

Test Coverage

  • Add test cases for the getReport selector, similar to the one in assets/js/modules/analytics/datastore/report.test.js.

QA Brief

  • Should introduce no changes to the end user.
  • Setup GA4
  • Smoke test Analytics and Dashboard Sharing, which should be unchanged.

QA:Eng Brief

  • Setup GA4
  • Verify the getReport selector is properly registered to the modules/analytics-4 datastore.
  • Verify the getReport selector fetches and returns data from the analytics-4/report endpoint.
  • Verify the getReport selector throws errors when fetched with invalid params

Changelog entry

  • Add getReport selector for Google Analytics 4.
@aaemnnosttv aaemnnosttv added P0 High priority Type: Enhancement Improvement of an existing feature labels Nov 18, 2022
@nfmohit nfmohit assigned nfmohit and unassigned nfmohit Nov 21, 2022
@techanvil
Copy link
Collaborator

Hey @nfmohit, this is looking good. The new selector will also need test coverage so please can you add to the Test Coverage section and consider whether the estimate will need adjusting as a result.

@nfmohit
Copy link
Collaborator

nfmohit commented Nov 21, 2022

Hey @nfmohit, this is looking good. The new selector will also need test coverage so please can you add to the Test Coverage section and consider whether the estimate will need adjusting as a result.

My bad, missed it, my apologies. I've updated the IB. Thank you for noticing, @techanvil!

@nfmohit nfmohit assigned techanvil and unassigned nfmohit Nov 21, 2022
@techanvil
Copy link
Collaborator

Thanks @nfmohit!

IB ✅

@mxbclang
Copy link

@derweili Just noting that this is a blocker for several issues in Sprint 92 along with #6174, so this should be a very high priority for completion post-break. Thank you!

@mxbclang
Copy link

mxbclang commented Jan 3, 2023

Unassigned from @derweili as this is a major blocker for Sprint 92 and he's out this week.

@eclarke1
Copy link
Collaborator

eclarke1 commented Jan 4, 2023

Should this go back into EB if it's unassigned? Worth flagging in the Slack channel too if this is a major blocker :)

@mxbclang
Copy link

mxbclang commented Jan 4, 2023

Thanks @eclarke1, I had forgotten to move it back. :) I flagged it in Slack yesterday but will flag again today seeing as there's been no movement yet.

@hussain-t hussain-t self-assigned this Jan 5, 2023
@aaemnnosttv
Copy link
Collaborator Author

There is a PR in progress for this #6287. During review we decided to make this issue dependent on #6172 but it could be that this one can resume now that 6172 has an IB and is in progress. I see @hussain-t has assigned himself here. @techanvil is working on 6172 and should be able to help answer any questions that may come up in the completion of this one.

@wpdarren
Copy link
Collaborator

wpdarren commented Feb 1, 2023

QA Update: ✅

Verified:

  • I have tested GA4 in a few scenarios
    • GA4 with a brand new site and Google analytics account
    • GA4 with oi.ie to make sure the data appear on main and entity dashboard.
    • Changed the GA4 property in Analytics settings
    • Created a new GA4 property in Analytics settings
  • Tested these scenarios with the ga4Reporting feature flag enabled and disabled.
  • Tested dashboard sharing to ensure no issues, but with a second admin and also editor.
  • Tested with an GA4 existing tag placed on the site to ensure the snippet and text appears.
  • No console warnings or errors appeared.
Screenshots

image
image
image

Left this for QA:Eng

@jimmymadon
Copy link
Collaborator

jimmymadon commented Feb 3, 2023

QA ✅

  • Tested the selector with invalid and valid params and the selector does return what the API endpoint returns. However, the endpoint currently does not return rows correctly so I have created a new issue to fix that.
    UPDATE: The endpoint does return one row for the single metric requested as seen in the screenshot (which is what is expected). During initial testing, the GA4 property ID was not set correctly resulting in 0 rows being returned regardless of the number or combination of metric/dimensions.
Screenshots Screenshot 2023-02-03 at 15 47 35 Screenshot 2023-02-03 at 15 50 43

@jimmymadon jimmymadon removed their assignment Feb 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Exp: SP P0 High priority QA: Eng Requires specialized QA by an engineer Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

9 participants