Remove inappropriate exception handling. #13442
Conversation
|
(Standard links)
|
|
@eileenmcnaughton this needs a rebase now |
Per discussion in civicrm@570243c#r31805899 the core logic has been changed to now accept a payment regardless of contribution status. This reflects the fact that a payment could be received for a fully paid contribution and we still need to record it. This exception is inappropriately narrow (& likely in the wrong place since it should probably be closer to where the decision is made about adding a payment rather than when it is being processed
eileenmcnaughton
force-pushed
the
paymen_line
branch
from
0f16362
to
db77f7d
Compare
|
done |
|
@JoeMurray @monishdeb can we merge this per previous discussions? |
|
I agree with the objective and approach in the code. I imagine @lcdservices does as well. @Monish? |
|
I definitely concur. |
|
I agree with this patch about removing that exception. Based on @lcdservices @JoeMurray my review I am merging it now. |
|
thanks @monishdeb |
|
Just a quick question: would this also allow for payments against failed contributions? If so I would gladly welcome this change as well. |
|
@magnolia61 good question! I guess it stops it from being blocked at one place in the code - it depends if that is the place blocking it in your flow |
|
@eileenmcnaughton I was thinking about also enabling a Pay Now button for failed contributions from the user dashboard. I will keep it in mind for the partial payment / Pay Now issue #12319 |
|
@magnolia61 so this change DID come off changes @jitendrapurohit proposed for that issue - I just wasn't prepared to see so many changes at once & am working through his changes |
|
Many thanks for splitting the changes into these separate PRs @eileenmcnaughton. Thank you :-) |
|
@jitendrapurohit I know it's kinda messing with you since your code is going stale but I also can't cope with so much change in one PR. The question I really want to resolve next is about what combo of messages going out we should support from Payment api - ie. Payment.confirmation & Contribution.confirmation when completing - there is discussion on it ... somewhere else |
Overview
Remove exception when a payment is recorded through the api and the contribution is not partially paid or pending + pay later
Before
Exception thrown - e.g when adding a payment to a completed contribution
After
Exception not thrown. This check is removed on the basis that the rule is outdated and too narrow
Technical Details
Per discussion in 570243c#r31805899
the core logic has been changed to now accept a payment regardless of contribution status. This reflects the
fact that a payment could be received for a fully paid contribution and we still need to record it.
This exception is inappropriately narrow (& likely in the wrong place since it should probably be closer to
where the decision is made about adding a payment rather than when it is being processed
Comments
@JoeMurray this follows on on our discussion - note that this change looks big because the 'else' got removed - requiring a big whitespace change but this really is just the removal of the exception in line 4501 - https://github.com/civicrm/civicrm-core/pull/13442/files?w=1 for without whitespace