dev/core#830 [REF] Add unit tests on contribution recur trxn_id, contribution recur processor id #14119
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
|
(Standard links)
|
|
Jenkins re test this please |
eileenmcnaughton
force-pushed
the
recur_meta
branch
2 times, most recently
from
df315ab to
f3a07bc
Compare
|
test this please |
…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
force-pushed
the
recur_meta
branch
from
f3a07bc to
23faf53
Compare
|
@seamuslee001 if we merge this I'll take a look at the date fields for contribution recur search |
|
This looks right to me, adding unique aliases for the fields change looks good and is backed up with some tests, I also believe we have enough tests in this area to show if this was to break anything, merging |
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
Clean up & additional tests for recurring contribution search fields
Before
Less tests, more special handling
After
More tests, less special handling
Technical Details
This is preliminary cleanup towards exposing cancel_reason as a search field, as committed to in proposal to add the field. This change
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
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 @mattwire 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 #14118
being merged as there could be a performance regression if we let more code hit that unnecessary join line
UPDATE - the unit tests showed that if I include fields in $fields without uniqueNames they can clobber the fields from other entities. I have added a whitelist to work through this with more care
Comments
@mattwire @colemanw @seamuslee001 @monishdeb FYI - I think this is fairly straightforward but the label follow up might need more discussion.