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

[CatalogPromotions] Catalog promotion event #12990

Merged
merged 14 commits into from
Aug 25, 2021

Conversation

Tomanhez
Copy link
Contributor

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

@Tomanhez Tomanhez added the Feature New feature proposals. label Aug 22, 2021
@Tomanhez Tomanhez requested a review from a team as a code owner August 22, 2021 20:08
@probot-autolabeler probot-autolabeler bot added the API APIs related issues and PRs. label Aug 22, 2021
@Tomanhez Tomanhez force-pushed the viewing-catalog-promotion-event branch 4 times, most recently from a86f298 to 4343048 Compare August 23, 2021 13:06
@Tomanhez Tomanhez force-pushed the viewing-catalog-promotion-event branch 5 times, most recently from d188b7b to b7c1301 Compare August 24, 2021 07:58
@Arminek Arminek force-pushed the viewing-catalog-promotion-event branch from b436a23 to bd090c7 Compare August 25, 2021 07:25
@@ -58,4 +65,12 @@ public function itShouldHaveCodeAndName(string $code, string $name): void
sprintf('Cannot find catalog promotions with code "%s" and name "%s" in the list', $code, $name)
);
}

/**
* @Then this catalog promotion should be usable
Copy link
Member

Choose a reason for hiding this comment

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

As we have this in the step, maybe it would be worth checking does the code from the event is the same as the code of promotion 💃 But it can be an improvement

@Zales0123 Zales0123 merged commit acd9c7e into Sylius:master Aug 25, 2021
@Zales0123
Copy link
Member

Zales0123 commented Aug 25, 2021

Thank you, Tomasz and Arkadiusz! 🥇

GSadee added a commit that referenced this pull request Aug 26, 2021
…dcoded rule and action (Tomanhez, Zales0123)

This PR was merged into the 1.11-dev branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | master
| Bug fix?        | no
| New feature?    | yes
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | replaces #12999, based on #12990
| License         | MIT

It's getting serious 💃 

This is the first PR proposing an **actual** application of Catalog Promotions. Pretty dummy one, as it has [hardcoded taxon rule](https://github.com/Sylius/Sylius/compare/master...Zales0123:catalog-promotions-application?expand=1#diff-6c6f0f742257810316abd6d080fa9a0ebba847270427aca418d6c7bad691fc1eR49) and [discount amount](https://github.com/Sylius/Sylius/compare/master...Zales0123:catalog-promotions-application?expand=1#diff-6c6f0f742257810316abd6d080fa9a0ebba847270427aca418d6c7bad691fc1eR57)... But the point of the change, is to start with some simple architecture of services and handle the event thrown in #12990 🖖 

I imagine we will be iterating over the proposed solution (especially `ProductCatalogPromotionProcessorInterface`), especially to take into account upcoming real Catalog Promotion rules and actions 🚀 

Commits
-------

dce183b Add behats for appling catalog promotions
fb050b4 Finish basic Behat scenarios implementation for Catalog Promotions
bf093e5 Prepare catalog promotion application structure with dummy processor
82b6d67 Register services
0d89fa9 PR review fixes
c61cb51 Remove "Product" from Catalog Promotions - related services
db052de Fix applicator wrong calculation
GSadee added a commit to Sylius/SyliusCoreBundle that referenced this pull request Aug 26, 2021
…dcoded rule and action (Tomanhez, Zales0123)

This PR was merged into the 1.11-dev branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | master
| Bug fix?        | no
| New feature?    | yes
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | replaces Sylius/Sylius#12999, based on Sylius/Sylius#12990
| License         | MIT

It's getting serious 💃 

This is the first PR proposing an **actual** application of Catalog Promotions. Pretty dummy one, as it has [hardcoded taxon rule](https://github.com/Sylius/Sylius/compare/master...Zales0123:catalog-promotions-application?expand=1#diff-6c6f0f742257810316abd6d080fa9a0ebba847270427aca418d6c7bad691fc1eR49) and [discount amount](https://github.com/Sylius/Sylius/compare/master...Zales0123:catalog-promotions-application?expand=1#diff-6c6f0f742257810316abd6d080fa9a0ebba847270427aca418d6c7bad691fc1eR57)... But the point of the change, is to start with some simple architecture of services and handle the event thrown in #12990 🖖 

I imagine we will be iterating over the proposed solution (especially `ProductCatalogPromotionProcessorInterface`), especially to take into account upcoming real Catalog Promotion rules and actions 🚀 

Commits
-------

dce183bc53e7a9084a484b2c11d85ebd523996f0 Add behats for appling catalog promotions
fb050b44a56c9186b82509725e358f8f5ed5bfd2 Finish basic Behat scenarios implementation for Catalog Promotions
bf093e555a959cb2da3c1f8da8d216e4b58f445f Prepare catalog promotion application structure with dummy processor
82b6d6754ac64cb4f0f92c52ebc46668821f2760 Register services
0d89fa95c84b33d5aa9c30ed97f22839a80b632f PR review fixes
c61cb5121a81bf7b1c38508bf4713e14e001f2f3 Remove "Product" from Catalog Promotions - related services
db052de2d57d25376e73c26788be03d03c628518 Fix applicator wrong calculation
lchrusciel added a commit to lchrusciel/Sylius-Standard that referenced this pull request Jan 13, 2022
GSadee added a commit to Sylius/Sylius-Standard that referenced this pull request Jan 14, 2022
…sciel)

This PR was merged into the 1.10-dev branch.

Discussion
----------

Content of PR:
- Changes to PHP, Node and Sf versions
- Usage of sync messenger transport in testing required for proper testing results
- Temporary removal of schema validation waiting for the Sylius release after: Sylius/Sylius#13214
- Whitespace removal in Behat tags - could be omitted, as it is minor stuff, but deprecation error was driving me nuts
- Due to some strange issue with test test execution(ref. https://github.com/Sylius/Sylius-Standard/actions/runs/1688323707) the [vendor/sylius/sylius/features/promotion/managing_catalog_promotions/toggling_catalog_promotion.feature:35](https://github.com/Sylius/Sylius/blob/54dcc4f2ed0a4fea5908fbea3bd69a0f27f16fa1/features/promotion/managing_catalog_promotions/toggling_catalog_promotion.feature#L35-L40) scenario broke in the UI(was fine in API), while was green when I trigger it locally. While run in separation, it was green also on CI, so I've decided to split it to separate run in CI what solved the issue 
- I had to add webprofiler to test cached env due to changes introduced here: Sylius/Sylius#12990. It is needed to register TracableMessegeBus for testing purposes

The build is finally green, but TBH I'm mindblown with the strangeness of errors. If anyone would like to solve them better, be my guests.
  
For the two last issues I've alse left comments in commits for future understanding of problems

Commits
-------

c20eed1 [Maintenance] Run GitHub actions on PHP 8, Node 14 & Symfony 5.4
dc2d180 [Config] Use doctrine transport as default for Symfony Messenger
92d9b2d [Config] Use sync transport for test env
32c5c8c [Maintenance] Temporary removal of schema validation due to changes in payum to support dbal v3
69b3b45 [Maintenance] Remove whitespace in tag defintion
468e37a [Behat] Run managing catalog promotion scenarios in separation
b910859 [Behat] Import web profiler test config into test cached
windragon0910 added a commit to windragon0910/symfony_ecom_framework that referenced this pull request Oct 25, 2023
windragon0910 added a commit to windragon0910/symfony_ecom_framework that referenced this pull request Oct 25, 2023
…sciel)

This PR was merged into the 1.10-dev branch.

Discussion
----------

Content of PR:
- Changes to PHP, Node and Sf versions
- Usage of sync messenger transport in testing required for proper testing results
- Temporary removal of schema validation waiting for the Sylius release after: Sylius/Sylius#13214
- Whitespace removal in Behat tags - could be omitted, as it is minor stuff, but deprecation error was driving me nuts
- Due to some strange issue with test test execution(ref. https://github.com/Sylius/Sylius-Standard/actions/runs/1688323707) the [vendor/sylius/sylius/features/promotion/managing_catalog_promotions/toggling_catalog_promotion.feature:35](https://github.com/Sylius/Sylius/blob/54dcc4f2ed0a4fea5908fbea3bd69a0f27f16fa1/features/promotion/managing_catalog_promotions/toggling_catalog_promotion.feature#L35-L40) scenario broke in the UI(was fine in API), while was green when I trigger it locally. While run in separation, it was green also on CI, so I've decided to split it to separate run in CI what solved the issue 
- I had to add webprofiler to test cached env due to changes introduced here: Sylius/Sylius#12990. It is needed to register TracableMessegeBus for testing purposes

The build is finally green, but TBH I'm mindblown with the strangeness of errors. If anyone would like to solve them better, be my guests.
  
For the two last issues I've alse left comments in commits for future understanding of problems

Commits
-------

c20eed169b7da9ce6307811e79d5ff28d535bb8e [Maintenance] Run GitHub actions on PHP 8, Node 14 & Symfony 5.4
dc2d180d062bae06811b82766a9a710f62949bf4 [Config] Use doctrine transport as default for Symfony Messenger
92d9b2daa305008d8e378b713dbe95108ac7ad9c [Config] Use sync transport for test env
32c5c8c82d7eb3a415910ab2bf75747b46283a4d [Maintenance] Temporary removal of schema validation due to changes in payum to support dbal v3
69b3b45f03dbe6ad303e019509d6d63e75193c76 [Maintenance] Remove whitespace in tag defintion
468e37ac7d2a6813457fe5df9579268d445c31cd [Behat] Run managing catalog promotion scenarios in separation
b910859efede1abe80b613c2fbda835118e32a0b [Behat] Import web profiler test config into test cached
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API APIs related issues and PRs. Feature New feature proposals.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants