Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
matiasperalta1 wants to merge 1 commit into
base: 18.0
Choose a base branch
from
+159 −22
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions sales_team_security/models/ir_rule.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,35 @@ def _compute_domain(self, model_name, mode="read"):
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"
Copy link
Contributor

@rousseldenis rousseldenis Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

group2 = "sales_team_security.group_sale_team_manager"
group3 = "sales_team.group_sale_salesman_all_leads"
if model_name == "res.partner" and not self.env.su:
if user.has_group(group_my_records) and not user.has_group(
group_all_records
):
domain_followers = [
# if user.has_group(group_my_records) and not user.has_group(
# group_all_records
# ):
# domain_followers = [
if user.has_group(group1) and not user.has_group(group3):
extra_domain = [
"|",
("message_partner_ids", "in", user.partner_id.ids),
"|",
Copy link
Contributor

@rousseldenis rousseldenis Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@rousseldenis rousseldenis Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# domain_user = [("user_id", "in", [user.id, False])]
# extra_domain = expression.OR([domain_followers, domain_user])
if user.has_group(group2):
# Ver todos los contactos de su equipo
team_user_ids = user.sale_team_id.member_ids.ids
extra_domain += [
("user_id", "in", team_user_ids + [False]),
]
else:
# Ver solo propios y sin asignar
extra_domain += [
("user_id", "in", [user.id, False]),
]

Copy link
Contributor

@rousseldenis rousseldenis Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

extra_domain = expression.normalize_domain(extra_domain)
res = expression.AND([extra_domain] + [res])
return res
71 changes: 70 additions & 1 deletion sales_team_security/models/res_partner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
# License AGPL-3 - See http://www.gnu.org/licenses/agpl-3.0.html


from lxml import etree

from odoo import api, fields, models


Expand All @@ -11,6 +13,62 @@ class ResPartner(models.Model):

# add indexes for better performance on record rules
user_id = fields.Many2one(index=True)
# team_id = fields.Many2one(
# "crm.team",
# string="Sales Team",
# compute="_compute_team_id",
# precompute=True, # avoid queries post-create
# ondelete="set null",
# readonly=False,
# store=True,
# index=True,
# )

@api.model
def get_view(self, view_id=None, view_type="form", **options):
"""
Patch view to inject the default value for the team_id and user_id.
"""
# FIXME: Use base_view_inheritance_extension when available
res = super().get_view(view_id, view_type, **options)
if view_type == "form":
eview = etree.fromstring(res["arch"])
xml_fields = eview.xpath("//field[@name='child_ids']")
if xml_fields:
context_str = (
xml_fields[0]
.get("context", "{}")
.replace(
"{",
"{'default_user_id': user_id,",
1,
)
)
xml_fields[0].set("context", context_str)
res["arch"] = etree.tostring(eview)
return res

@api.onchange("parent_id")
def _onchange_parent_id_sales_team_security(self):
"""If assigning a parent partner and the contact doesn't have
team or salesman, we put the parent's one (if any).
"""
if self.parent_id and self.parent_id.user_id and not self.user_id:
self.user_id = self.parent_id.user_id.id
Copy link
Contributor

@rousseldenis rousseldenis Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# @api.onchange("user_id")
# def _onchange_user_id_sales_team_security(self):
# if self.user_id.sale_team_id:
# self.team_id = self.user_id.sale_team_id

# @api.depends("parent_id")
# def _compute_team_id(self):
# for partner in self.filtered(
# lambda partner: not partner.team_id
# and partner.company_type == "person"
# and partner.parent_id.team_id
# ):
# partner.team_id = partner.parent_id.team_id

def _remove_key_followers(self, partner):
for record in self.mapped("commercial_partner_id"):
Expand All @@ -23,7 +81,7 @@ def _remove_key_followers(self, partner):
def _add_followers_from_salesmen(self):
"""Sync followers in commercial partner + delivery/invoice contacts."""
for record in self.commercial_partner_id:
followers = (record.child_ids + record).user_id.partner_id
followers = (record.child_ids + record).mapped("user_id.partner_id")
# Look for delivery and invoice addresses
childrens = record.child_ids.filtered(
lambda x: x.type in {"invoice", "delivery"}
Expand All @@ -35,6 +93,7 @@ def create(self, vals_list):
"""Sync followers on contact creation."""
records = super().create(vals_list)
records._add_followers_from_salesmen()
# records._sync_team_id_to_children()
return records

def write(self, vals):
Expand All @@ -50,4 +109,14 @@ def write(self, vals):
result = super().write(vals)
if "user_id" in vals or vals.get("type") in {"invoice", "delivery"}:
self._add_followers_from_salesmen()
# if "user_id" in vals or "team_id" in vals:
# self._sync_team_id_to_children()
return result

# def _sync_team_id_to_children(self):
# for parent in self:
# if parent.child_ids:
# for child in parent.child_ids:
# child.team_id = parent.team_id
# if parent.user_id:
# child.user_id = parent.user_id
32 changes: 17 additions & 15 deletions sales_team_security/tests/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ def setUpClass(cls):
"crm_team_id": cls.team.id,
}
)
cls.partner = cls.env["res.partner"].create({"name": "Test partner"})
cls.partner = cls.env["res.partner"].create(
{"name": "Test partner", "team_id": cls.team.id}
)
cls.partner_child_1 = cls.env["res.partner"].create(
{"name": "Child 1", "parent_id": cls.partner.id}
)
Expand All @@ -51,13 +53,12 @@ def setUpClass(cls):
cls.check_permission_subscribe = False

def _check_permission(self, salesman, team, expected):
vals = {"user_id": salesman.id if salesman else salesman}
# Check if the record has a team_id field
# This is needed for the test to work with different models
# such as sales_team_security_crm and sales_team_security_sale modules.
if "team_id" in self.record._fields:
vals["team_id"] = team.id if team else team
self.record.write(vals)
self.record.write(
{
"user_id": salesman.id if salesman else salesman,
"team_id": team.id if team else team,
}
)
domain = [("id", "=", self.record.id)]
if (
self.check_permission_subscribe
Expand All @@ -70,11 +71,12 @@ def _check_permission(self, salesman, team, expected):

@mute_logger("odoo.models.unlink")
def _check_whole_permission_set(self, extra_checks=True):
is_res_partner = self.record._name == "res.partner"
self._check_permission(False, False, True)
self._check_permission(self.user, False, True)
self._check_permission(self.user2, False, False)
self._check_permission(False, self.team, True)
if extra_checks:
self._check_permission(False, self.team2, False)
self._check_permission(self.user, self.team, True)
self._check_permission(self.user, self.team2, True)
self._check_permission(self.user2, self.team2, False)
Expand All @@ -85,19 +87,19 @@ def _check_whole_permission_set(self, extra_checks=True):
]
self._check_permission(False, False, True)
self._check_permission(self.user, False, True)
if not is_res_partner:
self._check_permission(self.user2, False, True)
self._check_permission(self.user2, False, True)
self._check_permission(False, self.team, True)
if extra_checks:
self._check_permission(False, self.team2, False)
self._check_permission(self.user, self.team, True)
if is_res_partner:
if self.record._name == "res.partner":
self.check_permission_subscribe = True
self._check_permission(self.user, self.team2, True)
self.check_permission_subscribe = False
else:
self._check_permission(self.user, self.team2, True)
self._check_permission(self.user2, self.team2, False)
if not is_res_partner:
self._check_permission(self.user2, self.team, True)
self._check_permission(self.user2, self.team, True)
# Add to group "See all leads"
self.user.groups_id = [
(4, self.env.ref("sales_team.group_sale_salesman_all_leads").id)
Expand All @@ -112,7 +114,7 @@ def _check_whole_permission_set(self, extra_checks=True):
self._check_permission(self.user2, self.team2, True)
self._check_permission(self.user2, self.team, True)
# Regular internal user
if extra_checks and is_res_partner:
if extra_checks:
self.user.groups_id = [(6, 0, [self.env.ref("base.group_user").id])]
self._check_permission(False, False, True)
self._check_permission(self.user, False, True)
Expand Down
35 changes: 35 additions & 0 deletions sales_team_security/tests/test_sales_team_security.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
# Copyright 2021 Tecnativa - Víctor Martínez
# License AGPL-3 - See http://www.gnu.org/licenses/agpl-3.0.html

from lxml import etree

from ..hooks import post_init_hook
from .common import TestCommon


Expand All @@ -15,11 +17,35 @@ def setUpClass(cls):
cls.record = cls.partner

def test_onchange_parent_id(self):
contact = self.env["res.partner"].create(
{"name": "Test contact", "parent_id": self.partner.id}
)
contact._onchange_parent_id_sales_team_security()
self.assertEqual(contact.team_id, self.team)
contact2 = self.env["res.partner"].create(
{"name": "Test contact", "parent_id": self.partner2.id}
)
contact2._onchange_parent_id_sales_team_security()
self.assertEqual(contact2.user_id, self.user)

def test_onchange_user_id(self):
contact = self.env["res.partner"].create(
{
"name": "Test contact",
"user_id": self.user.id,
}
)
contact._onchange_user_id_sales_team_security()
self.assertEqual(contact.team_id, self.team)

def test_assign_contacts_team(self):
contact = self.env["res.partner"].create(
{"name": "Test contact", "parent_id": self.partner.id, "team_id": False}
)
post_init_hook(self.env)
contact.env.invalidate_all()
self.assertEqual(contact.team_id, self.partner.team_id)

def test_change_user_id_partner(self):
self.partner.write({"user_id": self.user.id})
self.assertIn(self.user_partner, self.partner.message_partner_ids)
Expand All @@ -43,6 +69,15 @@ def test_change_user_id_partner_child_1(self):
self.assertNotIn(self.user_partner, self.partner_child_2.message_partner_ids)
self.assertIn(self.user2_partner, self.partner_child_2.message_partner_ids)

def test_partner_fields_view_get(self):
res = self.env["res.partner"].get_view(
view_id=self.ref("base.view_partner_form")
)
eview = etree.fromstring(res["arch"])
xml_fields = eview.xpath("//field[@name='child_ids']")
self.assertTrue(xml_fields)
self.assertTrue("default_team_id" in xml_fields[0].get("context", ""))

def test_partner_permissions(self):
self._check_whole_permission_set()

Expand Down
13 changes: 13 additions & 0 deletions sales_team_security/views/res_partner_view.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<?xml version="1.0" encoding="utf-8" ?>
<odoo>
<record id="view_partner_form" model="ir.ui.view">
<field name="name">Partner form (with sales info in contacts)</field>
<field name="model">res.partner</field>
<field name="inherit_id" ref="base.view_partner_form" />
<field name="arch" type="xml">
<field name="user_id" position="after">
<field name="team_id" />
</field>
</field>
</record>
</odoo>
Loading