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 more options for global URL parameter exclusions #22729

Open
wants to merge 52 commits into
base: 5.x-dev
Choose a base branch
from

Conversation

caddoo
Copy link
Contributor

@caddoo caddoo commented Oct 30, 2024

Description:

This PR introduces configurable exclusion types for query parameters, offering users predefined options to tailor which parameters are excluded from tracking and reporting. Previously, users could only manually define a custom list of exclusions. Now, with this addition, we provide options that cater to broader needs, such as excluding common PII (Personally Identifiable Information) parameters, to make configuration easier and more accessible.

Key Changes and Decisions

  • Original Intention for Parameter Retention: Initially, I considered allowing excluded parameters to remain in place when switching between non-custom exclusion types. This would let users easily toggle exclusion types without needing to reconfigure the list. However, to maintain backend simplicity, I opted not to retain excluded parameters across types.

  • Backwards Compatibility: This update is designed to be backwards compatible. If a user already has custom exclusions configured, the system will infer the exclusion type as "custom," ensuring that existing setups continue to function seamlessly without manual adjustments.

  • New Vue Component – ExcludedQueryParameterSettings: I created a dedicated Vue component to handle the exclusion configuration, as the ManageGlobalSettings component had become quite large and was handling multiple responsibilities. This new component helps isolate exclusion-related logic, making the codebase easier to maintain and extend.

  • Duplication of Exclusions Type List: The list of exclusion type parameters appears in both the backend and frontend. I chose to keep them separate for better readability and maintainability, especially from a development perspective. Since the list of excluded parameters is unlikely to change frequently, this approach should not introduce significant maintenance overhead.

  • Tracker Cache Expiry: After setting the exclusion type, I've expired the tracker cache. I'm not sure if this is needed?

Screen Recording 2024-11-04 at 10 07 56 PM

Review

@caddoo caddoo force-pushed the dev-18646-add-button-add-list-of-exclusions-globally branch 6 times, most recently from 012110b to f6b5ff4 Compare November 4, 2024 01:36
@caddoo caddoo force-pushed the dev-18646-add-button-add-list-of-exclusions-globally branch from f6b5ff4 to 3abdabe Compare November 4, 2024 01:42
@caddoo caddoo changed the title Add buttons to add/remove common PII query params for global settings Add more options for global URL parameter exclusions Nov 4, 2024
@caddoo caddoo force-pushed the dev-18646-add-button-add-list-of-exclusions-globally branch from 0a0f2c0 to 8269536 Compare November 4, 2024 05:03
@caddoo caddoo marked this pull request as ready for review November 4, 2024 09:14
@caddoo caddoo requested a review from a team November 4, 2024 09:14
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

@caddoo As you've requested a review, I've decided to roughly look through the changes and already leave some comments/thoughts even though the PR might not yet be fully finished.

plugins/SitesManager/API.php Outdated Show resolved Hide resolved
plugins/SitesManager/SitesManager.php Outdated Show resolved Hide resolved
plugins/SitesManager/API.php Outdated Show resolved Hide resolved
plugins/SitesManager/tests/UI/SitesManager_spec.js Outdated Show resolved Hide resolved
plugins/SitesManager/API.php Outdated Show resolved Hide resolved
@caddoo caddoo added this to the 5.2.0 milestone Nov 7, 2024
@caddoo caddoo requested a review from a team November 7, 2024 19:35
config/global.ini.php Outdated Show resolved Hide resolved
plugins/SitesManager/API.php Outdated Show resolved Hide resolved
plugins/SitesManager/API.php Outdated Show resolved Hide resolved
plugins/SitesManager/API.php Show resolved Hide resolved
plugins/SitesManager/API.php Outdated Show resolved Hide resolved
plugins/SitesManager/API.php Show resolved Hide resolved
@caddoo caddoo force-pushed the dev-18646-add-button-add-list-of-exclusions-globally branch from 3a0d40e to b80ece9 Compare November 11, 2024 01:20
@caddoo caddoo force-pushed the dev-18646-add-button-add-list-of-exclusions-globally branch 2 times, most recently from 1e2954b to 0998402 Compare November 11, 2024 02:28
@caddoo caddoo requested a review from mneudert November 11, 2024 02:30
plugins/SitesManager/API.php Outdated Show resolved Hide resolved
plugins/SitesManager/API.php Outdated Show resolved Hide resolved
@@ -73,6 +79,11 @@ public function getGlobalSettings()
$globalSettings['excludedQueryParametersGlobal'] = API::getInstance()->getExcludedQueryParametersGlobal();
$globalSettings['excludedUserAgentsGlobal'] = API::getInstance()->getExcludedUserAgentsGlobal();
$globalSettings['excludedReferrersGlobal'] = API::getInstance()->getExcludedReferrersGlobal();
$globalSettings['exclusionTypeForQueryParams'] = API::getInstance()->getExclusionTypeForQueryParams();

if ($globalSettings['exclusionTypeForQueryParams'] !== 'custom_exclusions') {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ($globalSettings['exclusionTypeForQueryParams'] !== 'custom_exclusions') {
if ($globalSettings['exclusionTypeForQueryParams'] !== SitesManager::URL_PARAM_EXCLUSION_TYPE_NAME_CUSTOM_EXCLUSIONS) {

We should prefer the constant if we have it.

Though I am wondering why we need that check here, and not also in another place where the query parameters are accessed:

public static function getTrackerExcludedQueryParameters($website)
{
$excludedQueryParameters = $website['excluded_parameters'];
$globalExcludedQueryParameters = API::getInstance()->getExcludedQueryParametersGlobal();
$excludedQueryParameters .= ',' . $globalExcludedQueryParameters;
return self::filterBlankFromCommaSepList($excludedQueryParameters);
}

Is there any special case where these two areas should differ in a way that cannot be managed inside the API methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, the controller should be dumb, so I've removed that check. The API handles this already. Take a look and let me know if that's what you are thinking, and you can resolve the thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants