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

feat: adds tax numbers to organization and customers #269

Merged
merged 6 commits into from
Oct 25, 2023

Conversation

kochie
Copy link
Contributor

@kochie kochie commented Oct 12, 2023

When creating invoices in most counties (i.e. Australia, NZ, USA, EU) a tax number (ABN, VAT) needs to be included on the invoice somewhere so that proper declarations can be made.

This PR adds support for tax numbers which are generalised as a text field.

image
image

@vercel
Copy link

vercel bot commented Oct 12, 2023

@kochie is attempting to deploy a commit to the Bigcapital Team on Vercel.

A member of the Team first needs to authorize it.

COPY ./packages/server/package*.json ./packages/server/

# RUN curl -fsSL https://get.pnpm.io/install.sh | sh -
RUN npm install -g pnpm
Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, after migrated from npm to pnpm I have to change the Dockerfile instructions too. btw this Dockerfile is not for docker-compose just for CI/CD to build and deploy the server container to the public registry.

@@ -0,0 +1,11 @@
exports.up = function (knex) {
return knex.schema.table('tenants_metadata', (table) => {
table.string('tax_number')
Copy link
Contributor

@abouolia abouolia Oct 12, 2023

Choose a reason for hiding this comment

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

the tax_number should be under tax_rates table, btw application is multi-tenant architecture and has two different type of databases, system database has all the users information and tenants databases each organization account has seperate database has everything related to it like invoices, items, bills, etc. that migration should be on tenants level.

To create a new tenant DB migration. You can from the root directory hit node packages/server/build/commands.js tenants:migrate:make create_new_table after building pnpm run build:server

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's the right place to put it, the tax number is a unique identifier associated with a company. So each tenant should have the ability to use a number. Also it's not actually associated with different tax rates (but depending on region the amount of tax does depend on it I'd suppose). The main reason I'm adding it is because in Australia it's a requirement to have an Australian Business Number (ABN) on any tax invoice.

You are right however that it should be added to all customers. I've actually done that already I'm just cleaning it up.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was my bad, you're absolutely right, I thought the tax number you want to add it under tax rates. Good work buddy.

@abouolia
Copy link
Contributor

Really appreciate your contribution, left some notes, your work is awesome and we can merge it after taking a look at these comments and some tweaks. again thanks for contribution

@abouolia abouolia self-assigned this Oct 25, 2023
@abouolia abouolia marked this pull request as ready for review October 25, 2023 19:40
Copy link
Contributor

@abouolia abouolia left a comment

Choose a reason for hiding this comment

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

Good work man,

  • Reverted the Kubernetes commit it's out of pull-request's context.
  • Removed the organization tax number from the setup form, always we want it the setup with minimum fields if the user wanted to add it they can go to the preferences.
  • Prettified code changes.
  • We will display the organization tax number on pdf document once we finish pdf printing.

Let us approve it and merge it for next release. We really appreciate your contribution keep push awesome work.

@abouolia abouolia merged commit e4a7f09 into bigcapitalhq:develop Oct 25, 2023
1 of 3 checks passed
@abouolia
Copy link
Contributor

@all-contributors please add @kochie for code.

@allcontributors
Copy link
Contributor

@abouolia

I've put up a pull request to add @kochie! 🎉

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.

2 participants