Fix financial acl permissions to respect check_permissions #14118
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This enables a test whereby check_permissions is passed in & fixes by following the todo. It also removes a couple of lines whereby the permissions are double-added in the contribution where clause - but only if a field for which no special handling exists -we should rely on the main initialize function (& over time make the inclusion more generic & consistent with our selectWhere approach) to avoid 1) unnecessary joins 2) ignoring the check_permission flag
|
(Standard links)
|
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Apr 24, 2019
…essor id This is preliminary cleanup towards exposing cancel_reason as a search field, as committed to in proposal to add the field. This change - adds unit tests for the trxn_id & processor_id fields - makes the handling of them more generic - adds contribution recur field data to the contribution query object so it can be accessed. Some points about the last of these. Basically we have a situation where the query object needs metadata to avoid field by field handling. We have done this is some wierd & wonderful ways with dependencies on 'import' & 'export' metadata info to the point where they were given to so many fields as to become meaningless. I think this method is better - ie. just adding the additional metadata to the query class for all the entities it deals with. A recently merged PR means 'where' data is still available using this method. Also I have added uniquenames to the contribution recur fields. Uniquenames no longer really matter outside - the query object - profiles - possibly import & export We only really expose recurring contribution info through the query object of these so it should be safe. In a future happy space apiv4 & namespacing will render uniquenames a distant memory. I couldn't rip out the special handling entirely without dealing with the labels that go into the qill. I figure we should merge this & then address that - but briefly the issue is that Matt has just changed all the labels to not quite work in this context so we need to discuss / agree how to manage 'Recurring Contribution Trxn ID' vs 'Trxn ID'. I also didn't want to rip out the special handling fully in advance of civicrm#14118 being merged as there could be a performance regression if we let more code hit that unnecessary join line
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Apr 25, 2019
…essor id This is preliminary cleanup towards exposing cancel_reason as a search field, as committed to in proposal to add the field. This change - adds unit tests for the trxn_id & processor_id fields - makes the handling of them more generic - adds contribution recur field data to the contribution query object so it can be accessed. Some points about the last of these. Basically we have a situation where the query object needs metadata to avoid field by field handling. We have done this is some wierd & wonderful ways with dependencies on 'import' & 'export' metadata info to the point where they were given to so many fields as to become meaningless. I think this method is better - ie. just adding the additional metadata to the query class for all the entities it deals with. A recently merged PR means 'where' data is still available using this method. Also I have added uniquenames to the contribution recur fields. Uniquenames no longer really matter outside - the query object - profiles - possibly import & export We only really expose recurring contribution info through the query object of these so it should be safe. In a future happy space apiv4 & namespacing will render uniquenames a distant memory. I couldn't rip out the special handling entirely without dealing with the labels that go into the qill. I figure we should merge this & then address that - but briefly the issue is that Matt has just changed all the labels to not quite work in this context so we need to discuss / agree how to manage 'Recurring Contribution Trxn ID' vs 'Trxn ID'. I also didn't want to rip out the special handling fully in advance of civicrm#14118 being merged as there could be a performance regression if we let more code hit that unnecessary join line
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Apr 25, 2019
…essor id This is preliminary cleanup towards exposing cancel_reason as a search field, as committed to in proposal to add the field. This change - adds unit tests for the trxn_id & processor_id fields - makes the handling of them more generic - adds contribution recur field data to the contribution query object so it can be accessed. Some points about the last of these. Basically we have a situation where the query object needs metadata to avoid field by field handling. We have done this is some wierd & wonderful ways with dependencies on 'import' & 'export' metadata info to the point where they were given to so many fields as to become meaningless. I think this method is better - ie. just adding the additional metadata to the query class for all the entities it deals with. A recently merged PR means 'where' data is still available using this method. Also I have added uniquenames to the contribution recur fields. Uniquenames no longer really matter outside - the query object - profiles - possibly import & export We only really expose recurring contribution info through the query object of these so it should be safe. In a future happy space apiv4 & namespacing will render uniquenames a distant memory. I couldn't rip out the special handling entirely without dealing with the labels that go into the qill. I figure we should merge this & then address that - but briefly the issue is that Matt has just changed all the labels to not quite work in this context so we need to discuss / agree how to manage 'Recurring Contribution Trxn ID' vs 'Trxn ID'. I also didn't want to rip out the special handling fully in advance of civicrm#14118 being merged as there could be a performance regression if we let more code hit that unnecessary join line
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Apr 26, 2019
…essor id This is preliminary cleanup towards exposing cancel_reason as a search field, as committed to in proposal to add the field. This change - adds unit tests for the trxn_id & processor_id fields - makes the handling of them more generic - adds contribution recur field data to the contribution query object so it can be accessed. Some points about the last of these. Basically we have a situation where the query object needs metadata to avoid field by field handling. We have done this is some wierd & wonderful ways with dependencies on 'import' & 'export' metadata info to the point where they were given to so many fields as to become meaningless. I think this method is better - ie. just adding the additional metadata to the query class for all the entities it deals with. A recently merged PR means 'where' data is still available using this method. Also I have added uniquenames to the contribution recur fields. Uniquenames no longer really matter outside - the query object - profiles - possibly import & export We only really expose recurring contribution info through the query object of these so it should be safe. In a future happy space apiv4 & namespacing will render uniquenames a distant memory. I couldn't rip out the special handling entirely without dealing with the labels that go into the qill. I figure we should merge this & then address that - but briefly the issue is that Matt has just changed all the labels to not quite work in this context so we need to discuss / agree how to manage 'Recurring Contribution Trxn ID' vs 'Trxn ID'. I also didn't want to rip out the special handling fully in advance of civicrm#14118 being merged as there could be a performance regression if we let more code hit that unnecessary join line
|
@pradpnayak @monishdeb could one of you review this? |
|
looks good to me. did a quick simple test from ui and api, it worked when ACL financial type is enabled or disabled. |
|
Thanks @pradpnayak |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Fixes bug where check_permissions = 0 is ignored when doing contribution.get in conjunction with financial acls
Before
Flag ignored
After
Flag heeded
Technical Details
This enables a test whereby check_permissions is passed in & fixes by following the todo. It also removes
a couple of lines whereby the permissions are double-added in the contribution where clause - but
only if a field for which no special handling exists -we should rely on the main initialize function
(& over time make the inclusion more generic & consistent with our selectWhere approach) to avoid
Comments
@monishdeb @pradpnayak can one of you review?