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

[Admin][Promotion] Fix removing taxon used in promotion rule #10365

Merged
merged 8 commits into from May 13, 2019

[Admin] Add flash to taxon deletion listener

  • Loading branch information...
GSadee committed May 10, 2019
commit 060e96952d19ab02422fc6358d65c0e393cda38e
@@ -13,12 +13,14 @@ Feature: Accessing an edit page of a promotion with a rule that contains a remov
Scenario: Accessing an edit page of a promotion with a rule that contains a removed taxon
Given there is a promotion "Christmas sale" with "Has at least one from taxons" rule configured with "T-Shirts" and "Mugs"
When I remove taxon named "Mugs"
Then I should be able to modify a "Christmas sale" promotion
Then I should be notified that "Christmas sale" promotion has been updated
And I should be able to modify a "Christmas sale" promotion
And the "Christmas sale" promotion should have "Has at least one from taxons" rule configured

@ui
Scenario: Accessing an edit page of a promotion with a rule that contains a removed taxon
Given there is a promotion "Christmas sale" with "Total price of items from taxon" rule configured with "Mugs" taxon and $100 amount for "United States" channel
When I remove taxon named "Mugs"
Then I should be able to modify a "Christmas sale" promotion
Then I should be notified that "Christmas sale" promotion has been updated
And I should be able to modify a "Christmas sale" promotion
And the "Christmas sale" promotion should not have any rule configured
@@ -14,16 +14,23 @@
namespace Sylius\Behat\Context\Ui\Admin;
use Behat\Behat\Context\Context;
use Sylius\Behat\NotificationType;
use Sylius\Behat\Page\Admin\Taxon\CreatePageInterface;
use Sylius\Behat\Service\NotificationCheckerInterface;
use Sylius\Component\Core\Model\PromotionInterface;
final class RemovingTaxonContext implements Context
{
/** @var CreatePageInterface */
private $createPage;
public function __construct(CreatePageInterface $createPage)
/** @var NotificationCheckerInterface */
private $notificationChecker;
public function __construct(CreatePageInterface $createPage, NotificationCheckerInterface $notificationChecker)
{
$this->createPage = $createPage;
$this->notificationChecker = $notificationChecker;
}
/**
@@ -35,4 +42,15 @@ public function iRemoveTaxonNamed(string $name): void
$this->createPage->open();
$this->createPage->deleteTaxonOnPageByName($name);
}
/**
* @Then I should be notified that :promotion promotion has been updated
*/
public function iShouldBeNotifiedThatPromotionHasBeenUpdated(PromotionInterface $promotion): void
{
$this->notificationChecker->checkNotification(
sprintf('Some rules of the promotions with codes %s have been updated.', $promotion->getCode()),
NotificationType::info()
);
}
}
@@ -276,6 +276,7 @@

<service id="sylius.behat.context.ui.admin.removing_taxons" class="Sylius\Behat\Context\Ui\Admin\RemovingTaxonContext">
<argument type="service" id="sylius.behat.page.admin.taxon.create" />
<argument type="service" id="sylius.behat.notification_checker" />
<tag name="fob.context_service" />
</service>

@@ -16,6 +16,8 @@
use Sylius\Component\Core\Model\TaxonInterface;
use Sylius\Component\Core\Promotion\Updater\Rule\TaxonAwareRuleUpdaterInterface;
use Symfony\Component\EventDispatcher\GenericEvent;
use Symfony\Component\HttpFoundation\Session\Flash\FlashBagInterface;
use Symfony\Component\HttpFoundation\Session\SessionInterface;
use Webmozart\Assert\Assert;
final class TaxonDeletionListener
@@ -26,20 +28,35 @@ final class TaxonDeletionListener
/** @var TaxonAwareRuleUpdaterInterface */
private $totalOfItemsFromTaxonRuleUpdater;
/** @var SessionInterface */
private $session;
public function __construct(
TaxonAwareRuleUpdaterInterface $hasTaxonRuleUpdater,
This conversation was marked as resolved by GSadee

This comment has been minimized.

Copy link
@lchrusciel

lchrusciel May 10, 2019

Member

Would it work with variadic in the constructor?

    public function __construct(
		SessionInterface $session,
        TaxonAwareRuleUpdaterInterface ....$ruleUpdaters
        
    ) {
        $this->session = $session;
        $this->ruleUpdaters = $ruleUpdaters;
    }

and iterate over them?

TaxonAwareRuleUpdaterInterface $totalOfItemsFromTaxonRuleUpdater
TaxonAwareRuleUpdaterInterface $totalOfItemsFromTaxonRuleUpdater,
SessionInterface $session
) {
$this->hasTaxonRuleUpdater = $hasTaxonRuleUpdater;
$this->totalOfItemsFromTaxonRuleUpdater = $totalOfItemsFromTaxonRuleUpdater;
$this->session = $session;
}
public function removeTaxonFromPromotionRules(GenericEvent $event): void
This conversation was marked as resolved by GSadee

This comment has been minimized.

Copy link
@lchrusciel

lchrusciel May 9, 2019

Member

I have two problems with this implementation. First of all, it will work only for taxons deleted through Resource Controller. Secondly, we are hardcoded to only two existing rules that contain relation to the taxon(or taxons).

I would prefer, to decouple this logic from ResourceController, but I'm not sure if it's a good place to do it. For sure, it is a good place to start a discussion.
According to the second problem, it could be resolved with some new interface and iteration over all promotion rules in our store. Something like: TaxonsAwarePromotionRule or something similar. WDYT?

This comment has been minimized.

Copy link
@GSadee

GSadee May 10, 2019

Author Member

We've discussed the first problem, that this behaviour could be associated with the resource event.
About the second one, I've moved the logic of removing taxon from configuration to separate services.

{
$taxon = $event->getSubject();
Assert::isInstanceOf($taxon, TaxonInterface::class);
$this->hasTaxonRuleUpdater->updateAfterDeletingTaxon($taxon->getCode());
$this->totalOfItemsFromTaxonRuleUpdater->updateAfterDeletingTaxon($taxon->getCode());
$firstUpdatedPromotionCodes = $this->hasTaxonRuleUpdater->updateAfterDeletingTaxon($taxon->getCode());
$secondUpdatedPromotionCodes = $this->totalOfItemsFromTaxonRuleUpdater->updateAfterDeletingTaxon($taxon->getCode());
$updatedPromotionCodes = array_unique(array_merge($firstUpdatedPromotionCodes, $secondUpdatedPromotionCodes));
if (!empty($updatedPromotionCodes)) {
/** @var FlashBagInterface $flashes */
$flashes = $this->session->getBag('flashes');
$flashes->add('info', [
'message' => 'sylius.promotion.update_rules',
'parameters' => ['%codes%' => implode(', ', $updatedPromotionCodes)],
]);
}
}
}
@@ -90,6 +90,7 @@
<service id="sylius.listener.taxon_deletion" class="Sylius\Bundle\CoreBundle\EventListener\TaxonDeletionListener">
<argument type="service" id="sylius.promotion_rule_updater.total_of_items_from_taxon" />
<argument type="service" id="sylius.promotion_rule_updater.has_taxon" />
<argument type="service" id="session" />
<tag name="kernel.event_listener" event="sylius.taxon.post_delete" method="removeTaxonFromPromotionRules" />
</service>
</services>
@@ -17,19 +17,24 @@
use Sylius\Component\Core\Model\TaxonInterface;
use Sylius\Component\Core\Promotion\Updater\Rule\TaxonAwareRuleUpdaterInterface;
use Symfony\Component\EventDispatcher\GenericEvent;
use Symfony\Component\HttpFoundation\Session\Flash\FlashBagInterface;
use Symfony\Component\HttpFoundation\Session\SessionInterface;
final class TaxonDeletionListenerSpec extends ObjectBehavior
{
function let(
TaxonAwareRuleUpdaterInterface $hasTaxonRuleUpdater,
TaxonAwareRuleUpdaterInterface $totalOfItemsFromTaxonRuleUpdater
TaxonAwareRuleUpdaterInterface $totalOfItemsFromTaxonRuleUpdater,
SessionInterface $session
): void {
$this->beConstructedWith($hasTaxonRuleUpdater, $totalOfItemsFromTaxonRuleUpdater);
$this->beConstructedWith($hasTaxonRuleUpdater, $totalOfItemsFromTaxonRuleUpdater, $session);
}
function it_adds_flash_that_promotions_have_been_updated(
TaxonAwareRuleUpdaterInterface $hasTaxonRuleUpdater,
TaxonAwareRuleUpdaterInterface $totalOfItemsFromTaxonRuleUpdater,
SessionInterface $session,
FlashBagInterface $flashes,
GenericEvent $event,
TaxonInterface $taxon
): void {
@@ -39,6 +44,33 @@ function it_adds_flash_that_promotions_have_been_updated(
$hasTaxonRuleUpdater->updateAfterDeletingTaxon('toys')->willReturn(['christmas', 'holiday']);
$totalOfItemsFromTaxonRuleUpdater->updateAfterDeletingTaxon('toys')->willReturn(['christmas']);
$session->getBag('flashes')->willReturn($flashes);
$flashes
->add('info', [
'message' => 'sylius.promotion.update_rules',
'parameters' => ['%codes%' => 'christmas, holiday'],
])
->shouldBeCalled()
;
$this->removeTaxonFromPromotionRules($event);
}
function it_does_nothing_if_no_promotion_has_been_updated(
TaxonAwareRuleUpdaterInterface $hasTaxonRuleUpdater,
TaxonAwareRuleUpdaterInterface $totalOfItemsFromTaxonRuleUpdater,
SessionInterface $session,
GenericEvent $event,
TaxonInterface $taxon
): void {
$event->getSubject()->willReturn($taxon);
$taxon->getCode()->willReturn('toys');
$hasTaxonRuleUpdater->updateAfterDeletingTaxon('toys')->willReturn([]);
$totalOfItemsFromTaxonRuleUpdater->updateAfterDeletingTaxon('toys')->willReturn([]);
$session->getBag('flashes')->shouldNotBeCalled();
$this->removeTaxonFromPromotionRules($event);
}
}
@@ -24,3 +24,5 @@ sylius:
out_of_stock: 'There is only %quantity% of this product in the stock.'
product_variant:
update_error: 'There was an unexpected problem with updating a product variant. Please, try to update product variant again.'
promotion:
update_rules: 'Some rules of the promotions with codes %codes% have been updated.'
@@ -22,9 +22,9 @@ final class TotalOfItemsFromTaxonRuleUpdater implements TaxonAwareRuleUpdaterInt
/** @var RepositoryInterface */
private $promotionRuleRepository;
public function __construct(RepositoryInterface $ruleUpdater)
public function __construct(RepositoryInterface $promotionRuleRepository)
{
$this->promotionRuleRepository = $ruleUpdater;
$this->promotionRuleRepository = $promotionRuleRepository;
}
public function updateAfterDeletingTaxon(string $taxonCode): array
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.