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

Remove analytics module from the codebase #7843

Closed
zutigrm opened this issue Nov 8, 2023 · 9 comments
Closed

Remove analytics module from the codebase #7843

zutigrm opened this issue Nov 8, 2023 · 9 comments
Labels
P2 Low priority Type: Infrastructure Engineering infrastructure & tooling

Comments

@zutigrm
Copy link
Collaborator

zutigrm commented Nov 8, 2023

Feature Description

Some things will not be needed since feature flag is removed. For example provisioning scope, UAProperty, and some test cases in assets/js/modules/analytics/datastore/accounts.test.js

Existing jest tests should be revisited as well.


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

Acceptance criteria

  • The analytics module which has by now become obsolete and replaced by analytics-4 should be entirely removed from the Site Kit plugin. This should include but may not be limited to:
    • The Modules\Analytics module class and all other classes in this namespace in PHP.
    • The modules/analytics datastore.
    • The analytics module on the client-side.

Implementation Brief

  • 7932 will remove most of the Analytics module related code, after it is merged, follow the AC to check if anything remained
  • Remove the pieces linked in the description
  • Check if any analytics components/modules/widgets counterpart are still present and remove them. Example AdminBarSessions should be replaced with AdminBarSessionsGA4, etc
  • Search for any remaining usage of MODULES_ANALYTICS store selector and replace it with MODULES_ANALYTICS_4
  • Remove assets/js/components/adminbar/common.stories.js and assets/js/components/wp-dashboard/common.stories.js
  • Search for modules/analytics/data/ through the codebase to identify presence of old Analytics route usage and update/remove the code accordingly

Test Coverage

  • In e2e plugin tests/e2e/plugins/module-setup-analytics-no-account.php, rename e2e/setup/analytics/account-created endpoint to e2e/setup/analytics-4/account-created. And replace its usage in e2e tests where applicable.
  • In tests/e2e/plugins/module-setup-analytics.php
    • Cleanup the remaining analytics routes, like properties-profiles, and profiles, but leave accounts-properties-profiles unless it was completely replaced by account-summaries, as at the time of writing we still use accounts from that response. If this is completely replaced, we can remove that endpoint too.
    • Remove the usage of these routes if they are still present. At the time of writing they are still used only in jest tests for accounts and account select.
  • Remove the usage of analytics/data/goals route from e2e tests, as it was used only by old analytics, GA4 does not have goals.
  • Remove analytics/data/report route from the e2e tests if still present
  • In .storybook/fetch-mocks.js, replace analytics with analytics-4
  • Replace any outstanding usage of MODULES_ANALYTICS store with MODULES_ANALYTICS_4 in stories and jest tests. Some of the files at the time of writing:
    • assets/js/components/ReportError.test.js
    • assets/js/components/ReportError.stories.js
    • assets/js/components/dashboard-sharing/DashboardSharingSettingsButton.stories.js
    • assets/js/components/dashboard-sharing/DashboardSharingSettings/index.stories.js
    • assets/js/components/notifications/GoogleTagIDMismatchNotification.stories.js
    • assets/js/components/notifications/WebDataStreamNotAvailableNotification.stories.js
    • assets/js/components/notifications/ZeroDataStateNotifications/index.stories.js

QA Brief

  • This PR removes the legacy analytics module. No specific checks are needed here. The SAM E2E testing should cover testing this.

Changelog entry

  • Remove the legacy analytics module (UA) from Site Kit.
@zutigrm zutigrm added the Type: Enhancement Improvement of an existing feature label Nov 8, 2023
@aaemnnosttv aaemnnosttv added P1 Medium priority Type: Infrastructure Engineering infrastructure & tooling and removed Type: Enhancement Improvement of an existing feature labels Nov 16, 2023
@nfmohit nfmohit changed the title Remove Unused Code After Removal of ga4Reporting Feature Flag Remove analytics module from the codebase Nov 29, 2023
@bethanylang bethanylang added P1 Medium priority and removed P1 Medium priority labels Nov 29, 2023
@bethanylang bethanylang added the Next Up Issues to prioritize for definition label Dec 12, 2023
@nfmohit nfmohit removed their assignment Dec 23, 2023
@tofumatt tofumatt self-assigned this Dec 28, 2023
@tofumatt
Copy link
Collaborator

I think the ACs here are good, it's tough to be exhaustive when writing these but this should cover it 😄

ACs 👍🏻

@tofumatt tofumatt removed their assignment Dec 28, 2023
@zutigrm zutigrm assigned zutigrm and unassigned zutigrm Jan 2, 2024
@techanvil techanvil self-assigned this Jan 2, 2024
@techanvil
Copy link
Collaborator

Hey @zutigrm, this IB is looking good. A few points:

  • It could be worth specifying a search for modules/analytics/data/ to identify any potentially remaining places where the old Analytics paths are being referenced and update/remove the code accordingly.
  • This should be somewhat implicit, but it would be worth making explicit that when replacing analytics with analytics-4, we should check that the code is also still in use. For example assets/js/modules/analytics/hooks/useExistingTagEffect.js can be completely removed rather than migrated, as it's superceded by assets/js/modules/analytics-4/hooks/useExistingTagEffect.js.
  • It's a minor point, but there doesn't really seem to be a need to single out assets/js/modules/analytics/datastore/goals.test.js in the IB, as all tests under assets/js/modules/analytics/datastore/ should be removed anyway by following the AC point to remove the modules/analytics datastore.

@techanvil techanvil assigned zutigrm and unassigned techanvil Jan 2, 2024
@zutigrm
Copy link
Collaborator Author

zutigrm commented Jan 3, 2024

@techanvil Thanks for the feedback.

It could be worth specifying a search for modules/analytics/data/ to identify any potentially remaining places...

Thanks, that's a good idea!

This should be somewhat implicit, but it would be worth making explicit that when replacing analytics with analytics-4, we should check that the code is also still in use. For example assets/js/modules/analytics/hooks/useExistingTagEffect.js can be completely removed rather than migrated, as it's superceded by assets/js/modules/analytics-4/hooks/useExistingTagEffect.js.

I actually didn't mention that, because it will be taken care of in #7932, it removes the entire analytics module (IB mentions moving over needed parts and removing assets/js/modules/analytics/, assets/js/googlesitekit-modules-analytics.js, etc), so functions/datastore, etc will be transferred by the time this issue is executed, in this issue, I just tried to cover some remaining pieces outside the module folders, which might only reference the old datastore

It's a minor point, but there doesn't really seem to be a need to single out assets/js/modules/analytics/datastore/goals.test.js in the IB, as all tests under...

Thanks, good catch! Removed

I updated the IB

@zutigrm zutigrm assigned techanvil and unassigned zutigrm Jan 3, 2024
@techanvil
Copy link
Collaborator

Great, thanks for updating/clarifying, @zutigrm. This IB LGTM ✅

@techanvil techanvil removed their assignment Jan 3, 2024
@bethanylang bethanylang added P2 Low priority and removed P1 Medium priority labels Jan 12, 2024
@ivonac4 ivonac4 added the QA: Eng Requires specialized QA by an engineer label Jan 16, 2024
@bethanylang bethanylang removed the Next Up Issues to prioritize for definition label Jan 19, 2024
@hussain-t hussain-t self-assigned this Feb 12, 2024
@bethanylang bethanylang removed the Next Up Issues to prioritize for definition label Feb 13, 2024
@hussain-t
Copy link
Collaborator

hussain-t commented Feb 15, 2024

Hi @jimmymadon, when you take over this, please make sure to replace adsenseLinked with adSenseLinked and adsenseLinkedLastSyncedAt with adSenseLinkedLastSyncedAt throughout the plugin. Thanks! cc: @zutigrm

@zutigrm
Copy link
Collaborator Author

zutigrm commented Feb 23, 2024

PR is ready, I am keeping it in Execution in the meantime until 7932 is merged. Check this update for more details

@aaemnnosttv aaemnnosttv assigned nfmohit and unassigned zutigrm Mar 4, 2024
hussain-t added a commit that referenced this issue Mar 7, 2024
hussain-t added a commit that referenced this issue Mar 7, 2024
…ogle/site-kit-wp into enhance/#7843-remove-analytics-module.
hussain-t added a commit that referenced this issue Mar 8, 2024
@hussain-t hussain-t assigned hussain-t and nfmohit and unassigned nfmohit and hussain-t Mar 8, 2024
hussain-t added a commit that referenced this issue Mar 8, 2024
@mohitwp mohitwp self-assigned this Mar 12, 2024
@nfmohit nfmohit removed their assignment Mar 12, 2024
@wpdarren wpdarren removed the QA: Eng Requires specialized QA by an engineer label Mar 13, 2024
@mohitwp mohitwp removed their assignment Mar 18, 2024
@mohitwp
Copy link
Collaborator

mohitwp commented Mar 18, 2024

QA Update ✅

  • Tested on feature branch.
  • Verified analytics functionality as part of E2E testing of SAM module.

@aaemnnosttv
Copy link
Collaborator

This LGTM although I did notice we're still bundling the legacy client services which should no longer be needed.

'Analytics',

We can clean this up in a follow-up. cc: @nfmohit

@nfmohit
Copy link
Collaborator

nfmohit commented Apr 2, 2024

This LGTM although I did notice we're still bundling the legacy client services which should no longer be needed.

'Analytics',

We can clean this up in a follow-up. cc: @nfmohit

Thank you for pointing this out, @aaemnnosttv. I've opened #8459 to address this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Low priority Type: Infrastructure Engineering infrastructure & tooling
Projects
None yet
Development

No branches or pull requests

10 participants