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

Fixed bug, when exception occurred on order with coupons cancel, made by guest after creating of customer account. #19423

Conversation

Winfle
Copy link
Contributor

@Winfle Winfle commented Nov 27, 2018

Description (*)

When customer do checkout as a guest using a valid coupon, he makes a new customer account.
During trying to cancel this order, admin does receive SQL exception:
SQLSTATE[23000]: Integrity constraint violation: 1452 Cannot add or update a child row: a foreign key constraint fails (mage.salesrule_customer, ...

Issue related to changes made: #19230

Fixed Issues (if relevant)

  1. Can't Cancel Order #19230 : I Can't Cancel Order

Scenario

Actual problem exist in scenarious when sales rule, used by guest user (using coupon code), it doesn't trigger its coupon usage per customer update, because there is no customer.
After creating of new customer (using of AccountDelegation action), Magento AssignOrderToCustomerObserver using customer_save_after_data_object event tries to assign to this customer order, that has been made previously.
As a result, this Magento\SalesRule\Model\Rule\Customer wasn't created for newly created customer, but normally it should. Furthermore, it should increase time_used attribute for new customer.
As a result, during trying to cancel this type of orders, sales rule plugin is trying to decrease amount of usages for given customer. Actually, method which is responsible for it is not good enough, so it allows to save empty data (empty instert), which provokes cascade adding by foreign keys. As result, admin cannot cancel this order and get SQL error.

Fixing

So, obviously we have to create this entity on customer registration, when Magento assigns order to related new customer.

As you can see, all data we need is set inside of AssignOrderToCustomerObserver and we cannot subscribe on same event, because only this observer (Sales) knows about order and customer, that its going to be assigned.
So, not to mixing contexts (Sales and SalesRule, because assigning of coupon and sales usages happens using Magento\SalesRule\Model\Coupon\UpdateCouponUsages), I decided to make few extending/improvements:

  1. Remove business logic from observer to related service, which will be responsible for delegating orders to customers.
  2. Fire up event event sales_order_customer_assign_after to let another module knows about certain customer has been attached to order.
  3. Catch in SalesRule module to create related salerule_customer model and increase usage time_used value to 1.
  4. Write functional testing, that represent all the flow.

So, this PR not only fixes exception on canceling orders, that has been made by guest before registering new account. But also fixes bug, when sales rule usage has not been increased on the same scenario.

Manual testing scenarios (*)

Checkout as a guest using a valid coupon
After placing the order, register an account using the create account button on the order confirmation page.
Login to the admin
Make sure to get the order to a processing state.
Try to cancel the order
Order is canceled correctly
Sales rule usage for newly created customer is made and set as 1

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)

… by guest

after creating of customer account.
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Nov 27, 2018

CLA assistant check
All committers have signed the CLA.

@magento-engcom-team
Copy link
Contributor

Hi @KravetsAndriy. 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-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.2-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@Winfle
Copy link
Contributor Author

Winfle commented Nov 27, 2018

Going to fix CI issues tomorrow

@Winfle Winfle requested a review from maghamed November 27, 2018 19:35
@Winfle
Copy link
Contributor Author

Winfle commented Nov 28, 2018

@magento-engcom-team give me 2.3-develop instance

@magento-engcom-team
Copy link
Contributor

Hi @KravetsAndriy. Thank you for your request. I'm working on Magento 2.3-develop instance for you

@magento-engcom-team
Copy link
Contributor

Hi @KravetsAndriy, here is your Magento instance.
Admin access: https://i-19423-2-3-develop.instances.magento-community.engineering/admin
Login: admin Password: 123123q
Instance will be terminated in up to 3 hours.

@sivaschenko sivaschenko self-assigned this Nov 28, 2018
@Winfle
Copy link
Contributor Author

Winfle commented Dec 14, 2018

@sivaschenko Hello. How can I fix this issue:

Avoid using static access to class '\Magento\TestFramework\Helper\Bootstrap' in method '__construct'.
As it present in major part of test suite of Magento ?
Thanks

@sivaschenko
Copy link
Member

Hi @Winfle , please move all the code from __construct to setUp method. Or setUpBeforeClass if you really need that run once for a test class.

@Winfle
Copy link
Contributor Author

Winfle commented Dec 21, 2018

@sivaschenko unfortunately, I moved it many times from setUp and __construct methods recursively, but it always shows me Static usage method message with related target method.
So, it static analysis has become too strict, and having static property inside of any method, will trigger this error message.

@sivaschenko
Copy link
Member

sivaschenko commented Dec 25, 2018

Hi @Winfle , thanks for the update! Please move the initialization to setup method. I'll try to figure out why static tests are failing in that case. Also, please, do not forget to sign the CLA for us to be able to process the pull request.

@Winfle
Copy link
Contributor Author

Winfle commented Dec 26, 2018

@sivaschenko you can see, that it's reproducible now.

@magento-engcom-team magento-engcom-team added this to the Release: 2.2.8 milestone Jan 3, 2019
@magento-engcom-team
Copy link
Contributor

Hi @sivaschenko, thank you for the review.
ENGCOM-3784 has been created to process this Pull Request

@sidolov sidolov mentioned this pull request Jan 14, 2019
@magento-engcom-team magento-engcom-team merged commit fdf7da4 into magento:2.2-develop Jan 14, 2019
@ghost
Copy link

ghost commented Jan 14, 2019

Hi @Winfle, 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 pushed a commit that referenced this pull request Jan 14, 2019
… cancel, made by guest after creating of customer account. #19423
@magento-engcom-team
Copy link
Contributor

Hi @Winfle. Thank you for your contribution.
We will aim to release these changes as part of 2.2.8.
Please check the release notes for final confirmation.

Please, consider to port this solution to 2.3 release line.
You may use Porting tool to port commits automatically.

@ecommercegluk
Copy link

Good afternoon,
I have this problem in Magento 2.2.4 and applied the changes to this pull request to the corresponding files (Magento installed via Composer), but apparently did not resolve.
I noticed that in the pull request some files were just modified, and in my store these files don't even exist. Does this fix apply in Magento 2.2.4?

@sivaschenko
Copy link
Member

Hi @ecommercegluk according to the comment #19423 (comment) the earliest version this pull request can be applicable to is 2.2.8

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