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.

Sign up for GitHub

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

Add FrontEndPaymentFormTrait to start to share functionality between Event and Contribution forms #13843

Merged
merged 1 commit into from
Mar 19, 2019
Merged

Add FrontEndPaymentFormTrait to start to share functionality between Event and Contribution forms #13843

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 2 additions & 11 deletions CRM/Contribute/Form/Contribution/Confirm.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
* form to process actions on the group aspect of Custom Data
*/
class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_ContributionBase {
use CRM_Financial_Form_FrontEndPaymentFormTrait;

/**
* The id of the contact associated with this contribution.
Expand Down Expand Up @@ -514,17 +515,7 @@ public function buildQuickForm() {
if ($invoicing) {
$getTaxDetails = FALSE;
$taxTerm = CRM_Utils_Array::value('tax_term', $invoiceSettings);
foreach ($this->_lineItem as $key => $value) {
foreach ($value as $k => $v) {
if (isset($v['tax_rate'])) {
if ($v['tax_rate'] != '') {
$getTaxDetails = TRUE;
// Cast to float to display without trailing zero decimals
$tplLineItems[$key][$k]['tax_rate'] = (float) $v['tax_rate'];
}
}
}
}
list($getTaxDetails, $tplLineItems) = $this->alterLineItemsForTemplate($tplLineItems);
$this->assign('getTaxDetails', $getTaxDetails);
$this->assign('taxTerm', $taxTerm);
$this->assign('totalTaxAmount', $params['tax_amount']);
Expand Down
69 changes: 69 additions & 0 deletions CRM/Financial/Form/FrontEndPaymentFormTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
<?php
/*
+--------------------------------------------------------------------+
| CiviCRM version 5 |
+--------------------------------------------------------------------+
| Copyright CiviCRM LLC (c) 2004-2019 |
+--------------------------------------------------------------------+
| This file is a part of CiviCRM. |
| |
| CiviCRM is free software; you can copy, modify, and distribute it |
| under the terms of the GNU Affero General Public License |
| Version 3, 19 November 2007 and the CiviCRM Licensing Exception. |
| |
| CiviCRM is distributed in the hope that it will be useful, but |
| WITHOUT ANY WARRANTY; without even the implied warranty of |
| MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. |
| See the GNU Affero General Public License for more details. |
| |
| You should have received a copy of the GNU Affero General Public |
| License and the CiviCRM Licensing Exception along |
| with this program; if not, contact CiviCRM LLC |
| at info[AT]civicrm[DOT]org. If you have questions about the |
| GNU Affero General Public License or the licensing of CiviCRM, |
| see the CiviCRM license FAQ at http://civicrm.org/licensing |
+--------------------------------------------------------------------+
*/

/**
*
* @package CRM
* @copyright CiviCRM LLC (c) 2004-2019
*/

/**
* This class holds functionality shared between various front end forms.
*/
trait CRM_Financial_Form_FrontEndPaymentFormTrait {

/**
* Alter line items for template.
*
* This is an early cut of what will ideally eventually be a hooklike call to the
* CRM_Invoicing_Utils class with a potential end goal of moving this handling to an extension.
*
* @param $tplLineItems
*
* @return array
*/
protected function alterLineItemsForTemplate($tplLineItems) {
$getTaxDetails = FALSE;
foreach ($tplLineItems as $key => $value) {
foreach ($value as $k => $v) {
if (isset($v['tax_rate'])) {
if ($v['tax_rate'] != '') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we collapse this if down to if(!empty($v['tax_rate']) as that's exactly what the 2 if statements here are doing

Copy link
Contributor

Choose a reason for hiding this comment

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

@mattwire what if the tax rate is 0% hypothetically like what if some line items have tax applied but not others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattwire Yes we can - I'm happy to add it in here but there is other cleanup to do in this function so I deliberately left the cleanup out of the first round to keep it simple

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seamuslee001 that's fine - if ANY rows have tax then the getTaxDetails = TRUE is assigned to the template

A good follow up question is 'what does getTaxDetails mean'

I see it used in a few places - where I would probably say it's the wrong way to assign

        {if $getTaxDetails}
          <th class="right">{ts}Subtotal{/ts}</th>
          <th class="right">{ts}Tax Rate{/ts}</th>
          <th class="right">{ts}Tax Amount{/ts}</th>
          <th class="right">{ts}Total Amount{/ts}</th>
        {/if}

And which probably rely on the variable being assigned somewhere completely different - so it's likely absolutely nothing is achieved.....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattwire Actually in the cleanup round changing from 2 lines to the same 2 conditions on one line makes more sense as adding assumptions that the template doesn't want to know about zero is probably a bigger leap - but I'd rather cleanup as PR 2

$getTaxDetails = TRUE;
// Cast to float to display without trailing zero decimals
$tplLineItems[$key][$k]['tax_rate'] = (float) $v['tax_rate'];
}
}
}
}
// @todo fix this to only return $tplLineItems. Calling function can check for tax rate and
// do all invoicing related assigns
// another discrete function (it's just one more iteration through a an array with only a handful of
// lines so the separation of concerns is more important than 'efficiency'
return [$getTaxDetails, $tplLineItems];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should fix this before we start sharing with other forms.... But I wanted to keep it really easy to replicate & verify

}

}