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

Fixes issue 23411 - Remove misplaced critical log "No such entity with customerId = xx" #25307

Merged
merged 6 commits into from
Nov 7, 2019

Conversation

hostep
Copy link
Contributor

@hostep hostep commented Oct 27, 2019

Description (*)

This is a new attempt at fixing #23411, this is based on #23952

Review comments from @ihor-sviziev have been taken into account.
The removal of the comment above the constant CHECKOUT_STATE_BEGIN was done to satisfy Magento's coding standard checks, it added no useful information.

Fixed Issues (if relevant)

  1. After upgrading to new version - No such entity with customerId critical logs #23411: After upgrading to new version - No such entity with customerId critical logs
  2. New Quote is not creating after order place for logged in customer #24009: New Quote is not creating after order place for logged in customer

Manual testing scenarios (*)

See: #23952 or #23411 (comment)

Questions or comments

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 are green)

There is no longer a critical error when a logged in customer has no
active quote - which is e.g. the case when an order was placed but no
new quote was yet created.

@see magento#23411
remove empty catch block
@m2-assistant
Copy link

m2-assistant bot commented Oct 27, 2019

Hi @hostep. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@hostep
Copy link
Contributor Author

hostep commented Oct 27, 2019

Pushed one final change: $this->setQuoteId($quote->getId()); => $this->setQuoteId($quoteByCustomer->getId());

This caused some functional test to fail, has something to do with persistent sessions.
All tests should be green now hopefully :)

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Oct 27, 2019

Hi @hostep,
Changes looks good.
Would be great to cover your changes with some test. Will you be able to do that?

@hostep
Copy link
Contributor Author

hostep commented Oct 27, 2019

Hi @ihor-sviziev

What kind of test did you have in mind?

Honestly I have not that much deep knowledge about this part of the Magento codebase.
The intention of this PR is just to try to get that highly annoying/distracting false log message out of the exception.log file :)

@ihor-sviziev
Copy link
Contributor

Hi @hostep,
Just analyzed - looks like cover this change with test will be too hard. I’m approving this PR without tests

@ihor-sviziev ihor-sviziev added Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests Award: bug fix labels Oct 27, 2019
@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-6185 has been created to process this Pull Request

@engcom-Bravo
Copy link
Contributor

engcom-Bravo commented Oct 28, 2019

Hello @hostep. Thank You for Your contribution. Would You please cover Your PR with the appropriate Auto-Tests.

@hostep
Copy link
Contributor Author

hostep commented Oct 28, 2019

@engcom-Bravo: can you clarify what you would like to see tested? Thanks! 🙂

@hostep
Copy link
Contributor Author

hostep commented Nov 1, 2019

Did some cleanup, will attempt to take a look at adding a new integration test as requested next.

@hostep
Copy link
Contributor Author

hostep commented Nov 1, 2019

Hi @ihor-sviziev, @engcom-Bravo or @engcom-Foxtrot

Can one of you guys take care of this required integration test you are asking for?
After looking for more than an hour on how to approach this and trying out some things, I still don't see it and I won't have anymore time/energy to keep searching. Writing tests is not my forté, so I would be appreciated if one of you (or somebody else) can take this over.

Thanks and sorry!

@ihor-sviziev ihor-sviziev added Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests and removed Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests labels Nov 4, 2019
@engcom-Bravo
Copy link
Contributor

✔️ QA Passed.

@engcom-Golf engcom-Golf added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests and removed Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests labels Nov 6, 2019
@m2-assistant
Copy link

m2-assistant bot commented Nov 7, 2019

Hi @hostep, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.4 milestone Nov 7, 2019
@hostep
Copy link
Contributor Author

hostep commented Nov 7, 2019

Thanks for adding a test @Nazar65 ! 🙂

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.

10 participants