-
Notifications
You must be signed in to change notification settings - Fork 35
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
Cart rules optimizer #110
base: dev
Are you sure you want to change the base?
Cart rules optimizer #110
Conversation
Thanks for contributing :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Une première lecture du code avec quelques demandes et questions.
Je n'ai pas encore testé le code.
Si tu veux l'améliorer simplement, tu peux lancer ./vendor/bin/phpstan analyse src/Commands/Optimize/CartRulesOptimizer.php
dans le dossier du module.
Et augmenter progressivement le niveau de test de phpstan (fichier .phpstan.neon)
config/services.yml
Outdated
@@ -103,3 +103,8 @@ services: | |||
class: FOP\Console\Commands\ThemeResetLayout | |||
tags: | |||
- { name: console.command } | |||
|
|||
fop.console.optimize_cart_rules.command: | |||
class: FOP\Console\Commands\Optimize\CartRulesOptimizer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Optimize" c'est très générique, plus on précis mieux c'est.
Là, pour le coup, on ne sait pas vraiment ce que va faire la commande.
Avec un truc du genre cartrules.remove_outdated
/ CartRulesCleanOutdated
par exemple, c'est plus parlant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bonjour @SebSept, modification effectuée
/** | ||
* {@inheritdoc} | ||
*/ | ||
protected function configure() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Si tu peux spécifier le type retourné par les fonctions, ça serait top.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SebSept c'est fait
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function execute(InputInterface $input, OutputInterface $output) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cette fonction doit retourner une valeur : 0 en cas de succes 1 en cas d'echec.
Tu peux aussi spécifier ça dans la signature de la fonction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SebSept c'est fait
|
||
$this->clearCartRules(); | ||
|
||
if ($this->logs['cart_rules'] === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Le mieux ça serait que clearCartRules() renvoit le nombre de règles.
Si on peut éviter de trimbaler une variables pour stocker la valeur, c'est plus fiable et plus économe en code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SebSept, c'est fait
|
||
if (!$this->checkDate($from_date)) { | ||
$io->error('Incorrect date parameter. Please type a date from format Y-m-d. Date given : ' . $from_date); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Le mieux, c'est surement d'interrompre le traitement ici.
Plutot que de continuer l'execution du script alors que le paramètre de date est incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SebSept c'est fait sauf que je traite des jours plutôt qu'une date maintenant
->setDescription('Clear and optimize your Prestashop Cart rules') | ||
->setHelp('Clear your Prestashop from useless row in cart rules table'); | ||
|
||
$this->addUsage('--from=[from-date] (format: Y-m-d, default: 1 month)'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ça serait pas mieux d'avoir comme paramètre une durée ?
Une durée qui corresponde aux nombre de jours depuis lequel les cartRules sont périmé.
Si j'écris un script avec cette commande, dans 6 mois, il faudra réécrire la commande pour changer la date.
Alors que je l'utilse sans paramètre ou avec un paramètre qui serait le nombre de jours, ça serait déjà ok.
Qu'en penses-tu ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SebSept, c'est effectivement plus logique avec des jours que une date.
J'ai fait un refactoring
{ | ||
$instance = Db::getInstance(); | ||
|
||
$instance->delete('cart_rule_combination', 'id_cart_rule_1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tu peux mettres un petit commentaire (en anglais) pour indiquer ce que fais chaque requete, s'il te plait ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SebSept, non traité pour l'instant
$instance->delete('cart_rule_product_rule_group', 'id_cart_rule IN (SELECT id_cart_rule FROM `ps_cart_rule` WHERE `date_to` < "' . $this->from_date . '" OR active = 0 OR quantity = 0)'); | ||
$instance->delete('cart_rule_product_rule', 'NOT EXISTS (SELECT 1 FROM `' . _DB_PREFIX_ . 'cart_rule_product_rule_group` WHERE `' . _DB_PREFIX_ . 'cart_rule_product_rule`.`id_product_rule_group` = `' . _DB_PREFIX_ . 'cart_rule_product_rule_group`.`id_product_rule_group`)'); | ||
$instance->delete('cart_rule_product_rule_value', 'NOT EXISTS (SELECT 1 FROM `' . _DB_PREFIX_ . 'cart_rule_product_rule` WHERE `' . _DB_PREFIX_ . 'cart_rule_product_rule_value`.`id_product_rule` = `' . _DB_PREFIX_ . 'cart_rule_product_rule`.`id_product_rule`)'); | ||
$instance->delete('cart_rule', '`date_to` < "' . $this->from_date . '" OR active = 0 OR quantity = 0'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
En fait, ça ne fait pas que supprimer les cartRule périmés comme je le pensais au début. D'où l'intéret de faire une description claire :D dans la commande et dans le message de PR.
Il vaut mieux être trop verbeux dans les explications que pas assez.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Du coup, ça aurait du sens de proposer une option pour supprimer ou pas les régles inactives ? et une pour celle avec une quantité nulle ?
ping @TheoAlloin . |
Tu sais, j'ai donné de mon temps pour faire cette review @TheoAlloin |
@TheoAlloin ? des nouvelles ? |
Bonjour, |
Merci @matthieu2607 avec plaisir. Êtes-vous sur le slack Friends of Presta pour en discuter ? Invitation sur https://friendsofpresta.org/ |
Bonjour @jf-viguier, |
2d58927
to
73e57c9
Compare
Related to this prestashop issue
Not a solution but a patch to remove deprecated cart rules when we have a lot of cart rules in database like :