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

Created new setting to restrict access to MTM #887

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

Conversation

snake14
Copy link
Contributor

@snake14 snake14 commented Sep 20, 2024

Description:

Created new setting to restrict access to MTM. It's a new dropdown in the MTM section of General Settings. It allows the selection of the minimum level of permission with access to MTM. When a user doesn't have access, the top navigation is removed and all controller and API methods return an error message.

Fixes: #602

Review

@snake14 snake14 marked this pull request as ready for review September 22, 2024 23:11
Copy link
Contributor

@AltamashShaikh AltamashShaikh left a comment

Choose a reason for hiding this comment

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

@snake14 Good work, but I found this feature breaking the Measurables > Settings page if we set the below setting. I assume similar would be the issue for No Data Screen

Screenshot from 2024-09-23 07-29-51

Screenshot from 2024-09-23 07-28-01

Screenshot from 2024-09-23 07-28-07

@snake14
Copy link
Contributor Author

snake14 commented Sep 23, 2024

@snake14 Good work, but I found this feature breaking the Measurables > Settings page if we set the below setting. I assume similar would be the issue for No Data Screen

Good catch @AltamashShaikh . I wondered why there was no permission checks on a couple of the API methods. That's probably why. I could remove the the new validation from those methods, but I wonder if we should hide those MTM sections that make the API call in the first place. I wouldn't think that a user that doesn't have MTM access should be installing the tracking code. What do you think @Stan-vw ?

@snake14
Copy link
Contributor Author

snake14 commented Sep 23, 2024

@snake14 Good work, but I found this feature breaking the Measurables > Settings page if we set the below setting. I assume similar would be the issue for No Data Screen

Good catch @AltamashShaikh . I wondered why there was no permission checks on a couple of the API methods. That's probably why. I could remove the the new validation from those methods, but I wonder if we should hide those MTM sections that make the API call in the first place. I wouldn't think that a user that doesn't have MTM access should be installing the tracking code. What do you think @Stan-vw ?

@AltamashShaikh I went ahead and hid the MTM code in those places.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide Tag Manager Section
2 participants