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

Jump to bottom

Minor refactor, use sales tax trait to simplify sales tax functions #12594

Merged
merged 1 commit into from
Aug 9, 2018
Merged

Minor refactor, use sales tax trait to simplify sales tax functions #12594

merged 1 commit into from
Aug 9, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Provide a trait for collection of sales tax functions

Before

Code more repetitive

After

Code less repetitive

Technical Details

A trait for forms that use sales tax provides a useful way of grouping together functions. In fact basically all forms that touch on financials do the equivalent of

$this->assignSalesTaxMetadataToTemplate();

In one way or another. For all back office forms we could simply call that from AbstractEditPayment in buildForm. Front end forms don't call that.

Sales tax functionality really needs some consolidation...

Comments

@pradpnayak @monishdeb @agilewarealok @mattwire

@civibot
Copy link

civibot bot commented Jul 30, 2018

(Standard links)

eileenmcnaughton
eileenmcnaughton commented Jul 30, 2018
View reviewed changes
CRM/Member/Form/Membership.php Outdated
@@ -1685,7 +1679,7 @@ public function submit() {
}
if ($taxAmount) {
$this->assign('totalTaxAmount', $totalTaxAmount);
$this->assign('taxTerm', CRM_Utils_Array::value('tax_term', $invoiceSettings));
$this->assign('taxTerm', $this->getSalesTaxTerm());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why would need this on Submit.... unless it's being used when sending mails in which case this is the wrong place

Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton Perhaps add that as a code comment for now - everything in this PR is NFC as far as I can tell, but if we remove this line it may no longer be...

@mattwire
Copy link
Contributor

Changes here look good. This is the sort of consolidation that I'd like to see more of in CiviCRM - can we clone @eileenmcnaughton?

I'm happy with this from a code POV, there's no functional changes. @KarinG may also want to take a quick look over...

As I know @agilewarealok has an interest in this could you do a more thorough review so that we can get it merged?

@mattwire mattwire mentioned this pull request Jul 30, 2018
Closed
@KarinG
Copy link
Contributor

KarinG commented Jul 30, 2018

Nice - many thumbs up for any step towards consolidating sales tax code.

eileenmcnaughton
eileenmcnaughton commented Jul 30, 2018
View reviewed changes
CRM/Member/Form/Membership.php
@@ -1685,7 +1679,8 @@ public function submit() {
}
if ($taxAmount) {
$this->assign('totalTaxAmount', $totalTaxAmount);
$this->assign('taxTerm', CRM_Utils_Array::value('tax_term', $invoiceSettings));
// Not sure why would need this on Submit.... unless it's being used when sending mails in which case this is the wrong place
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 added as a code comment

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lol - let me know if you feel this is mergeable

@eileenmcnaughton
Copy link
Contributor Author

@jusfreeman is this something you could get @agilewarealok to review. I'm pretty confident that it's pretty trivial clean up but as we start to get a sensible approach around these things it creates a path to clean stuff up

@jusfreeman
Copy link
Contributor

@eileenmcnaughton thanks for the ping. Does this relate to https://lab.civicrm.org/dev/core/issues/266 or is there another lab.civicrm issue for this one?

@eileenmcnaughton
Copy link
Contributor Author

@jusfreeman loosely related in that I took a look at the code & took a baby-cleanup step here #12593

We don't always track every PR in gitlab - for code cleanup there isn't a need for a separate high level concept approval it really just is at the code review level and gitlab just becomes an overhead without adding value

@KarinG
Copy link
Contributor

KarinG commented Aug 7, 2018

@mattwire is happy - I'm happy. @jusfreeman - do you have any concerns? I think this is not a question about the merge-ability of this PR but about all getting on board of the consolidation train.

@eileenmcnaughton eileenmcnaughton added the merge ready PR will be merged after a few days if there are no objections label Aug 7, 2018
@eileenmcnaughton
Copy link
Contributor Author

Thanks @KarinG I've added merge-ready to reflect the fact that this has been agreed mergeable but still seems worth keeping open for comments for a little longer

@jusfreeman
Copy link
Contributor

@eileenmcnaughton can unit tests be added at this level? If the goal here is to provide a single common function for calculating tax, then this would be the ideal place to have unit tests.

I would like to functionally test any CiviCRM tax bug fixes, but as this PR is a code clean-up - I'm not sure if you are looking for this type of input here.

@eileenmcnaughton
Copy link
Contributor Author

@jusfreeman at this stage the trait isn't really doing calculating of taxes particularly & it's a pretty minor refactor - I think if we go further down this path & start to consolidate more serious functionality here we definitely should add tests.

I'm not 100% sure if this trait would be where the calculation functions belong - it's in the form path & I'm imagining it more as a form helper to try to reduce the noise of relentless code repetition

@eileenmcnaughton
Copy link
Contributor Author

I should note I'm also not really convinced functions like this

  /**
   * Assign sales tax rates to the template.
   */
  public function assignSalesTaxRates() {
    $this->assign('taxRates', json_encode(CRM_Core_PseudoConstant::getTaxRates()));
  }

should exist. My feeling is that we should probably be adding data attributes to the form - but would need to do more tidying before being sure about that!

@eileenmcnaughton
Copy link
Contributor Author

I'm going to merge this based on discussion so far - there was a follow on PR which I will rebase but depending on enthusiasm I may let it go

@eileenmcnaughton eileenmcnaughton merged commit a26f426 into civicrm:master Aug 9, 2018
@eileenmcnaughton eileenmcnaughton deleted the sales_tax_trait branch August 9, 2018 03:34
@eileenmcnaughton
Copy link
Contributor Author

Actually I closed the follow on - #12593 - it didn't rebase cleanly & I wasn't sure if anyone was interested in reviewing. I can do some more tidy up if someone does want to review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattwire mattwire mattwire left review comments

Assignees
No one assigned
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@eileenmcnaughton @mattwire @KarinG @jusfreeman @civicrm-builder