-
-
Notifications
You must be signed in to change notification settings - Fork 825
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 weights to membership type links #27231
Conversation
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
021269c
to
98c76f3
Compare
98c76f3
to
ec82ef0
Compare
This includes moving the permissioning of those links to the financialacls extension (since there was already an affected test in that extension too).
ec82ef0
to
ce6cca3
Compare
I had to loosen the type hinting - I could fix pcpLinks to pass objectID but not 'block' |
@eileenmcnaughton also the manage extensions page passes through a string in the objectId not an int |
@eileenmcnaughton this appears to have caused a regression. To reproduce
|
argh the issue is probably that the extension is enabled for everyone - regardless of whether they want these acls - so it needs to check the setting |
@eileenmcnaughton is "the setting" now redundant with "the extension is enabled"? And if so should the upgrade code check the setting, conditionally enable the extension based on the setting, and then we can delete the setting? |
@colemanw the problem is the extension is 'enabled for all' - I'm not sure if we can safely disable it or now - there is still code in core that should be in the extension but perhaps that doesn't cause issues when we disable |
This includes moving the financial acl permissioning of those links to the financialacls extension (since there was already an affected test in that extension too).