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

Optimize cart rule performances #8402

Merged
merged 2 commits into from Nov 6, 2017

Conversation

Projects
None yet
6 participants
@jocel1
Contributor

jocel1 commented Oct 8, 2017

Questions Answers
Branch? develop
Description? Greaty optimize cart rules performances by making them easily cacheable
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket?
How to test? run tests

Important guidelines


This change is Reviewable

@jocel1 jocel1 changed the base branch from develop to 1.7.2.x Oct 8, 2017

@jocel1

This comment has been minimized.

Show comment
Hide comment
@jocel1

jocel1 Oct 8, 2017

Contributor

Created on 1.7.2.x as it has a big impact on performances (tested with a real customer). For this customer, the average generation time drops from 408ms to 275ms (33% faster on average) according to loadimpact.com with 11 differents URLs tested (home, categories, product pages, ...) and 200 unique visitors as the same time.

Contributor

jocel1 commented Oct 8, 2017

Created on 1.7.2.x as it has a big impact on performances (tested with a real customer). For this customer, the average generation time drops from 408ms to 275ms (33% faster on average) according to loadimpact.com with 11 differents URLs tested (home, categories, product pages, ...) and 200 unique visitors as the same time.

@jocel1 jocel1 changed the base branch from 1.7.2.x to develop Oct 9, 2017

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

@tomlev

More precisions needed please

Show outdated Hide outdated classes/CartRule.php
* @return bool
*/
public static function haveCartRuleToday($idCustomer)
{

This comment has been minimized.

@tomlev

tomlev Oct 12, 2017

Member

in this method you only check if cart rule starts or ends today
not if it is valid today
Is there a reason for this restriction ?

@tomlev

tomlev Oct 12, 2017

Member

in this method you only check if cart rule starts or ends today
not if it is valid today
Is there a reason for this restriction ?

This comment has been minimized.

@jocel1

jocel1 Oct 12, 2017

Contributor

I wanted to use the most generic query to make it easily cacheable

@jocel1

jocel1 Oct 12, 2017

Contributor

I wanted to use the most generic query to make it easily cacheable

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

@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Oct 12, 2017

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

Complexity increasing per file
==============================
- classes/CartRule.php  2
         

See the complete overview on Codacy

codacy-bot commented Oct 12, 2017

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

Complexity increasing per file
==============================
- classes/CartRule.php  2
         

See the complete overview on Codacy

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 31, 2017

Contributor

Hello @tomlev, can you reconsider your review? If it's ok for you, I'll merge before the freeze.

Contributor

mickaelandrieu commented Oct 31, 2017

Hello @tomlev, can you reconsider your review? If it's ok for you, I'll merge before the freeze.

@mickaelandrieu mickaelandrieu added this to the 1.7.3.0 milestone Oct 31, 2017

*/
public static function haveCartRuleToday($idCustomer)
{
static $haveCartRuleToday = array();

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Nov 2, 2017

Member

@tomlev, something to say regarding that line?

@Quetzacoalt91

Quetzacoalt91 Nov 2, 2017

Member

@tomlev, something to say regarding that line?

This comment has been minimized.

@jocel1

jocel1 Nov 2, 2017

Contributor

This one is not too annoying for the tests since we just have to change the idCustomer

@jocel1

jocel1 Nov 2, 2017

Contributor

This one is not too annoying for the tests since we just have to change the idCustomer

responded

@toutantic toutantic merged commit 5574dd5 into PrestaShop:develop Nov 6, 2017

2 checks passed

codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eternoendless eternoendless changed the title from CO: optimize cart rule performances to Optimize cart rule performances Dec 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment