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

Feature/account fiscal position rule migration 8.0 #10

Conversation

renatonlima
Copy link
Member

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 949533c on akretion:feature/account_fiscal_position_rule_migration_8.0 into 39702a5 on OCA:8.0.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling f4784fd on akretion:feature/account_fiscal_position_rule_migration_8.0 into 39702a5 on OCA:8.0.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling ca9afc8 on akretion:feature/account_fiscal_position_rule_migration_8.0 into 39702a5 on OCA:8.0.

else:
partner_addr = partner.address_get(['invoice'])
addr_id = partner_addr['invoice'] and \
partner_addr['invoice'] or None
Copy link
Member

Choose a reason for hiding this comment

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

Please do not use ... and ... or ... but ... if ... else ..., and do not use \ but parenthesis.

@guewen
Copy link
Member

guewen commented Sep 25, 2014

This module deserves tests.

@guewen
Copy link
Member

guewen commented Sep 25, 2014

Thanks for the work on the migration

ctx = dict(self._context)
ctx.update({'use_domain': ('use_invoice', '=', True)})
return self.env['account.fiscal.position.rule'].with_context(
ctx).apply_fiscal_mapping(result, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to update the context yourself. with_context() does that. You can write (with a better care of the length of the line, probably by extracting a part in a variable):

return self.env['account.fiscal.position.rule'].with_context(use_domain=('use_invoice', '=', True)).apply_fiscal_mapping(result, **kwargs)

@rvalyi
Copy link
Member

rvalyi commented Oct 9, 2014

@guewen yes we will put some tests before merging, we just didn't have time so far.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 234613e on akretion:feature/account_fiscal_position_rule_migration_8.0 into 39702a5 on OCA:8.0.

@max3903
Copy link
Sponsor Member

max3903 commented Oct 12, 2014

👍

1 similar comment
@jgrandguillaume
Copy link
Member

👍

jgrandguillaume added a commit that referenced this pull request Dec 1, 2014
…rule_migration_8.0

Feature/account fiscal position rule migration 8.0
@jgrandguillaume jgrandguillaume merged commit 5cb701b into OCA:8.0 Dec 1, 2014
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

6 participants