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

Create ACL for Klaviyo config settings #264

Merged
merged 3 commits into from
Dec 13, 2023
Merged

Conversation

snoop0x7b
Copy link
Contributor

Description

This pull request creates the ACL that's raised in this issue #160

It creates permissions for each klaviyo configuration area.

Manual Testing Steps

  1. php bin/magento cache:clean
  2. Log into the admin panel
  3. Go to system -> User Roles
  4. Check whether the new klaviyo roles appear (control+f klaviyo)

Pre-Submission Checklist:

  • You've updated the CHANGELOG following the steps here
  • Internal Only - If this is a release, please confirm the following:
    • The links in the changelog have been updated to point towards the new versions
    • The version has been incremented in the following places: module.xml and composer.json

NOTE: Please use the Changelogger cli tool to manage versioned file upgrades.

@snoop0x7b snoop0x7b requested a review from a team as a code owner September 20, 2023 20:37
@cykolln
Copy link
Contributor

cykolln commented Sep 20, 2023

@snoop0x7b Thanks for contributing! This won't work as expected because all sections except General use the same resource id Klaviyo_Reclaim::klaviyo_reclaim when we defined them in system.xml. We should separate these out to match what you have in acl.xml, this should be good to go if you:

  • update system.xml to reference the resource ids you defined in acl.xml, for example: update the <resource> tag for section klaviyo_reclaim_newsletter to be Klaviyo_Reclaim::klaviyo_reclaim_newsletter as defined in acl.xml

etc/acl.xml Outdated Show resolved Hide resolved
@cykolln
Copy link
Contributor

cykolln commented Nov 27, 2023

@snoop0x7b We are looking to release a new version of the module in December and can include these changes once the feedback above has been addressed. Thanks!

@snoop0x7b
Copy link
Contributor Author

@snoop0x7b We are looking to release a new version of the module in December and can include these changes once the feedback above has been addressed. Thanks!

Thanks! I'll take a look and make those changes! Sorry it took me a while to get to this.

@snoop0x7b
Copy link
Contributor Author

Thanks for the feedback on this, I have made the requested updates!

@cykolln
Copy link
Contributor

cykolln commented Dec 13, 2023

@snoop0x7b Looks good to me. Can you please update the changelog and then I can approve!

@smoucka smoucka mentioned this pull request Dec 13, 2023
4 tasks
@cykolln cykolln merged commit d52e176 into klaviyo:master Dec 13, 2023
3 checks passed
@cykolln
Copy link
Contributor

cykolln commented Dec 13, 2023

@snoop0x7b We want to get this in the next release so i'm going to go ahead and merge this in and we will update the changelog in a separate PR to reflect. Thanks for contributing!

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.

2 participants