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

[14.0][ADD] account_reconciliation_widget #359

Merged
merged 3 commits into from
Jan 23, 2021

Conversation

ozono
Copy link

@ozono ozono commented Dec 17, 2020

First working alpha version.

Please review and send your comments.

Known issues:

  • Some dirty code.
  • JS tests not yet reviewed and restored.
  • Python tests works, but would be convenient to write more.

@pedrobaeza
Copy link
Member

Works OK on a first functional test, although it's very weird all the suspense account + post thing.

"company_ids": self.mapped("company_id").ids,
}
action_context.update({"suspense_moves_mode": True})
action_context.update({"statement_line_ids": ids})

Choose a reason for hiding this comment

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

Why use update if you are instantiating the dictionary 2 lines up?

        action_context = {
            "show_mode_selector": False,
            "company_ids": self.mapped("company_id").ids,
            "suspense_moves_mode": True,
            "statement_line_ids": ids,
        }

Copy link
Author

Choose a reason for hiding this comment

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

You're right.


:param writeoff_vals: list of dicts containing values suitable for
account_move_line.create(). The data in vals will be processed to
create bot writeoff account.move.line and their enclosing

Choose a reason for hiding this comment

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

both

Copy link
Author

Choose a reason for hiding this comment

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

Ok.

"&",
("name", "!=", "/"),
("name", "ilike", search_str),
]

Choose a reason for hiding this comment

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

I know Odoo never tackled these types of things but the use of ilike can give significant performance hits on large tables. Even more ilikes in the same domain will probably never even return a result.

Copy link
Author

Choose a reason for hiding this comment

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

I agree about this domain performance... but here it returns desired lines and has been a long time without reported issues. Also, this function can be easy overrided in a custom module.

Choose a reason for hiding this comment

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

@PieterPaulussen This is not really true except for in a very basic untuned postgres installation. pg_trgm is a longstanding solution to openended ilike searches and any integrator who deals in mid sized tables of a few hundred thousand to a few tens of millions will already be monitoring for these and adding appropriate indexes.

The killer in that query with large tables will be more likely the generated IN clauses in non specific searches.

Choose a reason for hiding this comment

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

A trigram index is indeed a good solution in some cases. Unfortunately just adding indexes won't suffice when account.move.line grows even further to roughly 2.5 billion records as we have in of our long running projects.
We solved the 'where id in []' issue a couple of years ago by changing the clause to use a custom stored boolean flag. However, even with that implementation, it was insufficient.
If I remember correctly it was the join operations from AML with account.move and account.account which were even worse.
In the end we replaced the search box with a more elegant solution allowing users to search specific fields with different operators according to their needs.

Choose a reason for hiding this comment

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

@PieterPaulussen OK but that is not typical of any Odoo installation. Extreme outliers in terms of pushing Odoo have always had to rewrite those parts themself. To have that many move lines you have already done extensive customizations, changing the column type of id field, support for either BIGSERIAL or GUID style identifiers. Realistically, to have over 2.5bn records of just the small band of narrow accounts this is pre filtered to, that is way outside normal use.

Choose a reason for hiding this comment

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

Agreed, but I found it at least noteworthy to mention the issue on the PR.
If Odoo keeps growing at their current rate, its only a matter of time before supporting large datasets out of the box becomes mandatory. For OCA modules, should we not try to take it into account as much as possible when we can clearly foresee these types of issues? Solving this uniquely for each customer puts a heavy strain on project budgets, and might push customers away from the ERP.
Thanks for your comments though, you do make a good point.

Copy link
Member

Choose a reason for hiding this comment

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

If you have any suggestion for solving the performance bottleneck without losing features, please propose it as a new PR.

@pedrobaeza
Copy link
Member

Let's go with this as there are no more comments. Other later PRs can improve anything discovered in real tests:

/ocabot merge nobump

Thanks @ozono for the hardest work.

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-359-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit e0512d0 into OCA:14.0 Jan 23, 2021
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 26ca324. Thanks a lot for contributing to OCA. ❤️

@randall-vx
Copy link

@hbto

@hbto
Copy link

hbto commented Jan 24, 2021

@randall-vx ACK.

Thanks a lot.

Regards.

@alexis-via
Copy link
Contributor

Thanks for this big work on account_reconcile_widget!
I started to use it and I'm facing a problem: in the reconcile interface, I don't see the 'name' of the bank statement line (which is the same as the 'name' of the account.move.line) ; I only see the account move number... so the accountant is missing a very important information to work on the line!

bank_reconcile_label_issue

@alexis-via
Copy link
Contributor

I'm facing another issue: as you can see in the screenshot above, I have a button "Frais bancaires" (account.reconcile.model). This button has type = 'writeoff_button' and 1 account.reconcile.model.line configured as 100% on an expense account. When I click on that button, nothing happens. Do you face the same pb ?

@alexis-via
Copy link
Contributor

Remember that the field 'name' of account.bank.statement.line became 'payment_ref' in odoo v14. This is certainly the cause of the first issue I reported.

@moaadbourhim
Copy link

moaadbourhim commented Apr 1, 2021

I'm facing another issue: as you can see in the screenshot above, I have a button "Frais bancaires" (account.reconcile.model). This button has type = 'writeoff_button' and 1 account.reconcile.model.line configured as 100% on an expense account. When I click on that button, nothing happens. Do you face the same pb ?

@alexis-via Did you find a solution for this problem ?

@alexis-via
Copy link
Contributor

@moaadbourhim No, because I haven't tried to fix it so far. But we really need to fix it now.

@pedrobaeza
Copy link
Member

#372 fixes the behavior with reconciliation models of type writeoff_button.

@moaadbourhim
Copy link

@pedrobaeza @alexis-via
I think there one more problem, when you choose for example one analytic Account from the list (Analytic Acc.) and then when you check the cost and Revenue of this Analytic account you find the duplication of the lines in "account.analytic.line".

You can see that in the images

Choose for example Asustek - Wood Corner in analytic account :

analytic_acc

Check the Cost and Revenue of this Analytic Account :

cost_revenue

The list of cost and Revenue :

res

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.

9 participants