Merge lp:~agilebg/account-consolidation/7.0-fix-1334645-elbati into lp:~account-core-editors/account-consolidation/7.0

Proposed by Lorenzo Battistini on 2014-07-02
Status: Needs review
Proposed branch: lp:~agilebg/account-consolidation/7.0-fix-1334645-elbati
Merge into: lp:~account-core-editors/account-consolidation/7.0
Diff against target: 31 lines (+8/-6)
1 file modified
account_parallel_currency/account.py (+8/-6)
To merge this branch: bzr merge lp:~agilebg/account-consolidation/7.0-fix-1334645-elbati
Reviewer Review Type Date Requested Status
Leonardo Pistone 2014-07-02 Approve on 2014-07-07
Review via email: mp+225266@code.launchpad.net
To post a comment you must log in.
Leonardo Pistone (lepistone) wrote :

thanks for the fix Lorenzo.

Can you confirm that the test is correct and production code was actually broken?

review: Needs Information
Lorenzo Battistini (elbati) wrote :

I confirm.
Note that the bug only occurs when you create a new tax code without parent.

Leonardo Pistone (lepistone) wrote :

Hi Lorenzo,

I am still non convinced :)

This way the test is not actually testing a useful workflow, but just a "return True" workaround.

I'd be happier if the yaml test actually tested a realistic case. (or at least, clarify in the yaml file what's going on)

Thanks!

review: Needs Fixing
24. By Lorenzo Battistini on 2014-07-07

[IMP] when parent_id is missing, just don't fill the parent_id field of the parallel account

Lorenzo Battistini (elbati) wrote :

lep, right. I looked at it better and saw that if parent_id is missing, I should just not fill the parent_id field of the parallel account.

Leonardo Pistone (lepistone) wrote :

Thanks Lorenzo.

maybe you could refactor a bit putting

parent_parallel_tax_code_id = False

in an else: block a few lines down? otherwise, lgtm

review: Needs Fixing
25. By Lorenzo Battistini on 2014-07-07

[IMP] else block

Lorenzo Battistini (elbati) wrote :

On 07/07/2014 03:00 PM, Leonardo Pistone - camptocamp wrote:
> maybe you could refactor a bit putting
>
> parent_parallel_tax_code_id = False
>
> in an else: block a few lines down? otherwise, lgtm

Done

Leonardo Pistone (lepistone) wrote :

thanks

review: Approve

Unmerged revisions

25. By Lorenzo Battistini on 2014-07-07

[IMP] else block

24. By Lorenzo Battistini on 2014-07-07

[IMP] when parent_id is missing, just don't fill the parent_id field of the parallel account

23. By Lorenzo Battistini on 2014-07-02

[FIX] account_parallel_currency: test failure test/mapping_parallel_accounts.yml

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'account_parallel_currency/account.py'
2--- account_parallel_currency/account.py 2014-03-24 18:16:13 +0000
3+++ account_parallel_currency/account.py 2014-07-07 13:07:55 +0000
4@@ -454,9 +454,6 @@
5 def create_parallel_tax_codes(self, cr, uid, ids, context=None):
6 for tax_code in self.browse(cr, SUPERUSER_ID, ids, context):
7 for parallel_company in tax_code.company_id.parallel_company_ids:
8- if not tax_code.parent_id:
9- raise orm.except_orm(_('Error'),_('Tax code %s does not have parent')
10- % tax_code.code)
11 existing_ids = self.search(cr, SUPERUSER_ID, [
12 ('code', '=', tax_code.code),
13 ('company_id', '=', parallel_company.id),
14@@ -465,9 +462,14 @@
15 raise orm.except_orm(_('Error'),
16 _('Tax code %s already exists for company %s')
17 % (tax_code.code, parallel_company.name))
18- parent_parallel_tax_code_id = self._search_parallel_tax_code(
19- cr, SUPERUSER_ID, tax_code.parent_id.code, parallel_company,
20- context=context)
21+ if tax_code.parent_id:
22+ parent_parallel_tax_code_id = (
23+ self._search_parallel_tax_code(
24+ cr, SUPERUSER_ID, tax_code.parent_id.code,
25+ parallel_company, context=context)
26+ )
27+ else:
28+ parent_parallel_tax_code_id = False
29 new_id = self.create(cr, SUPERUSER_ID,{
30 'company_id': parallel_company.id,
31 'parent_id': parent_parallel_tax_code_id,

Subscribers

People subscribed via source and target branches