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

Financial type hook clean up and fix towards dev/core#2454 Extend financial acls view limitations to ContributionR… #19788

Merged
merged 2 commits into from
Mar 12, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

This does some cleanup on the financial type acl code moving the addition of permissions to a hook in the extension and adds ContributionRecur to the entities which get financial_type_id added as a filter for non-permissioned users

Before

Code all in core. No handling for contribution recur entity

After

Contribution recur should be limited via the api / any functions that actually call the api with checkPermissions (which many likely don't)

Technical Details

Comments

@civibot
Copy link

civibot bot commented Mar 12, 2021

(Standard links)

@civibot civibot bot added the master label Mar 12, 2021
@eileenmcnaughton
Copy link
Contributor Author

test this please

@colemanw
Copy link
Member

@eileenmcnaughton is this good, bad, or neutral vis-a-vis our goal of migrating the one-off financial type ACL code to the standard api permission model using BAO::getSelectWhereClause().

@eileenmcnaughton
Copy link
Contributor Author

@colemanw mostly neutral - in that this deals with the available permissions declaration, not the application of them.

However, the ongoing more of financial type acl code out of core to the core extension is working towards the right implementation

@colemanw
Copy link
Member

Ok, I see this is mostly about restructuring code not changing functionality. Merge on pass.

@eileenmcnaughton
Copy link
Contributor Author

@colemanw - I have the next commit - 8c0c0f6 - but now have some more thoughts so still working on it (I am worried that sites need to be able to remove the super permission - so will rework to that

@eileenmcnaughton eileenmcnaughton merged commit 77d871a into civicrm:master Mar 12, 2021
@eileenmcnaughton eileenmcnaughton deleted the per_fin_types branch March 12, 2021 04:04
return;
}
$actions = [
'add' => ts('add'),
Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton just a small thing this probably should be E::ts rather than ts given we are in an extension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I guess so - it works like that for core ext?

Copy link
Member

Choose a reason for hiding this comment

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

Yes it does.

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.

3 participants