[MIG] base_location_nuts: Migration to 10.0 #432

chienandalu · 2017-05-31T14:20:13Z

yajo · 2017-05-31T14:25:10Z

base_location_nuts/models/res_partner.py

+    @api.depends('country_id')
+    def _labels_get(self):
+        self.lbl_region = _('Region')
+        self.lbl_substate = _('Substate')

    @api.multi
    def onchange_state(self, state_id):


I'm pretty sure this kind of api is not available in v10. Please check that and use @api.onchange instead if possible.

yajo · 2017-05-31T14:27:22Z

base_location_nuts/views/res_partner_view.xml

-            <field name="nuts2_id"/>
-            <field name="nuts3_id"/>
-            <field name="nuts4_id"/>
+            <div width="1%" cell-class="oe_form_group_cell_label"


width=1% does something?

yajo · 2017-05-31T14:27:30Z

base_location_nuts/views/res_partner_view.xml

+                       options="{'no_open': True, 'no_create': True}"
+                       attrs="{'readonly': [('use_parent_address','=',True)]}"/>
+            </div>
+            <div width="1%" cell-class="oe_form_group_cell_label"


Same on width

yajo · 2017-05-31T14:27:40Z

base_location_nuts/views/res_partner_view.xml

-            <field name="nuts2_id"/>
-            <field name="nuts3_id"/>
-            <field name="nuts4_id"/>
+            <div width="1%" cell-class="oe_form_group_cell_label"


Same on width

yajo · 2017-05-31T14:27:47Z

base_location_nuts/views/res_partner_view.xml

+                       options="{'no_open': True, 'no_create': True}"
+                       attrs="{'readonly': [('use_parent_address','=',True)]}"/>
+            </div>
+            <div width="1%" cell-class="oe_form_group_cell_label"


Same on width.

yajo · 2017-05-31T14:28:14Z

base_location_nuts/views/res_partner_view.xml

        </filter>
    </field>
 </record>

-</data>
-</openerp>
+</odoo>


yajo · 2017-05-31T14:28:55Z

base_location_nuts/wizard/nuts_import.py

-from openerp import models, api, _
-from openerp.exceptions import Warning as UserError
+from odoo import _, api, models
+from odoo.exceptions import Warning


Import it as UserError; Warning is deprecated.

yajo · 2017-05-31T14:31:03Z

base_location_nuts/models/res_partner.py

-        return self._onchange_nuts(2)
+    region = fields.Many2one(comodel_name='res.partner.nuts',
+                             string="Region")
+    substate = fields.Many2one(comodel_name='res.partner.nuts',


Don't you need oldname in these cols? Or any other migrations?

pedrobaeza · 2017-06-08T16:57:44Z

Please rebase to get a green branch

pedrobaeza · 2017-06-08T23:52:28Z

Waiting fixes before reviewing

pedrobaeza · 2017-06-18T15:40:13Z

base_location_nuts/README.rst

pedrobaeza · 2017-06-18T15:40:34Z

base_location_nuts/__manifest__.py

        'views/res_partner_nuts_view.xml',
        'views/res_partner_view.xml',
        'wizard/nuts_import_view.xml',
        'security/ir.model.access.csv',
    ],
+    'images': [
+        'images/new_fields.png',
+    ],
    'author': 'Antiun Ingeniería S.L., '


Remove Antiun

pedrobaeza · 2017-06-18T15:40:41Z

base_location_nuts/__manifest__.py

    'author': 'Antiun Ingeniería S.L., '
+              'Tecnativa'


Comma at the end

pedrobaeza · 2017-06-18T15:40:47Z

base_location_nuts/__manifest__.py

              'Odoo Community Association (OCA)',
-    'website': 'http://www.antiun.com',
+    'website': 'http://www.tecnativa.com',


pedrobaeza · 2017-06-18T15:42:18Z

base_location_nuts/models/res_partner.py

+
+    @api.multi
+    def _onchange_nuts(self, level):
+        field = self["nuts%d_id" % level]


Please use getattr(self, "nuts%_id")

No, he is using the right thing. Attributes can get anything from the object, while keys can only get fields, so for security this is the recommended system to access fields with variable names, and even more when the variable comes from outside the method.

Please check slide 40 of https://www.odoo.com/es_ES/slides/slide/10-rules-for-safer-code-398

Well, this system can't be abused like the scenario of the slides. My fear is that this is not a documented way of accessing a field. The official way is with dot notation (and thus, getattr), but I see in the slides that is using this method, so go on...

Well, that presentation was given by @odony, so I guess that makes it somehow officially documented. Maybe a PR to docs would be good.

👍 let's make it more official: odoo/odoo@464fe4a

Thank your very much for the update on the documentation, Olivier!

Thanks, great little addition! 👏 👏

pedrobaeza · 2017-06-18T15:45:11Z

base_location_nuts/models/res_partner.py

+                self.nuts1_id = nuts1.id
+        return {
+            'domain': domain,
+        }

    @api.multi
    def onchange_state(self, state_id):


This method is not used anymore. Please find the equivalent one or declare a new unique onchange (def onchange_state_id_base_location_nuts(self):.

pedrobaeza · 2017-06-18T15:47:53Z

base_location_nuts/models/res_partner_nuts.py

+        'res.partner.nuts',
+        'parent_id',
+        "Children",
+        oldname="children")


There's no need of oldname in one2many fields, as they don't have a real column in the DB.

pedrobaeza · 2017-06-18T15:49:22Z

base_location_nuts/views/res_partner_view.xml

-                       options="{'no_open': True, 'no_create': True}"
-                       attrs="{'readonly': [('use_parent_address','=',True)]}"/>
-            </div>
+            <field name="nuts1_id"/>


Why do you remove the attrs attribute?

I didn't. That change comes from a previous commit (Tecnativa@eade50c)

pedrobaeza · 2017-06-18T15:50:06Z

base_location_nuts/wizard/nuts_import.py

-from openerp import models, api, _
-from openerp.exceptions import Warning
+from odoo import _, api, models
+from odoo.exceptions import Warning as UserError


Old alias. Now UserError is the valid one.

pedrobaeza · 2017-06-18T15:50:33Z

base_location_nuts/wizard/nuts_import.py

@@ -184,13 +181,14 @@ def run_import(self):
        nuts_to_delete = nuts_model.search(
            [('country_id', 'in', [x.id for x in self._countries.values()])])
        # Download NUTS in english, create or update
-        logger.info('Import NUTS 2013 English')
+        logger.info('Importing NUTS 2013 English')


Put ... at the end to indicate that is an operation that requires time.

pedrobaeza · 2017-06-20T16:11:06Z

Please increase test coverage for the onchanges and so on.

chienandalu · 2017-06-20T17:12:11Z

I was doing some test and was getting 404 from the NUTS Eurostat server. There's some issue with Ramon not accepting https request. It does it right on http though.

pedrobaeza · 2017-06-20T17:13:21Z

Well, change it then.

chienandalu · 2017-06-22T10:01:24Z

@yajo @pedrobaeza It's all green now

pedrobaeza

Remember to squash your commits before merging

chienandalu · 2017-06-23T07:00:27Z

Commits squashed

chienandalu · 2017-06-28T12:24:36Z

Yes, @pedrobaeza, my fault for copy-pasting and not passing flake locally prior to commit 😬
Can you review again, @yajo ?

pedrobaeza · 2017-06-28T12:28:49Z

Please put pre-commit linter in your git for avoiding these "surprises": https://github.com/OCA/maintainer-quality-tools/tree/master/git

chienandalu · 2017-06-28T12:40:48Z

@pedrobaeza Thanks!

yajo

Humm complex module, some suggestions to simplify it in some parts 😊

BTW, there are some security good practices that it's good to follow, see below.

Thanks for the good job!

yajo · 2017-07-03T12:55:40Z

base_location_nuts/models/res_partner.py

-            }
-            dict_recursive_update(result, changes)
+    def _onchange_nuts(self, level):
+        field = getattr(self, 'nuts%d_id' % level)


Do not use getattr, use self["nuts%d_id" % level] instead.

Se slides 40 and 41: https://www.odoo.com/es_ES/slides/slide/10-rules-for-safer-code-398

yajo · 2017-07-03T12:57:18Z

base_location_nuts/models/res_partner.py

+            self.country_id = country_id
+        if state_id and self.state_id != state_id:
+            self.state_id = state_id
+        if (level - 1) > 0:


😕 Why not just if level > 1?

yajo · 2017-07-03T12:58:48Z

base_location_nuts/models/res_partner.py

+        if state_id and self.state_id != state_id:
+            self.state_id = state_id
+        if (level - 1) > 0:
+            parent_id = getattr(field, 'parent_id')


Why not parent_id = field.parent_id?

yajo · 2017-07-03T13:00:32Z

base_location_nuts/models/res_partner.py

+            parent_id = getattr(field, 'parent_id')
+            if parent_id:
+                nuts_parent_level = 'nuts%d_id' % (level - 1)
+                parent_field = getattr(self, nuts_parent_level)


no getattr for accessing fields.

yajo · 2017-07-03T13:03:05Z

base_location_nuts/models/res_partner.py

+    def _onchange_nuts1_id(self):
+        return self._onchange_nuts(1)
+
+    @api.multi


All these api.multi can be dropped, now in v10 it's the default.

yajo · 2017-07-04T07:13:04Z

base_location_nuts/models/res_partner.py

+    def onchange_state_id_base_location_nuts(self):
+        if self.state_id:
+            # state = self.env['res.country.state'].browse(self.state_id)
+            result = {'value': {'country_id': self.state_id.country_id.id}}


self.country_id = self.state_id.country_id

yajo · 2017-07-04T07:14:25Z

base_location_nuts/models/res_partner.py

+                    self[field] = nuts_state
+                    result.setdefault("value", dict())
+                    result['value'][field] = nuts_state.id
+            return result


Remove lines 117-119, they are redundant. If you edit self under an onchange in the new api, you don't need to return a dict with values.

yajo · 2017-07-04T07:15:37Z

base_location_nuts/models/res_partner_nuts.py

-    parent_left = fields.Integer('Parent Left', select=True)
-    parent_right = fields.Integer('Parent Right', select=True)
+    child_ids = fields.One2many(comodel_name='res.partner.nuts',
+                                inverse_name='parent_id', string='Children')


Add oldname='children'

yajo · 2017-07-04T07:18:09Z

base_location_nuts/wizard/nuts_import.py

-                logger.info('Reading level=%s, id=%s' %
-                            (node.get('idLevel', 'N/A'),
-                             node.get('id', 'N/A')))
+                logger.debug(


Wrong indentation here?

yajo · 2017-07-04T07:20:09Z

base_location_nuts/wizard/nuts_import_view.xml

@@ -36,7 +35,7 @@
 <menuitem action="nuts_import_action"
          id="nuts_import_menu"
          name="Import NUTS 2013"
-          parent="base.menu_localisation"
+          parent="sales_team.menu_localisation"


It's sad to have to add the sales_team dependency just for putting the menu somewhere. Do you think there's another place where we can put it and skip this big dependency just for that? 🤔 Or maybe we really need this for other reason?

This family of modules fall into that menu historically. It's no really needed at all for anything else. There aren't much options on base views anyway... Ideas are welcomed 😄

Well, on base_location there is another menu defined: https://github.com/OCA/partner-contact/blob/10.0/base_location/views/better_zip_view.xml#L61

Right, @pedrobaeza , but this module doesn't depend on that one 😕 It only depended on base before...

Uhm, OK, let's go this way then.

chienandalu · 2017-07-05T14:53:01Z

@yajo Thanks for your comments :) I've tried to implement them all.

yajo · 2017-07-06T08:13:48Z

base_location_nuts/wizard/nuts_import.py

-                    'Reading level=%s, id=%s',
-                    node.get('idLevel', 'N/A'),
-                    node.get('id', 'N/A'))
+                logger.debug('Reading level=%s, id=%s',


The indentation problem I was referring to is that these are 8 spaces instead of the usual 4. (I mean from the line 195 to 196).

yajo · 2017-07-06T08:16:26Z

@chienandalu this looks almost perfect now. Could you please fix that little comment and squash all the latest migration-related commits to proceed with merge?

This makes NUTS labels and levels to be stored in the res.country object. Now creating l10n submodules is a piece of cake! Relational fields now follow guidelines on naming. Old name attribute used for backwards compatibility wherever needed. Also some methods have been renamed, and refactored to be smarter. Most cases l10n modules will just need to fill the res.contry table, and regions and substates domains will work out of the box. In case you still need to overwrite any method, splitting in smaller methods makes it easier too. Oh! And no need for recursive dictionary updates.

chienandalu · 2017-07-06T08:23:51Z

@yajo Fixed and commits squashed

yajo requested changes May 31, 2017

View reviewed changes

rafaelbn added this to the 10.0 milestone May 31, 2017

rafaelbn added the needs review label May 31, 2017

chienandalu force-pushed the 10.0-mig-base_location_nuts branch 2 times, most recently from 36bf980 to e0a675b Compare May 31, 2017 16:30

chienandalu force-pushed the 10.0-mig-base_location_nuts branch from e0a675b to c92125d Compare June 8, 2017 17:48

This was referenced Jun 13, 2017

[MIG] crm_location_nuts: Migration to 10.0 OCA/crm#161

Merged

[MIG] l10n_de_location_nuts: Migration to 10.0 OCA/l10n-germany#22

Merged

OCA deleted a comment from yajo Jun 18, 2017

pedrobaeza requested changes Jun 18, 2017

View reviewed changes

pedrobaeza approved these changes Jun 23, 2017

View reviewed changes

chienandalu force-pushed the 10.0-mig-base_location_nuts branch from b390999 to 5980b60 Compare June 23, 2017 06:59

chienandalu mentioned this pull request Jun 26, 2017

[MIG] l10n_es_location_nuts: Migration to 10.0 OCA/l10n-spain#527

Merged

1 task

yajo requested changes Jul 4, 2017

View reviewed changes

yajo reviewed Jul 6, 2017

View reviewed changes

yajo approved these changes Jul 6, 2017

View reviewed changes

OCA Transbot updated translations from Transifex

080b9e8

yajo and others added 9 commits July 6, 2017 10:21

Return dict() to make it easier for submodules to add domains.

aee310f

Fix KeyError: 'substate_id_level'.

83860f0

Use separate and explicit fields for each NUTS level

860e42c

Make method recieve the level as int.

600187d

Fix linter.

daeb62f

Fix typo.

77f72be

OCA Transbot updated translations from Transifex

fee65a0

[MIG] base_location_nuts: Migration to 10.0

f89cbf5

chienandalu force-pushed the 10.0-mig-base_location_nuts branch from d260433 to f89cbf5 Compare July 6, 2017 08:22

yajo merged commit 1a0000c into OCA:10.0 Jul 6, 2017

yajo deleted the 10.0-mig-base_location_nuts branch July 6, 2017 09:02

pedrobaeza mentioned this pull request Jul 6, 2017

Migration to version 10.0 #309

Closed

38 tasks

[MIG] base_location_nuts: Migration to 10.0 #432

[MIG] base_location_nuts: Migration to 10.0 #432

Conversation

chienandalu commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobaeza commented Jun 8, 2017

pedrobaeza commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobaeza commented Jun 20, 2017

chienandalu commented Jun 20, 2017

pedrobaeza commented Jun 20, 2017

chienandalu commented Jun 22, 2017

pedrobaeza left a comment

Choose a reason for hiding this comment

chienandalu commented Jun 23, 2017

chienandalu commented Jun 28, 2017

pedrobaeza commented Jun 28, 2017

chienandalu commented Jun 28, 2017

yajo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chienandalu commented Jul 5, 2017

Choose a reason for hiding this comment

yajo commented Jul 6, 2017

chienandalu commented Jul 6, 2017