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
Copy over relevant parts of the Analytics
class into Analytics_4
#7926
Comments
@nfmohit Because this deals with functionality after the removal of UA, but doesn't explicitly specify what that is in the AC nor is blocked by a UA removal issue, can you add the points you mention in the issue description to the ACs? Just so it's extra-clear what should be moved over and what shouldn't be 🙂 Then this should be good-to-go 👍🏻 |
Thank you for the kind review, @tofumatt. I intentionally wanted to leave this out as open-ended so that the specific discovery could be a part of the IB process. There might be other small pieces still needed outside of the methods/pieces that are mentioned in the issue description which might've been missed while authoring the design doc. According to your feedback, I have included them in the ACs, but also left another point about being cautious of any other piece that might still be needed. LMKWYT, thanks! |
Sounds good, that makes sense 👍🏻 Moving to IB 🙂 |
Hey @zutigrm, thanks for drafting this IB. A few notes:
|
@techanvil Thank you for the review.
This seems to be needed, as it is used in Analytics 4 method here
This is being done as part of #7637, which is switching to account summaries, so
This will be done as part of #7932 where final migration and removal of old analytics module is done, which also turns off the internal property and makes it only Analytics module, which makes more sense that is_connected will be checked against accountID, but I can include it here is well, if it makes sense to be done as part of this issue? Let me know what you think |
Thanks @zutigrm.
This
Good spot there, however, if you review the search results for Analytics
Good spot, and seeing as the change to |
@techanvil Got it, thanks! IB updated |
Thanks @zutigrm! IB LGTM. ✅ |
QA Update
|
@mohitwp This is normal as when you edited the settings above as a regression test, you were still editing the settings of the old Analytics module (not the Analytics 4) module. I have just updated the QAB to add a note for QA Eng to test this by saving Analytics 4 module settings. |
QA Update ✅Thanks @jimmymadon !
|
@mohitwp Should this now be in Approval? Thanks! |
@bethanylang It's require |
@mohitwp Ah, missed that, thank you! |
QA:Eng Verified ✅
|
Feature Description
In an effort to use
analytics-4
as the singular Analytics module, the goal is to make theAnalytics
class obsolete and only use theAnalytics_4
class going forward.Even though most of the
Analytics
class is no longer relevant after the removal of UA, there are still some parts that are still relevant, and thus should be copied over toAnalytics_4
. Some notable pieces are:handle_provisioning_callback
method: This method co-exists between bothAnalytics
andAnalytics_4
classes. They should be merged into one removing the need to use a hook to trigger the other.get_debug_fields
method: Since theaccountID
setting will be a part ofAnalytics_4
, itsget_debug_fields
method should be updated to include debug information for this setting.register
method: This method includes different callbacks to hooks that should be carefully evaluated and merged.is_tracking_disabled
&print_tracking_opt_out
methods: These methods implement the Analytics tracking opt-out for certain user groups. They should be merged accordingly.Analytics\Advanced_Tracking
class: This class should be migrated to theAnalytics_4
namespace, but not be instantiated, as it will be updated to work with GA4 as a part of #7145.Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Modules\Analytics
class that are still relevant after the removal of UA should be copied over to theModules\Analytics_4
class.Modules\Analytics_4
class as the only singular Analytics module class keeping in mind that theModules\Analytics
class will no longer be instantiated/will be removed soon (currently planned with #7932).handle_provisioning_callback
method: This method co-exists between bothAnalytics
andAnalytics_4
classes. They should be merged into one removing the need to use a hook to trigger the other.get_debug_fields
method: Since theaccountID
setting will be a part ofAnalytics_4
, itsget_debug_fields
method should be updated to include debug information for this setting.register
method: This method includes different callbacks to hooks that should be carefully evaluated and merged.is_tracking_disabled
&print_tracking_opt_out
methods: These methods implement the Analytics tracking opt-out for certain user groups. They should be merged accordingly.Analytics\Advanced_Tracking
class: This class should be migrated to theAnalytics_4
namespace, but not be instantiated, as it will be updated to work with GA4 as a part of #7145.analytics-4
module doesn't need it to function. If any other method is needed for theanalytics-4
module (e.g. for REST endpoints, populating/updating module settings, etc.), it should be copied to and adapted for theanalytics-4
module.Modules\Analytics_4
retains its functionality even without being present in theModules\Analytics
class, and if it is deemed safe to remove it fromModules\Analytics
, it should be eliminated.Implementation Brief
includes/Modules/Analytics.php
register
methodpropertyID
change, to the Analytics 4 settings change checkPROVISION_ACCOUNT_TICKET_ID
,READONLY_SCOPE
,PROVISION_SCOPE
andEDIT_SCOPE
constantsCopy over the(see comment)GET:accounts-properties-profiles
endpoint definition and it's usage increate_data_request
includes/Modules/Analytics_4.php
Analytics::X
to be used from within the Analytics 4 class where neededTest Coverage
tests/phpunit/integration/Modules/Analytics_4Test.php
tests, copy/adapt relevant parts fromAnalyticsTest
QA Brief
QA Eng
includes/Modules/Analytics_4.php
, within thesetup_info()
method, setinternal
tofalse
. Now go to the Settings Admin screen and you will see a row for Analytics 4. Click on this row and click the 'Edit' button. Ensure the newanalytics-4/SettingsEdit
component is rendered correctly.Changelog entry
The text was updated successfully, but these errors were encountered: