-
-
Notifications
You must be signed in to change notification settings - Fork 824
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
CRM-20252 Improve text when processing possibly-delayed-payments #9971
CRM-20252 Improve text when processing possibly-delayed-payments #9971
Conversation
eileenmcnaughton
commented
Mar 13, 2017
•
edited by civicrm-builder
Loading
edited by civicrm-builder
- CRM-20252: Improve text when processing possibly-delayed-payments
ac3b9ed
to
eaae177
Compare
eaae177
to
7a66680
Compare
test this please |
@@ -284,6 +283,28 @@ public function buildQuickForm() { | |||
$this->assign('friendURL', $url); | |||
} | |||
|
|||
$isPendingOutcome = TRUE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eileenmcnaughton should this be FALSE as the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it should default to !is_pay_later - ie if we are dealing with pay later then by default FALSE, otherwise TRUE & then we check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could make sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested & it is fine for pay later. So, for non-paylater it should reflect the outcome. I think it's safer if any doubt to give the 'we don't know yet' message
'invoice_id' => CRM_Utils_Array::value('invoiceID', $params), | ||
)); | ||
if (CRM_Core_PseudoConstant::getName('CRM_Contribute_BAO_Contribution', 'contribution_status_id', $contributionStatusID) === 'Pending' | ||
&& !empty($params['payment_processor_id']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eileenmcnaughton is this to exclude pay later options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, currently false is kinda the default & then if there is a payment processor & it is pending then we change to pending. I just double checked & the text is fine when pay later is in use
$contributionStatusID = civicrm_api3('Contribution', 'getvalue', array( | ||
'id' => CRM_Utils_Array::value('contributionID', $params), | ||
'return' => 'contribution_status_id', | ||
'invoice_id' => CRM_Utils_Array::value('invoiceID', $params), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eileenmcnaughton is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah - the point is the ipn could have updated the status - so looking to find out what it actually is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't that be able to be found through just the ID, i'm just wondering why the invoice_id is needed specifically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could usefully also react to failure here.... but as a follow up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah invoice id - because when I deployed it I found sometimes on the IPN return we have contribution id but not invoice id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks sane to me only question is over the default setting
Did you see my comment on the default? Basically it tested fine on pay later & I think it's a better default if there is any doubt |
Happy following the testing |
CRM-20252 Improve text when processing possibly-delayed-payments