[IMP] sales_security_team: revert changes to recover esencial functionality #3747

matiasperalta1 · 2025-06-06T19:53:53Z

No description provided.

legalsylvain · 2025-06-06T20:28:13Z

Hi. Could you change message to explain the changes ?

Thanks !

rousseldenis · 2025-06-30T14:46:05Z

sales_team_security/models/ir_rule.py

        user = self.env.user
-        group_my_records = "sales_team.group_sale_salesman"
-        group_all_records = "sales_team.group_sale_salesman_all_leads"
+        group1 = "sales_team.group_sale_salesman"


@matiasperalta1 IMHO, changing variables names from explicit to not explicit (groupX) is a drawback (and is not needed).

rousseldenis · 2025-06-30T14:47:13Z

sales_team_security/models/ir_rule.py

                    ("id", "=", user.partner_id.id),
                ]
-                domain_user = [("user_id", "in", [user.id, False])]
-                extra_domain = expression.OR([domain_followers, domain_user])


IMHO, using AND() and OR() is better in terms of readability.

rousseldenis · 2025-06-30T14:55:18Z

sales_team_security/models/ir_rule.py

+                extra_domain = [
                    "|",
                    ("message_partner_ids", "in", user.partner_id.ids),
+                    "|",


I think this is not useful as there is only one argument after this 'OR'

rousseldenis · 2025-06-30T14:57:39Z

sales_team_security/models/ir_rule.py

+                        ("team_id", "=", False),
+                        ("team_id", "=", user.sale_team_id.id),
+                    ]
+


If I summarize the change here:

If user is a manager, can see all contacts of a team (even with user assigned)

either, user can see only its contacts or contacts with no assignation and (no team or same team).

This should be reflected in documentation

rousseldenis · 2025-06-30T15:00:22Z

sales_team_security/models/res_partner.py


    # add indexes for better performance on record rules
    user_id = fields.Many2one(index=True)
+    team_id = fields.Many2one(


You introduce a new functionality here. Maybe this should be done in another module.

As @matiasperalta1 mentioned, and as I commented in the migration PR, Odoo removed this field in this commit odoo/odoo@d11691c
I'm not sure if reintroducing this field is good for the user experience.
For example, if I have the following settings in the sales teams:

With this new field, I can encounter inconsistencies like in the following image: the sales team is selected independently of the user, and the user is not a member of that team.

rousseldenis · 2025-06-30T15:03:37Z

sales_team_security/models/res_partner.py

+        if self.parent_id and self.parent_id.team_id and not self.team_id:
+            self.team_id = self.parent_id.team_id.id
+        if self.parent_id and self.parent_id.user_id and not self.user_id:
+            self.user_id = self.parent_id.user_id.id


IMHO, this is totally out of scope. Moreover this is already done by: https://github.com/odoo/odoo/blob/18.0/odoo/addons/base/models/res_partner.py#L397

Those changes should be done in computes.

rousseldenis · 2025-06-30T15:04:33Z

sales_team_security/models/res_partner.py

        """Sync followers in commercial partner + delivery/invoice contacts."""
-        for record in self.commercial_partner_id:
-            followers = (record.child_ids + record).user_id.partner_id
+        for record in self.mapped("commercial_partner_id"):


Those changes are not necessary as Odoo already include a mapped() when you access a relation through a multi recordset

nicolascol · 2025-07-04T13:35:26Z

Hi!

First, in OCA’s runboat, the sales_team_security module is not working. You can test it as follows:

Create a sales team with both "admin" and "demo" in the same team.
Assign a res.partner to "admin" as the "salesperson".
Assign the "demo" user to the sales group "see documents of own team only".
Log in as "demo".
The user is not able to see the corresponding res.partner, although they should.

Second, we have reintroduced the sales team field in res.partner because, in cases where a salesperson belongs to multiple sales teams, we don’t want the res.partner to be visible across all teams, only in the one it's assigned to. (as it is v17). One thing to keep in mind is that Odoo deprecated the sales team in res.partner because they didn't use it.

I’m sharing a video in Spanish, but you can use YouTube’s automatic translator.
https://www.youtube.com/watch?v=N2bgoG_EXUE
At the end of the video, you can see a small bug in this PR, which we're already working on.

@rousseldenis @pedrobaeza @matiasperalta1 @carlos-lopez-tecnativa

carlos-lopez-tecnativa

Please, check the CI

carlos-lopez-tecnativa · 2025-07-08T12:21:53Z

sales_team_security/models/res_partner.py


    # add indexes for better performance on record rules
    user_id = fields.Many2one(index=True)
+    team_id = fields.Many2one(


As @matiasperalta1 mentioned, and as I commented in the migration PR, Odoo removed this field in this commit odoo/odoo@d11691c
I'm not sure if reintroducing this field is good for the user experience.
For example, if I have the following settings in the sales teams:

With this new field, I can encounter inconsistencies like in the following image: the sales team is selected independently of the user, and the user is not a member of that team.

nicolascol · 2025-07-10T10:53:13Z

In previous versions, it was possible to select a different team than the salesperson's team in res.partner.
I understand that Odoo removed the field because it wasn't used much (it was a field visible only in debug mode) and it caused confusion for them in identifying which team a salesperson belonged to. On the other hand, with the "sales_team_security" module, our goal is to identify the customer.

Regarding the development, we have two options:

We can make the team of the salesperson compute automatically in res.partner. If the salesperson belongs to more than one team, we take the one with the lowest sequence.
We fix the display for the "Team Documents Only" group. We do not add team_id in res.partner and clarify in the README that the module, starting from version 18, may be non-functional if "multi-teams" is enabled.

We would prefer to go with option 1 because we believe it reflects the essence of the module, and several of our clients would be affected if we went with option 2.

pedrobaeza · 2025-07-10T13:45:10Z

IMO, team_id field shouldn't be restored in res.partner, but the security rules work on res.user~team_ids.

…nality

matiasperalta1 force-pushed the 18.0-h-92487-mnp branch 3 times, most recently from 3e016de to be27972 Compare June 9, 2025 20:28

matiasperalta1 force-pushed the 18.0-h-92487-mnp branch 5 times, most recently from 3b70e92 to 149cdcd Compare June 19, 2025 18:07

rousseldenis added enhancement needs review labels Jun 30, 2025

rousseldenis added this to the 18.0 milestone Jun 30, 2025

rousseldenis requested changes Jun 30, 2025

View reviewed changes

carlos-lopez-tecnativa reviewed Jul 8, 2025

View reviewed changes

matiasperalta1 force-pushed the 18.0-h-92487-mnp branch 4 times, most recently from ac8d1eb to ae393c0 Compare July 22, 2025 14:57

[IMP] sales_security_team: revert changes to recover esencial functio…

a5b01a3

…nality

matiasperalta1 force-pushed the 18.0-h-92487-mnp branch from ae393c0 to a5b01a3 Compare July 22, 2025 17:53

etobella force-pushed the 18.0 branch from 4e676b6 to a6df617 Compare October 8, 2025 08:27

Uh oh!

[IMP] sales_security_team: revert changes to recover esencial functionality #3747

Are you sure you want to change the base?

[IMP] sales_security_team: revert changes to recover esencial functionality #3747

Conversation

matiasperalta1 commented Jun 6, 2025

Uh oh!

legalsylvain commented Jun 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolascol commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlos-lopez-tecnativa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolascol commented Jul 10, 2025

Uh oh!

pedrobaeza commented Jul 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nicolascol commented Jul 4, 2025 •

edited

Loading