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.

Sign up for GitHub

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

Jump to bottom

Started adding settings not available in MTM but available via _paq #943

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

Started adding settings not available in MTM but available via _paq #943

wants to merge 11 commits into from

Conversation

AltamashShaikh
Copy link
Contributor

@AltamashShaikh AltamashShaikh commented Dec 2, 2024 •
edited
Loading

Description:

Started adding settings not available in MTM but available via _paq
Fixes: #PG-3907

Added below options under advanced setting in this PR

1. setCustomData
2. setDownloadExtensions
3. addDownloadExtensions
4. removeDownloadExtensions
5. setIgnoreClasses
6. setReferrerUrl
7. setAPIUrl
8. setPageViewId
9. setExcludedReferrers
10. setDownloadClasses
11. setLinkClasses
12. setCampaignNameKey
13. setCampaignKeywordKey

Review

@AltamashShaikh AltamashShaikh marked this pull request as draft December 2, 2024 08:45
@AltamashShaikh AltamashShaikh marked this pull request as ready for review December 12, 2024 07:07
snake14
snake14 reviewed Dec 13, 2024
View reviewed changes
Copy link
Contributor

@snake14 snake14 left a comment

Choose a reason for hiding this comment

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

Looks good. I didn't test all of the settings in the JS, but things look good in the UI and store the values as expected.

The only thing that I think that's missing is a new migration using plugins/TagManager/UpdateHelper/NewVariableParameterMigrator.php.

AltamashShaikh
AltamashShaikh commented Dec 13, 2024
View reviewed changes
Template/Variable/MatomoConfigurationVariable.php Outdated
$field->title = Piwik::translate('TagManager_MatomoConfigurationMatomoAddDownloadExtensionsTitle');
$field->description = Piwik::translate('TagManager_MatomoConfigurationMatomoAddDownloadExtensionsDescription');
$field->customFieldComponent = self::FIELD_VARIABLE_COMPONENT;
$field->uiControlAttributes['showAdvancedSettings'] = 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used to add the settings under Advanced settings as we filter the value based on this property in vue.

snake14
snake14 approved these changes Dec 15, 2024
View reviewed changes
Copy link
Contributor

@snake14 snake14 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 adding the migration. It ran successfully locally 👍

I just left once question in a comment.

Updates/5.2.1.php Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good. Would it be safer to name it 5.2.1-b1 in case there's a beta version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@snake14 Makes sense 👍
Done please check

snake14
snake14 approved these changes Dec 16, 2024
View reviewed changes
Copy link
Contributor

@snake14 snake14 left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snake14 snake14 snake14 approved these changes

Assignees
No one assigned
Labels
Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AltamashShaikh @snake14