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

Merged
merged 14 commits into from
Jan 6, 2025
Merged

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

merged 14 commits into from
Jan 6, 2025

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
14. setCustomData
15. setDownloadExtensions
16. addDownloadExtensions
17. removeDownloadExtensions
18. setIgnoreClasses
19. setReferrerUrl
20. setAPIUrl
21. setPageViewId
22. setExcludedReferrers
23. setDownloadClasses
24. setLinkClasses
25. setCampaignNameKey
26. setCampaignKeywordKey
27. setConsentGiven
28. rememberConsentGiven
29. forgetConsentGiven
30. discardHashTag
31. setExcludedQueryParams
32. setConversionAttributionFirstReferrer
33. setDoNotTrack
34. setLinkTrackingTimer
35. killFrame
36. setCountPreRendered
37. setRequestQueueInterval

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like 5.2.1 has been released, so it should probably be updated to 5.2.2-b1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated 👍

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 👍

@AltamashShaikh AltamashShaikh requested a review from snake14 January 3, 2025 05:30
snake14
snake14 reviewed Jan 6, 2025
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 aside from a few of the most recent configs not being in the migration 👍

@AltamashShaikh
Copy link
Contributor Author

Looks good aside from a few of the most recent configs not being in the migration 👍

Thanks @snake14 Added :)

@AltamashShaikh AltamashShaikh requested a review from snake14 January 6, 2025 01:45
snake14
snake14 approved these changes Jan 6, 2025
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 making the requested change. Looks good 👍

@AltamashShaikh AltamashShaikh merged commit 0d0d301 into 5.x-dev Jan 6, 2025
5 checks passed
@AltamashShaikh AltamashShaikh deleted the PG-3907-paq-settings-1 branch January 6, 2025 03:11
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