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

Proposed by Stefan Rijnhart (Opener)
Status: Merged
Merged at revision: 160
Proposed branch: lp:~therp-nl/banking-addons/ba61-company_awareness_bank_import_settings
Merge into: lp:banking-addons/6.1
Diff against target: 192 lines (+97/-37)
2 files modified
account_banking/account_banking.py (+74/-28)
account_banking/account_banking_view.xml (+23/-9)
To merge this branch: bzr merge lp:~therp-nl/banking-addons/ba61-company_awareness_bank_import_settings
Reviewer Review Type Date Requested Status
Alexandre Fayolle - camptocamp code review, no test Approve
Guewen Baconnier @ Camptocamp no test, code review Approve
Review via email: mp+145023@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

LGTM

review: Approve (no test, code review)
Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

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)
157. By Stefan Rijnhart (Opener)

[RFR] Style as per review

Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote :

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.

Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

> 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?

Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
81 select=True, required=True),81 select=True, required=True),
82 'journal_id': fields.many2one('account.journal', 'Journal',82 'journal_id': fields.many2one('account.journal', 'Journal',
83 required=True),83 required=True),
84 'partner_id': fields.related(
85 'company_id', 'partner_id',
86 type='many2one', relation='res.partner',
87 string='Partner'),
84 'default_credit_account_id': fields.many2one(88 'default_credit_account_id': fields.many2one(
85 'account.account', 'Default credit account', select=True,89 'account.account', 'Default credit account', select=True,
86 help=('The account to use when an unexpected payment was signaled. '90 help=('The account to use when an unexpected payment was signaled. '
@@ -127,44 +131,59 @@
127 #),131 #),
128 }132 }
129133
130 def _default_company(self, cursor, uid, context=None):134 def _default_company(self, cr, uid, context=None):
131 user = self.pool.get('res.users').browse(cursor, uid, uid, context=context)135 """
132 if user.company_id:136 Return the user's company or the first company found
133 return user.company_id.id137 in the database
134 company_ids = self.pool.get('res.company').search(138 """
135 cursor, uid, [('parent_id', '=', False)])
136 return len(company_ids) == 1 and company_ids[0] or False
137
138 def _default_journal(self, cr, uid, context=None):
139 domain = [('type', '=', 'bank')]
140 user = self.pool.get('res.users').read(139 user = self.pool.get('res.users').read(
141 cr, uid, uid, ['company_id'], context=context)140 cr, uid, uid, ['company_id'], context=context)
142 if user['company_id']:141 if user['company_id']:
143 domain.append(('company_id', '=', user['company_id'][0]))142 return user['company_id'][0]
143 return self.pool.get('res.company').search(
144 cr, uid, [('parent_id', '=', False)])[0]
145
146 def _default_partner_id(self, cr, uid, context=None, company_id=False):
147 if not company_id:
148 company_id = self._default_company(cr, uid, context=context)
149 return self.pool.get('res.company').read(
150 cr, uid, company_id, ['partner_id'],
151 context=context)['partner_id'][0]
152
153 def _default_journal(self, cr, uid, context=None, company_id=False):
154 if not company_id:
155 company_id = self._default_company(cr, uid, context=context)
144 journal_ids = self.pool.get('account.journal').search(156 journal_ids = self.pool.get('account.journal').search(
145 cr, uid, domain)157 cr, uid, [('type', '=', 'bank'), ('company_id', '=', company_id)])
146 return len(journal_ids) == 1 and journal_ids[0] or False158 return journal_ids and journal_ids[0] or False
147159
148 def _default_partner_bank_id(self, cr, uid, context=None):160 def _default_partner_bank_id(
149 user = self.pool.get('res.users').read(161 self, cr, uid, context=None, company_id=False):
150 cr, uid, uid, ['company_id'], context=context)162 if not company_id:
151 if user['company_id']:163 company_id = self._default_company(cr, uid, context=context)
152 bank_ids = self.pool.get('res.partner.bank').search(164 partner_id = self.pool.get('res.company').read(
153 cr, uid, [('company_id', '=', user['company_id'][0])])165 cr, uid, company_id, ['partner_id'], context=context)['partner_id'][0]
154 if len(bank_ids) == 1:166 bank_ids = self.pool.get('res.partner.bank').search(
155 return bank_ids[0]167 cr, uid, [('partner_id', '=', partner_id)], context=context)
156 return False168 return bank_ids and bank_ids[0] or False
157169
158 def _default_debit_account_id(self, cr, uid, context=None):170 def _default_debit_account_id(
171 self, cr, uid, context=None, company_id=False):
172 localcontext = context and context.copy() or {}
173 localcontext['force_company'] = (
174 company_id or self._default_company(cr, uid, context=context))
159 account_def = self.pool.get('ir.property').get(175 account_def = self.pool.get('ir.property').get(
160 cr, uid, 'property_account_receivable',176 cr, uid, 'property_account_receivable',
161 'res.partner', context=context)177 'res.partner', context=localcontext)
162 return account_def and account_def.id or False178 return account_def and account_def.id or False
163179
164 def _default_credit_account_id(self, cr, uid, context=None):180 def _default_credit_account_id(self, cr, uid, context=None, company_id=False):
181 localcontext = context and context.copy() or {}
182 localcontext['force_company'] = (
183 company_id or self._default_company(cr, uid, context=context))
165 account_def = self.pool.get('ir.property').get(184 account_def = self.pool.get('ir.property').get(
166 cr, uid, 'property_account_payable',185 cr, uid, 'property_account_payable',
167 'res.partner', context=context)186 'res.partner', context=localcontext)
168 return account_def and account_def.id or False187 return account_def and account_def.id or False
169188
170 def find(self, cr, uid, journal_id, partner_bank_id=False, context=None):189 def find(self, cr, uid, journal_id, partner_bank_id=False, context=None):
@@ -173,8 +192,35 @@
173 domain.append(('partner_bank_id','=',partner_bank_id))192 domain.append(('partner_bank_id','=',partner_bank_id))
174 return self.search(cr, uid, domain, context=context)193 return self.search(cr, uid, domain, context=context)
175194
195 def onchange_partner_bank_id(
196 self, cr, uid, ids, partner_bank_id, context=None):
197 values = {}
198 if partner_bank_id:
199 bank = self.pool.get('res.partner.bank').read(
200 cr, uid, partner_bank_id, ['journal_id'], context=context)
201 if bank['journal_id']:
202 values['journal_id'] = bank['journal_id'][0]
203 return {'value': values}
204
205 def onchange_company_id (
206 self, cr, uid, ids, company_id=False, context=None):
207 if not company_id:
208 return {}
209 result = {
210 'partner_id': self._default_partner_id(
211 cr, uid, company_id=company_id, context=context),
212 'journal_id': self._default_journal(
213 cr, uid, company_id=company_id, context=context),
214 'default_debit_account_id': self._default_debit_account_id(
215 cr, uid, company_id=company_id, context=context),
216 'default_credit_account_id': self._default_credit_account_id(
217 cr, uid, company_id=company_id, context=context),
218 }
219 return {'value': result}
220
176 _defaults = {221 _defaults = {
177 'company_id': _default_company,222 'company_id': _default_company,
223 'partner_id': _default_partner_id,
178 'journal_id': _default_journal,224 'journal_id': _default_journal,
179 'default_debit_account_id': _default_debit_account_id,225 'default_debit_account_id': _default_debit_account_id,
180 'default_credit_account_id': _default_credit_account_id,226 'default_credit_account_id': _default_credit_account_id,
181227
=== 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 @@
39 <field name="type">form</field>39 <field name="type">form</field>
40 <field name="arch" type="xml">40 <field name="arch" type="xml">
41 <form string="Default Import Settings for Bank Account">41 <form string="Default Import Settings for Bank Account">
42 <field name="company_id" />42 <field name="company_id"
43 widget='selection'
44 groups="base.group_multi_company"
45 on_change="onchange_company_id(company_id)" />
43 <separator string="Bank Account Details" colspan="4" />46 <separator string="Bank Account Details" colspan="4" />
44 <field name="partner_bank_id" /> <!-- Needs domain for used companies /-->47 <field name="partner_id" invisible="1"/>
45 <field name="journal_id" domain="[('type','=','bank')]" />48 <field name="partner_bank_id"
49 domain="[('partner_id', '=', partner_id)]"
50 on_change="onchange_partner_bank_id(partner_bank_id)" />
51 <field name="journal_id"
52 domain="[('type','=','bank'),
53 ('company_id', '=', company_id)]" />
46 <separator string="Default Accounts for Unknown Movements" colspan="4" />54 <separator string="Default Accounts for Unknown Movements" colspan="4" />
47 <field name="default_credit_account_id" />55 <field name="default_credit_account_id"
48 <field name="default_debit_account_id" />56 domain="[('company_id', '=', company_id)]" />
57 <field name="default_debit_account_id"
58 domain="[('company_id', '=', company_id)]" />
49 <separator string="Generation of Bank Costs Invoices" colspan="4" />59 <separator string="Generation of Bank Costs Invoices" colspan="4" />
50 <field name="bank_partner_id" />60 <field name="bank_partner_id" />
51 <field name="costs_account_id" attrs="{'required': [('bank_partner_id', '&lt;&gt;', False)]}" />61 <field name="costs_account_id"
52 <field name="invoice_journal_id" attrs="{'required': [('bank_partner_id', '&lt;&gt;', False)]}" />62 attrs="{'required': [('bank_partner_id', '!=', False)]}"
63 domain="[('company_id', '=', company_id)]" />
64 <field name="invoice_journal_id"
65 attrs="{'required': [('bank_partner_id', '!=', False)]}"
66 domain="[('company_id', '=', company_id)]" />
53 </form>67 </form>
54 </field>68 </field>
55 </record>69 </record>
@@ -60,8 +74,8 @@
60 <field name="arch" type="xml">74 <field name="arch" type="xml">
61 <tree string="Default Import Settings for Bank Account">75 <tree string="Default Import Settings for Bank Account">
62 <field name="company_id" />76 <field name="company_id" />
63 <field name="partner_bank_id" /> <!-- Needs domain for used companies /-->77 <field name="partner_bank_id" />
64 <field name="journal_id" domain="[('type','=','bank')]" />78 <field name="journal_id" />
65 </tree>79 </tree>
66 </field>80 </field>
67 </record>81 </record>

Subscribers

People subscribed via source and target branches