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

Add support for entering tax as a percentage of Total #4

Merged
merged 2 commits into from
Mar 1, 2020

Conversation

arimbun
Copy link
Contributor

@arimbun arimbun commented Dec 5, 2019

Simple support to configure tax as a percentage of Total in the billing.yaml file.

@relistan
Copy link
Owner

Sorry I missed this when you opened it! Will review tomorrow. Thanks for the contribution @arimbun

@arimbun
Copy link
Contributor Author

arimbun commented Jan 11, 2020

@relistan No problem, did you have a chance to review it?

@@ -33,6 +33,10 @@ billables:
unit_price: 10.23
currency: €

# You may enter tax as a percentage here (from 0 to 1.0).
tax:
percentage: 0.1
Copy link
Owner

Choose a reason for hiding this comment

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

We really need to support a default tax and an overridable tax rate per item. Some items may be taxable at different rates.

@@ -33,6 +33,10 @@ billables:
unit_price: 10.23
currency: €

# You may enter tax as a percentage here (from 0 to 1.0).
tax:
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
tax:
default_tax:

Copy link
Owner

Choose a reason for hiding this comment

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

You'll need to make various adjustments elsewhere in the code to make this work also. Didn't try to find them all.

@@ -65,6 +65,10 @@ func (b *BillableItem) Strings() []string {
}
}

type TaxDetails struct {
Copy link
Owner

Choose a reason for hiding this comment

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

We should call this something like DefaultTax instead.

b.textFormat(widths[len(widths)-2], 4, "Tax", "1", 0, "R", true, 0, "")
b.textFormat(widths[len(widths)-1], 4, "0", "1", 0, "R", true, 0, "")
taxText := billables[0].Currency + " " + niceFloatStr(tax)
b.textFormat(widths[len(widths)-2], 4, "GST", "1", 0, "R", true, 0, "")
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
b.textFormat(widths[len(widths)-2], 4, "GST", "1", 0, "R", true, 0, "")
b.textFormat(widths[len(widths)-2], 4, "Tax", "1", 0, "R", true, 0, "")

GST is a region-specific tax for certain countries. Let's stay with generic "Tax"

Copy link
Owner

Choose a reason for hiding this comment

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

Alternatively, make this configurable in the default_tax block in the config.

Copy link
Owner

@relistan relistan left a comment

Choose a reason for hiding this comment

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

@arimbun MANY apologies for taking so darn long to get to this. It's a side project and finding time can be a challenge. This looks pretty good. I would like to eventually allow item-specific tax calculation, so I've pointed out where I'd like to change what you've got here. Otherwise, good to go. Thank you for the contribution! Again, sorry about how long it took to get to it. @rolandtritsch 's PR was waiting on me for quite awhile as well.

@relistan
Copy link
Owner

relistan commented Mar 1, 2020

Hey @arimbun I've got a little time to deal with this today so I'm going to merge it as-is and do a little work on it now. Thanks again for the contribution!

@relistan relistan merged commit bb93890 into relistan:master Mar 1, 2020
@rolandtritsch
Copy link
Contributor

@relistan @arimbun Nice!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants