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

[12.0][MIG] hr_expense_invoice #584

Merged
merged 14 commits into from
May 10, 2019
Merged

[12.0][MIG] hr_expense_invoice #584

merged 14 commits into from
May 10, 2019

Conversation

kittiu
Copy link
Member

@kittiu kittiu commented Apr 23, 2019

Done Migration

pedrobaeza and others added 13 commits April 23, 2019 13:37
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.
…l amount (#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.
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/
Updated by Update PO files to match POT (msgmerge) hook in Weblate.
@pedrobaeza pedrobaeza added this to the 12.0 milestone Apr 23, 2019
@OCA-git-bot OCA-git-bot mentioned this pull request Apr 23, 2019
32 tasks
@pedrobaeza
Copy link
Member

Please check Travis

@kittiu
Copy link
Member Author

kittiu commented Apr 23, 2019

Please check Travis

Still WIP.

@kittiu
Copy link
Member Author

kittiu commented Apr 23, 2019

@pedrobaeza during learning it to migration, I also have some opinion here,
#585

@kittiu
Copy link
Member Author

kittiu commented Apr 24, 2019

Done. Only part of the code that can not happen in test.
image

@kittiu
Copy link
Member Author

kittiu commented Apr 24, 2019

Done. Only part of the code that can not happen in test.
image

Q: when we face the part of code that can't write test to cover like this, normally, how do we go about?

@pedrobaeza
Copy link
Member

You have to write use case that triggers that part of code, or let it uncovered with the codecov warning.

@kittiu
Copy link
Member Author

kittiu commented Apr 24, 2019

You have to write use case that triggers that part of code, or let it uncovered with the codecov warning.

Yah :)

@tbaden
Copy link
Member

tbaden commented Apr 26, 2019

@kittiu ready for review or still wip?

@kittiu
Copy link
Member Author

kittiu commented Apr 26, 2019

Yes ready. Test is green now.

Copy link
Member

@tbaden tbaden left a comment

Choose a reason for hiding this comment

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

code review: lgtm
just a minor detail, but it's optional

<?xml version="1.0" encoding="utf-8"?>
<odoo>

<record id="hr_expense_view_view" model="ir.ui.view">
Copy link
Member

Choose a reason for hiding this comment

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

"ugly" id (view_view)
please use hr_expense_view_form (optional)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thank you.


invoice_id = fields.Many2one(
comodel_name="account.invoice",
string='Invoice',
Copy link
Contributor

Choose a reason for hiding this comment

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

The string param is not necessary on v12 (in this case)

Copy link
Member Author

@kittiu kittiu May 4, 2019

Choose a reason for hiding this comment

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

Sorry, but why? invoice_id is a new field for hr.expense also, why no need string='Invoice'

Copy link
Member Author

Choose a reason for hiding this comment

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

By the way, I also see oldname='invoice'. Do we still need it for migration to v12?
What is it actually used for?

Copy link
Contributor

Choose a reason for hiding this comment

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

because the string is computed using the field name, so there is any need to explicitly set the string param

Copy link
Member Author

Choose a reason for hiding this comment

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

@hugho-ad, you mean "invoice_id" will be display as string "Invoice" anyway, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes @kittiu

Copy link
Member Author

Choose a reason for hiding this comment

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

Note. But in this case, may I also change it to "Vendor Bill", and it is domain to in_invoice + open status. I think Vendor Bill is also more meaningful. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes @kittiu

When the label is different, the string parameter is necessary.

def _get_account_move_line_values(self):
move_line_values_by_expense = super()._get_account_move_line_values()
for expense_id, move_lines in move_line_values_by_expense.items():
expense = self.browse(expense_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expense = self.browse(expense_id)
expense = self.browse(expense_id)
if not expense. invoice_id:
return move_line_values_by_expense

To avoid the next iteration, that is not necessary when the expense does not have an invoice.

With this remove the check expense.invoice_id in the next if

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@api.multi
def action_sheet_move_create(self):
precision = self.env['decimal.precision'].precision_get('Account')
expense_line_ids = self.mapped('expense_line_ids').filtered(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expense_line_ids = self.mapped('expense_line_ids').filtered(
expense_line_ids = self.mapped('expense_line_ids').filtered('invoice_id')

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@kittiu
Copy link
Member Author

kittiu commented May 4, 2019

We also still have same problem as in v11 according to this issue
#573

Copy link
Contributor

@luistorresm luistorresm left a comment

Choose a reason for hiding this comment

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

Technical Review

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@pedrobaeza pedrobaeza merged commit 8729ad2 into OCA:12.0 May 10, 2019
@kittiu
Copy link
Member Author

kittiu commented May 10, 2019

@pedrobaeza we will have this issue pending - #573
But it shouldn't affect this PR directly.

Thank you.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

When I try to pay the expense to the employee appears the message:
1

@kittiu
Copy link
Member Author

kittiu commented Sep 3, 2019

I think this is related to account type or reconcile flag used in account of advance.

You can try different one?

@ghost
Copy link

ghost commented Sep 10, 2019

@kittiu I don't understand what you want to say. Can you explain?

@ghost
Copy link

ghost commented Nov 18, 2019

@kittiu I didn't understand what you wanted to say. Can you explain it better again, please?

@kittiu
Copy link
Member Author

kittiu commented Nov 19, 2019

@gelo-landoo
We do not have this problem. I think it is about accounting code config.
Or would you send me URL and step to reproduce the problem?

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.

None yet