Merge lp:~therp-nl/banking-addons/ba61-company_awareness_bank_import_settings into lp:banking-addons/6.1

[Stefan Rijnhart (Opener)]

This branch adds company awareness when configuring bank statement import settings. Accounts, journals and bank accounts are now filtered by the company setting on the payment mode. Only bank accounts from the company's partner can be selected. Upon selection of the bank account, the associated journal is selected automatically.

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

LGTM

review: Approve (no test, code review)

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

nitpicking (line numbers refer the to preview diff)

line 8: missing space after colon

line 29: it looks like all other methods in that file use "cr" as argument name. Time to make this one conform with the convention?

line 35-38: this is verging on abuse, IMO. I much prefer something like

if user['company_id']:
    return user['company_id'][0]
else:
    company_ids = self.pool.get('res.company').search(
                         cursor, uid, [('parent_id', '=', False)])
    return company_ids[0]

in the XML, I think using '!=' rather than '<>' is much nicer

Nothing blocking in the above.

review: Approve (code review, no test)

[lp:~therp-nl/banking-addons/ba61-company_awareness_bank_import_settings]

157. By Stefan Rijnhart (Opener) on 2013-01-30

[RFR] Style as per review

[Stefan Rijnhart (Opener) (stefan-opener)]

Hi Alexandre,

thank you for the style notes. I followed them up only with a slight variation in the return statements of _default_company(). Additionally, I removed the void company_id keyword argument to this method.

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

> Hi Alexandre,
>
> thank you for the style notes. I followed them up only with a slight variation
> in the return statements of _default_company(). Additionally, I removed the
> void company_id keyword argument to this method.

Thanks a lot for taking my contribution into account.

Indeed, no one seems to pass that argument...

As for the return statement, there are way too many "X and Y or Z" in openerp related code. I understand the historical context, the need for a single statement in many places. IMO when a single statement is not mandatory (e.g. in a return statement) the full blown if: else: construct should be prefered. In other case, I would like to see a generalisation of the use of Python's ternary operator (Y if X else Z) which does not feature the pitfall of "X and Y or Z" (which returns Z if X is true and Y is false). I'm not asking for this in my reviews unless I spot a case where Y can be false. Do you have an opinion on this?

[Stefan Rijnhart (Opener) (stefan-opener)]

Not really, no. But I would be happy to follow any set of best practices that the community can agree on. I was surprised that I could not find a general Python style guide on this, apart from a short statent in Google's [1]

[1] http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Conditional_Expressions#Conditional_Expressions

=== modified file 'account_banking/account_banking.py'
--- account_banking/account_banking.py	2012-12-15 15:19:49 +0000
+++ account_banking/account_banking.py	2013-01-30 11:37:19 +0000
@@ -81,6 +81,10 @@
                                            select=True, required=True),
         'journal_id': fields.many2one('account.journal', 'Journal',
                                       required=True),
+        'partner_id': fields.related(
+            'company_id', 'partner_id',
+            type='many2one', relation='res.partner',
+            string='Partner'),
         'default_credit_account_id': fields.many2one(
             'account.account', 'Default credit account', select=True,
             help=('The account to use when an unexpected payment was signaled. '
@@ -127,44 +131,59 @@
         #),
     }
 
-    def _default_company(self, cursor, uid, context=None):
-        user = self.pool.get('res.users').browse(cursor, uid, uid, context=context)
-        if user.company_id:
-            return user.company_id.id
-        company_ids = self.pool.get('res.company').search(
-            cursor, uid, [('parent_id', '=', False)])
-        return len(company_ids) == 1 and company_ids[0] or False
-
-    def _default_journal(self, cr, uid, context=None):
-        domain = [('type', '=', 'bank')]
+    def _default_company(self, cr, uid, context=None):
+        """
+        Return the user's company or the first company found
+        in the database
+        """
         user = self.pool.get('res.users').read(
             cr, uid, uid, ['company_id'], context=context)
         if user['company_id']:
-            domain.append(('company_id', '=', user['company_id'][0]))
+            return user['company_id'][0]
+        return self.pool.get('res.company').search(
+            cr, uid, [('parent_id', '=', False)])[0]
+    
+    def _default_partner_id(self, cr, uid, context=None, company_id=False):
+        if not company_id:
+            company_id = self._default_company(cr, uid, context=context)
+        return self.pool.get('res.company').read(
+            cr, uid, company_id, ['partner_id'],
+            context=context)['partner_id'][0]
+
+    def _default_journal(self, cr, uid, context=None, company_id=False):
+        if not company_id:
+            company_id = self._default_company(cr, uid, context=context)
         journal_ids = self.pool.get('account.journal').search(
-            cr, uid, domain)
-        return len(journal_ids) == 1 and journal_ids[0] or False
-
-    def _default_partner_bank_id(self, cr, uid, context=None):
-        user = self.pool.get('res.users').read(
-            cr, uid, uid, ['company_id'], context=context)
-        if user['company_id']:
-            bank_ids = self.pool.get('res.partner.bank').search(
-                cr, uid, [('company_id', '=', user['company_id'][0])])
-            if len(bank_ids) == 1:
-                return bank_ids[0]
-        return False
-
-    def _default_debit_account_id(self, cr, uid, context=None):
+            cr, uid, [('type', '=', 'bank'), ('company_id', '=', company_id)])
+        return journal_ids and journal_ids[0] or False
+
+    def _default_partner_bank_id(
+            self, cr, uid, context=None, company_id=False):
+        if not company_id:
+            company_id = self._default_company(cr, uid, context=context)
+        partner_id = self.pool.get('res.company').read(
+            cr, uid, company_id, ['partner_id'], context=context)['partner_id'][0]
+        bank_ids = self.pool.get('res.partner.bank').search(
+            cr, uid, [('partner_id', '=', partner_id)], context=context)
+        return bank_ids and bank_ids[0] or False
+
+    def _default_debit_account_id(
+            self, cr, uid, context=None, company_id=False):
+        localcontext = context and context.copy() or {}
+        localcontext['force_company'] = (
+            company_id or self._default_company(cr, uid, context=context))
         account_def = self.pool.get('ir.property').get(
             cr, uid, 'property_account_receivable',
-            'res.partner', context=context)
+            'res.partner', context=localcontext)
         return account_def and account_def.id or False
 
-    def _default_credit_account_id(self, cr, uid, context=None):
+    def _default_credit_account_id(self, cr, uid, context=None, company_id=False):
+        localcontext = context and context.copy() or {}
+        localcontext['force_company'] = (
+            company_id or self._default_company(cr, uid, context=context))
         account_def = self.pool.get('ir.property').get(
             cr, uid, 'property_account_payable',
-            'res.partner', context=context)
+            'res.partner', context=localcontext)
         return account_def and account_def.id or False
 
     def find(self, cr, uid, journal_id, partner_bank_id=False, context=None):
@@ -173,8 +192,35 @@
             domain.append(('partner_bank_id','=',partner_bank_id))
         return self.search(cr, uid, domain, context=context)
 
+    def onchange_partner_bank_id(
+            self, cr, uid, ids, partner_bank_id, context=None):
+        values = {}
+        if partner_bank_id:
+            bank = self.pool.get('res.partner.bank').read(
+                cr, uid, partner_bank_id, ['journal_id'], context=context)
+            if bank['journal_id']:
+                values['journal_id'] = bank['journal_id'][0]
+        return {'value': values}
+
+    def onchange_company_id (
+            self, cr, uid, ids, company_id=False, context=None):
+        if not company_id:
+            return {}
+        result = {
+            'partner_id': self._default_partner_id(
+                cr, uid, company_id=company_id, context=context),
+            'journal_id': self._default_journal(
+                cr, uid, company_id=company_id, context=context),
+            'default_debit_account_id': self._default_debit_account_id(
+                cr, uid, company_id=company_id, context=context),
+            'default_credit_account_id': self._default_credit_account_id(
+                cr, uid, company_id=company_id, context=context),
+            }
+        return {'value': result}
+
     _defaults = {
         'company_id': _default_company,
+        'partner_id': _default_partner_id,
         'journal_id': _default_journal,
         'default_debit_account_id': _default_debit_account_id,
         'default_credit_account_id': _default_credit_account_id,

=== modified file 'account_banking/account_banking_view.xml'
--- account_banking/account_banking_view.xml	2012-05-02 14:28:52 +0000
+++ account_banking/account_banking_view.xml	2013-01-30 11:37:19 +0000
@@ -39,17 +39,31 @@
             <field name="type">form</field>
             <field name="arch" type="xml">
                 <form string="Default Import Settings for Bank Account">
-                    <field name="company_id" />
+                    <field name="company_id"
+                           widget='selection'
+                           groups="base.group_multi_company"
+                           on_change="onchange_company_id(company_id)" />
                     <separator string="Bank Account Details" colspan="4" />
-                    <field name="partner_bank_id" /> <!-- Needs domain for used companies /-->
-                    <field name="journal_id" domain="[('type','=','bank')]" />
+                    <field name="partner_id" invisible="1"/>
+                    <field name="partner_bank_id"
+                           domain="[('partner_id', '=', partner_id)]"
+                           on_change="onchange_partner_bank_id(partner_bank_id)" />
+                    <field name="journal_id"
+                           domain="[('type','=','bank'),
+                                    ('company_id', '=', company_id)]" />
                     <separator string="Default Accounts for Unknown Movements" colspan="4" />
-                    <field name="default_credit_account_id" />
-                    <field name="default_debit_account_id" />
+                    <field name="default_credit_account_id"
+                           domain="[('company_id', '=', company_id)]" />
+                    <field name="default_debit_account_id"
+                           domain="[('company_id', '=', company_id)]" />
                     <separator string="Generation of Bank Costs Invoices" colspan="4" />
                     <field name="bank_partner_id" />
-                    <field name="costs_account_id" attrs="{'required': [('bank_partner_id', '&lt;&gt;', False)]}" />
-                    <field name="invoice_journal_id" attrs="{'required': [('bank_partner_id', '&lt;&gt;', False)]}" />
+                    <field name="costs_account_id"
+                           attrs="{'required': [('bank_partner_id', '!=', False)]}"
+                           domain="[('company_id', '=', company_id)]" />
+                    <field name="invoice_journal_id"
+                           attrs="{'required': [('bank_partner_id', '!=', False)]}"
+                           domain="[('company_id', '=', company_id)]" />
                 </form>
             </field>
         </record>
@@ -60,8 +74,8 @@
             <field name="arch" type="xml">
                 <tree string="Default Import Settings for Bank Account">
                     <field name="company_id" />
-                    <field name="partner_bank_id" /> <!-- Needs domain for used companies /-->
-                    <field name="journal_id" domain="[('type','=','bank')]" />
+                    <field name="partner_bank_id" />
+                    <field name="journal_id" />
                 </tree>
             </field>
         </record>