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

[CatalogPromotions][API] Removal of catalog promotions #14270

Merged
merged 6 commits into from
Sep 15, 2022
Merged

[CatalogPromotions][API] Removal of catalog promotions #14270

merged 6 commits into from
Sep 15, 2022

Conversation

Rafikooo
Copy link
Contributor

@Rafikooo Rafikooo commented Aug 31, 2022 •
edited
Loading

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

This PR introduced a way to remove catalog promotions. As this operation may have an impact on the prices of the products, it is divided into two parts. First, ( if catalog promotion is eligible ) disabling considered promotion, and second, actually remove it. We can take advantage of Symfony messenger and the prepared configuration of three transports ( failed included ) or perform it without the messenger - synchronously.

image

Configuration of transports:
image

@Rafikooo Rafikooo added the Feature New feature proposals. label Aug 31, 2022
@Rafikooo Rafikooo requested a review from a team as a code owner August 31, 2022 19:53
@probot-autolabeler probot-autolabeler bot added the API APIs related issues and PRs. label Aug 31, 2022
@Rafikooo Rafikooo changed the title [WIP] Feature/removal of catalog promotions/api v2 [CatalogPromotions][API] Removal of catalog promotions Sep 1, 2022
@probot-autolabeler probot-autolabeler bot added the Maintenance CI configurations, READMEs, releases, etc. label Sep 2, 2022
@Rafikooo Rafikooo mentioned this pull request Sep 8, 2022
Merged
coldic3
coldic3 reviewed Sep 15, 2022
View reviewed changes
src/Sylius/Bundle/CoreBundle/CatalogPromotion/Processor/CatalogPromotionRemovalProcessor.php
$catalogPromotion = $this->getCatalogPromotion($catalogPromotionCode);

if ($catalogPromotion->getState() === CatalogPromotionStates::STATE_INACTIVE) {
$this->announceInactiveCatalogPromotionRemoval($catalogPromotionCode);
Copy link
Contributor

Choose a reason for hiding this comment

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

If in this case Catalog Promotion goes into the processing state and then is removed?

GSadee
GSadee approved these changes Sep 15, 2022
View reviewed changes
src/Sylius/Bundle/ApiBundle/Controller/RemoveCatalogPromotionAction.php
try {
$this->catalogPromotionRemovalProcessor->removeCatalogPromotion($request->attributes->get('code'));

return new Response(status: Response::HTTP_ACCEPTED);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return new Response(status: Response::HTTP_ACCEPTED);
return new JsonResponse(status: Response::HTTP_ACCEPTED);

?

src/Sylius/Bundle/CoreBundle/CatalogPromotion/Processor/CatalogPromotionRemovalProcessor.php
$this->eventBus->dispatch(new CatalogPromotionEnded($catalogPromotionCode));
}

private function announceInactiveCatalogPromotionRemoval(string $catalogPromotionCode): void
Copy link
Member

Choose a reason for hiding this comment

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

You are dispatching here a command, not an event, so announce is not the best word in my opinion, I would be for simplifying it to:

Suggested change
private function announceInactiveCatalogPromotionRemoval(string $catalogPromotionCode): void
private function removeInactiveCatalogPromotion(string $catalogPromotionCode): void

@GSadee GSadee merged commit f517e29 into Sylius:1.12 Sep 15, 2022
@GSadee
Copy link
Member

GSadee commented Sep 15, 2022

Thanks, Rafał! 🥇

This was referenced Sep 15, 2022
Merged
Merged
GSadee added a commit that referenced this pull request Sep 16, 2022
@GSadee
minor #14311 [CatalogPromotions][Swagger] HTTP Response statuses docu…
bd28203 
…mented for CatalogPromotion removing (Rafikooo)

This PR was merged into the 1.12 branch.

Discussion
----------

| Q               | A                                                            |
|-----------------|--------------------------------------------------------------|
| Branch?         | 1.12 <!-- see the comment below -->                  |
| Bug fix?        | yes                                                       |
| New feature?    | no                                                       |
| BC breaks?      | no                                                       |
| Deprecations?   | no <!-- don't forget to update the UPGRADE-*.md file --> |
| Related tickets | #14270
| License         | MIT                                                          |

Missing swagger responses block:
<img width="1049" alt="image" src="https://user-images.githubusercontent.com/40125720/190454287-24f1268f-51b3-4998-a01c-33825f3e68e5.png">



Commits
-------

0140915 [CatalogPromotions][Swagger] HTTP Response statuses documented for CatalogPromotion removing
GSadee added a commit that referenced this pull request Sep 16, 2022
@GSadee
feature #14291 [CatalogPromotions][UI] Removal of catalog promotions …
e4b0bce 
…(Rafikooo)

This PR was merged into the 1.12 branch.

Discussion
----------

| Q               | A                                                            |
|-----------------|--------------------------------------------------------------|
| Branch?         | 1.12 <!-- see the comment below -->                  |
| Bug fix?        | no                                                       |
| New feature?    | yes                                                       |
| BC breaks?      | yes                                                       |
| Deprecations?   | no <!-- don't forget to update the UPGRADE-*.md file --> |
| Related tickets | #14270
| License         | MIT                                                          |

<img width="1159" alt="image" src="https://user-images.githubusercontent.com/40125720/189155094-6d322ff0-d207-4193-9ff4-aa00af69e429.png">


Commits
-------

15f3696 [CatalogPromotions][Behat][UI] Removing catalog promotion scenarios implemented
c95d73a [CatalogPromotions][UI] Removing catalog promotion
GSadee added a commit that referenced this pull request Sep 16, 2022
@GSadee
feature #14314 [CatalogPromotions][Docs] Removing of catalog promotio…
4f19c64 
…ns documented (Rafikooo)

This PR was merged into the 1.12 branch.

Discussion
----------

| Q               | A                                                            |
|-----------------|--------------------------------------------------------------|
| Branch?         | 1.12 <!-- see the comment below -->                  |
| Bug fix?        | no                                                       |
| New feature?    | yes                                                       |
| BC breaks?      | no                                                       |
| Deprecations?   | no <!-- don't forget to update the UPGRADE-*.md file --> |
| Related tickets | #14270 #14291
| License         | MIT                                                          |

<img width="791" alt="image" src="https://user-images.githubusercontent.com/40125720/190518685-4b30f80e-8c19-411f-ab15-6669318391a2.png">

<img width="799" alt="image" src="https://user-images.githubusercontent.com/40125720/190518521-82f1814d-6fc6-4d91-9702-b32c52f24c1e.png">



Commits
-------

620a7ba [CatalogPromotions][Docs] Removing of catalog promotions documented
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coldic3 coldic3 coldic3 left review comments

@GSadee GSadee GSadee approved these changes

@lchrusciel lchrusciel Awaiting requested review from lchrusciel

@Zales0123 Zales0123 Awaiting requested review from Zales0123

@AdamKasp AdamKasp Awaiting requested review from AdamKasp

@NoResponseMate NoResponseMate Awaiting requested review from NoResponseMate

Assignees
No one assigned
Labels
API APIs related issues and PRs. Feature New feature proposals. Maintenance CI configurations, READMEs, releases, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Rafikooo @GSadee @Zales0123 @NoResponseMate @coldic3 @AdamKasp