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

Improve getContextualValue performances by reducing the number of calls to getOrderTotal #8410

Merged
merged 2 commits into from Oct 12, 2017

Conversation

Projects
None yet
4 participants
@jocel1
Contributor

jocel1 commented Oct 11, 2017

Questions Answers
Branch? develop
Description? improve getContextualValue (calls for example in order history) by implementing a local cache and reducing the call to getOrderTotal
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket?
How to test? run tests. Load the command history page

Important guidelines


This change is Reviewable

@prestonBot

This comment has been minimized.

Show comment
Hide comment
@prestonBot

prestonBot Oct 11, 2017

Collaborator

Hi!

These(s) commit(s) name(s) seems to be incomplete or malformed, regarding our guidelines:

`Improve getContextualValue performances by reducing the number of calls to getOrderTotal` is malformed or incomplete.

A valid commit name can be, for instance:

BO: Shows company in BO search if B2B is enabled

Would you mind to amend your commits' names?

To do this, open a command line window and use git commit --amend for the commit's name. See GitHub's help page for more information.

Note: this must be done via the command line: you can't do this just by changing the title of the pull-request from the GitHub interface! :)

example

Thank you!

(note: this is an automated message, but answering it will reach a real human )

Collaborator

prestonBot commented Oct 11, 2017

Hi!

These(s) commit(s) name(s) seems to be incomplete or malformed, regarding our guidelines:

`Improve getContextualValue performances by reducing the number of calls to getOrderTotal` is malformed or incomplete.

A valid commit name can be, for instance:

BO: Shows company in BO search if B2B is enabled

Would you mind to amend your commits' names?

To do this, open a command line window and use git commit --amend for the commit's name. See GitHub's help page for more information.

Note: this must be done via the command line: you can't do this just by changing the title of the pull-request from the GitHub interface! :)

example

Thank you!

(note: this is an automated message, but answering it will reach a real human )

@jocel1

This comment has been minimized.

Show comment
Hide comment
@jocel1
Contributor

jocel1 commented Oct 11, 2017

if (!array_key_exists($context->cart->id, $cartAmountCache)) {
if (Tax::excludeTaxeOption()) {
$cartAmountCache[$context->cart->id]['te'] = $context->cart->getOrderTotal(false, Cart::ONLY_PRODUCTS);
$cartAmountCache[$context->cart->id]['ti'] = $cartAmountCache[$context->cart->id]['te'];

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Oct 11, 2017

Member

Hmm not sure about this line. This is tempting to make the shortcut if the tax is excluded, but did you make sure the results are the same? It may have an impact on the shipping price.

@Quetzacoalt91

Quetzacoalt91 Oct 11, 2017

Member

Hmm not sure about this line. This is tempting to make the shortcut if the tax is excluded, but did you make sure the results are the same? It may have an impact on the shipping price.

This comment has been minimized.

@jocel1

jocel1 Oct 11, 2017

Contributor

Yes I've checked inside the getOrderTotal(). If ONLY_PRODUCTS is used, the $with_taxes is changed to false if Tax::excludeTaxeOption is set, and we don't use it before.

@jocel1

jocel1 Oct 11, 2017

Contributor

Yes I've checked inside the getOrderTotal(). If ONLY_PRODUCTS is used, the $with_taxes is changed to false if Tax::excludeTaxeOption is set, and we don't use it before.

This comment has been minimized.

@jocel1

jocel1 Oct 11, 2017

Contributor

It only has an impact in ONLY_DISCOUNTS, BOTH and ONLY_SHIPPING mode

@jocel1

jocel1 Oct 11, 2017

Contributor

It only has an impact in ONLY_DISCOUNTS, BOTH and ONLY_SHIPPING mode

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Oct 11, 2017

Member

We thought we did because the variable can be found a few lines before.
capture du 2017-10-11 11-21-56

But if we have a look at the tests, it seems good.

@Quetzacoalt91

Quetzacoalt91 Oct 11, 2017

Member

We thought we did because the variable can be found a few lines before.
capture du 2017-10-11 11-21-56

But if we have a look at the tests, it seems good.

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Oct 11, 2017

Member

@jocel1, would you mind adding a test for the comment I opened?

At the end of tests/Integration/classes/CartGetOrderTotalTest.php:

    public function testSameTotalWithoutTax()
    {
        Configuration::set('PS_TAX', false);
        $product = self::makeProduct('Hello Product', 10, self::getIdTaxRulesGroup(20));
        $cart = self::makeCart();

        $cart->updateQty(1, $product->id);

        $this->assertEquals(
            $cart->getOrderTotal(false, Cart::ONLY_PRODUCTS),
            $cart->getOrderTotal(true, Cart::ONLY_PRODUCTS));
    }
Member

Quetzacoalt91 commented Oct 11, 2017

@jocel1, would you mind adding a test for the comment I opened?

At the end of tests/Integration/classes/CartGetOrderTotalTest.php:

    public function testSameTotalWithoutTax()
    {
        Configuration::set('PS_TAX', false);
        $product = self::makeProduct('Hello Product', 10, self::getIdTaxRulesGroup(20));
        $cart = self::makeCart();

        $cart->updateQty(1, $product->id);

        $this->assertEquals(
            $cart->getOrderTotal(false, Cart::ONLY_PRODUCTS),
            $cart->getOrderTotal(true, Cart::ONLY_PRODUCTS));
    }
@jocel1

This comment has been minimized.

Show comment
Hide comment
@jocel1

jocel1 Oct 11, 2017

Contributor

yes sure :)

Contributor

jocel1 commented Oct 11, 2017

yes sure :)

@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Oct 11, 2017

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- tests/Integration/classes/CartGetOrderTotalTest.php  1
- classes/CartRule.php  5
         

See the complete overview on Codacy

codacy-bot commented Oct 11, 2017

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- tests/Integration/classes/CartGetOrderTotalTest.php  1
- classes/CartRule.php  5
         

See the complete overview on Codacy

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 11, 2017

@jocel1

This comment has been minimized.

Show comment
Hide comment
@jocel1

jocel1 Oct 11, 2017

Contributor

I've actually added tests for all possible modes, none change the result :)

Contributor

jocel1 commented Oct 11, 2017

I've actually added tests for all possible modes, none change the result :)

@Quetzacoalt91

Awesome! Thanks

@Quetzacoalt91 Quetzacoalt91 added this to the 1.7.3.0 milestone Oct 12, 2017

@Quetzacoalt91 Quetzacoalt91 merged commit 44e195f into PrestaShop:develop Oct 12, 2017

2 checks passed

codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment