Merge lp:~camptocamp/banking-addons/bank-statement-reconcile-7.0-fix-related-balance-check into lp:banking-addons/bank-statement-reconcile-70

[Leonardo Pistone]

[lp:~camptocamp/banking-addons/bank-statement-reconcile-7.0-fix-related-balance-check]

134. By Leonardo Pistone on 2014-03-06

[fix] account_statement_ext: trigger for a related field

135. By Leonardo Pistone on 2014-03-06

[fix] other related fields found in the statement

[Leonardo Pistone (lepistone)]

I added here a very similar fix (with tests) for the other related fields in the same object.

[Pedro Manuel Baeza (pedro.baeza)]

Hi, Leonardo, thanks for the patch.

Instead of defining a function _us, you can use lambda for resolving that simple expression:

lambda self, cr, uid, ids, context=None: ids

The other store function is also bad constructed, because it's triggered on 'account.bank.statement', but you have to query 'account.statement.profile' object. This is the correct code (also putting arguments in their usual form):

def _get_statement_from_profile(self, cr, uid, ids, context=None):
    triggered = []
    profile_obj = self.pool['account.statement.profile']
    for profile in profile_obj.browse(cr, uid, ids, context=context):
        triggered += [st.id for st in profile.bank_statement_ids]
    return triggered

Regards.

review: Needs Fixing (code review)

[Leonardo Pistone (lepistone)]

Thanks for reviewing Pedro.

1. I know we could use lambdas. They both look correct to me, and readability also doesn't change much, IMHO.
2. That is a known weirdness of OpenERP (I put a comment for that reason). If you put a pdb in, you will see that self is the profile, non the statement, even though we are in the statement class.

But none of these points if worth fighting for: let's see if some other reviewer comes up.

[Pedro Manuel Baeza (pedro.baeza)]

Hi, Leonardo,

Curious, I don't know about that behaviour. Don't you think then that it's better to rename self to profile_obj or something similar to avoid bad interpretations?

I also see cleaner the lamba expression, but this is a matter of taste, so I'm not going to block the MP for this reason.

Regards.

[lp:~camptocamp/banking-addons/bank-statement-reconcile-7.0-fix-related-balance-check]

136. By Leonardo Pistone on 2014-03-07

[imp] docstring for a function field trigger

[Leonardo Pistone (lepistone)]

Good point, it's not clear which is the path of least surprise here. The 'self' name is a convention that we could change to profile_obj, but that is weird as well to my eyes.

So I just expanded the docstring to be a bit more informative.

thanks!

[Pedro Manuel Baeza (pedro.baeza)]

You have my approval then.

Thanks for your changes.

Regards

review: Approve (code review)

[Stéphane Bidoul (Acsone) (sbi)]

Hi Leonardo,

A clean we to avoid the weirdness is to put the _get_statement(_from_profile) in the AccountStatementProfile class and reference it like this:

store={
               'account.bank.statement': (_us, ['profile_id'], 10),
               'account.statement.profile': (
                  AccountStatementProfile._get_statement, ['partner_id'], 10),
},

That worked for me in other situations.

[Leonardo Pistone (lepistone)]

Stéphane,

I understand your point, but that also has the weird point that the method that you need to write some code far away in class A to make a function field in class B.

My opinion is that there are a few ways to do that, but if we name variables and methods properly, and maybe add a docstring, IMO it's good enough.

Thanks!

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

I prefer to use self.pool['account.statement.profile'] in trigger methods even if self is already the instance of 'account.statement.profile' just for clarity and explicitness. But this is correct too, so LGTM.
Thanks

review: Approve (code review)

[Yannick Vaucher @ Camptocamp (yvaucher-c2c)]

As Stephane's comment is a just a comment and not a need fixing I'll proceed with merging this one.

=== modified file 'account_statement_ext/__openerp__.py'
--- account_statement_ext/__openerp__.py	2014-03-06 10:48:37 +0000
+++ account_statement_ext/__openerp__.py	2014-03-07 10:20:51 +0000
@@ -79,7 +79,7 @@
           'security/ir.model.access.csv',
           'security/ir_rule.xml'],
  'demo_xml': [],
- 'test': [],
+ 'test': ['test/test_profile_related_fields.yml'],
  'installable': True,
  'images': [],
  'auto_install': False,

=== modified file 'account_statement_ext/statement.py'
--- account_statement_ext/statement.py	2014-03-06 10:48:37 +0000
+++ account_statement_ext/statement.py	2014-03-07 10:20:51 +0000
@@ -159,6 +159,23 @@
 
         return profile_ids[0] if profile_ids else False
 
+    def _get_statement_from_profile(self, cr, uid, profile_ids, context=None):
+        """Stored function field trigger.
+
+        Weirdness warning: we are in the class account.bank.statement, but
+        when the ORM calls this, self is an account.statement.profile.
+
+        Returns a list of account.bank.statement ids to recompute.
+
+        """
+        triggered = []
+        for profile in self.browse(cr, uid, profile_ids, context=context):
+            triggered += [st.id for st in profile.bank_statement_ids]
+        return triggered
+
+    def _us(self, cr, uid, ids, context=None):
+        return ids
+
     _columns = {
         'profile_id': fields.many2one(
             'account.statement.profile',
@@ -166,28 +183,41 @@
             required=True,
             states={'draft': [('readonly', False)]}),
         'credit_partner_id': fields.related(
-                        'profile_id',
-                        'partner_id',
-                        type='many2one',
-                        relation='res.partner',
-                        string='Financial Partner',
-                        store=True,
-                        readonly=True),
+            'profile_id',
+            'partner_id',
+            type='many2one',
+            relation='res.partner',
+            string='Financial Partner',
+            store={
+                'account.bank.statement': (_us, ['profile_id'], 10),
+                'account.statement.profile': (
+                    _get_statement_from_profile, ['partner_id'], 10),
+            },
+            readonly=True),
         'balance_check': fields.related(
-                        'profile_id',
-                        'balance_check',
-                        type='boolean',
-                        string='Balance check',
-                        store=True,
-                        readonly=True),
+            'profile_id',
+            'balance_check',
+            type='boolean',
+            string='Balance check',
+            store={
+                'account.bank.statement': (_us, ['profile_id'], 10),
+                'account.statement.profile': (
+                    _get_statement_from_profile, ['balance_check'], 10),
+            },
+            readonly=True
+        ),
         'journal_id': fields.related(
-                        'profile_id',
-                        'journal_id',
-                        type='many2one',
-                        relation='account.journal',
-                        string='Journal',
-                        store=True,
-                        readonly=True),
+            'profile_id',
+            'journal_id',
+            type='many2one',
+            relation='account.journal',
+            string='Journal',
+            store={
+                'account.bank.statement': (_us, ['profile_id'], 10),
+                'account.statement.profile': (
+                    _get_statement_from_profile, ['journal_id'], 10),
+            },
+            readonly=True),
         'period_id': fields.many2one(
                         'account.period',
                         'Period',

=== added directory 'account_statement_ext/test'
=== added file 'account_statement_ext/test/test_profile_related_fields.yml'
--- account_statement_ext/test/test_profile_related_fields.yml	1970-01-01 00:00:00 +0000
+++ account_statement_ext/test/test_profile_related_fields.yml	2014-03-07 10:20:51 +0000
@@ -0,0 +1,78 @@
+-
+  In order to test the related fields in the profile, I first need to create a profile
+-
+  !record {model: account.statement.profile, id: profile_test1}:
+    name: Bank EUR Profile
+    journal_id: account.bank_journal
+    commission_account_id: account.a_expense
+    company_id: base.main_company
+    partner_id: base.res_partner_1
+    journal_id: account.check_journal
+    balance_check: True
+-
+  I create a second profile
+-
+  !record {model: account.statement.profile, id: profile_test2}:
+    name: Second profile
+    journal_id: account.bank_journal
+    commission_account_id: account.a_expense
+    company_id: base.main_company
+    partner_id: base.res_partner_3
+    journal_id: account.bank_journal
+    balance_check: True
+-
+  Now I create a statement.
+-
+  !record {model: account.bank.statement, id: statement_test1}:
+    name: Statement 1
+    profile_id: profile_test1
+    company_id: base.main_company
+-
+  Now I check that the related data come from the profile
+-
+  !assert {model: account.bank.statement, id: statement_test1, string: Initial data should come from the profile}:
+    - balance_check is True
+    - credit_partner_id.id == _ref('base.res_partner_1')
+    - journal_id.id == _ref('account.check_journal')
+-
+  Now I set the balance flag to False in the profile
+-
+  !record {model: account.statement.profile, id: profile_test1}:
+    balance_check: False
+-
+  It should be false in the statement as well.
+-
+  !assert {model: account.bank.statement, id: statement_test1, string: We should not check the balance}:
+    - balance_check is False
+-
+  Now I change the journal in the profile
+-
+  !record {model: account.statement.profile, id: profile_test1}:
+    journal_id: account.cash_journal
+-
+  The journal should propagate to the statement
+-
+  !assert {model: account.bank.statement, id: statement_test1, string: The journal should be updated}:
+    - journal_id.id == _ref('account.cash_journal')
+-
+  Now I change the partner in the profile
+-
+  !record {model: account.statement.profile, id: profile_test1}:
+    partner_id: base.res_partner_2
+-
+  The partner should propagate to the statement
+-
+  !assert {model: account.bank.statement, id: statement_test1, string: The partner should be updated}:
+    - credit_partner_id.id == _ref('base.res_partner_2')
+-
+  Now I change the profile associated to the statement.
+-
+  !record {model: account.bank.statement, id: statement_test1}:
+    profile_id: profile_test2
+-
+  The statement should receive information from the new statement.
+-
+  !assert {model: account.bank.statement, id: statement_test1, string: All information should be probagated from the new profile}:
+    - balance_check is True
+    - credit_partner_id.id == _ref('base.res_partner_3')
+    - journal_id.id == _ref('account.bank_journal')