-
-
Notifications
You must be signed in to change notification settings - Fork 681
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
11.0 mig account invoice fiscal position update #381
11.0 mig account invoice fiscal position update #381
Conversation
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). | ||
|
||
|
||
{ | ||
'name': 'Invoice Fiscal Position Update', | ||
'version': '10.0.1.0.1', | ||
'version': '11.0.1.0.1', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
11.0.1.0.0
@@ -38,10 +41,13 @@ Credits | |||
Contributors | |||
------------ | |||
|
|||
* Roel Adriaans <roel@road-support.nl> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put you in last position, as this history is supposed to be from beginning to recent contributions
@pedrobaeza Changes Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please squash your 3 migration commits in one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some comments in order to make it usable in multicompany environments.
product = line.product_id | ||
if inv_type in ('out_invoice', 'out_refund'): | ||
account = ( | ||
product.property_account_income_id or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should ensure the company of the property for multicompany environments
with_context(force_company=self.company_id.id)
lambda tax: tax.company_id == self.company_id) | ||
else: | ||
account = ( | ||
product.property_account_expense_id or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same
@RoelAdriaans-old are you planning to consider the latest changes? Or should we better make a pull request that supersedes this one? |
@jbeficent Sorry forgot about it.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
I noticed test_inv_fiscal_pos_update is failing in my CI. I think, it is because I run test after install l10n location.
|
Tests should search for an existing account first instead of creating it, or use it a weird code that is not going to choke (example: |
|
||
def test_fiscal_position_id_change(self): | ||
partner = self.res_partner_model.create(dict(name="George")) | ||
account_export_id = self.account_model.sudo().create({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
account_export_id = self.account_model.sudo().search(
[('code', '=', '710000')], limit=1
)
if not account_export_id:
account_export_id = self.account_model.sudo().create({
'code': "710000",
'name': "customer export account",
'user_type_id': self.account_user_type.id,
'reconcile': True,
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I'll try to make the changes this weekend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional test done. LGTM
@RoelAdriaans can you please finish this? |
…on, tax mapping could be wrong set
I have fixed the test creating an unique account code, so I merge. |
Migrate to 11.0
CLA Bot will probably give a warning, because my github acocunt is not known, mailed cla@ about this.
@RoelAdriaans-B-informed is me with signed cla, but company account.