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
[10.0] Restore account hierarchy and views #505
Conversation
Hey @minhhq09, thank you for your Pull Request. It looks like some users haven't signed our Contributor License Agreement, yet.
Appreciation of efforts, |
account_parent/README.rst
Outdated
Configuration | ||
============= | ||
|
||
Need to set the group show chart of account structure to view the chart of account heirarchy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heirarchy should be hierarchy
<field name="implied_ids" eval="[(4, ref('account.group_account_manager'))]"/> | ||
<field name="users" eval="[(4, ref('base.user_root'))]"/> | ||
</record> | ||
</odoo> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should change the logo of the module.
Maybe this one: https://upload.wikimedia.org/wikipedia/commons/9/9c/Categorisation-hierarchy-top2down.svg
Hey @minhhq09,
Appreciation of efforts, |
account_parent/models/account.py
Outdated
|
||
@api.model | ||
def _move_domain_get(self): | ||
context = dict(self._context or {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function should return 2 values:
- domain
- params
Then you can build safely the SQL query avoiding SQL injection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comment :)
I did rewrite the query to avoiding SQL injection in compute function.
The function compute for each account. So we just need get domain in here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should separate code and parameters.
Resources:
account_parent/models/account.py
Outdated
{'show_parent_account': True}).search([ | ||
('id', 'child_of', [account.id])]) | ||
params = [tuple(sub_accounts.ids)] | ||
self.env.cr.execute(query, params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that we could execute the query only once and outside the for loop using a group by l.account_id
in the query. This should help with performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like i say above. Compute value just for each account. In Vietnamese, the parent account can sum debit/credit value of childrent account. Example: I want report debit, credit, balance off account: 111 - Cash. The function will return sum debit, credit of childrent account is: 1111, 1112, 1113
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you open the Chart of Account Hierarchy, you display several accounts and for each balance, sum debit and sum credit at the same time.
You should execute the SQL query only once for all accounts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's funtion fields will be automated compute value. I think it don't the same when you click on button and compute value of all account.
The wizard no call to that function. It just show hierarchy view with already exist data. When you find any account you also can see debit, credit, balance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am 99% sure that if you add self.ensure_one()
as the first line of def _compute_values
you won't be able to open the hierarchical view of accounts.
I think that the function _compute_values
will be called with self that will be a recordset of multiple accounts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, i will try fix it one more time.
Thank JC,
'company_id': self.company_id | ||
}) | ||
|
||
# Test that the open chart cannot open window |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From:
# Test that the open chart cannot open window
To
# Test that the function account_chart_open_window can be executed
# without arguments/special context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay :)
account_parent/README.rst
Outdated
============== | ||
|
||
This module will be very useful for those who are still using v7/v8 | ||
because of the no parent account and chart of account heirarchy view in the latest versions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 iterations of heirarchy
instead of hierarchy
in the code of this PR
account_parent/models/account.py
Outdated
|
||
@api.model | ||
def _move_domain_get(self): | ||
context = dict(self._context or {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should separate code and parameters.
Resources:
parent_id = fields.Many2one( | ||
'account.account', 'Parent Account', ondelete='restrict') | ||
child_ids = fields.One2many( | ||
'account.account', 'parent_id', 'Child Accounts') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi everyone,
Why don't we just implement the Hierarchy implemented in Odoo/master
that just landed a few days ago at commit odoo/odoo@5967600
This new approach, I think could fit to everyone allowing to provide Hierarchy,
Using the current way can lead to huge overheads to compute the recursive
values of the parent-children accounts, I am aware that we need the Hierarchy
but this new approach leaves the computation of the parent-child values to
reporting and the parent-children relationship is separated into two, that hierarchy
is only left to parents which bear no moves (account.group) and the account.account
is the ultimate children that bears only moves.
As @fpodoo explained at odoo/odoo#17367 (comment)
This is more elegant and less power/resource consuming.
This computed fields will lead to the overhead that once crippled the previous odoo versions.
Best Regards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
@api.model | ||
def search(self, args, offset=0, limit=None, order=None, count=False): | ||
context = self._context or {} | ||
if not context.get('show_parent_account', False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's make better if we check show_parent_account
is True. May be, we will change many code.
Because another line in all module don't has set this context before. If we check fail, all line will go into condition
if not context.get('show_parent_account', False):
> if context.get('show_parent_account'):
So with version 11 it seems that:
I wonder what is the best way to have a hierarchy of accounts with balance in version 11 ... :/ |
@pedrobaeza @rlizana Can you put a link from that work, i'm interested too. |
Guys, is there something done already for this topic. We are also searching for an addon to get this backed hierarchy back into odoo. |
@pedrobaeza @rlizana @congdpt any news would be welcome :) |
Restore account hierarchy and views