-
-
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
Move financial acl check on Main contribution page to the financial acl extension #27797
Conversation
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
Good to see stuff getting moved out of core! |
} | ||
|
||
/** | ||
* Get a value from the participant being acted on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eileenmcnaughton this doc block comment seems wrong to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seamuslee001 surely you are not accusing me of copy & paste!
use EntityLookupTrait; | ||
|
||
/** | ||
* Get the value for a field relating to the event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again @eileenmcnaughton this doc block seems to be incorrect for this trait as well
ext/financialacls/financialacls.php
Outdated
if (!financialacls_is_acl_limiting_enabled()) { | ||
return; | ||
} | ||
if (str_starts_with($formName, 'CRM_Contribute_Form_Contribution')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm so this would also cover the Backoffice form as well as frontend forms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add an _ here at the end to make sure we just capture front end forms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seamuslee001 I've added the _
per your original suggestion
I think there is a pre-existing but in the backoffice form & it will require unravelling those actions to fix so I scaled this back to the form I'm working on.
Just taking merge on pass off until the issue around whether this is applying to backoffice forms is resolved |
thanks @seamuslee001 am now extending to back office propertly |
27ec40c
to
7b35a6c
Compare
@eileenmcnaughton can you rebase this now please |
Note that after our snaffu with memberships I tested with the setting enabled & disabled
thanks @seamuslee001 - done |
Overview
Move financial acl check on Main contribution page to the financial acl extension
Note that after our snaffu with memberships I tested with the setting enabled & disabled
Before
Code is in core
After
Code is in the extension
Technical Details
Comments