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

Disable Admin Usage Data Collection by Default #45

Merged
merged 2 commits into from
Oct 9, 2023

Conversation

pykettk
Copy link
Contributor

@pykettk pykettk commented Oct 8, 2023

Description (*)

Updates default behaviour to disable the data collection implemented by Magento_AdminAnalytics.

Fixed Issues

Fixes #43

Manual testing scenarios (*)

  1. Install the 1.0.0 preview and log into the backend as an admin user.
  2. Observe the related configuration option is disabled upon installation
    • Stores -> Settings -> Configuration -> Advanced -> Admin -> Admin Usage

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

@pykettk pykettk requested a review from a team as a code owner October 8, 2023 09:59
Copy link
Contributor

@Vinai Vinai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your PR!
There are a number of additional places that need to be changed, too.

The module is still referenced in the mageos-magento2 root composer.json:
https://github.com/mage-os/mageos-magento2/blob/2.4-develop/composer.json#L114

Also the module PageBuilderAdminAnalytics is built on top of it, and the composer package
mage-os/module-page-builder-admin-analytics depends on mage-os/module-admin-analytics.
https://github.com/mage-os/mageos-magento2-page-builder/tree/develop/app/code/Magento/PageBuilderAdminAnalytics

This module in turn is required in the page-builder metapackage:
https://github.com/mage-os/mageos-magento2-page-builder/blob/develop/app/code/Magento/_metapackage/composer.json#L8

Given it is likely more modules also depend on the AdminAnalytics module, and we want to promote Mage-OS as a drop-in replacement for Magento Open Source, I think it probably is better to short-circuit it instead of removing it.

Maybe we automatically opt-out of data collection as a first step. If we can go further and disable tracking on a deeper level without removing the module that would be good, too.

Removal then should be scheduled for a later date.

@pykettk pykettk force-pushed the remove-admin-analytics-module branch from 93f2ee9 to 6c94cb7 Compare October 8, 2023 16:56
@pykettk
Copy link
Contributor Author

pykettk commented Oct 8, 2023

Thanks, @Vinai ! I've changed the contents of this PR to now disable the collection by default instead of removing the module itself 👍

@pykettk pykettk changed the title Remove Magento_AdminAnalytics Module Disable Admin Usage Data Collection by Default Oct 8, 2023
@pykettk pykettk requested a review from Vinai October 8, 2023 16:57
@Vinai
Copy link
Contributor

Vinai commented Oct 8, 2023

@pykettk Does this hide the modal on the initial login into the admin panel?
If not, I think that would be a good change to include, too.

@pykettk
Copy link
Contributor Author

pykettk commented Oct 8, 2023

@Vinai Just to echo my message elsewhere, it looks like I lost a commit when rebasing 🤦‍♂️ I've just re-committed my change. I had opted to rely on the value of the related admin config option to determine whether to show this block or not, rather than simply never showing it.

@Vinai
Copy link
Contributor

Vinai commented Oct 8, 2023

Thank you! Very good.

I wonder, do we change the URL in here?

            <block name="tracking" as="tracking" template="Magento_AdminAnalytics::tracking.phtml" ifconfig="admin/usage/enabled">
                <arguments>
                    <argument name="tracking_url" xsi:type="string">//assets.adobedtm.com/a7d65461e54e/37baabec1b6e/launch-177bc126c8e6.min.js</argument>
                    <argument name="metadata" xsi:type="object">Magento\AdminAnalytics\ViewModel\Metadata</argument>
                </arguments>
            </block>

I guess we should set up our own tracking as an alternative 🤣

EDIT:
I never looked at the info that was collected before:

$scriptString = '
    var adminAnalyticsMetadata = {
        "secure_base_url": "' . $block->escapeJs($metadata->getSecureBaseUrlForScope()) . '",
        "version": "' . $block->escapeJs($metadata->getMagentoVersion()) . '",
        "product_edition": "' . $block->escapeJs($metadata->getProductEdition()) . '",
        "user": "' . $block->escapeJs($metadata->getCurrentUser()) . '",
        "mode": "' . $block->escapeJs($metadata->getMode()) . '",
        "store_name_default": "' . $block->escapeJs($metadata->getStoreNameForScope()) . '",
        "admin_user_created": "' . $block->escapeJs($metadata->getCurrentUserCreatedDate()) . '",
        "admin_user_logdate": "' . $block->escapeJs($metadata->getCurrentUserLogDate()) . '",
        "admin_user_role_name": "' . $block->escapeJs($metadata->getCurrentUserRoleName()) . '"
    };
';

If someone enables it, then Adobe will suddenly register installs for Community Edition 1.0.0 ...

@pykettk
Copy link
Contributor Author

pykettk commented Oct 8, 2023

Change it for real or to something ineffective? Sorry, tone is hard through text 😅

EDIT: That might be fun 😅 I only looked into this recently when Adobe pushed an update to that external location and broke the admin for many people...

@Vinai
Copy link
Contributor

Vinai commented Oct 8, 2023

Probably we should use some black hole URL.

@pykettk
Copy link
Contributor Author

pykettk commented Oct 8, 2023

Probably we should use some black hole URL.

Any one you're particularly interested in using?

@Vinai
Copy link
Contributor

Vinai commented Oct 8, 2023

Or we could submit even more troll values, like version 2.5.7-p2 enterprise edition.

@Vinai
Copy link
Contributor

Vinai commented Oct 8, 2023

Probably we should use some black hole URL.

Any one you're particularly interested in using?

No idea what to use. I would have to search for one.

@pykettk
Copy link
Contributor Author

pykettk commented Oct 8, 2023

Removing the value renders a <script> tag with an empty src value - perhaps this is sufficient? We could also edit the template to only produce output if the tracking_url argument exists? 🤷‍♂️

@Vinai
Copy link
Contributor

Vinai commented Oct 9, 2023

Let's just remove the script value so it renders an empty script src and not modify the template.

@Vinai Vinai merged commit 4b6a2b4 into mage-os:2.4-develop Oct 9, 2023
4 checks passed
@Vinai Vinai mentioned this pull request Oct 9, 2023
5 tasks
@pykettk pykettk deleted the remove-admin-analytics-module branch October 9, 2023 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove adobe data-collection code and modal
2 participants