[REF] account: create 'account.reconciliation.widget' model #22274
Conversation
Gorash
force-pushed
the
master-ref-widget-reconciation-chm
branch
4 times, most recently
from
7331c88
to
6eb63aa
Compare
There was a problem hiding this comment.
A few details ;)
Furthermore (and this is much more important), your refactoring will impact account_batch_deposit module (which adds the support for batch deposits to the reconciliation wizard). Could you create an enterprise branch and modify it in accordance with the new version of the reconciliation wizard, please ? :) (For your information, there is some change coming for this module, currently awaiting review ; if it is merged before you're done, account_batch_deposit won't exist anymore, and will be renamed account_batch_payment)
| def auto_reconcile(self, st_line_ids, num_already_reconciled_lines=0): | ||
| """ :param st_line_ids | ||
| :param num_already_reconciled_lines | ||
| """ |
There was a problem hiding this comment.
There is no use in listing the parameters like this if you don't explain what they contain ;)
| :param str: | ||
| :param offset: | ||
| :param limit: | ||
| """ |
There was a problem hiding this comment.
Same as before, listing the parameters without explanation is pointless.
| """ Get statement lines of the specified statements or all unreconciled statement lines and try to automatically reconcile them / find them a partner. | ||
| Return ids of statement lines left to reconcile and other data for the reconciliation widget. | ||
| """ | ||
| bank_statement_ids = self.env['account.bank.statement'].browse(bank_statement_ids) |
There was a problem hiding this comment.
For consistency with the rest of your code, I'd rather go for:
bank_statements = self.env['account.bank.statement'].browse(bank_statement_ids)
(and then use bank_statements instead of ban_statement_ids in what follows, of course)
This way, we know that a variable called ..._ids in a list of ids, while ... is directly a recordset.
|
|
||
| Account_move_line = self.env['account.move.line'] | ||
| Account = self.env['account.account'] | ||
| Currency = self.env['res.currency'] |
There was a problem hiding this comment.
Storing these three models in separate variables is pointless here, as we only use them once in the lines that follow.
There was a problem hiding this comment.
In many of the odoo methods, we declare the model variables at the top of the method in order to quickly know what are the links to the other models. I use the same guide lines.
| #################################################### | ||
|
|
||
| @api.model | ||
| # _get_move_lines_for_reconciliation |
There was a problem hiding this comment.
little typo: it's get_move_lines_for_reconciliation (without underscore) ;)
There was a problem hiding this comment.
No. _ prefix is use for private methods and do not have access from rpc or other. It's private methods only called by this model
There was a problem hiding this comment.
oups, exact, the comment is wrong, I look the refactoring, sorry
| return data | ||
|
|
||
| @api.model | ||
| # _get_reconciliation_proposition |
There was a problem hiding this comment.
small typo : get_reconciliation_proposition (no underscore)
| return Account_move_line | ||
|
|
||
| @api.model | ||
| # _get_reconciliation_proposition |
There was a problem hiding this comment.
get_reconciliation_proposition (no underscore)
|
|
||
| Account_move_line = self.env['account.move.line'] | ||
|
|
||
| # todo: remove crappy, self is an empty record set |
There was a problem hiding this comment.
Not sure I understand what you mean in this TODO; could you make it a little more explicit ? :)
There was a problem hiding this comment.
old copy past, but I refactored this code and I forget to remove the comment
| raise UserError(_('A reconciliation must involve at least 2 move lines.')) | ||
|
|
||
| account_move_line = self.env['account.move.line'].browse(move_line_ids) | ||
| writeoff_lines = self.env['account.move.line'] |
There was a problem hiding this comment.
Why moving this outside the condition ?
addons/account/models/__init__.py
Outdated
| @@ -13,3 +13,4 @@ | |||
| from . import company | |||
| from . import res_config_settings | |||
| from . import account_cash_rounding | |||
| from . import widget_reconciliation | |||
There was a problem hiding this comment.
Naming it reconciliation_widget would sound more like actual English ;p
|
I forgot to create the pr in enterprise, sorry |
|
@oco-odoo ok ? (I'll merge the commit to adapt the pr in the previous commit after) |
|
@Gorash Yup! For me, it's alright :) (also the enteprise branch) Laura will test it, and then we can merge :) |
Gorash
force-pushed
the
master-ref-widget-reconciation-chm
branch
3 times, most recently
from
c2d76ab
to
25b5ed1
Compare
|
|
||
| @api.model | ||
| # get_move_lines_for_reconciliation | ||
| def _get_move_lines(self, aml_accounts, partner_id, excluded_ids=None, search_str=False, offset=0, limit=None): |
There was a problem hiding this comment.
Can you motivate the removal of the parameters overlook_partner and additional_domain?
There was a problem hiding this comment.
I made a modification, to have a method returning the domain. additional_domain and overlook_partner are never used in master or enterprise. If a community module needs to use it, just make an inheritance.
There was a problem hiding this comment.
overlook_partner is also use by _get_common_sql_query, I did not change this method, but it was never use in the get_move_lines_for_reconciliation method calls.
| <field name="amount">750.0</field> | ||
| <field name="date" eval="time.strftime('%Y')+'-01-01'"/> | ||
| </record> | ||
|
|
There was a problem hiding this comment.
do you use this demo data somewhere?
There was a problem hiding this comment.
Yes, in the new tests.
I have add a tour to test different type of reconciliation. There will be more tests coming (according to information provided by Laura)
|
Taking advantage of this refactoring, maybe you could also consider adding the companies like in #23070. It could be interesting and useful for people who work in more than one company. 😉 |
|
@mreficent We will see this, but in a second time, so that people have already benefited from refactoring |
Gorash
force-pushed
the
master-ref-widget-reconciation-chm
branch
6 times, most recently
from
6bc8772
to
e391395
Compare
|
[IMP] account_batch_deposit: remove comments useful for forward-port wrong module, Christophe ... can you rework the history when you're done with this branch? thanks |
|
@qdp-odoo of course, I copied the message from the enterprise branch without paying attention ^^ |
Contains widget reconciliation methods. This refactoring does not modify the feature, some errors have been notified in the code but not modified.
… sql Simplification of the search method of the lines lines (for the propositions) of the two requests in one. The sql query returns the two matching lines without filtering after the request because we inject the rules of access rules directly into it. This is an optimization in terms of clarity and speed. In addition to the fix because the previous browse does not use access rules.
Gorash
force-pushed
the
master-ref-widget-reconciation-chm
branch
from
e391395
to
c7b4d5c
Compare
|
cool. let me know when you're done on this task so that I can close the PR |
Contains widget reconciliation methods. This refactoring does not modify
the feature, some errors have been notified in the code but not modified.
Description of the issue/feature this PR addresses:
Current behavior before PR:
Desired behavior after PR is merged:
--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr