← Back to team overview

avanzosc team mailing list archive

Re: [Merge] lp:~avanzosc/sepa-tools/7.0 into lp:sepa-tools

 

Review: Needs Fixing code review

Some comments inline.

Regards.

Diff comments:

> === added directory 'l10n_es_iban_converter'
> === added file 'l10n_es_iban_converter/__init__.py'
> --- l10n_es_iban_converter/__init__.py	1970-01-01 00:00:00 +0000
> +++ l10n_es_iban_converter/__init__.py	2014-08-04 12:54:19 +0000
> @@ -0,0 +1,24 @@
> +# -*- encoding: utf-8 -*-
> +########################################################################
> +#
> +# @authors: Ignacio Ibeas <ignacio@xxxxxxxxxx>
> +# Copyright (C) 2013  Acysos S.L.
> +#
> +#This program is free software: you can redistribute it and/or modify
> +#it under the terms of the GNU General Public License as published by
> +#the Free Software Foundation, either version 3 of the License, or
> +#(at your option) any later version.
> +#
> +# This module is GPLv3 or newer and incompatible
> +# with OpenERP SA "AGPL + Private Use License"!
> +#
> +#This program is distributed in the hope that it will be useful,
> +#but WITHOUT ANY WARRANTY; without even the implied warranty of
> +#MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +#GNU General Public License for more details.
> +#
> +#You should have received a copy of the GNU General Public License
> +#along with this program.  If not, see http://www.gnu.org/licenses.
> +########################################################################
> +
> +from . import wizard
> 
> === added file 'l10n_es_iban_converter/__openerp__.py'
> --- l10n_es_iban_converter/__openerp__.py	1970-01-01 00:00:00 +0000
> +++ l10n_es_iban_converter/__openerp__.py	2014-08-04 12:54:19 +0000
> @@ -0,0 +1,47 @@
> +# -*- encoding: utf-8 -*-
> +########################################################################
> +#
> +# @authors: Ignacio Ibeas <ignacio@xxxxxxxxxx>
> +# Copyright (C) 2013  Acysos S.L.
> +#
> +#This program is free software: you can redistribute it and/or modify
> +#it under the terms of the GNU General Public License as published by
> +#the Free Software Foundation, either version 3 of the License, or
> +#(at your option) any later version.
> +#
> +# This module is GPLv3 or newer and incompatible
> +# with OpenERP SA "AGPL + Private Use License"!
> +#
> +#This program is distributed in the hope that it will be useful,
> +#but WITHOUT ANY WARRANTY; without even the implied warranty of
> +#MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +#GNU General Public License for more details.
> +#
> +#You should have received a copy of the GNU General Public License
> +#along with this program.  If not, see http://www.gnu.org/licenses.
> +########################################################################
> +
> +{
> +    "name": "IBAN Converter - Spanish localization",
> +    "version": "1.0",
> +    "depends": [
> +        "base",
> +        "base_iban",
> +        "l10n_es_partner",
> +#         "account"
> +    ],
> +    "author": "Acysos S.L., Avanzosc S.L.",
> +    "website": "http://www.acysos.com, http://www.avanzosc.com";,
> +    "category": "Tools",
> +    "complexity": "normal",
> +    "description": """
> +This module create one shorcut and one action in res.partner object to convert
> +CCC to IBAN.

and viceversa.

> +    """,
> +    "data": [
> +        'wizard/wizard_partner_cc_iban_view.xml',
> +        'views/res_partner_view_ext.xml',
> +    ],
> +    'installable': True,
> +    'auto_install': False,
> +}
> 
> === added directory 'l10n_es_iban_converter/i18n'
> === added file 'l10n_es_iban_converter/i18n/es.po'
> --- l10n_es_iban_converter/i18n/es.po	1970-01-01 00:00:00 +0000
> +++ l10n_es_iban_converter/i18n/es.po	2014-08-04 12:54:19 +0000
> @@ -0,0 +1,78 @@
> +# Translation of Odoo Server.
> +# This file contains the translation of the following modules:
> +#	* l10n_es_iban_converter
> +#
> +msgid ""
> +msgstr ""
> +"Project-Id-Version: Odoo Server 8.0rc1\n"
> +"Report-Msgid-Bugs-To: \n"
> +"POT-Creation-Date: 2014-08-04 12:45+0000\n"
> +"PO-Revision-Date: 2014-08-04 12:45+0000\n"
> +"Last-Translator: <>\n"
> +"Language-Team: \n"
> +"MIME-Version: 1.0\n"
> +"Content-Type: text/plain; charset=UTF-8\n"
> +"Content-Transfer-Encoding: \n"
> +"Plural-Forms: \n"
> +
> +#. module: l10n_es_iban_converter
> +#: field:wizard.partner.cc.iban,bank_state:0
> +msgid "Bank Account Type"
> +msgstr "Tipo de cuenta bancaria"
> +
> +#. module: l10n_es_iban_converter
> +#: model:ir.model,name:l10n_es_iban_converter.model_res_partner_bank
> +msgid "Bank Accounts"
> +msgstr "Cuentas de banco"
> +
> +#. module: l10n_es_iban_converter
> +#: view:wizard.partner.cc.iban:0
> +msgid "Cancel"
> +msgstr "Cancelar"
> +
> +#. module: l10n_es_iban_converter
> +#: field:wizard.partner.cc.iban,create_uid:0
> +msgid "Created by"
> +msgstr "Creado por"
> +
> +#. module: l10n_es_iban_converter
> +#: field:wizard.partner.cc.iban,create_date:0
> +msgid "Created on"
> +msgstr "Creado el"
> +
> +#. module: l10n_es_iban_converter
> +#: field:wizard.partner.cc.iban,id:0
> +msgid "ID"
> +msgstr "ID"
> +
> +#. module: l10n_es_iban_converter
> +#: field:wizard.partner.cc.iban,write_uid:0
> +msgid "Last Updated by"
> +msgstr "Actualizado por"
> +
> +#. module: l10n_es_iban_converter
> +#: field:wizard.partner.cc.iban,write_date:0
> +msgid "Last Updated on"
> +msgstr "Actualizado el"
> +
> +#. module: l10n_es_iban_converter
> +#: view:wizard.partner.cc.iban:0
> +msgid "Ok"
> +msgstr "Ok"
> +
> +#. module: l10n_es_iban_converter
> +#: model:ir.actions.act_window,name:l10n_es_iban_converter.action_partner_to_cciban
> +#: view:wizard.partner.cc.iban:0
> +msgid "Update CC IBAN"
> +msgstr "Actualizar CC IBAN"
> +
> +#. module: l10n_es_iban_converter
> +#: model:ir.model,name:l10n_es_iban_converter.model_wizard_partner_cc_iban
> +msgid "Wizard Partner CC IBAN"
> +msgstr "Wizard Partner CC IBAN"
> +
> +#. module: l10n_es_iban_converter
> +#: view:wizard.partner.cc.iban:0
> +msgid "You are going to update account number in partners banks, select which type to do you want to change"
> +msgstr "Va a actualizar los números de cuenta de los partner seleccionados, elija a que tipo de cuenta quiere cambiar"
> +
> 
> === added file 'l10n_es_iban_converter/i18n/l10n_es_iban_converter.pot'
> --- l10n_es_iban_converter/i18n/l10n_es_iban_converter.pot	1970-01-01 00:00:00 +0000
> +++ l10n_es_iban_converter/i18n/l10n_es_iban_converter.pot	2014-08-04 12:54:19 +0000
> @@ -0,0 +1,78 @@
> +# Translation of Odoo Server.
> +# This file contains the translation of the following modules:
> +#	* l10n_es_iban_converter
> +#
> +msgid ""
> +msgstr ""
> +"Project-Id-Version: Odoo Server 8.0rc1\n"
> +"Report-Msgid-Bugs-To: \n"
> +"POT-Creation-Date: 2014-08-04 12:44+0000\n"
> +"PO-Revision-Date: 2014-08-04 12:44+0000\n"
> +"Last-Translator: <>\n"
> +"Language-Team: \n"
> +"MIME-Version: 1.0\n"
> +"Content-Type: text/plain; charset=UTF-8\n"
> +"Content-Transfer-Encoding: \n"
> +"Plural-Forms: \n"
> +
> +#. module: l10n_es_iban_converter
> +#: field:wizard.partner.cc.iban,bank_state:0
> +msgid "Bank Account Type"
> +msgstr ""
> +
> +#. module: l10n_es_iban_converter
> +#: model:ir.model,name:l10n_es_iban_converter.model_res_partner_bank
> +msgid "Bank Accounts"
> +msgstr ""
> +
> +#. module: l10n_es_iban_converter
> +#: view:wizard.partner.cc.iban:0
> +msgid "Cancel"
> +msgstr ""
> +
> +#. module: l10n_es_iban_converter
> +#: field:wizard.partner.cc.iban,create_uid:0
> +msgid "Created by"
> +msgstr ""
> +
> +#. module: l10n_es_iban_converter
> +#: field:wizard.partner.cc.iban,create_date:0
> +msgid "Created on"
> +msgstr ""
> +
> +#. module: l10n_es_iban_converter
> +#: field:wizard.partner.cc.iban,id:0
> +msgid "ID"
> +msgstr ""
> +
> +#. module: l10n_es_iban_converter
> +#: field:wizard.partner.cc.iban,write_uid:0
> +msgid "Last Updated by"
> +msgstr ""
> +
> +#. module: l10n_es_iban_converter
> +#: field:wizard.partner.cc.iban,write_date:0
> +msgid "Last Updated on"
> +msgstr ""
> +
> +#. module: l10n_es_iban_converter
> +#: view:wizard.partner.cc.iban:0
> +msgid "Ok"
> +msgstr ""
> +
> +#. module: l10n_es_iban_converter
> +#: model:ir.actions.act_window,name:l10n_es_iban_converter.action_partner_to_cciban
> +#: view:wizard.partner.cc.iban:0
> +msgid "Update CC IBAN"
> +msgstr ""
> +
> +#. module: l10n_es_iban_converter
> +#: model:ir.model,name:l10n_es_iban_converter.model_wizard_partner_cc_iban
> +msgid "Wizard Partner CC IBAN"
> +msgstr ""
> +
> +#. module: l10n_es_iban_converter
> +#: view:wizard.partner.cc.iban:0
> +msgid "You are going to update account number in partners banks, select which type to do you want to change"
> +msgstr ""
> +
> 
> === added directory 'l10n_es_iban_converter/views'
> === added file 'l10n_es_iban_converter/views/res_partner_view_ext.xml'
> --- l10n_es_iban_converter/views/res_partner_view_ext.xml	1970-01-01 00:00:00 +0000
> +++ l10n_es_iban_converter/views/res_partner_view_ext.xml	2014-08-04 12:54:19 +0000
> @@ -0,0 +1,10 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<openerp>
> +    <data>
> +        <!-- ACTION partner To CC IBAN -->
> +        <act_window id="action_partner_to_cciban" key2="client_action_multi"
> +            name="Update CC IBAN" res_model="wizard.partner.cc.iban"

Better to put "Convert CCC <-> IBAN"

> +            src_model="res.partner" view_id="wizard_partner_cc_iban_view"
> +            view_mode="form" target="new" view_type="form" />
> +    </data>
> +</openerp>
> \ No newline at end of file
> 
> === added directory 'l10n_es_iban_converter/wizard'
> === added file 'l10n_es_iban_converter/wizard/__init__.py'
> --- l10n_es_iban_converter/wizard/__init__.py	1970-01-01 00:00:00 +0000
> +++ l10n_es_iban_converter/wizard/__init__.py	2014-08-04 12:54:19 +0000
> @@ -0,0 +1,24 @@
> +# -*- encoding: utf-8 -*-
> +########################################################################
> +#
> +# @authors: Ignacio Ibeas <ignacio@xxxxxxxxxx>
> +# Copyright (C) 2013  Acysos S.L.
> +#
> +#This program is free software: you can redistribute it and/or modify
> +#it under the terms of the GNU General Public License as published by
> +#the Free Software Foundation, either version 3 of the License, or
> +#(at your option) any later version.
> +#
> +# This module is GPLv3 or newer and incompatible
> +# with OpenERP SA "AGPL + Private Use License"!
> +#
> +#This program is distributed in the hope that it will be useful,
> +#but WITHOUT ANY WARRANTY; without even the implied warranty of
> +#MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +#GNU General Public License for more details.
> +#
> +#You should have received a copy of the GNU General Public License
> +#along with this program.  If not, see http://www.gnu.org/licenses.
> +########################################################################
> +
> +from . import wizard_partner_cc_iban
> 
> === added file 'l10n_es_iban_converter/wizard/wizard_partner_cc_iban.py'
> --- l10n_es_iban_converter/wizard/wizard_partner_cc_iban.py	1970-01-01 00:00:00 +0000
> +++ l10n_es_iban_converter/wizard/wizard_partner_cc_iban.py	2014-08-04 12:54:19 +0000
> @@ -0,0 +1,116 @@
> +# -*- encoding: utf-8 -*-
> +########################################################################
> +#
> +# @authors: Ignacio Ibeas <ignacio@xxxxxxxxxx>
> +# Copyright (C) 2013  Acysos S.L.
> +#
> +#This program is free software: you can redistribute it and/or modify
> +#it under the terms of the GNU General Public License as published by
> +#the Free Software Foundation, either version 3 of the License, or
> +#(at your option) any later version.
> +#
> +# This module is GPLv3 or newer and incompatible
> +# with OpenERP SA "AGPL + Private Use License"!
> +#
> +#This program is distributed in the hope that it will be useful,
> +#but WITHOUT ANY WARRANTY; without even the implied warranty of
> +#MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +#GNU General Public License for more details.
> +#
> +#You should have received a copy of the GNU General Public License
> +#along with this program.  If not, see http://www.gnu.org/licenses.
> +########################################################################
> +
> +from openerp.osv import fields, orm
> +from openerp.tools.translate import _
> +
> +from openerp.addons.base_iban.base_iban import _pretty_iban
> +
> +
> +_mapping = {'A': '10', 'B': '11', 'C': '12', 'D': '13', 'E': '14', 'F': '15',
> +            'G': '16', 'H': '17', 'I': '18', 'J': '19', 'K': '20', 'L': '21',
> +            'M': '22', 'N': '23', 'O': '24', 'P': '25', 'Q': '26', 'R': '27',
> +            'S': '28', 'T': '29', 'U': '30', 'V': '31', 'W': '32', 'X': '33',
> +            'Y': '34', 'Z': '35'}
> +
> +
> +class WizardPartnerCcIban(orm.TransientModel):
> +    _name = "wizard.partner.cc.iban"
> +    _description = "Wizard Partner CC IBAN"
> +
> +    def _bank_type_get(self, cr, uid, context=None):
> +        bank_type_obj = self.pool['res.partner.bank.type']

Very good choice to fill it from existing types! But you can directly call '_bank_type_get' method of res.partner.bank, and you will get the same result. How this can be done:

def _bank_type_get(self, cr, uid, context=None):
    partner_bank_obj = self.pool['res.partner.bank']
    return partner_bank_obj._bank_type_get(cr, uid, context=context)

> +        result = []
> +        type_ids = bank_type_obj.search(cr, uid, [])
> +        bank_types = bank_type_obj.browse(cr, uid, type_ids, context=context)
> +        for bank_type in bank_types:
> +            result.append((bank_type.code, bank_type.name))
> +        return result
> +
> +    _columns = {
> +        'bank_state': fields.selection(_bank_type_get, 'Bank Account Type',
> +                                       required=True),
> +    }
> +
> +    def update_cc_iban(self, cr, uid, ids, context=None):
> +        if context is None:
> +            context = {}
> +        data = self.read(cr, uid, ids, context=context)[0]
> +        bank_obj = self.pool['res.partner.bank']
> +        partner_obj = self.pool['res.partner']
> +        partner_ids = context.get('active_ids')
> +        if partner_ids:
> +            for partner in partner_obj.browse(cr, uid, partner_ids,
> +                                              context=context):
> +                if partner.bank_ids:
> +                    for bank in partner.bank_ids:
> +                        new_data = {}
> +                        country = bank.country_id
> +                        if not country:
> +                            country = partner.country_id
> +                            new_data.update({'country_id': country.id})

For these single updates, better to put:

new_data['country_id'] = country.id

> +                        if bank.state != data['bank_state'] and bank.state == 'bank':

Better to make:

if bank.state != data['bank_state']:
    continue

And to not continue the rest of the code. Better readibility and there is no other operations done (like bank and BIC assignment). This is to be consistent and fast (nor writing each partner bank data).

> +                            iban = self.convert_to_iban(cr, uid, bank.acc_number, country.code,
> +                                                        context=context)
> +                            values = bank_obj.onchange_banco(cr, uid, ids, iban,
> +                                                             country.id,
> +                                                             'iban',
> +                                                             context=context)
> +                            new_data.update({'acc_number': values['value']['acc_number'],
> +                                             'state': 'iban'})
> +                        elif bank.state != data['bank_state'] and bank.state == 'iban':
> +                            ccc = self.convert_to_ccc(cr, uid, bank.acc_number,
> +                                                      context=context)
> +                            values = bank_obj.onchange_banco(cr, uid, ids, ccc,
> +                                                    country.id,
> +                                                    'bank',
> +                                                    context=context)
> +                            new_data.update({'acc_number': values['value']['acc_number'],
> +                                             'state': 'bank'})
> +                        if (not bank.bank or not bank.bank_bic) and 'bank' in values['value']:
> +                            bank_data = bank_obj.onchange_bank_id(cr, uid, ids, values['value']['bank'], context=context)

Is this PEP8 80 cols compliant?

> +                            new_data.update({'bank': values['value']['bank'],
> +                                             'bank_bic': bank_data['value']['bank_bic'],
> +                                             'bank_name': bank_data['value']['bank_name']})
> +                        bank_obj.write(cr, uid, [bank.id], new_data,
> +                                       context=context)
> +        return {'type': 'ir.actions.act_window_close'}
> +
> +    def convert_to_iban(self, cr, uid, acc_number, country_code, context=None):
> +        code_char = _mapping[country_code[:1]] + _mapping[country_code[1:]]
> +        ccc = acc_number.replace(" ", "")
> +        for key, replacement in _mapping.items():
> +            ccc_number = ccc.replace(key, replacement)
> +        ccc_convert = int(ccc_number + code_char + '00')
> +        remainder = ccc_convert % 97
> +        control_digit = 98 - remainder
> +        if control_digit < 10:
> +            control_digit = '0'+str(control_digit)

Put space between '+' for PEP8

> +        else:
> +            control_digit = str(control_digit)
> +        iban = _pretty_iban(country_code + control_digit + str(ccc))
> +        return iban
> +
> +    def convert_to_ccc(self, cr, uid, acc_number, context=None):
> +        ccc = acc_number.replace(" ", "")
> +        return ccc[4:]
> \ No newline at end of file
> 
> === added file 'l10n_es_iban_converter/wizard/wizard_partner_cc_iban_view.xml'
> --- l10n_es_iban_converter/wizard/wizard_partner_cc_iban_view.xml	1970-01-01 00:00:00 +0000
> +++ l10n_es_iban_converter/wizard/wizard_partner_cc_iban_view.xml	2014-08-04 12:54:19 +0000
> @@ -0,0 +1,28 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<openerp>
> +    <data>
> +
> +        <record id="wizard_partner_cc_iban_view" model="ir.ui.view">
> +            <field name="name">wizard.partner.cc.iban.view</field>
> +            <field name="model">wizard.partner.cc.iban</field>
> +            <field name="type">form</field>
> +            <field name="arch" type="xml">
> +                <form string="Update CC IBAN">
> +                    <group colspan="4">
> +                        <separator colspan="4" />
> +                        <label
> +                            string="You are going to update account number in partners banks, select which type to do you want to change" />
> +                        <field name="bank_state" nolabel="1" />
> +                    </group>
> +                    <group colspan="4">

Use version="7.0" with footer for the wizard.

> +                        <button icon='gtk-cancel' special="cancel"
> +                            string="Cancel" />
> +                        <button name="update_cc_iban" icon='gtk-ok'
> +                            type="object" string="Ok" />
> +                    </group>
> +                </form>
> +            </field>
> +        </record>
> +
> +    </data>
> +</openerp>
> \ No newline at end of file
> 


-- 
https://code.launchpad.net/~avanzosc/sepa-tools/7.0/+merge/229446
Your team Avanzosc Developers is subscribed to branch lp:~avanzosc/sepa-tools/7.0.


References