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

10.0 Port invoice_fiscal_position_update #191

Merged
merged 5 commits into from
Apr 7, 2017

Conversation

alexis-via
Copy link
Contributor

No description provided.

@pedrobaeza
Copy link
Member

Please rename to account_invoice_fiscal...

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.

Remove POT file

@pedrobaeza pedrobaeza mentioned this pull request Oct 11, 2016
34 tasks
else:
account = (
product.property_account_expense_id or
product.categ_id.property_account_expense_categ_id)
taxes = product.supplier_taxes_id
taxes = taxes or account.tax_ids
taxes = product.supplier_taxes_id.filtered(
Copy link
Member

Choose a reason for hiding this comment

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

Do the filtered only one time for better code readibility

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I do the filtering only one time here, the "or" won't work the same way.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you can do it at the end, after the or and thus, you don't need to make 3 filtered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But if you think about it carefully, you'll see that doing it at the end will modify the behavior: if you have a tax in another company on a product, then

taxes = taxes or account.tax_ids

will copy taxes on taxes and then it will be filtered and the taxes will be removed. With the current code, it would have taken account.tax_ids.
I'm just saying that to underline the fact that moving the 3 filtered at the end won't give the same result.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but you can reduce to 2 filtered instead of 3:

if inv_type in ('out_invoice', 'out_refund'):
    account = (
       product.property_account_income_id or
       product.categ_id.property_account_income_categ_id)
   taxes = product.taxes_id
else:
    account = (
       product.property_account_expense_id or
       product.categ_id.property_account_expense_categ_id)
   taxes = product.supplier_taxes_id
taxes = taxes.filtered(lambda tax: tax.company_id == self.company_id)
taxes = taxes or account.tax_ids.filtered(
    lambda tax: tax.company_id == self.company_id)

@pedrobaeza
Copy link
Member

It would be good to start all 10.0 modules with tests.

@alexis-via
Copy link
Contributor Author

@pedrobaeza Feel free to propose a PR to add tests. I won't have time to do it myself (and, as I explained in my email on the ML, it's not the goal the of OCA v10 Taskforce).

@mourad-ehm
Copy link
Contributor

👍 code review & module tested on runbot.

@mourad-ehm
Copy link
Contributor

Hi @pedrobaeza, @alexis-via, I added tests.

@pedrobaeza
Copy link
Member

Thanks for the tests, @mourad-ehm. There's a mix in the branch with other module included in it. Since the moment I said about the tests, they were developed by my colleague on version 9. You can see them here: https://github.com/OCA/account-invoicing/blob/9.0/account_invoice_fiscal_position_update/tests/test_account_invoice.py. It's a pity to duplicate efforts. Sorry for not saying anything here, but I didn't remember my request. You can check if there's something missing in your tests that can be taken from there.

@alexis-via
Copy link
Contributor Author

The tests in the module account_invoice_fiscal_position_update were added in v9 after the creation of this PR for v10.

@pedrobaeza
Copy link
Member

@alexis-via That's what I'm saying and again sorry for not communicating here for not duplicating efforts.

@mourad-ehm
Copy link
Contributor

ping @alexis-via

@alexis-via alexis-via merged commit 3a9e7f7 into OCA:10.0 Apr 7, 2017
BT-dherreros pushed a commit to BT-dherreros/account-invoicing that referenced this pull request Apr 29, 2019
Syncing from upstream OCA/account-invoicing (12.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants