[FIX] sales_team: align team member company to team company

[stpa-odoo]

Steps to reproduce the issue:

(Easier to reproduce client side with CRM installed)

0. Install crm
1. Create Second Company
2. Create New User with Current Company as Default Company and Second Company in Allowed Companies
3. Create CRM Team belonging to Second Company
4. Add New User to Members of CRM Team
5. Receive error:

[New User] belongs to company [Current Company] and "Sales Team" (crm_team_id: [CRM Team]) belongs to another company.

Explanation:

crm.team.member is created when adding res.users to crm.team.member_ids. Contrary to res.users, a company_check is done when linking crm.team and crm.team.member together. crm.team.member.company_id is related to user_id.company_id and, in the case above, does not match crm.team.company_id, raising an error because of it.

Fix reasoning:

Removing restriction, as we want to avoid other multi-company issues by changing the behaviour.

opw-4214192

---

@nd-dew note:
Situation recap

So it was proposed to modify the field definition, seems to me that removing check_company from the field definition is a legal move, since it is an ORM level constraint (doesn't change db schema).

However looking at the tests it seems like this is desired limitation.

Note that #171079 introduced checking for allowed companies, but the default company still takes priority.

[robodoo]

[nd-dew]

Don't we need to generate translations for the error that you added 🤔

[nd-dew]

Would add note that "crm" module is needed to reproduce.

[nd-dew]

Taking over the PR, recap: on the write

• in BaseModel._check_company we grab company_id from crm.team.member (that's the default company of the user)
• we check if crm.team is a subset of that company ---> It is not

Reconsidering STPA solution, it was rather deep for 16 version as it modified field.

Experiment: can we actually skip thee additional check in the CrmTeam.write method ?

[nd-dew]

Experiment 1: Not sure what's the purpose of that manual check, but it doesn't matter in this case. _check_company will be called either way because crm_team_id is a field marked with _check_company=True

reverting

EDIT: The purpose of the manual check was the same as in my PR, so to enforce constraint both ways (when writing on crm_team)

[nd-dew]

Moved company check into a constraint

Inferred Specs

Before doing so grabbing all requirements from tests and related task to ensure consistency

Motivation

Seems like this case wasn't considered at the time and it is possible to fix that and by using api.constraint, so why not since we don't touch db schema, and make constraints cleaner

[reth-odoo]

Note for reviewer, if we're willing to ask for an exception for old stable we could probably just add a related "company_ids" field and get framework support for free.
Probably not worth it though, current fix seems ok.

Leaving mostly documentation comments, just one thing for the test we usually prefer using the test helpers in our modules.
Tested manually, seems to work fine otherwise.

@jeh-odoo 🍞 hi
Do you have an opinion on this? It's relatively touchy so not sure for old stable but it could block valid flows (this comes from a ticket, in the first place)

Would say worth fixing at least in more recent versions, if nothing else

[jeh-odoo]

Hello there

Not the first time I see this feedback I think, and the fix seems ok to me so why not.
I remember reading some mail about fix in older version and the need to be more strict.
So is it considered a bug? I would say yes.
Is it a traceback or a critical issue ? I would say no to this one, and there should be some workaround if I'm not wrong.
Is it a touchy behavior ? Yes.

So I think that I would be more at ease to fix this in a newish version than in 16.0. As a side note, it made me think about the task 2951093.

Cheers!

[nd-dew]

Re-Target: 16 -> 18

Agreed, moving the fix into 18.0

[reth-odoo]

oop 🆙

[nd-dew]

hey @jeh-odoo, can we merge it?

[jeh-odoo]

Hello @nd-dew
Thanks for the reminder on this one 🙂
Had another look and retested the whole thing. Seems good enough for me, just one last picky comment that I saw.

Cheers!

[nd-dew]

Revisiting this!

Thanks you all for reviews. Applied all suggestions.

I went over this one more time and I've noticed one edge-case that isn't covered.
Constraint on the crm_team_member is sufficient to for changing members on the sales team, but it doesn't cover a case when a user is in already in a sales team and the sales team changes companies.

I solve that by one more constraint on the CrmTeam ;)

[tde-banana-odoo]

Zboing

[tde-banana-odoo]

Isn't _check_company called by _check_company_auto sufficient ? Looking quickly at the code, if we change company_id, it should recompute constraints of relational fields. But I see we did not define check_company on specific fields ?

[nd-dew]

Good point, seems like moving it to crm_team_mamber_ids field seem to work in the same way

The check_company cannot replace the custom constrains method because it only checks the company_id against the main record's company_id.
It does not consider allowed companies unless the model defines company_ids, which crm.team does not.
... it would take us back to the error we started with

[nd-dew]

Trying tor reduce the constrains method with check_company -> Can't

index 6c8f52655aec..520ab435c9c9 100644
--- a/addons/sales_team/models/crm_team.py
+++ b/addons/sales_team/models/crm_team.py
@@ -114,7 +114,8 @@ class CrmTeam(models.Model):
     crm_team_member_ids = fields.One2many(
         'crm.team.member', 'crm_team_id', string='Sales Team Members',
         context={'active_test': True},
-        help="Add members to automatically assign their documents to this sales team.")
+        help="Add members to automatically assign their documents to this sales team.",
+        check_company=True)
     crm_team_member_all_ids = fields.One2many(
         'crm.team.member', 'crm_team_id', string='Sales Team Members (incl. inactive)',
         context={'active_test': False})
@@ -129,19 +130,6 @@ class CrmTeam(models.Model):
     dashboard_button_name = fields.Char(string="Dashboard Button", compute='_compute_dashboard_button_name')
     dashboard_graph_data = fields.Text(compute='_compute_dashboard_graph')
 
-    @api.constrains('company_id')
-    def _constrains_company_members(self):
-        for team in self.filtered('company_id'):
-            invalid_members = team.crm_team_member_ids.filtered(
-                lambda m: team.company_id not in m.user_id.company_ids
-            )
-            if invalid_members:
-                raise UserError(_("The following team members are not allowed in company '%(company)s' of the Sales Team '%(team)s': %(users)s",
-                    company=team.company_id.display_name,
-                    team=team.name,
-                    users=", ".join(invalid_members.mapped('user_id.name'))
-                ))
-

Can't do that
The check_company attribute cannot replace the custom method because it only checks the company_id against the main record's company_id. It does not consider allowed companies unless the model defines company_ids, which crm.team does not.