Skip to content
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-20569: Add priceset validation on a partial payment use-case and other improvements #11000

Merged
merged 2 commits into from
Nov 30, 2017

Conversation

monishdeb
Copy link
Member

@monishdeb monishdeb commented Sep 19, 2017

Technical Details

Contain two necessary changes:

  • Add a priceset validation where sum of the selected price options is GTE then the expected total amount if 'Partially paid' contribution status is chosen in backoffice form
  • Consider partial_payment_total other then total_amount in CRM_Price_BAO_PriceSet::processAmount(...)

Comments

This PR extracts some of the improvements from #10352


@@ -814,9 +814,16 @@ public static function priceSetValidation($priceSetId, $fields, &$error, $allowN
list($componentName) = explode(':', $fields['_qf_default']);
// now we have all selected amount in hand.
$totalAmount = array_sum($selectedAmounts);
$actualTotalAmount = CRM_Utils_Array::value('partial_payment_total', $fields, CRM_Utils_Array::value('total_amount', $fields));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so $actualTotalAmount is actually

// The form offers a field to enter the amount paid. This may differ from the amount that is due to complete the purchase
$totalPaymentAmountEnteredOnForm =  CRM_Utils_Array::value('partial_payment_total', $fields, CRM_Utils_Array::value('total_amount', $fields));

@@ -814,9 +814,16 @@ public static function priceSetValidation($priceSetId, $fields, &$error, $allowN
list($componentName) = explode(':', $fields['_qf_default']);
// now we have all selected amount in hand.
$totalAmount = array_sum($selectedAmounts);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$totalAmount = $sumOfSelectedOptions really?

$actualTotalAmount >= $totalAmount && // if total amount is equal to all selected amount in hand
(CRM_Utils_Array::value('contribution_status_id', $fields) == CRM_Core_PseudoConstant::getKey('CRM_Contribute_DAO_Contribution', 'contribution_status_id', 'Partially paid'))
) {
$error['total_amount'] = ts('For partially paid contribution, amount must be less then the sum of all selected amount in hand');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ts('You have specified the status Partially Paid but have entered an amount that equals or exceeds the amount due. Please adjust the status of the payment or the amount');

@eileenmcnaughton
Copy link
Contributor

I have made some wording comments.

I can accept this fix on the basis that

  1. we have a confusing UI. When entering an event you select the total to pay. Separately there is a the ability to enter a payment. However this section confuses payments with contributions - allowing the 'status' to be entered - but the status applies to contribution.
  2. The above is bad and awful
  3. @monishdeb & I put a tonne of work into trying to do a UI fix around a similar ambiguity in another location. Community feedback was that it was better but it got derailed over whether another approach would be better / agreeing a response to some issues.
  4. Hence I think it's more appropriate just to block the existing UI from letting something bad through than fixing it in this case.

So pending some readability improvements I'm OK with this.

I note the partial_payment_amount being passes around is a bit tangental to the other fix & should really be a separate JIRA/PR. However, it seems like a minor addition of the param being passed & consistent with other code uses

@monishdeb
Copy link
Member Author

Thanks @eileenmcnaughton for your feedback. Made those changes, please have a look!

@eileenmcnaughton eileenmcnaughton merged commit 36dc8a2 into civicrm:master Nov 30, 2017
sluc23 pushed a commit to ixiam/civicrm-core that referenced this pull request Jan 10, 2018
CRM-20569: Add priceset validation on a partial payment use-case and other improvements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants