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 calculation of 'Total' column under "Last Orders" listing on the admin dashboard #21283

Conversation

rav-redchamps
Copy link
Contributor

@rav-redchamps rav-redchamps commented Feb 16, 2019

Description (*)

The calculation of 'Total' column under "Last Orders" listing on the admin dashboard was wrong. It has below problems.

  1. It wasn't considering the refunded discount amount
  2. It was subtracting discount canceled amount instead of adding when viewing data for a particular website or store

Fixed Issues (if relevant)

  1. Wrong order amount on dashboard on Last orders listing when order has discount and it is partially refunded #21281: Wrong order amount on the dashboard on Last orders listing when the order has discount and it is partially refunded
  2. Negative order amount in dashboard latest order when order is cancelled where coupon has been used #18754: Negative order amount in dashboard latest order when order is cancelled where coupon has been used

Manual testing scenarios (*)

Testing fix for issue##18754

  1. Place an order with a coupon code, for example, 4.95 EUR off
  2. Cancel the order
  3. Go to dashboard select a specific store to view data and notice the total column under "Last Orders" listing

negative-total

Testing fix for issue##21281

  1. Add a product with quantity 3 to Cart
  2. Create a 10% discount coupon from admin and apply it to the cart
  3. Go to checkout and place order
  4. Invoice the order from admin, if placed using the offline payment method
  5. Create a credit memo for the order to refund 2 quantities out of 3
  6. Now notice the "Total" column for order on the admin dashboard under "Last Orders" listing. The refunded discount amount on credit memo is not considered in the total.

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)

…ixed Negative order amount in dashboard when viewing particular store data
@magento-engcom-team
Copy link
Contributor

Hi @rav-redchamps. 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.3-develop instance - deploy vanilla Magento instance

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

@rav-redchamps
Copy link
Contributor Author

@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

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

@rogyar
Copy link
Contributor

rogyar commented Feb 16, 2019

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @rogyar. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @rogyar, here is your new Magento instance.
Admin access: https://pr-21283.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@swnsma swnsma self-assigned this Feb 16, 2019
@swnsma swnsma self-requested a review February 16, 2019 12:36
@rogyar
Copy link
Contributor

rogyar commented Feb 16, 2019

Hi @rav-redchamps. Thank you for your collaboration. There's still an issue in case of partially refunded order with a discount. Steps to reproduce the same as you described for issue #21281. Product price: $10, discount: $5 (per item), qty ordered: 3, qty refunded: 2

Order totals
screenshot 2019-02-16 at 13 50 52

Dashboard result
screenshot 2019-02-16 at 13 51 24

Problem: the negative value in dashboard's order total for all store views and for a specific store view.

Could you check it out, please?

@rogyar rogyar self-requested a review February 16, 2019 12:56
@swnsma swnsma removed their assignment Feb 16, 2019
@swnsma swnsma removed their request for review February 16, 2019 13:08
@rav-redchamps
Copy link
Contributor Author

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @rav-redchamps. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @rav-redchamps, here is your new Magento instance.
Admin access: https://pr-21283.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@rav-redchamps
Copy link
Contributor Author

@rogyar just fixed it and tested. Kindly give a check one more time.

@rogyar rogyar self-assigned this Feb 16, 2019
@magento magento deleted a comment from magento-engcom-team Feb 18, 2019
@magento magento deleted a comment from magento-engcom-team Feb 18, 2019
@rogyar
Copy link
Contributor

rogyar commented Feb 18, 2019

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @rogyar. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @rogyar, here is your new Magento instance.
Admin access: https://pr-21283.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@rogyar
Copy link
Contributor

rogyar commented Feb 18, 2019

@rav-redchamps now it looks much better. Thank you.

@VasylShvorak
Copy link
Contributor

✔️ QA passed

@p-bystritsky p-bystritsky force-pushed the dashboard-last-order-total-fix branch 2 times, most recently from 42f115b to 81c7e26 Compare March 7, 2019 10:15
@ghost ghost assigned sidolov Mar 13, 2019
* @return string
*/
protected function getTotalsExpression(
$storeId,
$baseSubtotalRefunded,
$baseSubtotalCanceled,
$baseDiscountCanceled
$baseDiscountCanceled,
$baseDiscountRefunded = '0'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, we are not allowed to change public and protected methods signature according to our Backward Compatible Development Guide

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sidolov

I have observed that you have initialized "$baseDiscountRefunded = '0'" in the method signature. So is this the change you are talking about and already did yourself? or you would like me to remove the additional added parameter to function's signature completely?

@sidolov
Copy link
Contributor

sidolov commented Mar 21, 2019

@p-bystritsky , please, take a look at changes, we should rework them according to the Backward Compatibility Guide

@p-bystritsky p-bystritsky force-pushed the dashboard-last-order-total-fix branch from 81c7e26 to de511c0 Compare March 22, 2019 08:38
@p-bystritsky
Copy link
Contributor

@sidolov done.

@p-bystritsky p-bystritsky force-pushed the dashboard-last-order-total-fix branch from de511c0 to 57c4db4 Compare March 22, 2019 10:55
@magento-engcom-team magento-engcom-team merged commit 57c4db4 into magento:2.3-develop Mar 26, 2019
@ghost
Copy link

ghost commented Mar 26, 2019

Hi @rav-redchamps, 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.

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.

7 participants