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

Support financial ACLs #731

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

Support financial ACLs #731

wants to merge 27 commits into from

Conversation

jensschuppe
Copy link
Collaborator

@jensschuppe jensschuppe commented Aug 14, 2024

  • Mandates tab on contact summary
  • Create Mandate form
  • CiviSEPA reports
  • CiviSEPA dashboard
    • contribution list
  • Generating SEPA groups
  • Mandate repairs?
  • Single Mandate edit form
  • Action Provider Find Mandate action

systopia-reference: 25698

@jensschuppe jensschuppe added bug Bug in the code status:needs work There is code, but it needs additional work before it should be reviewed labels Aug 14, 2024
@jensschuppe jensschuppe self-assigned this Aug 14, 2024
@jensschuppe
Copy link
Collaborator Author

jensschuppe commented Aug 14, 2024

ab6ba39 replaces manual SQL queries with API4 calls (with checkPermissions) for the Mandates tab in the contact summary, which should make the financialacls extension add clauses for allowed financial types to resulting SQL queries. This requires civicrm/civicrm-core#30877 for the Financial ACLs extension to create correct queries for JOINs.

The commit also adds a TODO for retrieving how many of the last installments have failed, which is being done with some regex in another SQL query, but does not expose unpermissioned contribution information - the query should be replaced eventually, but does no harm for now.

@jensschuppe jensschuppe force-pushed the apiAndACLs branch 4 times, most recently from 2b32610 to 615c4ab Compare August 20, 2024 09:57
@jensschuppe
Copy link
Collaborator Author

The API4 SepaMandate.get action now has a dedicated action class which adds uniquely aliased joins to civicrm_contribution and civicrm_contribution_recur in order for the Financial ACLs extension to check permissions for joined entities.

The Create Mandate form uses this action for retrieving mandates to clone/replace. Also, the form only allows permissioned financial types to be selected. The API3 action SepaMandate.createfull checks the financial type parameter against those.

@jensschuppe
Copy link
Collaborator Author

The SepaTransactionGroup.get action now has a dedicated action class which adds uniquely aliased joins to SepaContributionGroup and Contribution entities in order for the Financial ACLs extension to check permissions for joined contributions.

The CiviSEPA dashboard uses the SepaTransactionGroup.get API4 action and thus does not display transaction groups with contributions of financial types the user does not have permission for. This approach shows transaction groups with contributions of mixed financial types (and only show the number of permissioned contributions) - I think this is not correct and those groups should be hidden from the user entirely instead.

@jensschuppe
Copy link
Collaborator Author

Refactored the contribution list view per CiviSEPA transaction group to use API4, effectively blocking access to groups with contributions of financial types the user does not have permissions for.

@jensschuppe
Copy link
Collaborator Author

There is a new (global) setting for whether to create transaction groups by financial types instead of by creditor/collection date/mandate type only.

Updating OOFF groups is implemented, implementation for RCUR groups is pending.

Cleanup of contributions still works the same (ensuring each contribution is in only one transaction group).

@jensschuppe
Copy link
Collaborator Author

Report queries seem to be already covered by Financial ACLs.

Copy link
Collaborator

@dontub dontub left a comment

Choose a reason for hiding this comment

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

Currently I cannot judge the questions in the TODOs. Probably I've commented some translations that weren't changed, but just moved to a different position.

CRM/Sepa/BAO/SEPAMandate.php Show resolved Hide resolved
CRM/Sepa/BAO/SEPATransactionGroup.php Outdated Show resolved Hide resolved
CRM/Sepa/BAO/SEPAMandate.php Show resolved Hide resolved
CRM/Sepa/Form/CreateMandate.php Outdated Show resolved Hide resolved
CRM/Sepa/Form/RetryCollection.php Outdated Show resolved Hide resolved
CRM/Sepa/Logic/Batching.php Show resolved Hide resolved
CRM/Sepa/Logic/Batching.php Outdated Show resolved Hide resolved
CRM/Sepa/Logic/Batching.php Outdated Show resolved Hide resolved
}
$this->assign('contact_id', $contactId);
$this->assign(
'financialacls',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd name it financialacls_installed or financialacls_available.

CRM/Sepa/Page/MandateTab.php Outdated Show resolved Hide resolved
@jensschuppe jensschuppe changed the title Respect financial ACLs Support financial ACLs Oct 10, 2024
@jensschuppe
Copy link
Collaborator Author

I've left out issues with translation and naming for now, but they should still be taken care of …

@dontub
Copy link
Collaborator

dontub commented Oct 28, 2024

I've left out issues with translation and naming for now, but they should still be taken care of …

I think at least renaming financialacls to a more appropriate name should be done before merging this.

CRM/Sepa/Logic/Batching.php Show resolved Hide resolved
CRM/Sepa/Logic/Batching.php Show resolved Hide resolved
CRM/Sepa/Logic/Batching.php Show resolved Hide resolved
CRM/Sepa/Logic/Batching.php Show resolved Hide resolved
Civi/Api4/SepaTransactionGroup.php Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug in the code status:needs work There is code, but it needs additional work before it should be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants