[IP-717] Implements hotfix for item discount calculation

[Kovah]

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

---

Quotes / invoices created after the update to 1.5.10 will get a modifier that enables the new discount/tax calculation. This way old invoices are not recalculated but new ones will use the correct calculation method. Credits go to @mjmunger.

[Kovah]

@mjmunger Would you like to take a look into this?

[mjmunger]

Happy to check it out a little later today. Finishing up unit tests for a package that works with InvoicePlane's database, so I have use cases ready to go.

[mjmunger]

Took a quick look at it just now... do you mind if I clean this up?

[Kovah]

Sure, feel free to propose changes. It's a quick and dirty solution.

[mjmunger]

I've refactored a bunch of things. I am not sure how you want to receive the changes. Obviously, I don't have push rights to your local fork (that would be most convenient). How do you want to do this?

[mjmunger]

also, while refactoring, I discovered that your "mark tasks complete for this invoice" section appears to have never worked.

https://github.com/InvoicePlane/InvoicePlane/blob/master/application/modules/invoices/controllers/Ajax.php

line 465 sets $item to a boolean value.
472-476 checks if $item->item_task_id is set. Of course it's not set. it's false. It will always be false if delete() succeeds.

[Kovah]

You may send the patch file to mail@invoiceplane.com so I can apply the changes you made.

[mjmunger]

Patch submitted. Updates are in the CHANGELOG.md, which I have added (not sure why it wasn't there before?)

[Kovah]

@mjmunger I implemented your new calculation approach for quotes. However, the QuoteItemBase has a flaw which cause I couldn't find. I added a comment explaining what's wrong. Maybe you could take a look at it?

[mjmunger]

I see the problem. Needs an update to the factory, and needs another decorator. Also, checking to see if base path is defined on those is an unneeded conditional because 1.) The .htaccess file rewrites all requests to the front controller doesn't allow access anyway, and 2.) Even if it did, that doesn't actually do anything except load the class without performing work. I'll fix it.

[mjmunger]

I did not pull your current version to work with since I am already famliar with the changes needed, I just re-did it from my copy here. Sending you the patches now.

[Kovah]

@mjmunger Implemented your changes. Autoloading is handled by Composer, working perfectly well. Also moved the source files into the application directory to keep the current directory structure.

[mjmunger]

Anything else that needs to be done? Or can we publish v1.5.10?

[Kovah]

If it's working correctly version 1.5.10 can be published.

[mjmunger]

I'll pull and test.

[mjmunger]

Doing a pull / test now. Will advise in 30 minutes.

[mjmunger]

Revision 0a113a1 has the autoloader I put in pre-maturely removed. Composer does handle autloading for composer compliant packages, but these classes are not a composer compliant package, so Composer doesn't now about them.

Moving src to applications/ is a somewhat valid choice, so we can keep it, but the autoload.php file needs to be put back, and the path where the autoloader looks needs to be updated to applications/src/.

Having fixed this in dev, the invoices are not calculating correctly again. :-(

[Kovah]

Classes are loaded correctly after running composer install.

[Kovah]

@mjmunger Are there any news on the testing?

[mjmunger]

It's on my list, but I haven't been able to get to it. We do billing once / month, so it is about to get more important very soon.

[Kovah]

@miquel-cabanas Could you help me out here? Would just need a check of quotes / invoices to be working correctly. Would like to publish 1.5.10 in the next week(s). :)

[mjmunger]

We've got it working internally, and it is a priority for us to push this out to you, but we have to get a few things handled on the business side first. Will try to do this in the next couple days. Get Outlook for Android<https://aka.ms/ghei36> On Sun, Sep 9, 2018 at 3:27 AM -0400, "Kovah" <notifications@github.com<mailto:notifications@github.com>> wrote: @miquel-cabanas<https://github.com/miquel-cabanas> Could you help me out here? Would just need a check of quotes / invoices to be working correctly. Would like to publish 1.5.10 in the next week(s). :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#638 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AGE1_mTrYVoOQdFosXu3hr4wASAB8k27ks5uZMMEgaJpZM4VxGSn>.

[miquel-cabanas]

I am currently checking the language issue reported in IP-531. I think it is fixed, but when investigating it I have found another minor language problem. When I'm done with it I will have a look at this PR.

[IssueHuntBot]

@BoostIO funded this issue with $20. Visit this issue on Issuehunt

[Kovah]

Funding will be split 75:25 for development and testing.
75% go to @mjmunger
25% go to a user who does additional testing.

[Kovah]

I would like to publish 1.5.10 with this fix on the upcoming sunday (2018-10-07).
Therefore I will do additional testing with various versions and states. Clean install and existing databases.

[miquel-cabanas]

There is a minor issue in the function get_item_discount_total() in file application/src/ItemBase.php that does not affect the calculations, but may cause confusion in the future:

• rename $tax_total as $discount_total

public function get_item_discount_total()
{
    $quantity = (int)round($this->item_quantity * 100, 2);
    $discount = (int)round($this->item_discount_amount * 100, 2);
    $tax_total = $quantity * $discount / 100 / 100;
    return $tax_total;
}

I will keep reviewing the code and will comment anything I found, although, from what I have read so far, I suspect they will be just minor issues like this one.

[miquel-cabanas]

Sorry to be picky 🙏 but the names of files, classes and variables do not seem to abide to CI conventions:

• File Naming
• Class and Method Naming

For instance, the file ItemBase.php should be renamed as Item_base.php and the class ItemBaseshould be renamed as Item_base.

On the other side, variable names seems to be fine

• Variable Names

There's no need to change them to test the code since CI does not seem to be enforcing its standards as of now, but the names should be changed before accepting the pull request just in case a future version of CI becomes more strict.

[miquel-cabanas]

Good news! I am picky again 🙏 which means I'm finding nothing really serious to comment 😉

Some functions in invoices/controllers/ajax.php are still using echo json_encode($response); instead of the new $this->respond($response); but that could be fixed by a later pull request.

Other than this, I think the changes in this file are ok.