CRM-20852 Show Tax term on UI #10640

mattwire · 2017-07-11T10:27:27Z

Ref CRM-20772. Show the tax term on the UI when possible (eg. "VAT" instead of "tax").

mattwire · 2017-08-04T15:17:22Z

@JoeMurray @monishdeb @eileenmcnaughton Any of you have time to review this PR? There's no functional change, it's a tweak to the UI in various places to display sales tax a little bit more nicely. Pinging you as I've seen lot's of nice contribution related UI changes coming through from you :-)

eileenmcnaughton · 2017-08-05T07:39:50Z

There are 2 things in this - neither of which I'm the expert on - 1 is the translation & @mlutfy would know the best approach there. The second is the rounding of line items & where is safe to do so. I think this probably should be on the form layer rather than a utility function - but I'm not familiar enough with that function to know if that is an issue with this change @KarinG knows the rounding best I think

eileenmcnaughton · 2017-08-05T07:40:29Z

(as an aside - you should rebase to get rid of the extraneous commits - git rebase -i origin/master (do a get fetch -f first)

mattwire · 2017-08-07T09:48:17Z

@eileenmcnaughton Thanks for the rebase tip. Now done, and I've added a note to the dev docs issue so we can include this workflow: https://github.com/civicrm/civicrm-dev-docs/issues/192

monishdeb · 2017-08-07T09:50:42Z

Cool.. will be providing my feedback shortly !!

monishdeb · 2017-08-07T12:52:46Z

templates/CRM/Contribute/Form/Contribution.tpl

@@ -653,7 +654,7 @@ CRM.$(function($) {
        var totalTaxAmount = taxAmount + Number(totalAmount);
        totalTaxAmount = formatMoney( totalTaxAmount, 2, separator, thousandMarker );

-        $("#totalTaxAmount" ).html('Amount with tax : <span id="currencySymbolShow">' + currencySymbol + '</span> '+ totalTaxAmount);
+        $("#totalTaxAmount" ).html('Amount with ' + taxTerm + ' : <span id="currencySymbolShow">' + currencySymbol + '</span> '+ totalTaxAmount);


I think this text should be passed through ts(...) as $("#totalTaxAmount" ).html(ts('Amount ...

Thanks @monishdeb I've fixed that now

monishdeb · 2017-08-07T13:07:07Z

I have tested the patch and ensured that Tax term are correctly shown in desired pages, except one place where I left comment. However, casting with float won't cause any regression or unintended side-effect. But, I like to have @KarinG feedback re rounding, before merge :)

KarinG · 2017-08-07T15:23:07Z

@mattwire - thanks for rebasing this - looking at this now.

KarinG · 2017-08-07T17:02:17Z

Hi @mattwire ! I've pulled your Edits into a 4.6 project that has extensive Sales Tax set up. Below is my summary screenshot;

Only thing that did not display properly for me was online -> Contribution page for Membership -> Confirm and Thank you screens; they both show too many zeros [middle section of screenshot]. Both the invoice PDF and the Contribution view generated from it are fine.

But Math is fine and many displays are much nicer - so don't let this stop the edits from being merged in.

KarinG · 2017-08-07T17:10:38Z

CRM/Price/BAO/LineItem.php

@@ -301,7 +301,8 @@ public static function getLineItems($entityId, $entity = 'participant', $isQuick
      $taxRates = CRM_Core_PseudoConstant::getTaxRates();
      if (isset($lineItems[$dao->id]['financial_type_id']) && array_key_exists($lineItems[$dao->id]['financial_type_id'], $taxRates)) {
        // We are close to output/display here - so apply some rounding at output/display level - to not show Tax Rate in all 8 decimals
-        $lineItems[$dao->id]['tax_rate'] = round($taxRates[$lineItems[$dao->id]['financial_type_id']], 3);
+        // Cast to float so trailing zero decimals are removed.
+        $lineItems[$dao->id]['tax_rate'] = (float) round($taxRates[$lineItems[$dao->id]['financial_type_id']], 3);
      }


This is fine - I'm cautiously optimistic with the cast we can remove the round(,3) - but let's keep that in mind for later.

I've just checked, and removed the round here. It's only used for display so we don't need it with the cast.

KarinG · 2017-08-07T17:29:27Z

Of course it would not be too hard to get rid of the trailing zeros in Confirm and ThankYou as well :-)

…The actual change is a cast to float, but a small amount of rearranging the function was required so it was actually set. Added note that buildQuickform() in Confirm.php and Thankyou.php share quite a bit of code and would benefit from refactoring

mattwire · 2017-08-07T21:31:54Z

@monishdeb @KarinG Thanks for spending the time to review! @KarinG Awesome screenshot! I just pushed a couple of commits which I think address all your comments.

KarinG · 2017-08-08T01:14:40Z

CRM/Contribute/Form/Contribution/Confirm.php

@@ -523,10 +523,11 @@ public function buildQuickForm() {
      $getTaxDetails = FALSE;
      $taxTerm = CRM_Utils_Array::value('tax_term', $invoiceSettings);


I feel these edits are going to require a lot more testing; multiple line items; online contribution form; w/ sales taxes;
I would prefer we merge your TaxTerm work in now: either delete this commit - or by replace it with a less invasive commit -> a TPL edit. All we really want is to change how line.tax_rate is displayed in the LineItem.tpl

<td class="right">{$taxTerm} ({$line.tax_rate}%)</td>

So there doesn't seem to be an easy way of stripping zero decimals in smarty - there's things like this but it seems a bit heavy for a tpl: https://stackoverflow.com/questions/26565650/smarty-trim-zeros-from-the-end-of-string-float-value. I have just pushed a simpler, less invasive version of the previous patch. It still uses the cast to float as before, but only makes a single line change to Confirm.php (Thankyou.php requires a block of code to be moved so the lineItem variables are set for display but this is lower risk as Thankyou.php does not save anything, only displays to the end user). I have run tests with multiple line items on the contribution pages and can find no issues with displaying the line items (both use LineItem.tpl for actual display). @KarinG @monishdeb what do you think of the updated patch?

… forms. The actual change is a cast to float, but a small amount of rearranging the function was required so it was actually set. Added note that buildQuickform() in Confirm.php and Thankyou.php share quite a bit of code and would benefit from refactoring" This reverts commit 4b58e3b.

… safer change

KarinG · 2017-08-08T13:11:56Z

Thank you @mattwire - do you know if there is a test specific to online contributions - multiple line items w/ sales taxes? (not at a desk right now).

mattwire · 2017-08-09T10:55:20Z

It seems we're a bit light on unit tests in that area :-(

KarinG · 2017-08-09T12:34:28Z

So my thoughts are we should not make edits to Confirm and ThankYou php code if we don't have an existing test that proves that the edits are ok; Let's remove that commit; get this PR merged - then you can work on the more critical issue (support for more decimals) - first?

eileenmcnaughton · 2017-08-09T14:34:59Z

@mattwire this test appears to give some cover - I've not read it carefully enough to know how it fits with the change you are making - there are some others in that class that might also

https://github.com/aydun/civicrm-core/blob/50d7b4ea81fe3b87319604300d3e2eb8fa05857e/tests/phpunit/api/v3/ContributionPageTest.php#L1585-L1585

KarinG · 2017-08-09T14:43:48Z

That's the test I added [to secure the don't round sales taxes prematurely fix]. For sure it secures back-end entry [I tested the test left and right for that scenario] - but I'm not sure a callAPISuccess('contribution_page', 'submit', $submitParams); properly tests front-end Confirm/ThankYou pathway? I came across such a callAPISuccess('contribution_page', 'submit', $submitParams); test yesterday that passes - yet the feature it tried to secure - has failed in Core.

eileenmcnaughton · 2017-08-09T15:05:59Z

Contribution_page.submit tests the postProcess (from Confirm I guess) - I don't think it tests Thank you at all. We don't have any precedent for testing thank you pages - but they are display pages rather than data saving pages.

Not sure about the test that passed that you mention - presumably you hit a different variant of factors

… forms - safer change" This reverts commit ec59f9c.

mattwire · 2017-08-14T15:57:54Z

@KarinG I've broken out the changes to Confirm/Thankyou.php to a new issue https://issues.civicrm.org/jira/browse/CRM-21062 with PR#10856

So this one should now be good to merge?

KarinG · 2017-08-14T16:08:55Z

I'm comfortable with these edits. It's a good improvement (to start displaying actual TaxTerm) and there will be another day when we can be certain the tests are in place to ensure Confirm.php edits don't have unexpected consequences.

mattwire · 2017-08-28T10:54:53Z

Hey @monishdeb is this something you are happy to merge? Based on approval and testing from @KarinG

monishdeb · 2017-09-30T15:26:58Z

Sorry for the delay. Merging based on @KarinG approval and my code review.

monishdeb · 2017-09-30T15:27:40Z

@mattwire can you please close related JIRA issue(s)? Thanks

mattwire mentioned this pull request Jul 11, 2017

WIP CRM-20772 Price set calculation precision when sales tax enabled #10612

Closed

civicrm-builder added the master label Aug 4, 2017

Show tax term (eg. VAT) on UI when possible

cc3f313

mattwire force-pushed the CRM-20772_showtaxterm branch 2 times, most recently from 52d2d0a to cc3f313 Compare August 7, 2017 09:41

monishdeb reviewed Aug 7, 2017

View reviewed changes

Translate this string

2597efb

KarinG reviewed Aug 7, 2017

View reviewed changes

mattwire added 3 commits August 7, 2017 22:15

Remove round that's no longer required

8684f61

Translation friendly smarty

54bcc92

KarinG reviewed Aug 8, 2017

View reviewed changes

mattwire added 2 commits August 8, 2017 09:08

Remove trailing zeros in tax rate for online Confirm/Thankyou forms -…

ec59f9c

… safer change

Revert "Remove trailing zeros in tax rate for online Confirm/Thankyou…

9fdb48b

… forms - safer change" This reverts commit ec59f9c.

monishdeb merged commit 9868668 into civicrm:master Sep 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-20852 Show Tax term on UI #10640

CRM-20852 Show Tax term on UI #10640

mattwire commented Jul 11, 2017 •

edited by civicrm-builder

Loading

mattwire commented Aug 4, 2017

eileenmcnaughton commented Aug 5, 2017

eileenmcnaughton commented Aug 5, 2017

mattwire commented Aug 7, 2017

monishdeb commented Aug 7, 2017

monishdeb Aug 7, 2017

mattwire Aug 7, 2017

monishdeb commented Aug 7, 2017

KarinG commented Aug 7, 2017

KarinG commented Aug 7, 2017

KarinG Aug 7, 2017

mattwire Aug 7, 2017

KarinG commented Aug 7, 2017

mattwire commented Aug 7, 2017

KarinG Aug 8, 2017 •

edited

Loading

mattwire Aug 8, 2017

KarinG commented Aug 8, 2017

mattwire commented Aug 9, 2017

KarinG commented Aug 9, 2017

eileenmcnaughton commented Aug 9, 2017

KarinG commented Aug 9, 2017

eileenmcnaughton commented Aug 9, 2017

mattwire commented Aug 14, 2017

KarinG commented Aug 14, 2017

mattwire commented Aug 28, 2017

monishdeb commented Sep 30, 2017

monishdeb commented Sep 30, 2017

		@@ -523,10 +523,11 @@ public function buildQuickForm() {
		$getTaxDetails = FALSE;
		$taxTerm = CRM_Utils_Array::value('tax_term', $invoiceSettings);

CRM-20852 Show Tax term on UI #10640

CRM-20852 Show Tax term on UI #10640

Conversation

mattwire commented Jul 11, 2017 • edited by civicrm-builder Loading

mattwire commented Aug 4, 2017

eileenmcnaughton commented Aug 5, 2017

eileenmcnaughton commented Aug 5, 2017

mattwire commented Aug 7, 2017

monishdeb commented Aug 7, 2017

monishdeb Aug 7, 2017

Choose a reason for hiding this comment

mattwire Aug 7, 2017

Choose a reason for hiding this comment

monishdeb commented Aug 7, 2017

KarinG commented Aug 7, 2017

KarinG commented Aug 7, 2017

KarinG Aug 7, 2017

Choose a reason for hiding this comment

mattwire Aug 7, 2017

Choose a reason for hiding this comment

KarinG commented Aug 7, 2017

mattwire commented Aug 7, 2017

KarinG Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

mattwire Aug 8, 2017

Choose a reason for hiding this comment

KarinG commented Aug 8, 2017

mattwire commented Aug 9, 2017

KarinG commented Aug 9, 2017

eileenmcnaughton commented Aug 9, 2017

KarinG commented Aug 9, 2017

eileenmcnaughton commented Aug 9, 2017

mattwire commented Aug 14, 2017

KarinG commented Aug 14, 2017

mattwire commented Aug 28, 2017

monishdeb commented Sep 30, 2017

monishdeb commented Sep 30, 2017

mattwire commented Jul 11, 2017 •

edited by civicrm-builder

Loading

KarinG Aug 8, 2017 •

edited

Loading