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] account: journal dashboard graph wrong value #29936

Conversation

william-andre
Copy link
Contributor

OPW 1918926

Current behavior:
The sql query groups by date,id in a intermediary table instead of the result. This allows to get data in the wrong order if the statements were not produced sequentially. The fill values are computed in the wrong order and may override correct values.

Desired behavior:
There is no override of the values correctly computed.

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@C3POdoo C3POdoo added the OE the report is linked to a support ticket (opw-...) label Jan 4, 2019
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Jan 4, 2019
@qdp-odoo
Copy link
Contributor

qdp-odoo commented Jan 9, 2019

I don't quite get the use case you want to cover.
If I encoded 2 bank statements the same day, in the wrong order, I want to have the last statement of the day, I suppose, which is why max(id) is probably wrong but your patch doesn't seem to fix that.

I think you need to order by name, date instead

Copy link
Contributor

@qdp-odoo qdp-odoo left a comment

Choose a reason for hiding this comment

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

needs some explanation

@william-andre
Copy link
Contributor Author

As @Olivier-LAURENT said in the OPW, here is what happened with the code before this commit
003-sql-query-with-misplaced-orderby-clause-1

and here is what happens after
004-sql-query-with-rightplaced-orderby-clause-1

It could give this kind of result
001-bank-statmnt-graph-1

When actually from the data it should have bee strictly growing. This happens because if the data is fetched in the wrong order from the database, some values override the already computed values here https://github.com/odoo/odoo/blob/ede086b2e1417432dc372e3bbb38fdf4772ff071/addons/account/models/account_journal_dashboard.py#L68-L75
because https://github.com/odoo/odoo/blob/ede086b2e1417432dc372e3bbb38fdf4772ff071/addons/account/models/account_journal_dashboard.py#L66 doesn't get the data in the right order.

@qdp-odoo
Copy link
Contributor

@robodoo r+

@qdp-odoo
Copy link
Contributor

@william-andre got it, thx for the detailed explanation... The only regret I have is that it won't solve the following use case: encoding 2 statements for the same date, but in the wrong order. The join on max(id) will take the last encoded statements whereas you'd probably want to take the max(name). That's the use case I initially thought you wanted to fix.

Anyway, as this is quite an edge case (I'm not even sure someone will ever face it) and given that a join on names seems more fuzzy, I've accepted the PR.

Thanks!

@robodoo robodoo added merging 👷 and removed CI 🤖 Robodoo has seen passing statuses merging 👷 labels Jan 17, 2019
@robodoo
Copy link
Contributor

robodoo commented Jan 17, 2019

Staging failed: ci/runbot on e99fc0dea52d87b5ee0a3ef239853dda1e4b9919 (view more at http://runbot.odoo.com/runbot/build/429599)

@qdp-odoo
Copy link
Contributor

@william-andre needs a rebase?

OPW 1918926

Current behavior:
  The sql query groups by date,id in a intermediary table instead of the result. This allows to get data in the wrong order if the statements were not produced sequentially. The fill values are computed in the wrong order and may override correct values.

Desired behavior:
  There is no override of the values correctly computed.
@william-andre william-andre force-pushed the 10.0-fix-accounting-dashboard-graphs-wan branch from ede086b to e75b6fd Compare January 17, 2019 12:37
@william-andre
Copy link
Contributor Author

@qdp-odoo done

@KangOl
Copy link
Contributor

KangOl commented Jan 17, 2019

@robodoo retry

@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses r+ 👌 and removed error 🙅 labels Jan 17, 2019
robodoo pushed a commit that referenced this pull request Jan 17, 2019
OPW 1918926

Current behavior:
  The sql query groups by date,id in a intermediary table instead of the result. This allows to get data in the wrong order if the statements were not produced sequentially. The fill values are computed in the wrong order and may override correct values.

Desired behavior:
  There is no override of the values correctly computed.

closes #29936
@robodoo robodoo added merging 👷 and removed CI 🤖 Robodoo has seen passing statuses merging 👷 labels Jan 17, 2019
@robodoo
Copy link
Contributor

robodoo commented Jan 17, 2019

Staging failed: ci/runbot (view more at http://runbot.odoo.com/runbot/build/429794)

@KangOl
Copy link
Contributor

KangOl commented Jan 17, 2019

@robodoo retry again

@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses r+ 👌 and removed error 🙅 labels Jan 17, 2019
@robodoo
Copy link
Contributor

robodoo commented Jan 17, 2019

Merged, thanks!

@robodoo robodoo closed this Jan 17, 2019
@qdp-odoo
Copy link
Contributor

again and again and again

@KangOl KangOl deleted the 10.0-fix-accounting-dashboard-graphs-wan branch January 17, 2019 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI 🤖 Robodoo has seen passing statuses OE the report is linked to a support ticket (opw-...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants