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

Fixed bug for calculating tax rates. Issue IP-717 #637

Closed
wants to merge 1 commit into from

Conversation

@mjmunger
Copy link

mjmunger commented Aug 3, 2018

Pull Request Checklist

  • My code follows the code formatting guidelines.
  • I have an issue ID for this pull request.
  • I selected the corresponding branch.
  • I have rebased my changes on top of the corresponding branch.

Issue Type (Please check one or more)

  • Bugfix
  • Improvement of an existing Feature
  • New Feature
@mjmunger

This comment has been minimized.

Copy link
Author

mjmunger commented Aug 3, 2018

Issue: https://development.invoiceplane.com/browse/IP-717
This is a major bug because it means everyone has been over-charging customers and over-collecting tax on any items with discounts.

@Kovah

This comment has been minimized.

Copy link
Member

Kovah commented Aug 4, 2018

Comment about this added to the corresponding forums post: https://community.invoiceplane.com/t/topic/6652/2

@Kovah Kovah closed this Aug 4, 2018
@mjmunger

This comment has been minimized.

Copy link
Author

mjmunger commented Aug 4, 2018

This is probably the most unfortunate response I've run into in my professional development career. You've basically said: "it doesn't matter that a core functionality of what invoicing should be able to do has been broken completely since at least 2015, it's too inconvenient to fix it so we're not going to. "Users of this package are therefore unwittingly opening themselves up to problems from the local tax authorities.

The responsible thing to do would have been to disable discounts on taxable items and /or at least put a warning on the screen letting users know that they would be over collecting tax.

Having looked at the code to try and review this to fix it on our end ourselves, I understand that you just can't simply do it because the code is a mess full of nested ifs and nested fors. There's not a single unit test, and refactoring would be an absolute nightmare.

The reason that we found it is because we have written a package to do automated recurring billing, that works in concert with invoiceplane, and the only way we can see to move forward is to completely disable discounts for any taxable items.

You owe it to your users to do the right thing and inform them that this is an issue if they apply discounts.

As to your comments that it would break existing invoices and quotes, THEY ARE ALREADY BROKEN.

@Kovah

This comment has been minimized.

Copy link
Member

Kovah commented Aug 5, 2018

Thank you for your response. I think you took something wrong and maybe a little bit too personal.
I am aware of the issue, but to correct you, I do indeed care about it and it's not "too inconvenient to fix it". There is an open issue for this bug and it will be fixed but not in that way you suggested.

You wrote that the invoices are already broken. No, they are not. They are working for all use cases except the particular feature where both taxes and discounts are applied to an item. There is a reason why your solution of just switching the calculation is not yet implemented (and it was proposed some time ago already): all existing invoices would be rendered incorrect across all installations and for all users. This is absolutely not acceptable, even if there is a bug in that particular calculation.
A suitable solution would require to rewrite the calculation process in a non-destructive way.

However, I agree that users should be informed about this issue and I will make sure that the next release contains a warning.

@mjmunger

This comment has been minimized.

Copy link
Author

mjmunger commented Aug 6, 2018

I think you took something wrong and maybe a little bit too personal.

While I realize this is an open source project, most users are not going to audit the entire thing prior to using it. They are going to take it at face value that "it works" and that any known issues will be in the README.md. They are especially going to assume that simple math is going to work as advertised.

If I sound offended, it's because I'm shocked you didn't take action to protect your users. Once discovered, you should have disabled the feature moving forward once it was discovered. You didn't do that. Instead, you now have thousands of new and additional users who have created liabilities with their local tax authorities for falsely collecting too much tax - a problem that can be devastating for a business - especially small ones that cannot afford lawsuits being initiated from their government. In most countries, this is a criminal offense and under a theory of strict liability, the accused can have big problems even though they didn't know they were collecting too much tax.

You're safe in your ivory tower because of your license in the project, but that doesn't absolve you from making good decisions to protect your user community.

As the maintainer of open source software, it's your responsibility to address things like this that could be catastrophic to your users. This is categorically different from "the header doesn't show up correctly on mobile phones." If that's broken, it can wait until there is a bounty, or someone comes along that wants it bad enough, they will code it themselves. But this tax problem puts your user's in legal risk.

I didn't take it personal, I find it irresponsible that you've so cavalierly skipped over the importance of staying in compliance with the tax authority to the detriment of your users.

As I previously stated, the proper response would be to disable this back in 2015 instead of letting it go and pile up more users with more liabilities to their tax authorities over the following 3 years. I consider myself lucky that this only affected a hand-ful of users, whom I will be writing checks to today to refund them for the incorrectly collected taxes.

Some of your users will have 3-years worth of customer to refund. If / when they ever find out about it.

Moving on to the accounting side of things, the longer you wait to push out a fix for this, the longer people's accounting and books will be improperly done, and they will have an even greater problem getting the books straightened out.

You wrote that the invoices are already broken. No, they are not. They are working for all use cases except the particular feature where both taxes and discounts are applied to an item

A feature is broken when any of its comprising features does not work as expected.

true + true + true + true + true + true + true + false = false.

To say that a car is OK to drive means that the expected use cases and functionality of the car, which are readily available are working as expected.

What you're saying here is: "I realize that the car's seat belts were held to the chassis with tape, and would instantly become detached during a car crash, but the design is fine because the users can drive the car and click their seat belts. Never mind what happens when they get in a crash. They can put their seat belts on, and it clicks making them feel secure. The design works."

all existing invoices would be rendered incorrect across all installations and for all users.

Where did you study math? All invoices that do not have discounts applied to them would be entirely unaffected.

Only invoices where incorrect discounts and taxes were applied would be affected, and those need to be corrected in order to stay in compliance with tax collection regulations, proper accounting procedures, and to just be honest with your user's customers.

So, when I say "they are already broken" it's because this situation exists. Regardless if you choose ti ignore it to preserve the existing invoices and their incorrect totals, the situation exists.

Without a proper fix, those invoices which are already existing and which the totals have been wrong and will continue to be wrong. And - in the real world - when the tax authorities realize you have collected too much tax, they will come after you.

As you have correctly stated, this problem only exists where there is tax AND a discount was applied to the item. This still represents thousands of customers and a large amount of cash. But for all other invoices, this is not only a non-issue, but it would not be affected by a proper fix / update.

A suitable solution would require to rewrite the calculation process in a non-destructive way.

Based on what I see in the code so far, the solution should be to update the Ajax function that does these calculations in the first place. It appears that all other calculations stem from this "entry point" of the calculations. (Code is hard to follow and dissect because the cyclomatic complexity is rather high, so I haven't found much else to work with). Then, write an update script that loads all invoices and re-calculates them to reflect the actual truth of the calculations. Differences should be summarized in a report (CSV) showing how many invoices were affected, which customers are OWED money because too much tax was collected, and which invoices were under-collected as a result of these calculations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.