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

Shipping method estimation loads all customer's quotes #9744

Closed
sydekumf opened this issue May 23, 2017 · 29 comments
Closed

Shipping method estimation loads all customer's quotes #9744

sydekumf opened this issue May 23, 2017 · 29 comments
Assignees
Labels
bug report Component: Checkout Event: WroCD Contribution day in Wro Issue: Cannot Reproduce Cannot reproduce the issue on the latest `2.4-develop` branch Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed

Comments

@sydekumf
Copy link

sydekumf commented May 23, 2017

Preconditions

Magento 2.1.7

When you enter the checkout Magento tries to estimate the shipping method and costs. Therefore it loads available quotes with this logic: Magento\Quote\Model\QuoteRepository::getForCustomer(). For some reason it tries to load all quotes available for this customer, even quotes which are no longer active or whose products do not longer exist.
This leads to an exception in Magento\Quote\Model\ResourceModel\Quote\Item\Collection::_assignProducts() where it tries to assign the not existing products. The exception is hidden because of this in Magento\Quote\Model\Webapi\ParamOverriderCartId:

} catch (NoSuchEntityException $e) {
            /* do nothing and just return null */
        }

In the frontend it leads to another error which has nothing to do with this error: #1443

I am not sure what is the correct behavior:

  • Should old quotes get deleted?
  • Is it a wrong behavior that it tries to load all quotes?
  • Should the exception handling be much better?

Steps to reproduce

  1. In a customer's account have several quotes and old quotes and delete some of the quote's products
  2. Enter checkout with a new quote

Expected result

  1. Checkout is possible
  2. The error handling is more transparent and exceptions do not get hidden

Actual result

  1. Misleading error message from GET /V1/carts/mine/items is returning "cartId is a required field" #1443 is shown
  2. Checkout is not possible
@korostii
Copy link
Contributor

korostii commented May 23, 2017

Experienced this error myself, with similar findings. Pretty sure I wrote about it somewhere, but cannot find it now.
the catch (NoSuchEntityException $e) seems to have been added in order to cover the case when the customer isn't logged in.

Should the exception handling be much better?

Yes, please. I'll take that, plus, independently, a fix for the underlying error.

Underlying error can probably be fixed by simply adding a foreign key constraint in between cart item and product entity tables, preferably with an "ON DELETE CASCADE" =)

@collymore
Copy link

collymore commented May 25, 2017

Hi, we're having the same issue and can possible shed some light. The error occurs because although there are quotes for the user they all have is_active = 0.

I'm not sure why they have so many quotes though and would have expected them to be removed.

The query that is run when trying to get the quote from within Magento\Quote\Model\Webapi\ParamOverriderCartId is

SELECT quote.* FROM quote WHERE (quote.customer_id=38) AND (store_id IN ('1')) AND (is_active = 1) ORDER BY updated_at DESC LIMIT 1

So it's always trying to get the latest quote but as non are active, then it doesn't' return any for this user.

The error always happens after making a purchase and then added products to the cart in the same session. If you logout and back in a new active quote is created.

@MikeSheward
Copy link

Has anyone found a workaround for this?

@zakgrindle
Copy link

I am also experiencing this issue on Magento 2.1.7. First checkout after login is fine, second checkout attempt results in the 'cartId is a required field' error.

Any assistance would be greatly appreciated.

@burgh8wp
Copy link

burgh8wp commented Aug 3, 2017

This is still present in Magento 2.1.7 occurs on 2nd checkout for logged in user only for us, using steps described above.

@magento-engcom-team magento-engcom-team added 2.1.x bug report Component: Checkout Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed and removed G1 Passed labels Sep 7, 2017
@magento-engcom-team magento-engcom-team added the Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed label Sep 20, 2017
@magento-engcom-team
Copy link
Contributor

@sydekumf, thank you for your report.
We were not able to reproduce this issue by following the steps you provided. If you'd like to update it, please reopen the issue.
We tested the issue on 2.1.9

@magento-engcom-team magento-engcom-team added the Issue: Cannot Reproduce Cannot reproduce the issue on the latest `2.4-develop` branch label Sep 25, 2017
@sydekumf
Copy link
Author

Hi, I do not have any other steps to reproduce. Maybe ask the other guys who can reproduce it, if they can add any further information?
I also checked the code of 2.1.9: You still have the following in it:

} catch (NoSuchEntityException $e) {
            /* do nothing and just return null */
        }

I think there should be a better approach to this, not just hiding exceptions...

@pvalium
Copy link

pvalium commented Sep 29, 2017

Same problem in 2.1.8 EE. Occurs on 2nd checkout for logged in user, using steps described above.

@wget
Copy link

wget commented Oct 2, 2017

@magento-engcom-team If you need credentials to a Magento 2 installation experiencing this issue, let me know.

@magento-engcom-team
Copy link
Contributor

Per multiple requests – reopening this for the further investigation.
Even though for now we can not reproduce the issue in question, we will attempt different approaches.

@magento-engcom-team
Copy link
Contributor

magento-engcom-team commented Oct 2, 2017

@korostii

Underlying error can probably be fixed by simply adding a foreign key constraint in between cart item and product entity tables, preferably with an "ON DELETE CASCADE" =)

Foreign key is implemented programmatically in this specific case, please refer to

public function execute(\Magento\Catalog\Api\Data\ProductInterface $product)
{
$this->itemResource->getConnection()->delete(
$this->itemResource->getMainTable(),
'product_id = ' . $product->getId()
);
}

@wget
Copy link

wget commented Oct 2, 2017

The correct link is:

public function execute(\Magento\Catalog\Api\Data\ProductInterface $product)
{
$this->itemResource->getConnection()->delete(
$this->itemResource->getMainTable(),
'product_id = ' . $product->getId()
);
}

@magento-engcom-team
Copy link
Contributor

@wget Thank you for pointing out. Comment updated.

@JeroenVanLeusden
Copy link
Member

JeroenVanLeusden commented Oct 2, 2017

Also encountered this issue. Seems that Magento\Quote\Model\ResourceModel\Quote::loadByCustomerId() tries to fetch the customers quote but there isn't any active, like @collymore mentioned.

@wget
Copy link

wget commented Oct 2, 2017

I can reproduce this issue with the following steps on 2.1.8 CE. This is rather easy.

  • Put an article in your cart.
  • Go to the checkout page located at https://<BASE_URL>/checkout/
  • Open that same page in another tab.
  • On one of the tab, click to order the product. Go to the other tab and select another shipping address for example. You got the cartId message.

This is pretty normal I get this error in this particular (silly) use case (even if a more straightforward error message should be displayed), I'm not discussing on this one. But I have to admit, when not being silly, I got this error message by doing other steps and by switching from one payment method to the other etc. The investigation goes on on my side as now I'm not able to reproduce (got the error one time in the afternoon though). I will update you when I can find the exact steps to reproduce it completely.

@magento-engcom-team
Copy link
Contributor

@wget Thank you for the update.
Please pay attention to the fact, that if you are using Magento Commerce (Enterprise Edition), the programmatical foreign key, mentioned above, is implemented in a different way, using queue. Having a consumer running is crucial in that case.
On a side note: the exception masking, mentioned in the thread before, is indeed a bad practice, however, it would not influence/fix particular problem described in the ticket.

@wget
Copy link

wget commented Oct 3, 2017

@magento-engcom-team Thanks for the precision, but like I wrote, I'm actually using the Community Edition (2.1.8 CE).

@magento-engcom-team magento-engcom-team added the Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed label Oct 11, 2017
@klein0r
Copy link
Contributor

klein0r commented Oct 13, 2017

We are getting the same issue with Magento CE 2.1.9.

The reason we get this issue is, that the checkout loads another (older) quote for the customer which is still active. Since the quotes are ordered by updated_at DESC it loads the older one, since the new one has "00-00-0000 00:00:00" as updated_at.

The older quote contains a product which is no longer present, so the quote can't be loaded, the %cart_id% can't be replaced (due to NoSuchEntityException) and we are getting the error cartId is required.

So our customer has multiple active quotes. Why can this happen?
When are the quotes set to is_active = 0?

@pvalium
Copy link

pvalium commented Oct 27, 2017

I've solved following this steps:
-Adding sql relation to cart product item, to avoid 'missing' products if you manipulate database by hand (deleting products, using magmi, etc)
-Creating a event in sales_quote_save_before, and checking that update_at field of quote is not null. If is null, i set current_date to update_at.

Now i can make several orders without logout, and all cart strange behaviour has dissappeared.

@sydekumf
Copy link
Author

Hi @magento-engcom-team,

I think I understand the problem now with the QuoteItemsCleaner. But what is the reason, that you did not introduce a database foreign key on delete cascade constraint?
What would be the consequence if we introduce this constraint on our own?

Thanks :-)

@korostii
Copy link
Contributor

Hi @magento-engcom-team

From the investigation we can see that this behavior may be possible in Enterprise Edition

I've seen it happen on CE as well.

Foreign key is implemented programmatically in this specific case, please refer to

Well, obviously, whatever that thing is - it's not working. Is there a particular reason why it was implemented that way? Such an approach seems quite fragile, at best. (i.e. why couldn't you just add this constraint, as several people above suggest?)

On a side note: the exception masking, mentioned in the thread before, is indeed a bad practice, however, it would not influence/fix particular problem described in the ticket.

Why would it "influence/fix particular problem"? No-one said that, that would be nonsense.

The point is that exception masking hides the original exception, ignores invalid application state, making it much harder to trace and debug the code and identify the real root cause behind the second exception which appears elsewhere and seems quite cryptic.

In this particular case it's even worse: the exception throw+catch are used as a control structure in order to pass information (exception is explicitly said to be an "okay" behavior in comments) which is none better then using goto in your code AND it's also bad for performance.

Obviously, exception handling is a separate issue and should be handled independently as there are many other offending pieces of code just like the one referenced here (although this one is the most annoying), but it shouldn't be forgotten and ignored neither, wouldn't you agree?

@sydekumf
Copy link
Author

So there is one thing we did to fix this problem for ever:

ALTER TABLE quote
                CHANGE updated_at 
                updated_at TIMESTAMP NOT NULL
                    DEFAULT CURRENT_TIMESTAMP 
                    ON UPDATE CURRENT_TIMESTAMP;

For a fresh insert the updated_at was 0000000... which of course lead to some problems when you order the quotes DESC. No the updated_at gets the timestamp with a fresh insert and then it gets loaded at first quote by DESC and the error will never appear again :-)

@magento-engcom-team
Copy link
Contributor

@sydekumf @korostii

Is there a particular reason why it was implemented that way?

why couldn't you just add this constraint, as several people above suggest?

It's implemented this way as Magento Commerce Edition uses different approach to clean up.

What would be the consequence if we introduce this constraint on our own?

It is possible that adding a constraint on your own will now have any negative consequences. However, there is no guarantee, only testing can confirm.

@korostii
Copy link
Contributor

Hello again @magento-engcom-team

As far as I can see, you've only addressed one of the issues brought up in this issue report with your comments.
Suppose you're saying (please do correct me if I misunderstood) that you won't be willing to change the old cart cleanup mechanism to guarantee the deletion of invalid quote items as a constraint.
Even so, you could still look into the other parts of the issue in order to mitigate the Magento's behavior upon meeting such (however unlikely) corner cases:

Is it a wrong behavior that it tries to load all quotes?
Should the exception handling be much better?

Handling any of those should IMHO significantly reduce the overall level of pain brought by this issue.

@dmanners dmanners added the Event: WroCD Contribution day in Wro label May 18, 2018
@birmo
Copy link

birmo commented May 18, 2018

#WroCD
I am going to take this issue. Could you assign me?

@ghost
Copy link

ghost commented Feb 13, 2019

Similar or identical github issues for this same error:

#9744

#7299

#1443

#5847

shipperhq/module-shipper#27

#6522

All related, tried every solution on all pages and still have the issue, including @sydekumf solution here:
#9744 (comment)
this solution #7299 (comment)
this solution #7299 (comment)
and checked if this solution would apply but it didn't:
#5847 (comment)

Issue MAGETWO-84524 is tracking it to some degree and still isn't fixed in Magento 2.3

How is this the most popular ecommerce store in the US if it doesn't even have a functioning checkout. This happens for me using authnet directpost regardless of what I do, guest checkout, or new, using stored address, or not. Only the error message will change at times, but it's always one of the error messages given in the links above.

Issue is as described, there is a quote_id in the quote table but it is not active. If I switch it manually to be active then it has duplicate foreign key when trying to replace the order. With active set to 0, get the cartId error.

The only solution anyone seems to say works 100% is buying a 3rd part module to do authorize.net payments which seems absurd, as paypal also doesn't work, citing invoice id errors.

@m2-assistant
Copy link

m2-assistant bot commented Jan 10, 2020

Hi @engcom-Delta. Thank you for working on this issue.
Looks like this issue is already verified and confirmed. But if you want to validate it one more time, please, go though the following instruction:

  • 1. Add/Edit Component: XXXXX label(s) to the ticket, indicating the components it may be related to.

  • 2. Verify that the issue is reproducible on 2.4-develop branch

    Details- Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
    - If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
    - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!

  • 3. If the issue is not relevant or is not reproducible any more, feel free to close it.


@engcom-Delta engcom-Delta removed the Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed label Jan 10, 2020
@engcom-Delta
Copy link
Contributor

Hi @SoloDevAtrix @sydekumf, thank you for your reports. I am not able to reproduce issue by steps you described on clean 2.4-develop
In Magento\Quote\Model\QuoteRepository::getForCustomer() I always get last active quote, but not all quotes for customer.
#9744issueeDebug

And exception under catch (NoSuchEntityException $e) in Magento\Quote\Model\Webapi\ParamOverriderCartId now has appropriate message:
#9744issue

Please feel free to comment, reopen or create new ticket according to the Issue reporting guidelines
if you are still facing this issue on the latest 2.4-develop branch. Thank you for collaboration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report Component: Checkout Event: WroCD Contribution day in Wro Issue: Cannot Reproduce Cannot reproduce the issue on the latest `2.4-develop` branch Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed
Projects
None yet
Development

No branches or pull requests