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

Fix(core) incorrect quantity adjustment #983

Merged
merged 5 commits into from
Aug 18, 2021
Merged

Fix(core) incorrect quantity adjustment #983

merged 5 commits into from
Aug 18, 2021

Conversation

WilliamMilne
Copy link
Contributor

Fixed some edge cases for quantity adjustment. Addresses issue #931.

@michaelbromley
Copy link
Member

I pulled down your branch and played around with it locally. One thing I noticed is that the new test seems to pass event when I revert your changes to order.service.ts. Can you confirm whether you see the same?

If so, could you see if you can create a test which fails before the changes, and passes after them?

@quangpdt
Copy link

The old service is passed with new test is understandable. The incorrect quantity is only returned when we make another query to get the Active order, which is not included in the test case.

@WilliamMilne: Thanks for the fix, I have been looking for the solution for couple days and tried to fix it myself, but clearly it's not covered enough comparing to yours.

Personally, I found this issue is a critical one and need to be fix asap. Would be great if @michaelbromley could help us out, the PR owner has not given new update for more than a month.

@WilliamMilne
Copy link
Contributor Author

Hi @quangpdt - I'm actually taking a look at this today, hopefully will have it resolved soon! Sorry that things have been so busy lately!

@WilliamMilne
Copy link
Contributor Author

WilliamMilne commented Aug 16, 2021

@quangpdt and @michaelbromley - I was able to get a test to fail on master and pass on this branch by adding a GetActiveOrder query and checking the quantity. On master, the quantity was 101, but on the branch, it was the correct value of 100. Unfortunately I've hit a small snag and am encountering some failing tests that appear to be failing on both master and the branch:
image
image

@michaelbromley do you have any suggestions on how to fix these issues so I can push the latest changes? Thanks!!

Also thank you so much for your suggestion @quangpdt! Excited to see if this helps us wrap up the issue!

@WilliamMilne
Copy link
Contributor Author

@michaelbromley - it appears that the customers[0].email issue was solved by deleting the existing e2e data in the folder.

I was also using an outdated version of node, which was causing the asset server test to crash. I've pushed the latest change now! Sorry about the multiple comments!

@quangpdt
Copy link

Thanks for the great fix @WilliamMilne!

@michaelbromley
Copy link
Member

Thank you @quangpdt for your input and @WilliamMilne for your effort to wrap this up. I will review again and all being well, merge this tomorrow.

@michaelbromley michaelbromley merged commit 2441ce7 into vendure-ecommerce:master Aug 18, 2021
@michaelbromley
Copy link
Member

Looks good, thanks all! Will do a patch release this week including this fix.

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

Successfully merging this pull request may close these issues.

3 participants