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

Fix item based promotion issue #1811

Merged

Conversation

umpirsky
Copy link
Contributor

Steps to reproduce:

  • Add something to cart, with some "order item adjustment" (promotion) applied.
  • Edit the promotion settings, makes it ineligible.
  • Update the cart, you will see the "order item adjustment" still attached. Ideally it should have been removed.

Thanks for reporting it @kayue!

@@ -79,6 +79,7 @@ protected function createUser($email, $password, $enabled = true, array $roles =
$user = $this->getUserRepository()->createNew();
$user->setFirstname($this->faker->firstName);
$user->setLastname($this->faker->lastName);
$user->setUsername($email);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not enough, see #1809.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stloyd OK, thanks. This was enough for me to load fixtures. Not related to this fix anyway.

@umpirsky umpirsky force-pushed the fix/item-based-promotion-revert branch from ec49888 to 2210154 Compare August 22, 2014 12:51
@kayue
Copy link
Contributor

kayue commented Aug 27, 2014

@pjedrzejewski This should be ready, kindly review.

@kayue
Copy link
Contributor

kayue commented Sep 15, 2014

ping @pjedrzejewski

pjedrzejewski pushed a commit that referenced this pull request Sep 15, 2014
@pjedrzejewski pjedrzejewski merged commit 877299d into Sylius:master Sep 15, 2014
@pjedrzejewski
Copy link
Member

Thank you @umpirsky @kayue, sorry it took so long.

@umpirsky umpirsky deleted the fix/item-based-promotion-revert branch September 15, 2014 13:31
} catch (UnsupportedTypeException $exception) {
if (!$eligibleRules) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the first rule throws an exception? This would set eligible to false prematurely. Perhaps we should do something that's more like the AuthorizationDecisionManager and rules could be more like voters. Then we could allow rules to abstain from certain votes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants