10.0 #3963 partner relation in tab port #459

daramousk · 2017-08-16T15:14:57Z

Port of module partner_relation_in_tab to v10

hbrunn · 2017-08-16T15:19:23Z

before I do an in-depth review:

use short headers as in https://github.com/OCA/maintainer-tools/blob/master/template/module/__init__.py
replace the description key by OCA's readme template https://github.com/OCA/maintainer-tools/blob/master/template/module/README.rst
add tests, we want this to be covered 100%
travis will probably come back with some complains, fix them too

NL66278 · 2017-09-04T11:26:45Z

I would propose a new name for the module, as it is an extension of partner_multi_relation (that was renamed from partner_relations). So to keep the link with partner_multi_relation, of which this is an extension, I would propose partner_multi_relation_tabs.

NL66278 · 2017-09-04T11:35:17Z

partner_relations_in_tab/model/res_partner.py

+        return self.env.cr.fetchall()
+
+    def _create_relation_type_tab(self, rel_type, inverse, field_names):
+        '''Create an xml node containing the relation's tab to be added to the


Double quotes for function docstring!!

NL66278 · 2017-09-04T11:46:29Z

partner_relations_in_tab/model/res_partner.py

+                view.append(etree.Element('field', name='id',
+                                          invisible='True'))
+                field_names.append('id')
+            for rel_type in own_tab_types:


Code can be mutch simplified if you just pass rel_type to underlying methods. No need then for separate function calls with inverse = True of inverse = False. inverse is translated back to left or right in underlying methods anyway, which just confuses things. (I know you just migrated the old code, and I think this was there as well, but now a good opportunity to clean up).

NL66278 · 2017-09-04T11:54:08Z

partner_relations_in_tab/model/res_partner_relation_type.py

+    own_tab_left = fields.Boolean('Show in own tab', default=False)
+    own_tab_right = fields.Boolean('Show in own tab', default=False)
+
+    def _update_res_partner_fields(self):


Thinking about performance I really do not like the way this is implemented. With any tab added or deleted, all the tabfields, also those not touched at all, are created from scratch. I should not be to difficult (and probably even less code) to just check for changes in the own_tab_left field or the own_tab_right field, and create/delete the tabfields accordingly.

@NL66278 How can this be done? Keep in mind that that self here is a recordset not the model so it does not search on all the records, only on those who had their own_tab_left and/or own_tab_right changed.

NL66278 · 2017-09-04T12:27:46Z

partner_relations_in_tab/model/res_partner.py

+class ResPartner(Model):
+    _inherit = 'res.partner'
+
+    def _get_relation_ids_select(self, field_name, arg):


This function is not needed anymore. In the old version of partner_relations, all relations where shown in a tab. This override was to exclude the relations from the main tab, that were shown in their own tab.

NL66278 · 2017-09-04T12:42:37Z

Somehow the names for the relations in the tab are shown including their ID. The id (and comma separator) should not be shown. I tried a screenprint, but github fails in the upload.

NL66278 · 2017-09-04T12:58:33Z

If I add a relation on a tab, the inverse relation is somehow not created or never shown, on the connected relation.

daramousk · 2017-09-05T13:20:45Z

@NL66278 The inverse relation is not shown probably because the following domain assembled and in place:

			<page string="name" attrs="{'invisible': ['|', ('id', '=', False), ('is_company', '=', False)]}" modifiers="{'invisible': ['|', ['id', '=', false], ['is_company', '=', false]]}">
				<field context="{'default_type_id': 1, 'default_left_partner_id': id, 'active_test': False}" name="relation_ids_own_tab_1_left" modifiers="{}"/>
			</page>

NL66278 · 2017-09-11T07:55:30Z

@daramousk I found the problem with the inverse relation not showing up. The problem was limited to symmetrical relations. When a relation is symmetrical, some fields have to be the same for left and right. In the main module this is done here: https://github.com/OCA/partner-contact/blob/10.0/partner_multi_relation/models/res_partner_relation_type.py#L88 You have to override this method and fill own_tab_right with the value of own_tab_left.

NL66278 · 2017-09-11T09:31:39Z

@daramousk One more thing: in the past we often had directories like model (singular), view (singular). This should really be models (plural) and views (plural).

daramousk · 2017-09-12T11:31:03Z

@NL66278 Both implemented

NL66278 · 2017-09-12T11:39:43Z

partner_relations_in_tab/models/res_partner_relation_type.py

+    def onchange_is_symmetric(self):
+        """Set right side to left side if symmetric."""
+        if self.is_symmetric:
+            self.update({


Like always, first call super, then add just the update for own_tab_right. Who knows what other modules will add symmetrical fields?

NL66278 · 2017-09-12T11:41:36Z

@daramousk So we are left with:

Rename of the module
Solving the issue of relations shown with not only their name, but also their id
The comment just made on symmetrical fields and calling super method

daramousk · 2017-09-12T12:48:30Z

@NL66278 Implemented all of three points

NL66278 · 2017-09-12T14:25:56Z

@daramousk On testing the symmetrical stuff still does not work. (Did you test it?) The error is already in the existing module partner_multi_relation:
When the field is_symmetric is changed, values are copied from left to right fields. Great. But after that the *_right fields should be kept synchronized with the *_left fields. This can most easily be done by a routine called from the create and write methods. If is_symmetric is in vals (and True), or if symmetric is not in vals, but true in the existing record, go through all the keys in vals ending in _left, and copy their values to _right.
As long as other modules keep the convention (maybe put a comment in the code) of putting _left and _right at the end of the field-names, this will work, and the onchange handler for is_symmetric want be needed any more.

NL66278 · 2017-09-13T19:18:18Z

@daramousk The synchronization between left and right will in many cases not be done. If you have an existing record and just change the is_symmetric flag, the *_right fields will not be updated, because the *-left fields are not in vals. The easiest way to do this is to do the update after the super.write or second create. Normally I am a bit allergic for extra database actions, because this will result in a second update, or an update right after insert, but the frequency of these actions should be very low, and in programming it is much simpler.

This is a very good thing to put in a test case.

By the way the test_person_to_person_relation method contains also tests for company to person relations. Make separate test cases and give them appropiate names.

Succes!

daramousk · 2017-09-21T08:37:49Z

@NL66278 Everything pushed here as well, with new changes to increase code coverage

NL66278 · 2017-09-21T10:32:03Z

partner_multi_relation/models/res_partner_relation_type_selection.py

@@ -1,5 +1,5 @@
 # -*- coding: utf-8 -*-
-# © 2014-2016 Therp BV <http://therp.nl>
+# © 2017 Therp BV <http://therp.nl>


All the copyrights for existing files should be 2014-2017. Only the really new files should be 2017. Please check this everywhere.

NL66278 · 2017-09-21T10:36:22Z

partner_multi_relation_tabs/tests/test_relations.py

+        res_partner_relation_type_model = self.env['res.partner.relation.type']
+        res_partner_relation_type_selection_model = self.env[
+            'res.partner.relation.type.selection']
+        p2p_rel = res_partner_relation_type_model.create(


Please for all create and write calls with multiple fields do:

some_model.create({ 'first_field': 'first_value', 'second_field': 'second_value', })

So put ( and { on one line,
then indent four spaces on all subsequent lines
then }) on last line.

NL66278 · 2017-09-21T10:40:49Z

In the test for multiple relations in tabs I miss:

A test to see if the new relation is in the special One2many field created
A test to see if an existing type where you have own tab for the left and not for the right, if this changes after you change to is_symmetric.
And please use the assertSomething methods to test if everything OK. If need be just assertTrue or assert False. Compare with existing unit tests.

NL66278 · 2017-09-28T13:33:34Z

Copyright, now you changed the copyright date in a lot of files that do not have any other change. Please revert those, so the files do not show up in the Pull Request for nothing.

NL66278 · 2017-09-28T13:35:04Z

And the test in the ..._tabs module fail. Please check and correct

daramousk · 2017-09-29T10:22:32Z

@NL66278 Tests fixed. As for the changed files, all of them have been changed not only because of the dates but because they had long headers, see Holger's first comment.

NL66278 · 2017-09-29T11:49:21Z

@daramousk Thanks a lot. Now squash all the commits into one and it would be ready to merge.

daramousk · 2017-09-29T12:40:50Z

@NL66278 Squashed everything

NL66278

👍 Tested and LGTM

gfcapalbo · 2017-10-05T09:18:17Z

partner_multi_relation/__init__.py

@@ -1,5 +1,5 @@
 # -*- coding: utf-8 -*-
-# © 2013-2016 Therp BV <http://therp.nl>


Atomize these MRs make one for partner_multi_relation v10 and one for partner_multi_relation_in_tab v10.
Put on your second MR the note that it depends on partner_multi_relation being merged.

@gfcapalbo I do not see the need for this, as it is all part of one functional package.

gfcapalbo · 2017-10-05T09:19:17Z

partner_multi_relation/models/res_partner_relation_type.py

-                'contact_type_right': self.contact_type_left,
-                'partner_category_right': self.partner_category_left,
-            })
-


What where the reasons to remove the onchange and replace it with a write/create overwrite ?

@gfcapalbo Using write/create means that values will be computed the correct way, even if relations inserted/modified through back-end. For instance on import.

gfcapalbo · 2017-10-05T09:47:03Z

partner_multi_relation/models/res_partner_relation_type.py

+        name = vals.get('name')
+        if name:
+            right_vals['name_inverse'] = name
+        left_keys = filter(lambda key: key.endswith('_left'), vals.keys())


Obsessive check , but possible scenario
will cause problems if somebody has a field that ends with _left in vals other than the ones we want. filtering explicitly contact_type_left and partner_category_left looks safer.

@gfcapalbo Doing this in a generic way is needed to make sure that when extention modules add extra _left and _right fields, thses will be covered automatically. So this is quit intentional.

ok. i had forwarned it was obsessive,
Second proposal. modularize this in a function "_get_left_keys" where the extending modules can add their left keys and protect against my . albeit , rare scenario.

gfcapalbo · 2017-10-05T09:55:13Z

partner_multi_relation/models/res_partner_relation_type.py

    @api.multi
    def write(self, vals):
        """Handle existing relations if conditions change."""
        self.check_existing(vals)
+        is_symmetric = vals.get('is_symmetric')
+        if is_symmetric or self.is_symmetric and is_symmetric is not False:


this covers is_symmetric True or None in vals, Ok.

gfcapalbo · 2017-10-05T09:57:48Z

partner_multi_relation/__init__.py

@@ -1,5 +1,5 @@
 # -*- coding: utf-8 -*-


Every MR should be for a single module, increases probability of merge. Divide it in two for the two modules

ignore. one functional package

gfcapalbo · 2017-10-05T10:07:38Z

partner_multi_relation/__init__.py

@@ -1,5 +1,5 @@
 # -*- coding: utf-8 -*-
-# © 2013-2016 Therp BV <http://therp.nl>


gfcapalbo · 2017-10-05T10:11:29Z

partner_multi_relation/models/res_partner_relation_type.py

+        name = vals.get('name')
+        if name:
+            right_vals['name_inverse'] = name
+        left_keys = filter(lambda key: key.endswith('_left'), vals.keys())


ok. i had forwarned it was obsessive,
Second proposal. modularize this in a function "_get_left_keys" where the extending modules can add their left keys and protect against my . albeit , rare scenario.

NL66278 · 2017-10-06T13:53:16Z

I found a nasty bug in the override of fields_view_get of res_partner. You will need to cherrypick the commit added to this branch: https://github.com/nl66278/partner-contact/tree/10.0-partner_multi_relation_tabs-apimodel For some reason I cannot make a PR to your branch, The commit basically puts @api.model before the method declaration.

daramousk · 2017-10-06T14:46:07Z

@NL66278 Rebased and squashed

NL66278 · 2018-04-13T09:03:31Z

@daramousk You might want to close this PR in favour of this one (building on your work): #498. Please review also.

hbrunn added this to the 10.0 milestone Aug 16, 2017

hbrunn added the needs review label Aug 16, 2017

NL66278 reviewed Sep 4, 2017

View reviewed changes

daramousk force-pushed the 10.0-#3963-partner-relation-in-tab-port branch from dde2489 to 8960d7d Compare September 5, 2017 13:10

NL66278 reviewed Sep 12, 2017

View reviewed changes

daramousk force-pushed the 10.0-#3963-partner-relation-in-tab-port branch 2 times, most recently from f84e871 to bd357c3 Compare September 13, 2017 15:10

NL66278 reviewed Sep 21, 2017

View reviewed changes

daramousk force-pushed the 10.0-#3963-partner-relation-in-tab-port branch from 36b9450 to 3e9e327 Compare September 29, 2017 12:15

NL66278 approved these changes Sep 29, 2017

View reviewed changes

gfcapalbo reviewed Oct 5, 2017

View reviewed changes

[PORT] Ports partner_relations_in_tab to version 10.0

a343225

daramousk force-pushed the 10.0-#3963-partner-relation-in-tab-port branch from 6127c35 to a343225 Compare October 6, 2017 14:43

daramousk closed this Apr 18, 2018

		@@ -1,5 +1,5 @@
		# -- coding: utf-8 --
		# © 2013-2016 Therp BV <http://therp.nl>

10.0 #3963 partner relation in tab port #459

10.0 #3963 partner relation in tab port #459

Conversation

daramousk commented Aug 16, 2017

hbrunn commented Aug 16, 2017

NL66278 commented Sep 4, 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

NL66278 commented Sep 4, 2017

NL66278 commented Sep 4, 2017

daramousk commented Sep 5, 2017

NL66278 commented Sep 11, 2017

NL66278 commented Sep 11, 2017

daramousk commented Sep 12, 2017

Choose a reason for hiding this comment

NL66278 commented Sep 12, 2017

daramousk commented Sep 12, 2017

NL66278 commented Sep 12, 2017

NL66278 commented Sep 13, 2017

daramousk commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NL66278 commented Sep 21, 2017

NL66278 commented Sep 28, 2017

NL66278 commented Sep 28, 2017

daramousk commented Sep 29, 2017

NL66278 commented Sep 29, 2017

daramousk commented Sep 29, 2017

NL66278 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

gfcapalbo Oct 5, 2017 • edited Loading

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

NL66278 commented Oct 6, 2017 • edited Loading

daramousk commented Oct 6, 2017

NL66278 commented Apr 13, 2018

gfcapalbo Oct 5, 2017 •

edited

Loading

NL66278 commented Oct 6, 2017 •

edited

Loading