Skip to content

Conversation

AlienAtSystem
Copy link
Contributor

@AlienAtSystem AlienAtSystem commented Dec 3, 2024

When having multiple companies selected while creating a sales order, the default sales team and the default company can mismatch due to how the default sales team is chosen.

Before the fix:
The compatible sales teams are chosen from all active companies, with no preference for the current main active company. This can cause a sales team to be chosen from another company, even if there were one available for the current company, potentially resulting in a company-mismatched sales order that causes rights issues.

After the fix:
When selecting the default sales team, it checks at appropriate spots whether the team candidates belong to the main active company and if so, gives preference to them to prevent a mismatch.


I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo
Copy link
Contributor

robodoo commented Dec 3, 2024

Pull request status dashboard

@C3POdoo C3POdoo requested a review from a team December 3, 2024 08:13
@C3POdoo C3POdoo added the Sales Sales label Dec 3, 2024
Copy link
Contributor

@reth-odoo reth-odoo left a comment

Choose a reason for hiding this comment

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

Hi 👋

I'm not clear on whether there's an actual issue with teams being selected from disallowed company, or if this is just to improve usability

Could you add/modify a test for this too?


Isn't it odd to only do it in some cases?
Can't you add .sorted(lambda team: team.company_id == self.env.company, reverse=True) on the two searches? This way the ones compatible with the current company will be at the start of the list if there are any

@AlienAtSystem
Copy link
Contributor Author

It is correct that this is mostly a usability feature, although in case of external marketplace connectors, a strangely set default sales team could result in errors.

@AlienAtSystem
Copy link
Contributor Author

@reth-odoo Could you please review again, I integrated your suggestion with the .sorted() and added a test. Unfortunatly, I made a typo with the last commit message so the CI complains, but it's not going to matter given the commits will be squashed.

Copy link
Contributor

@reth-odoo reth-odoo left a comment

Choose a reason for hiding this comment

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

If it's "just" usability I'm a bit less inclined to touch the ordering in this method as the docstring specifies we return the first team "in order of _order" and this fallback is used by some touchy models that potentially originate from automated flow (superuser/public user default company is kind of irrelevant here).

At the start of the method we set valid_cids to only contain companies from env.companies, and that should already be restricting the teams to the companies the user has selected in the company selector (allowed_company_ids). So I would think in most regular flows there's no issue.

Is there a reason this doesn't work for your use case? Maybe we can fix it elsewhere?

@AlienAtSystem
Copy link
Contributor Author

This is a matter of consistency: The secondary companies selected should not affect the behaviour of matters that stay solely within the main company. The current function does not do so. Assume we have a priority ordering (via the sequence) as follows:

  1. TeamA, company_id = 1
  2. Team0, company_id = False
  3. TeamB, company_id = 2
    If we are only in the environment of res.company(2,), then when we create a new sale order, it has company_id = 2 and team_id = Team0. If we also add res.company(1,) to our active companies, suddenly our default team is TeamA, even though it's not compatible with company_id = 2 of the sale order.

Copy link
Contributor

@reth-odoo reth-odoo left a comment

Choose a reason for hiding this comment

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

If we are only in the environment of res.company(2,), then when we create a new sale order, it has company_id = 2 and team_id = Team0. If we also add res.company(1,) to our active companies, suddenly our default team is TeamA, even though it's not compatible with company_id = 2 of the sale order.

Ah, so the issue is that the default team is not compatible with the record. Then indeed that's more of an issue, I hadn't considered that. But what if the company_id is not based on the default company of the user? Or what if we passed in a specific user, shouldn't we use their preferred company?

Would say if we change it, we should have to pass in the "preferred company" based on the record (through a context key, in stable).

For sale orders, we already ensure the team is compatible via domain=self.env['crm.team']._check_company_domain(company_id), so I wouldn't change anything. But for crm leads I think it may be legitimate to prefer "a team of the same company as the lead", which won't always be "self.env.company".

Let me know if that works for you.

@AlienAtSystem
Copy link
Contributor Author

You make good points, and it's likely better to go that route with the company_id of the record in question. I'll however have to check concerning the domain of the sales order, as it clearly did not work for our system. It might be execution order (since _compute_team_id does not depend on company_id) or something about NewID for new records that might have interfered and caused the mismatch. I'll check more thoroughly and then likely open a new PR if needed.

@reth-odoo
Copy link
Contributor

@AlienAtSystem nice then, let me know if there's another issue/if you open a separate PR :)

until then 🫡

@AlienAtSystem
Copy link
Contributor Author

@reth-odoo I analyzed, and the issue turned out to be the preference order in _get_default_team_id() for "Teams I am a member in" over "Teams I'm not a member in but fit the domain". This makes using the domain for the company_id restriction less than ideal, because it's not the highest priority.
However, since changing the priority order in _get_default_team_id would obviously break a lot, I'll just leave it as a comment here instead of trying to fix it with a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Sales Sales

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants