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

Migrate Sell > Catalog > Catalog price rule create/edit action #13716

Merged
merged 54 commits into from Nov 12, 2019

Conversation

@zuk3975
Copy link
Contributor

zuk3975 commented May 8, 2019

Questions Answers
Branch? develop
Description? Migrate Sell > Catalog > Catalog price rule create/edit action. Condition groups should go to separate PR.
Type? refacto
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? part of #10548
How to test? use /admin-dev/index.php/sell/catalog/catalog-price-rules link to reach migrated catalog price rules list. Click edit or create new to see the form. The Condition groups (at the bottom of the page) are not migrated yet and should be done in separate PR.

This change is Reviewable

@zuk3975 zuk3975 requested a review from PrestaShop/prestashop-core-developers as a code owner May 8, 2019
@matks matks added the migration label May 9, 2019
@matks matks mentioned this pull request May 10, 2019
1 of 33 tasks complete
@@ -58,8 +58,11 @@ public function getData($catalogPriceRuleId)
$editableCatalogPriceRule = $this->bus->handle(new GetCatalogPriceRuleForEditing((int) $catalogPriceRuleId));
$price = $editableCatalogPriceRule->getPrice();
$leaveInitialPrice = false;
if (-1.0 === $price) {

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud May 13, 2019

Contributor

Watch out with float comparaison in PHP.

admin-dev/themes/new-theme/js/app/utils/datepicker.js Outdated Show resolved Hide resolved
*/
_handle() {
const checkboxVal = $(`${this.$checkboxSelector}:checked`).val();
const isFieldEnabled = parseInt(checkboxVal, 10);

This comment has been minimized.

Copy link
@sarjon

sarjon May 15, 2019

Contributor

What's 10?

$specificPriceRule->from_quantity = $command->getFromQuantity();
$specificPriceRule->price = $command->getPrice();
$specificPriceRule->from = $command->getFrom();
$specificPriceRule->to = $command->getTo();

This comment has been minimized.

Copy link
@sarjon

sarjon May 15, 2019

Contributor

I think that getFrom() and getTo() should have better naming. Maybe getDateFrom() or similar, wdyt?

$specificPriceRule->to = $command->getTo();
$specificPriceRule->reduction_type = $command->getReductionType();
$specificPriceRule->reduction_tax = $command->isTaxIncluded();
$specificPriceRule->reduction = $command->getReduction();

This comment has been minimized.

Copy link
@sarjon

sarjon May 15, 2019

Contributor

Maybe getReductionAmount()?

This comment has been minimized.

Copy link
@zuk3975

zuk3975 May 15, 2019

Author Contributor

Not sure, doesn't seem that it would clarify much, also because of reduction types being amount and percentage

$groupId,
$fromQuantity,
$reduction,
$shopId = null,

This comment has been minimized.

Copy link
@sarjon

sarjon May 15, 2019

Contributor

Is shop association really optional?

/**
* Provides data transfer object for editing CatalogPriceRule
*/
class GetCatalogPriceRuleForEditing

This comment has been minimized.

Copy link
@sarjon

sarjon May 15, 2019

Contributor

As discussed before, should we rename it to GetCatalogPriceRuleForEditForm? ping @matks

This comment has been minimized.

Copy link
@zuk3975

zuk3975 May 15, 2019

Author Contributor

As discussed before, should we rename it to GetCatalogPriceRuleForEditForm? ping @matks

I guess you mean rename to 'GetCatalogPriceRuleForUpdating' ? or smth associated with update

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 17, 2019

Contributor

No, I mean GetCatalogPriceRuleForEditForm.

This comment has been minimized.

Copy link
@matks

matks Jul 19, 2019

Contributor

I'm sorry I might have forgotten something here :/ why do you want to modify the naming ?

I'm not against it, I just want to understand 😅
And then we might need to write a convention and modify it everywhere

/**
* Provides data for editing CatalogPriceRule
*/
class EditableCatalogPriceRule

This comment has been minimized.

Copy link
@sarjon

sarjon May 15, 2019

Contributor

I think this should be called CatalogPriceRuleForEditForm. ping @matks

This comment has been minimized.

Copy link
@matks

matks Jul 19, 2019

Contributor

👍

This comment has been minimized.

Copy link
@zuk3975

zuk3975 Jul 22, 2019

Author Contributor

As I see all the other handlers are named like Get[name]ForEditing

/**
* @param CommandBusInterface $commandBus
* @param $isMultishopEnabled
* @param $contextShopId

This comment has been minimized.

Copy link
@sarjon

sarjon May 15, 2019

Contributor

Missing type hints.

@zuk3975

This comment has been minimized.

Copy link
Contributor Author

zuk3975 commented May 15, 2019

@matks, ready for review

/**
* {@inheritdoc}
*/
public function handle(AddCatalogPriceRuleCommand $command)

This comment has been minimized.

Copy link
@zuk3975

zuk3975 May 15, 2019

Author Contributor

@matks, I found this smartyCache cleaning unnecessary, but I'm not really sure, wdyt?

This comment has been minimized.

Copy link
@Matt75

Matt75 May 15, 2019

Contributor

You can read about that here : #13627 (comment)

In fact cache need to be cleared in order to update price in all existing smarty cache but doing here is not really good as you can read on my report.

This comment has been minimized.

Copy link
@matks

matks Jul 18, 2019

Contributor

Very interesting, thanks @Matt75 !
@zuk3975 I suggest we remove it for now but create a dedicated issue for that.

I can imagine 3 solutions for this issue (there might be more):

  • display a warning message after a catalog price rule has been created "eyh ! you created a rule, you probably want to clear the cache to make sure FO display is right !" with a link/button to perform the clear (additionnaly: using an ajax request so we dont freeze the screen ❤️)
  • put a timer to clear cache 1 minute after last catalog price rule modification in order to avoid to perform 1 clear by rule created
  • allow merchant to control whether he wants to clear the cache manually or automatically with a boolean configuration value

This comment has been minimized.

Copy link
@zuk3975

zuk3975 Jul 19, 2019

Author Contributor

created a reminder issue for that #14761

@zuk3975 zuk3975 changed the title [WIP] Migrate Sell > Catalog > Catalog price rule create/edit action Migrate Sell > Catalog > Catalog price rule create/edit action May 17, 2019
}) }}

{{ ps.form_group_row(catalogPriceRuleForm.reduction_type, {}, {
'label': 'Reduction Type'|trans({}, 'Admin.Catalog.Feature'),

This comment has been minimized.

Copy link
@LouiseBonnard

LouiseBonnard May 21, 2019

Contributor

Hey, please write Reduction type without the second capital letter, thank you.

@zuk3975 zuk3975 changed the title Migrate Sell > Catalog > Catalog price rule create/edit action [WIP] Migrate Sell > Catalog > Catalog price rule create/edit action Jun 6, 2019
@matks matks changed the title [WIP] Migrate Sell > Catalog > Catalog price rule create/edit action Migrate Sell > Catalog > Catalog price rule create/edit action Jun 7, 2019
;
if ($this->isMultishopEnabled) {
$builder->add('id_shop', ChoiceType::class, [

This comment has been minimized.

Copy link
@zuk3975

zuk3975 Jun 13, 2019

Author Contributor

@matks, I see that shop field is not in the specs doc. Is it missed by mistake (I guess, yes, otherwise we don't know which shop to target), or it really shouldn't be there?

This comment has been minimized.

Copy link
@matks

matks Jul 19, 2019

Contributor

ping @marionf who will have the answer 😄

This comment has been minimized.

Copy link
@matks

matks Aug 26, 2019

Contributor

ping @marionf again 😄
The specifications doc does mention the "shop" input for multistore, is that expected or a small mistake ?

use Symfony\Component\Validator\Constraint;
/**
* Provides reduction validation by reduction type

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 17, 2019

Contributor

Constraint for validating reduction?

$message = $constraint->invalidPercentageMessage;
}
if (DomainConstraintException::INVALID_REDUCTION_TYPE === $exceptionCode) {

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 17, 2019

Contributor

I think you could merge it into elseif.

public $invalidAmountMessage = 'Reduction value is invalid.';
public $invalidPercentageMessage = 'Reduction value is invalid.';

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 17, 2019

Contributor

Why do you need 3 different properties for messages? Aren't 2 enough:
$invalidTypeMessage - when reduction type is invalid (for example type is not percent nor amount)
$invalidValueMessage - when reduction value is invalid (for example value is 110 when percent is type)

This comment has been minimized.

Copy link
@zuk3975

zuk3975 Jun 17, 2019

Author Contributor

But the cap of 100% should be applied only for percentage value, else its constrained to be >= 0. So the error messages differs a bit

This comment has been minimized.

Copy link
@zuk3975

zuk3975 Jun 17, 2019

Author Contributor

I updated the default messages in constraints, now it should make sense

$fromQuantity,
$reductionType,
$reductionValue,
$shopId,

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 17, 2019

Contributor

Can it only be created for single shop?

This comment has been minimized.

Copy link
@zuk3975

zuk3975 Jun 17, 2019

Author Contributor

according to legacy - yes, according to current specs - i don't see shop selection, but waiting for @matks response if it's not missed by mistake.

/**
* Provides data transfer object for editing CatalogPriceRule
*/
class GetCatalogPriceRuleForEditing

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 17, 2019

Contributor

No, I mean GetCatalogPriceRuleForEditForm.

$to,
$reductionType,
$includeTax,
$reduction

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 17, 2019

Contributor

Do you think Reduction VO could be used instead?

* @param float $reduction
*/
public function __construct(
$catalogPriceRuleId,

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 17, 2019

Contributor

I think you can use VO here.

{
return [
$this->translator->trans('Amount', [], 'Admin.Global') => 'amount',
$this->translator->trans('Percentage', [], 'Admin.Global') => 'percentage',

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 17, 2019

Contributor

How about using constants for amount and percentage from VO?

])
->add('reduction', ReductionType::class, [
'constraints' => [
new ReductionConstraint($this->getReductionMessages()),

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 17, 2019

Contributor

I think Constraint suffix is not needed.

@zuk3975 zuk3975 changed the title Migrate Sell > Catalog > Catalog price rule create/edit action [WIP] Migrate Sell > Catalog > Catalog price rule create/edit action Jul 18, 2019
* <input type="text" class="form-control"
* data-format="YYYY-MM-DD HH:mm:ss" // provide data-format attr in case you need custom format
* />
* </div>
*/
const init = function initDatePickers() {

This comment has been minimized.

Copy link
@matks

matks Jul 18, 2019

Contributor

We need a more accurate name for this class, because we might need multiple date-pickers extensions. Some with YYYY-MM-DD, some with only YYYY-MM, some side-by-side, some other displays ... so we need to name them accurately in order to avoid confusion and pick the one we need

This comment has been minimized.

Copy link
@zuk3975

zuk3975 Jul 19, 2019

Author Contributor

Actually I would like to wait for #14748 this to be solved, as I guess it will modify this js file.

This comment has been minimized.

Copy link
@matks

matks Aug 26, 2019

Contributor

#14748 has been fixed by #14914 😉

const $ = window.$;

$(() => {
new PriceFieldAvailabilityHandler('#catalog_price_rule_leave_initial_price', '#catalog_price_rule_price');

This comment has been minimized.

Copy link
@matks

matks Jul 18, 2019

Contributor

Please create a dedicated JS file to contain these selectors, like here:
https://github.com/PrestaShop/PrestaShop/blob/develop/admin-dev/themes/new-theme/js/pages/employee/employee-form-map.js
And use it like this
https://github.com/PrestaShop/PrestaShop/blob/develop/admin-dev/themes/new-theme/js/pages/employee/EmployeeForm.js

This will help our friends from the QA team to target them and build E2E tests 😉

/**
* {@inheritdoc}
*/
public function handle(AddCatalogPriceRuleCommand $command)

This comment has been minimized.

Copy link
@matks

matks Jul 18, 2019

Contributor

Very interesting, thanks @Matt75 !
@zuk3975 I suggest we remove it for now but create a dedicated issue for that.

I can imagine 3 solutions for this issue (there might be more):

  • display a warning message after a catalog price rule has been created "eyh ! you created a rule, you probably want to clear the cache to make sure FO display is right !" with a link/button to perform the clear (additionnaly: using an ajax request so we dont freeze the screen ❤️)
  • put a timer to clear cache 1 minute after last catalog price rule modification in order to avoid to perform 1 clear by rule created
  • allow merchant to control whether he wants to clear the cache manually or automatically with a boolean configuration value
*
* @throws PrestaShopException
*/
private function createSpecificPriceRuleFromCommand(EditCatalogPriceRuleCommand $command)

This comment has been minimized.

Copy link
@matks

matks Jul 18, 2019

Contributor

It's not exactly create right ? 😄

Fetch ? Get ? Populate ? Hydrate ?

sprintf('Failed to create specific price rule')
);
}
$specificPriceRule->deleteConditions();

This comment has been minimized.

Copy link
@matks

matks Jul 18, 2019

Contributor

👍 nice operations split

*/
private function createSpecificPriceRuleFromCommand(AddCatalogPriceRuleCommand $command)
{
$specificPriceRule = new SpecificPriceRule();

This comment has been minimized.

Copy link
@matks

matks Jul 19, 2019

Contributor

Must be boring to write these fields, but it's definitely very readable and maintanable 👍

sprintf('Failed to update specific price rule with id %s', $specificPriceRule->id)
);
}
$specificPriceRule->deleteConditions();

This comment has been minimized.

Copy link
@matks

matks Jul 19, 2019

Contributor

So we need to call this everytime we edit it ?

This comment has been minimized.

Copy link
@zuk3975

zuk3975 Jul 19, 2019

Author Contributor

At least its how its done in legacy, but this should be checked more in-depth on separate PR of condition groups. Added a reminder here #10549 (comment)

$specificPriceRule->reduction_tax = $command->isTaxIncluded();
}
if (null !== $command->getReduction()) {
$specificPriceRule->reduction_type = $command->getReduction()->getType();

This comment has been minimized.

Copy link
@matks

matks Jul 19, 2019

Contributor

Interesting 🤔

private $fromQuantity;
/**
* @var Reduction

This comment has been minimized.

Copy link
@matks

matks Jul 19, 2019

Contributor

This is a good idea 👍

}
$this->context->buildViolation($message, $params)
->setTranslationDomain('Admin.Notifications.Error')

This comment has been minimized.

Copy link
@matks

matks Jul 19, 2019

Contributor

Can you check it works (check with 2 languages) ? I believe @mickaelandrieu told me once we could not use Symfony validator because there was only 1 translation catalog available.

This comment has been minimized.

Copy link
@zuk3975

zuk3975 Jul 19, 2019

Author Contributor

You are right, it is not working 😞

}
try {
new Reduction($value['type'], $value['value']);

This comment has been minimized.

Copy link
@matks

matks Jul 19, 2019

Contributor

So you're using validation checks in Reduction that throw DomainConstraintExceptions and catch them in order to write the right error message and avoid breaking the flow ..?

Euh ... 🤔it works but it hijacks the standard workflow of these validation checks.

I've never seen this way of validating an object 😅 it might feel weird, but maybe it's right.
Need more opinion, ping @PierreRambaud @jolelievre @eternoendless @mickaelandrieu

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jul 19, 2019

Contributor

Look weird to use a constructor to validate data 🤔
ping @eternoendless

This comment has been minimized.

Copy link
@matks

matks Aug 26, 2019

Contributor

I agree with @PierreRambaud, please move the validation logic outside of Reduction constructor 😉

This comment has been minimized.

Copy link
@sarjon

sarjon Sep 24, 2019

Contributor

@matks although I agree that it's not the most convenient way to validate data, but it works.

We could of course duplicate validation that is inside Reduction constructor, but we can't move it out. Do you have anything else on your mind? :)

/**
* @return Reduction
*/
public function getReduction(): Reduction

This comment has been minimized.

Copy link
@matks

matks Jul 19, 2019

Contributor

php7 💪
I ❤️ that !

{
try {
return new DateTime($dateTime);
} catch (Exception $e) {

This comment has been minimized.

Copy link
@matks

matks Jul 19, 2019

Contributor

Are you sure every Exception thrown from previous line are because of bad format 😄?

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Sep 26, 2019

Contributor

As the phpdoc says, yes.

If time contains an invalid date/time format, then an exception is now thrown. Previously an error was emitted

This comment has been minimized.

Copy link
@zuk3975

zuk3975 Sep 27, 2019

Author Contributor

indeed, exception should be thrown only due to wrong format 😲 . Imo, the current message looks clear anyway

@zuk3975 zuk3975 force-pushed the zuk3975:m/catalog-price-rules-edit branch from 09e1b75 to 0da87c9 Jul 19, 2019
/**
* @var float|null
*/
private $price;

This comment has been minimized.

Copy link
@matks

matks Jul 19, 2019

Contributor

Maybe we should start using Number from https://github.com/PrestaShop/decimal here to carry the data

Sure, it'll get casted to float when the data is thrown back in the legacy
But at least it'll be the right data structure in the migrated part of the code

Wdyt ?

This comment has been minimized.

Copy link
@zuk3975

zuk3975 Jul 22, 2019

Author Contributor
/**
* Provides data transfer object for editing CatalogPriceRule
*/
class GetCatalogPriceRuleForEditing

This comment has been minimized.

Copy link
@matks

matks Jul 19, 2019

Contributor

I'm sorry I might have forgotten something here :/ why do you want to modify the naming ?

I'm not against it, I just want to understand 😅
And then we might need to write a convention and modify it everywhere

/**
* Thrown when unable to update catalog price rule
*/
class UpdateCatalogPriceRuleException extends CatalogPriceRuleException

This comment has been minimized.

Copy link
@matks

matks Jul 19, 2019

Contributor

CannotUpdateCatalogPriceRuleException ? FailureToUpdateCatalogPriceRuleException ?

This comment has been minimized.

Copy link
@zuk3975

zuk3975 Jul 19, 2019

Author Contributor

I thought the exception suffix should already indicate that something failed

/**
* Provides data for editing CatalogPriceRule
*/
class EditableCatalogPriceRule

This comment has been minimized.

Copy link
@matks

matks Jul 19, 2019

Contributor

👍

*/
private function assertIsAllowedType(string $type)
{
if ($type !== self::TYPE_AMOUNT && $type !== self::TYPE_PERCENTAGE) {

This comment has been minimized.

Copy link
@matks

matks Jul 19, 2019

Contributor

If you use an in_array(), it will be easier to add new types later 😉

;
if ($this->isMultishopEnabled) {
$builder->add('id_shop', ChoiceType::class, [

This comment has been minimized.

Copy link
@matks

matks Jul 19, 2019

Contributor

ping @marionf who will have the answer 😄

path: /
methods: [GET]
defaults:
_controller: 'PrestaShopBundle:Admin\Sell\Catalog\CatalogPriceRule:create'

This comment has been minimized.

Copy link
@matks

matks Jul 19, 2019

Contributor

Not create I think 😄

@sarjon

This comment has been minimized.

Copy link
Contributor

sarjon commented Aug 5, 2019

@zuk3975 did you address all the comments from @matks review?

@zuk3975 zuk3975 force-pushed the zuk3975:m/catalog-price-rules-edit branch from b0926c9 to 1b300df Nov 11, 2019
@sarahdib sarahdib added QA ✔️ and removed waiting for QA labels Nov 12, 2019
@Progi1984 Progi1984 merged commit ef9ab64 into PrestaShop:develop Nov 12, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@colinegin

This comment has been minimized.

Copy link
Collaborator

colinegin commented Nov 13, 2019

Hello,

FYI i've just created a new issue to change some wording on this page.
Here is the issue : #16402

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