Skip to content

Conversation

@cgarcia-solvos
Copy link

Partner Restrictions by Helpdesk Team
Allows dynamic control over which contact (partner_id) on ticket, based on the configuration of the assigned Helpdesk Team (team_id). The logic works as follows:

  • If the team has a default contact, it will fill the contact field with that value.
  • If the team has a unique contact, only that contact will be available.
  • If the team has a list of allowed contacts, only those contacts will be available.
  • If the team has no restrictions, all contacts will be available.

@cgarcia-solvos cgarcia-solvos force-pushed the 17.0-add-helpdesk_mgmt_team_allowed_partners branch 2 times, most recently from 61f0302 to 06b8fdf Compare November 6, 2025 09:06
@cgarcia-solvos
Copy link
Author

@DantePereyra @ChristianSantamaria Could you review? Thanks!

Copy link
Contributor

@DantePereyra DantePereyra left a comment

Choose a reason for hiding this comment

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

Tested on runboat, problems found:

  • When ticket had team and contact assigned, and that team add or changes default_partner, it gives an OWL error.
  • With default_partner on team, team and contact assigned on ticket, saved, and then change partner (with allowed partners or not) gives an strange error.

Also, I can't check if test are working.

def _compute_partner_id_domain(self):
for record in self:
if record.team_id:
if record.team_id.default_partner_id:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if record.team_id.default_partner_id:
if record.team_id.default_partner_id and not record.partner_id:

With this you can only select the default partner, and will give an error when saving. Try this.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

why dont't you use the field: commercial_partner_id?
When a contact from a company open a ticket, do you what to apply the restriction on the contact or on the company?

Copy link
Author

Choose a reason for hiding this comment

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

The purpose of this feature is not to restrict which user/contact can open a ticket, but to restrict which partner can be assigned to the ticket depending on the helpdesk team.

In our use case, an internal user creates a ticket (or assigns an existing one) and then sets the team_id. Once the team is chosen:

the partner_id is auto-filled with the team’s default_partner_id, and the partner_id field is restricted to a specific list of contacts configured on the team (default_partner_id and allowed_partner_ids).

This behaviour is independent from the contact/company of the user who opens the ticket (portal or internal). We are configuring which partners are allowed for a given team, not which company the ticket creator belongs to.

For this reason I’m using partner_id directly instead of commercial_partner_id. In our scenario, a team can be dedicated to specific contacts, even within the same company.

Comment on lines +41 to +60
@api.constrains("team_id", "partner_id")
def _check_partner_allowed_by_team(self):
for record in self:
if not record.team_id or not record.partner_id:
continue
if (
record.team_id.is_unique_partner
and record.partner_id != record.team_id.default_partner_id
):
raise ValidationError(
_("This partner is not allowed for selected team")
)
elif (
record.team_id.allowed_partner_ids
and record.partner_id not in record.team_id.allowed_partner_ids
and record.partner_id != record.team_id.default_partner_id
):
raise ValidationError(
_("This partner is not allowed for selected team")
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is unnecessary since you will not be capable of select different partners with above restrictions, but maybe I'm wrong.

Copy link
Author

Choose a reason for hiding this comment

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

It’s necessary because tickets allow the creation of a partner 'on the fly'. I didn’t want to restrict that functionality, but I did want to block the possibility of assigning this new partner to the ticket, since it’s neither included in the 'allowed_partner_ids' group nor set as the 'default_partner_id'.

Copy link
Contributor

@DantePereyra DantePereyra left a comment

Choose a reason for hiding this comment

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

Not a request, just a suggestion :)

Comment on lines 20 to 22
for record in self:
if not record.default_partner_id:
record.is_unique_partner = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for record in self:
if not record.default_partner_id:
record.is_unique_partner = False
for record in self.filtered(lambda r: not r.default_partner_id):
record.is_unique_partner = False

Could you try this to see if it works right?

Copy link
Author

Choose a reason for hiding this comment

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

It works right, thanks!

@cgarcia-solvos cgarcia-solvos force-pushed the 17.0-add-helpdesk_mgmt_team_allowed_partners branch from c5b8583 to 8e0ba62 Compare November 11, 2025 10:53
Copy link
Contributor

@DantePereyra DantePereyra left a comment

Choose a reason for hiding this comment

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

Tested! 👍 Code seems fine to me.

Partner Restrictions by Helpdesk Team
Allows dynamic control over which contact (partner_id) on ticket,
based on the configuration of the assigned Helpdesk Team (team_id). The logic works as follows:
- If the team has a default contact, it will fill the contact field with that value.
- If the team has a unique contact, only that contact will be available.
- If the team has a list of allowed contacts, only those contacts will be available.
- If the team has no restrictions, all contacts will be available.
@cgarcia-solvos cgarcia-solvos force-pushed the 17.0-add-helpdesk_mgmt_team_allowed_partners branch from 8e0ba62 to 8c374b4 Compare November 18, 2025 20:03
@DantePereyra
Copy link
Contributor

Hi @rrebollo! Tagging you here to see if you can take a look at this as an ADD, as suggested in the other PR, Thanks!

@cgarcia-solvos cgarcia-solvos changed the title [ADD] helpdesk_mgmt_team_customer [17.0] [ADD] helpdesk_mgmt_team_customer Nov 25, 2025
@rrebollo
Copy link

rrebollo commented Dec 1, 2025

@cgarcia-solvos @DantePereyra

Design Review: Functional Rationale and Architectural Alignment

Hi authors,

Thanks for submitting this PR. I've reviewed the code for helpdesk_mgmt_team_customer and, from a technical perspective, it looks solid. It correctly uses a computed domain and a server-side constraint to enforce the rules.

However, as a fellow contributor, I have a few questions about the module's positioning and architectural choices, which would be essential to clarify in the README to ensure strong community alignment and future maintainability.

1. Use Case & Business Justification (The "Why")

Could you please elaborate on the specific business process this module is targeting?

We typically see constraints defined around the Customer (e.g., "This Customer can only raise tickets in Team A"). This module imposes the constraint in the reverse direction (Team A $\to$ only allowed Customers).

Is this designed primarily for highly segmented scenarios like Managed Service Providers (MSPs) or internal support teams where agents often switch roles but must be strictly prevented from associating tickets with out-of-scope contracts? Providing this context will solidify the module's legitimate place in the OCA/helpdesk repository.


2. Functional Philosophy (The "What": Restriction Ambiguity)

The module implements a hybrid control mechanism:

  1. Guidance: Dynamic domain filtering in the view (partner_id_domain).
  2. Hard Restriction: A Python constraint (@api.constrains) blocking the save operation.

This suggests that even if an agent bypasses the UI domain, the server will enforce the rule. Is this hard restriction a non-negotiable requirement of the use case?

Sometimes, functional flexibility is key (e.g., allowing an exception for an emergency ticket). If the rule is for data hygiene/guidance only, the constraint is unnecessary; if it's for data integrity/security, it's essential, but the rationale should be clear.


3. Architectural Design Choice (The "How": Record Rules vs. Constraints)

This is my main point regarding principle alignment (KISS, Separation of Concerns). You chose to implement the restriction as a transactional constraint on the helpdesk.ticket object.

I suggest considering the approach used in modules like sales_team_security, which achieves access restriction primarily through Row-Level Security (ir.rule) on the res.partner model.

  • Row-Level Security Pattern: Unauthorized partners are simply invisible to the user at the database level. This removes the possibility of selection errors and reduces the need for complex custom Python constraints.
  • Your Current Pattern: Allows the user to see all partners globally, but blocks the specific association on the ticket.

Was the Record Rule pattern considered? If so, why was it discarded in favor of custom constraints? The answer is critical:

  • If the agents must have global read access to all partners for other operational reasons (e.g., billing, general contact lookups), then your current approach is justified.
  • If the agents should not be able to see unauthorized partners at all, the Record Rule approach would be simpler and more aligned with Odoo's native security architecture.

Next Steps (my suggestion)

To move this PR forward, please add a section to the README.rst addressing these three points. Clarifying the Why and the choice of the Constraint Pattern will help prevent other reviewers from raising the same questions and solidify the module's position in the OCA.

Thanks again for the great work!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants