[IMP] *hr_expense*: account.move creation overhaul #166396
Conversation
|
|
JulienAlardot
force-pushed
the
master-add-move-in-draft-expense-jual
branch
from
510b48c to
9ebbf65
Compare
JulienAlardot
force-pushed
the
master-add-move-in-draft-expense-jual
branch
2 times, most recently
from
87a22c7 to
a096b34
Compare
|
@JulienAlardot runbot is red, can you rebuild/investiguate please? :) thanks |
It's red because it's far from ready 😭 |
| def button_cancel(self): | ||
| # EXTENDS account | ||
| res = super().button_cancel() | ||
| own_expense_moves = self.filtered(lambda move: move.expense_sheet_id.payment_mode == 'own_account') |
There was a problem hiding this comment.
why only for payment_mode == own_account? 🤔
There was a problem hiding this comment.
added a new comment explaining why (company expenses can be canceled through their move/payment being canceled, not employee ones)
| @@ -238,7 +238,7 @@ def _compute_amount(self): | |||
| def _compute_from_account_move_ids(self): | |||
| for sheet in self: | |||
| if sheet.payment_mode == 'company_account': | |||
| if sheet.account_move_ids: | |||
| if sheet.account_move_ids.filtered(lambda move: move.state != 'draft'): | |||
There was a problem hiding this comment.
what about canceled moves? I think you need to filter them out as well, or the computation will be wrong
| if sheet.account_move_ids.filtered(lambda move: move.state != 'draft'): | |
| if sheet.account_move_ids.filtered(lambda move: move.state == 'posted'): |
There was a problem hiding this comment.
When the expense is paid by the company we want to consider the canceled one as "paid"
Not in the employee case
| else: | ||
| sheet.state = 'post' | ||
| else: | ||
| sheet.state = sheet.approval_state # Submit & approved without a move case |
There was a problem hiding this comment.
I wish we had only 1 state field... why can't we?
There was a problem hiding this comment.
the expense sheet state is a combination of 3 different logic (the expenses themselves, the approval process, the payment process). And those should synchronize themselves, taking some manager input (approval) in the middle.
I feel it would be too messy to move from what we have right now (computed state with a second field approval_state to properly take the input from the manager.
Even though, now with these changes the line between the two last processes is a bit blurry
JulienAlardot
force-pushed
the
master-add-move-in-draft-expense-jual
branch
5 times, most recently
from
62da5b6 to
946dff0
Compare
JulienAlardot
force-pushed
the
master-add-move-in-draft-expense-jual
branch
4 times, most recently
from
607978b to
3f19b3a
Compare
| # And cancelling the move != cancelling the expense | ||
| res = super().button_cancel() | ||
| own_expense_moves = self.filtered(lambda move: move.expense_sheet_id.payment_mode == 'own_account') | ||
| own_expense_moves.write({'expense_sheet_id': False, 'ref': False}) |
There was a problem hiding this comment.
if an account.move gets cancelled, reset to draft, re-posted... you'll lose the link: the expense gonna be paid twice
There was a problem hiding this comment.
In all the cases, if you already have a payment done and you reset/revert/delete/cancel your move, you have the warning about outstanding amounts to make sure you're not paying twice. As for any normal vendor bill
In the case of the cancel without any payment though, as it's not the same kind of cancel than an "expense cancel". There is no way to pay your employee again without unlinking the two models (else the synchro goes awry)
The previous behavior wasn't doing that, but was also blocking the expense in a dead zone. Never allowing it to be paid for something that could just be an accounting error
| return self.env.ref('hr_expense.mt_expense_paid') | ||
| return super()._track_subtype(init_values) | ||
| if 'state' not in init_values: | ||
| return super()._track_subtype(init_values) |
There was a problem hiding this comment.
not related to the task, but I think having custom subtypes are far from being great from a performance point of view. 🤔 Maybe to be removed in a next refactoring, unless it brings a real added value, if we can see it makes a significant amount of additional queries
This moves the creation of account.move at the approval step of an expense report. Allowing better accounting tax management Before this change: The moves & payments were created & posted when posting the expense report After this change: Approving the report creates draft account moves & payments Posting the report post the accounting entries task-id: 3943064
JulienAlardot
force-pushed
the
master-add-move-in-draft-expense-jual
branch
from
3f19b3a to
17e7e85
Compare
|
@robodoo r+ |
This moves the creation of account.move at the approval step of an expense report. Allowing better accounting tax management Before this change: The moves & payments were created & posted when posting the expense report After this change: Approving the report creates draft account moves & payments Posting the report post the accounting entries closes #166396 Task-id: 3943064 Related: odoo/enterprise#63010 Signed-off-by: Quentin De Paoli <[email protected]>
This moves the creation of account.move at the approval step of an expense report. Allowing better accounting tax management
Before this change:
The moves & payments were created & posted
when posting the expense report
After this change:
Approving the report creates draft account moves & payments Posting the report post the accounting entries
task-id: 3943064
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