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.

Sign up for GitHub

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

Jump to bottom

[CatalogPromotion][API][UI] Fixed discount action #13325

Merged
merged 11 commits into from Nov 26, 2021
Merged

[CatalogPromotion][API][UI] Fixed discount action #13325

merged 11 commits into from Nov 26, 2021

Conversation

GSadee
Copy link
Member

@GSadee GSadee commented Nov 24, 2021

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets
License MIT

@GSadee GSadee added Feature New feature proposals. API APIs related issues and PRs. labels Nov 24, 2021
@GSadee GSadee requested a review from a team as a code owner November 24, 2021 06:35
@probot-autolabeler probot-autolabeler bot added Admin AdminBundle related issues and PRs. Documentation Documentation related issues and PRs - requests, fixes, proposals. labels Nov 24, 2021
@GSadee GSadee force-pushed the catalog-promotion-fixed-discount branch 4 times, most recently from 835dbf6 to 3478fe3 Compare November 24, 2021 14:58
lchrusciel
lchrusciel reviewed Nov 24, 2021
View reviewed changes
...otion/applying_catalog_promotions/applying_all_available_types_of_catalog_promotions.feature Outdated Show resolved Hide resolved
...n/applying_catalog_promotions/applying_catalog_promotions_with_fixed_discount_action.feature Outdated Show resolved Hide resolved
...atalog_promotions/applying_catalog_promotions_with_taking_minimum_price_into_account.feature Outdated Show resolved Hide resolved
features/promotion/managing_catalog_promotions/creating_catalog_promotion.feature Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Api/Admin/ManagingCatalogPromotionsContext.php Outdated Show resolved Hide resolved
src/Sylius/Bundle/CoreBundle/spec/Calculator/FixedDiscountPriceCalculatorSpec.php Show resolved Hide resolved
src/Sylius/Bundle/CoreBundle/spec/Calculator/PercentageDiscountPriceCalculatorSpec.php Show resolved Hide resolved
src/Sylius/Bundle/CoreBundle/spec/Calculator/PercentageDiscountPriceCalculatorSpec.php Show resolved Hide resolved
src/Sylius/Bundle/PromotionBundle/Form/Type/CatalogPromotionActionType.php Outdated Show resolved Hide resolved
src/Sylius/Bundle/PromotionBundle/Validator/CatalogPromotionActionValidator.php
public function validate($value, Constraint $constraint): void
{
/** @var CatalogPromotionAction $constraint */
Assert::isInstanceOf($constraint, CatalogPromotionAction::class);

/** @var CatalogPromotionActionInterface $value */
if ($value->getType() !== CatalogPromotionActionInterface::TYPE_PERCENTAGE_DISCOUNT) {
if (!in_array($value->getType(), $this->actionTypes, true)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that taking into account this snippet:
image
one may add custom validator for a new action type, yet won't be able to use it, because they would need to override this service definition as we are hardcoded for two choices anyway

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, it looks the same with validators for scopes. What is your suggestion? The simplest one is probably to have a separate parameter that is an array of these types 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter would be a good improvement. However, going deeper what would you say, that these consts would be defined by calculators themselves? Then we could tag them and use their const as a tag attribute and here use another tag iterator. Going further, we could define expectations for static method with the name, but I'm not sure if it makes sense to introduce it day before the end of sprint
ref. https://symfony.com/doc/current/service_container/tags.html#tagged-services-with-index

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm 👌🏻 for refactoring, but I would be for making it in a separate PR for both actions and scopes

@GSadee GSadee force-pushed the catalog-promotion-fixed-discount branch 2 times, most recently from fcd8898 to 50c9270 Compare November 25, 2021 08:22
@GSadee GSadee force-pushed the catalog-promotion-fixed-discount branch 2 times, most recently from ea5c411 to f056098 Compare November 25, 2021 09:31
@GSadee GSadee force-pushed the catalog-promotion-fixed-discount branch from f056098 to 969595a Compare November 25, 2021 10:26
@lchrusciel lchrusciel mentioned this pull request Nov 25, 2021
Merged
@Zales0123 Zales0123 merged commit 4644f74 into Sylius:master Nov 26, 2021
@Zales0123
Copy link
Member

Thanks, Grzegorz! 🥇

@GSadee GSadee deleted the catalog-promotion-fixed-discount branch November 26, 2021 07:32
This was referenced Dec 15, 2021
Merged
Merged
Closed
AdamKasp added a commit that referenced this pull request Dec 16, 2021
@AdamKasp
bug #13397 [CatalogPromotion][Admin] Fix displaying fixed discount ac…
551d34d 
…tion on catalog show page (GSadee)

This PR was merged into the 1.11 branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | 1.11
| Bug fix?        | yes
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | fixes #13325
| License         | MIT

<!--
 - Bug fixes must be submitted against the 1.10 or 1.11 branch(the lowest possible)
 - Features and deprecations must be submitted against the master branch
 - Make sure that the correct base branch is set

 To be sure you are not breaking any Backward Compatibilities, check the documentation:
 https://docs.sylius.com/en/latest/book/organization/backward-compatibility-promise.html
-->


Commits
-------

98c7cd3 [CatalogPromotion][Admin] Fix displaying fixed discount action on catalog show page
AdamKasp added a commit that referenced this pull request Dec 16, 2021
@AdamKasp
feature #13398 [Documentation][CatalogPromotion] Add cookbook about c…
955517f 
…reating a custom action + minor improvements (GSadee)

This PR was merged into the 1.11 branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | 1.11
| Bug fix?        | no
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | partially fixes ##13325 (comment)
| License         | MIT

This PR contains a minor refactor of parameters with types of actions/scopes passed to validators.

<!--
 - Bug fixes must be submitted against the 1.10 or 1.11 branch(the lowest possible)
 - Features and deprecations must be submitted against the master branch
 - Make sure that the correct base branch is set

 To be sure you are not breaking any Backward Compatibilities, check the documentation:
 https://docs.sylius.com/en/latest/book/organization/backward-compatibility-promise.html
-->


Commits
-------

7f33db6 [CatalogPromotion] Create parameters with collections of scopes and actions types
08e9a0c [Documentation][CatalogPromotion] Add cookbook about creating a custom action
c4816cd [Documentation][CatalogPromotion] Minor improvements to cookbook about creating a custom scope
e45593d [Documentation][CatalogPromotion] Fixes to cookbooks about creating custom action and scope
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lchrusciel lchrusciel lchrusciel left review comments

@Zales0123 Zales0123 Zales0123 approved these changes

@SirDomin SirDomin SirDomin approved these changes

@AdamKasp AdamKasp AdamKasp approved these changes

Assignees
No one assigned
Labels
Admin AdminBundle related issues and PRs. API APIs related issues and PRs. Documentation Documentation related issues and PRs - requests, fixes, proposals. Feature New feature proposals.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@GSadee @Zales0123 @lchrusciel @SirDomin @AdamKasp