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.

Sign up for GitHub

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

Jump to bottom

Fixed calculation in SQL query if there are no refunds #25

Merged
merged 1 commit into from
Apr 5, 2022
Merged

Fixed calculation in SQL query if there are no refunds #25

merged 1 commit into from
Apr 5, 2022

Conversation

Progi1984
Copy link
Member

@Progi1984 Progi1984 commented Nov 26, 2021 •
edited
Loading

Questions Answers
Description? Fixed calculation in SQL query if there are no refunds
Type? bug fix
BC breaks? no
Deprecations? no
Fixed ticket? Relative to #22 (Detected by @florine2623)
How to test? Please indicate how to best verify that this PR is correct.

@Progi1984 Progi1984 requested a review from a team November 26, 2021 14:03
@Progi1984 Progi1984 added this to the 2.1.0 milestone Nov 26, 2021
@Progi1984 Progi1984 added the bug label Nov 26, 2021
@Progi1984 Progi1984 mentioned this pull request Nov 26, 2021
Merged
PierreRambaud
PierreRambaud previously approved these changes Nov 29, 2021
View reviewed changes
@florine2623 florine2623 self-assigned this Nov 29, 2021
@florine2623
Copy link
Contributor

Hello @Progi1984 ,

I'm having an issue in the total Sales in Sales and Orders stats page.
In this example I should have €42.60 instead of €39.80
Screenshot 2021-11-29 at 13 40 19
Screenshot 2021-11-29 at 13 40 50

Could you check please ?
Thanks!

@Progi1984
Copy link
Member Author

I check it in the next week ;)

@Progi1984
Copy link
Member Author

@florine2623 It's logical as the calculation is based on total paid real (so that counts on payments)

@florine2623
Copy link
Contributor

Hello @Progi1984 ,

I'm not sure I understand your answer.

There's always a 2.80€ difference between the displayed total Sales and the amount in the table : NOK
Here are a few examples :
Screenshot 2022-01-03 at 16 16 17
Screenshot 2022-01-03 at 16 29 18

Also, there is a lot a number after the comma, the amount is not completely visible.

matthieu-rolland
matthieu-rolland reviewed Mar 2, 2022
View reviewed changes
Copy link
Contributor

@matthieu-rolland matthieu-rolland left a comment

Choose a reason for hiding this comment

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

Rebase needed 👍

@Progi1984 Progi1984 closed this Mar 29, 2022
@Progi1984 Progi1984 reopened this Mar 29, 2022
@matks
Copy link
Contributor

matks commented Apr 4, 2022

@Progi1984 Sorry. I don't understand how this PR went off the radar for so long.

florine2623
florine2623 approved these changes Apr 5, 2022
View reviewed changes
Copy link
Contributor

@florine2623 florine2623 left a comment

Choose a reason for hiding this comment

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

Hello @Progi1984 ,

Thanks for the PR, it is looking good !
I tested with multiple orders, validated orders, validated then canceled orders, orders with multiple products or just one, checked CSV exports, checked distribution of order statuses, checked graphs.

It is QA ✅

@kpodemski kpodemski merged commit a663471 into PrestaShop:dev Apr 5, 2022
@kpodemski
Copy link
Contributor

Thanks @Progi1984 @florine2623

@Progi1984 Progi1984 deleted the fixCalculation branch April 5, 2022 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PierreRambaud PierreRambaud PierreRambaud approved these changes

@matks matks matks approved these changes

@florine2623 florine2623 florine2623 approved these changes

@matthieu-rolland matthieu-rolland Awaiting requested review from matthieu-rolland

Assignees

@florine2623 florine2623

Labels
bug QA ✔️
Projects
None yet
Milestone
2.1.0
Development

Successfully merging this pull request may close these issues.
6 participants
@Progi1984 @florine2623 @matks @kpodemski @PierreRambaud @matthieu-rolland