CRM-19273 extract code to get financial items and filter those already cancelled. #11300
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
|
test this please |
2 similar comments
|
test this please |
|
test this please |
…y cancelled. This is the first of the broken-out changes in civicrm#10962 that results in an actual functionality change. The code that retrieves the relevant financial items is separated into it's own function. But then a php loop iterates through and eliminates any items that have already been reversed. This is a scenario identified in the unit tests on 10962 and occurs when the the option value has is changed and then changed again - the items from the first change should not be re-changed.
Due to being mis-named these tests have never run. When they do run they expose issues which are addressed in PR 10962. This commits causes the part that actually works in these tests to run.
eileenmcnaughton
force-pushed
the
towards1
branch
from
b0692ef to
e5e7a6a
Compare
|
@eileenmcnaughton I did a extensive review on various pay-later-change-fee-selection scenarios for each kind of price field x their different amounts. Encountered a issue in the parent PR #10962 and mentioned the issue and the fix there. However, this extracted code didn't cause any issue as per my test result. Also the UT is accepting the fix (also ensured that it's not tweaked though to make it pass ;)) Hence I am merging this PR. |
|
Please make necessary adjustment in #10962 by resolving conflicts and including my fix. And assign me back for review. Thanks! |
sluc23
pushed a commit
to ixiam/civicrm-core
that referenced
this pull request
Jan 10, 2018
CRM-19273 extract code to get financial items and filter those already cancelled.
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
This is the first of the broken-out changes in #10962 that results in
an actual functionality change. The code that retrieves the relevant financial items is separated into
it's own function. But then a php loop iterates through and eliminates any items that have
already been reversed. This is a scenario identified in the unit tests on 10962 and occurs when
the the option value has is changed and then changed again - the items from the first change
should not be re-changed.
Before
When an item is changed to a value & changed again the financial item for the reversal will be identified both times
After
Already reversed items are filtered out.
Technical Details
This is a stand-alone part of #10962 and due to the complexity of that PR I have been breaking out small chunks for review. However, the test that demonstrates this can only be run once all parts of that PR are merged, and I don't think this chunk can be UI tested since there are a range of other bugs going on in the overall code until it is all merged. The loop through $financialItemResult looks at each row to check if it was already reversed