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

[17.0][MIG] hr_expense_invoice: Migration to 17.0 #256

Merged
merged 53 commits into from
Sep 10, 2024

Conversation

desdelinux
Copy link
Contributor

@desdelinux desdelinux commented Jul 15, 2024

Actions performed:
- Deprecated hr.expense.reference field as part of 1.
- Merged the hr.expense.is_editable and hr.expense.sheet_is_editable
fields as part of 1.
- Renamed hr.expense.unit_amount to hr.expense.price_unit.
- Renamed hr.expense.attachment_number to hr.expense.nb_attachment.
- Renamed hr.expense.approve_expense_sheets() to
hr.expense.action_approve_expense_sheets().
- Renamed hr.expense.account_move_id to hr.expense.account_move_ids.
- Renamed hr.expense.total_amount to
hr.expense.total_amount_currency.
- Inherit compute of hr.expense.sheet.state to consider expenses with
linked invoices.
- Switched from old %-based string formatting to f-strings.
- Replaced the use of attrs in views with their equivalent Python
expressions as part of 2.
- Adapted the hr.expense.sheet records creation in tests.
- Updated to use the employee partner from hr.employee.work_contact_id
instead of hr.employee.address_home_id since it has been split into
multiple fields.
- Added the hr.expense.amount_residual field to maintain the one
invoice per expense functionality.
- Adapted test assertions for hr.expense.sheet.state since it is now
a computed field.

pedrobaeza and others added 30 commits July 15, 2024 17:48
Set supplier invoices on HR expenses
====================================

This module should be used when a supplier invoice is paid by an employee. It
allows to set  a supplier invoice for each expense line, adding the
corresponding journal items to transfer the debt to the employee.

Installation
============

Install the module the regular way.

Configuration
=============

You don't need to configure anything more to use this module.

Usage
=====

Instead of coding a full expense line, select an existing supplier invoice,
and then the rest of the fields will be auto-filled and grayed.

When you generate the expenses account entries, lines with invoices filled
will be generated as opposite of the payable move line of the invoice, and
both will be reconciled, letting the employee payable account as the only
open balance.

Known issues / Roadmap
======================

* Multiple payment terms for a supplier invoice are not handled correctly.
* Partial reconcile supplier invoices are also not correctly handled.

OCA Transbot updated translations from Transifex
…se view

OCA Transbot updated translations from Transifex
…l amount (OCA#237)

On the same expense, when we have 2 or more lines with different invoices, and each invoices have the same total amount, reconcile is not possible.

The fix is to exclude reconcile account.move.line, and the first time if we have more than one line to reconcile on the same amount, we keep the first.

OCA Transbot updated translations from Transifex
OCA Transbot updated translations from Transifex
[UPD] Update hr_expense_invoice.pot
Currently translated at 100.0% (4 of 4 strings)

Translation: hr-11.0/hr-11.0-hr_expense_invoice
Translate-URL: https://translation.odoo-community.org/projects/hr-11-0/hr-11-0-hr_expense_invoice/de/

Update translation files

Updated by Update PO files to match POT (msgmerge) hook in Weblate.
[UPD] Update hr_expense_invoice.pot

Update translation files

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: hr-12.0/hr-12.0-hr_expense_invoice
Translate-URL: https://translation.odoo-community.org/projects/hr-12-0/hr-12-0-hr_expense_invoice/
Add expense info to invoice info or create/edit
From expense sheet, add action "Create Invoice" from multiple expenses
Change the way reference invoice_id is checked.
- No more onchange invoice_id that set values to expense
- Instead, check amount on expense and invoice during post entry
- Change the way to allow reconcile with > 2 account move lines
Currently translated at 100.0% (4 of 4 strings)

Translation: hr-12.0/hr-12.0-hr_expense_invoice
Translate-URL: https://translation.odoo-community.org/projects/hr-12-0/hr-12-0-hr_expense_invoice/de/
Currently translated at 100.0% (4 of 4 strings)

Translation: hr-12.0/hr-12.0-hr_expense_invoice
Translate-URL: https://translation.odoo-community.org/projects/hr-12-0/hr-12-0-hr_expense_invoice/es/

[UPD] README.rst

[UPD] Update hr_expense_invoice.pot

[UPD] README.rst

hr_expense_invoice 12.0.1.3.0

Update translation files

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: hr-12.0/hr-12.0-hr_expense_invoice
Translate-URL: https://translation.odoo-community.org/projects/hr-12-0/hr-12-0-hr_expense_invoice/
hr_expense_invoice 12.0.1.3.1
[UPD] Update hr_expense_invoice.pot

Update translation files

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: hr-expense-13.0/hr-expense-13.0-hr_expense_invoice
Translate-URL: https://translation.odoo-community.org/projects/hr-expense-13-0/hr-expense-13-0-hr_expense_invoice/

Update translation files

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: hr-expense-13.0/hr-expense-13.0-hr_expense_invoice
Translate-URL: https://translation.odoo-community.org/projects/hr-expense-13-0/hr-expense-13-0-hr_expense_invoice/
If not, the total amount of the expense won't match to invoices
with taxes.

[UPD] README.rst
… partners.

- Intercept properly the sheet paid action for not marking it as paid when
  reconciling the expense lines that belongs to invoices.
- Prevent set invoices paid when sheet paid by company.
- Some docstring.

Co-authored-by: Pedro M. Baeza <pedro.baeza@tecnativa.com>
Co-authored-by: Víctor Martínez <victor.martinez@tecnativa.com>

hr_expense_invoice 13.0.1.1.2

[UPD] Update hr_expense_invoice.pot

Update translation files

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: hr-expense-13.0/hr-expense-13.0-hr_expense_invoice
Translate-URL: https://translation.odoo-community.org/projects/hr-expense-13-0/hr-expense-13-0-hr_expense_invoice/
…ing to sheet

hr_expense_invoice 13.0.1.2.0

[UPD] Update hr_expense_invoice.pot

Update translation files

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: hr-expense-13.0/hr-expense-13.0-hr_expense_invoice
Translate-URL: https://translation.odoo-community.org/projects/hr-expense-13-0/hr-expense-13-0-hr_expense_invoice/
…ted with expense and improve domain to prevent set same invoice in different expenses.
…e is set)

hr_expense_invoice 13.0.1.3.0

[UPD] Update hr_expense_invoice.pot

Update translation files

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: hr-expense-13.0/hr-expense-13.0-hr_expense_invoice
Translate-URL: https://translation.odoo-community.org/projects/hr-expense-13-0/hr-expense-13-0-hr_expense_invoice/
@xmglord xmglord force-pushed the 17.0-mig-hr_expense_invoice branch 4 times, most recently from be6e661 to 1a6445b Compare August 21, 2024 02:46
@xmglord
Copy link
Contributor

xmglord commented Aug 21, 2024

@pedrobaeza @luisg123v all comments were applied, please review again.

return action

def _compute_state(self):
"""Set proper state according linked invoices."""

Choose a reason for hiding this comment

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

"according to"

Copy link
Contributor

Choose a reason for hiding this comment

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

done

@@ -27,6 +27,9 @@ class HrExpense(models.Model):
comodel_name="account.move",
inverse_name="source_invoice_expense_id",
)
amount_residual = fields.Monetary(
string="Amount Due", compute="_compute_amount_residual", store="True"

Choose a reason for hiding this comment

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

The store attribute should be a boolean

Copy link
Contributor

Choose a reason for hiding this comment

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

done

@@ -112,3 +143,30 @@ def action_view_invoices(self):
action["view_mode"] = "tree,form"
action["domain"] = [("id", "in", invoice_ids)]
return action

def _compute_state(self):

Choose a reason for hiding this comment

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

Missing api.depends, it could be an empty one.

Copy link
Contributor

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's needed if you are overriding an existing one.

sheets_with_paid_invoices = (
sheets_with_invoices - company_account_sheets
).filtered(
lambda sheet: all(

Choose a reason for hiding this comment

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

You could filter by previous state to discard invalid candidates, e.g. draft ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is already filtered in line 155. This is the original compute.

company_account_sheets = sheets_with_invoices.filtered(
lambda sheet: sheet.payment_mode == "company_account"
)
company_account_sheets.write({"state": "done"})

Choose a reason for hiding this comment

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

You should not write inside compute methods

company_account_sheets.state = "done"

Same below.

Copy link
Contributor

Choose a reason for hiding this comment

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

done

@@ -140,8 +113,8 @@
icon="fa-book"
name="action_view_invoices"
groups="account.group_account_invoice"
invisible="invoice_count == 0"

Choose a reason for hiding this comment

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

not invoice_count

Copy link
Contributor

Choose a reason for hiding this comment

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

done

@xmglord xmglord force-pushed the 17.0-mig-hr_expense_invoice branch from 1a6445b to 6c9a848 Compare August 22, 2024 16:37
@xmglord
Copy link
Contributor

xmglord commented Aug 22, 2024

@luisg123v please review

Copy link

@luisg123v luisg123v left a comment

Choose a reason for hiding this comment

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

@xmglord we're almost done, just only one pending comment.

@xmglord xmglord force-pushed the 17.0-mig-hr_expense_invoice branch 2 times, most recently from 5b2214b to e69abe3 Compare August 24, 2024 05:19
Copy link

@luisg123v luisg123v left a comment

Choose a reason for hiding this comment

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

LGTM 👍

CC @pedrobaeza

@xmglord
Copy link
Contributor

xmglord commented Aug 28, 2024

@pedrobaeza friendly reminder

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Hi, sorry for the delay. In a test in runboat, everything seems to work better.

Functionally, I only miss the journal entries smart-button to navigate to the created "transfer entries" in a expense report with expenses paid by employee linked to vendor bills. Is it possible to have it?

Technically, I have put several comments inline.

@@ -27,56 +27,31 @@ class HrExpense(models.Model):
comodel_name="account.move",
inverse_name="source_invoice_expense_id",
)
amount_residual = fields.Monetary(
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need this extra field? If there's a strong reason, put a comment above documenting the reason to add it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I already add the reason.

@@ -88,7 +63,7 @@ def _prepare_own_account_transfer_move_vals(self):
],
limit=1,
)
employee_partner = self.employee_id.sudo().address_home_id.commercial_partner_id
employee_partner = self.employee_id.sudo().work_contact_id
Copy link
Member

Choose a reason for hiding this comment

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

Although I understand the reason for this change (as the private address has been removed), this is not truly correct, as the work contact is not really representing the employee, and you can't put a journal entry linked to a children contact. How Odoo handles this in other parts where the debt is to the employee?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I took it from Odoo.

Example 1:
v17
v16

Example 2:
v17
v16

@@ -153,27 +128,26 @@ def _onchange_invoice_id(self):
"""
if self.invoice_id:
self.quantity = 1
self.reference = self.invoice_id.name
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should add the vendor bill number in the expense name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done but since both models, hr.expense and account.move have smart buttons to go from one to another and since in v17 the filtering is more robust. I think is not necessary to have it. Maybe something like:

self.name = self.name or self.invoice_id.name

What do you think?

hr_expense_invoice/models/hr_expense.py Outdated Show resolved Hide resolved
@@ -112,3 +143,30 @@ def action_view_invoices(self):
action["view_mode"] = "tree,form"
action["domain"] = [("id", "in", invoice_ids)]
return action

def _compute_state(self):
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's needed if you are overriding an existing one.

Actions performed:
- Deprecated `hr.expense.reference` field as part of [1].
- Merged the `hr.expense.is_editable` and `hr.expense.sheet_is_editable`
  fields as part of [1].
- Renamed `hr.expense.unit_amount` to `hr.expense.price_unit`.
- Renamed `hr.expense.attachment_number` to `hr.expense.nb_attachment`.
- Renamed `hr.expense.approve_expense_sheets()` to
  `hr.expense.action_approve_expense_sheets()`.
- Renamed `hr.expense.account_move_id` to `hr.expense.account_move_ids`.
- Renamed `hr.expense.total_amount` to
  `hr.expense.total_amount_currency`.
- Inherit compute of `hr.expense.sheet.state` to consider expenses with
  linked invoices.
- Switched from old %-based string formatting to f-strings.
- Replaced the use of `attrs` in views with their equivalent Python
  expressions as part of [2].
- Adapted the `hr.expense.sheet` records creation in tests.
- Updated to use the employee partner from `hr.employee.work_contact_id`
  instead of `hr.employee.address_home_id` since it has been split into
  multiple fields.
- Added the `hr.expense.amount_residual` field to maintain the one
  invoice per expense functionality.
- Adapted test assertions for `hr.expense.sheet.state` since it is now
  a computed field.

Co-authored-by: desdelinux <luigys@vauxoo.com>

[1]: odoo/odoo#130244
[2]: odoo/odoo#104741
@xmglord
Copy link
Contributor

xmglord commented Sep 4, 2024

Hi @pedrobaeza

In v16 was not possible to have the smart button since the field related to it was a one2many field but in this version is possible and I already add it. Could you please test it functionally again?

@xmglord
Copy link
Contributor

xmglord commented Sep 9, 2024

@pedrobaeza friendly reminder

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

OK, let's move on. The contact associated to the move doesn't convince me, as it's not correct, but is the same on Vanilla Odoo, so let's not block it for now.

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 17.0-ocabot-merge-pr-256-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit c18487e into OCA:17.0 Sep 10, 2024
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

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.