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

Can't delete last item in cart if Minimum Order is Enable #6151 #9714

Merged
merged 3 commits into from
May 27, 2017

Conversation

storbahn
Copy link
Contributor

@storbahn storbahn commented May 20, 2017

Description

During editing the cart (adding or removing products from cart) the minimum order amount (mam) will be checked. This caused that the last product could not be removed from cart. In this fix the validation of the mam is removed during editing the cart. The mam will checked later during checkout.

Fixed Issues (if relevant)

  1. Can't delete last item in cart if Minimum Order is Enable  #6151

Manual testing scenarios

See steps to reproduce #6151:

  1. Put item in cart
  2. Go to admin -> SALES -> sales
  3. Enable -> Minimum Order Amount
  4. Minimum Amount = 10 or something
  5. Remove item from cart
  6. Expected result: cart is empty

…ng, editing or deleting products from quote
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented May 20, 2017

CLA assistant check
All committers have signed the CLA.

@ishakhsuvarov ishakhsuvarov self-assigned this May 20, 2017
@ishakhsuvarov ishakhsuvarov added this to the May 2017 milestone May 20, 2017
@@ -117,10 +117,6 @@ public function assign($cartId, \Magento\Quote\Api\Data\AddressInterface $addres
$address->setSaveInAddressBook($saveInAddressBook);
$address->setCollectShippingRates(true);

if (!$quote->validateMinimumAmount($quote->getIsMultiShipping())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The only other check for Minimum Amount occurs in the Checkout Index controller and it is not sufficient, since checkout flow may be performed using Web API without calling the controller.
Please consider moving this check to Place Order call.

@@ -179,7 +186,8 @@ public function __construct(
\Magento\Checkout\Model\Session $checkoutSession,
\Magento\Customer\Model\Session $customerSession,
\Magento\Customer\Api\AccountManagementInterface $accountManagement,
\Magento\Quote\Model\QuoteFactory $quoteFactory
\Magento\Quote\Model\QuoteFactory $quoteFactory,
\Magento\Quote\Model\Quote\Validator\MinimumOrderAmount\ValidationMessage $minimumAmountErrorMessage
Copy link
Contributor

Choose a reason for hiding this comment

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

Due to backwards compatibility policy we can not introduce a new required constructor argument as it may break any code which extends from this class.
Please change this argument to be optional and use ObjectManager to get the dependency, it will be refactored later when such changes are allowed.
Example:

public function __construct(
    \Magento\Quote\Model\OldDependency $old,
    \Magento\Quote\Model\NewDependency $new = null
) {
    $this->old = $old;
    $this->new = $new ?: \Magento\Framework\App\ObjectManager::getInstance()
            ->get(\Magento\Quote\Model\NewDependency::class);
}

@@ -122,6 +122,11 @@ class QuoteManagementTest extends \PHPUnit_Framework_TestCase
protected $quoteMock;

/**
* @var \Magento\Quote\Model\Quote\Validator\MinimumOrderAmount\ValidationMessage
*/
protected $minimumAmountErrorMessage;
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not encourage inheritance based API's even in unit tests.
Please change this to private.

@ishakhsuvarov
Copy link
Contributor

This could potentially fix #8601

@bka
Copy link
Contributor

bka commented May 21, 2017

@ishakhsuvarov: addressed your comments, maybe you can have a look at it again

@magento-team magento-team merged commit 4dfa556 into magento:develop May 27, 2017
magento-team pushed a commit that referenced this pull request May 27, 2017
[EngCom] Public Pull Requests
 - MAGETWO-69379 use payment method name to make checkbox of agreements more unique #6207 #9717
 - MAGETWO-69378 #4272: v2.0.4 Credit memos with adjustment fees cannot be fully refunded with a second credit memo #9715
 - MAGETWO-69375 Can't delete last item in cart if Minimum Order is Enable #6151 #9714
 - MAGETWO-69230 #7279 bill-to name and ship-to name truncated to 20 chars #9654
 - MAGETWO-69155 Fix coding standard in Magento AdminNotification module #9627
@magento-team
Copy link
Contributor

@storbahn thank you for your contribution to Magento 2 project

@vkublytskyi
Copy link

@storbahn This issue still valid for Magento 2.1. Would you be interested in backporting of this solution to 2.1-develop?

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

Successfully merging this pull request may close these issues.

6 participants