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

Change when item existence is checked in order to update / add new item, see #9445 #9553

Closed
wants to merge 1 commit into from

Conversation

frosit
Copy link

@frosit frosit commented May 8, 2017

Description

This commit corrects a validation within a quote save method. Because this function checks whether getItemById($itemId) returns an item too late (while already being in the update part) the exception message Cart %1 does not contain item %2 is shown. By doing this validation one line earlier, it can continue in the else part if this item is not known to the quote. In the else part, a new product is added to the cart as it should.

It's not easy to reproduce. In order to reproduce and verify this, i've setup a small module in a repository that can be used. The README file contains more in-depth explanation.

https://github.com/frosit/module-devpoc.git

Fixed Issues (if relevant)

  1. Cart 860 does not contain item 1204 #9445: Cart % does not contain item %

Manual testing scenarios

  1. Follow the write-up and use the POC module at https://github.com/frosit/module-devpoc.git

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented May 8, 2017

CLA assistant check
All committers have signed the CLA.

@okorshenko okorshenko self-assigned this May 8, 2017
@okorshenko okorshenko added this to the May 2017 milestone May 8, 2017
@okorshenko
Copy link
Contributor

Hi @frosit
Thank you for your contribution. Unfortunately we can not accept this pull request. By design we should not create new cart item based on existing item (when item id is defined). If we will accept your fix, it will add incorrect behavior to the system. We executed extended set of the automated tests against your change and a lot of tests failed due to incorrect system behavior.

@cygnet-aamunshi
Copy link

Hello,

I have replicated the issue. If we try to add multiple products programmatically then we get the said error for the first instance. After that it works smoothly and all the products are added to cart without any issue.

But, if we clear the browser cache and again try to add the same bunch of products then it will give same error for the first time and after that it again works fine.

As reported by @frosit getItemById($itemId) returns null even if the item is there in the cart. I have verified it in database as well.

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.

4 participants