[18.0][IMP] mail_activity_team: Improve default team ID retrieval and update field defaults

[CRogos]

Solves: #114

@StefanRijnhart could you have a look in this change?

What do you think about replacing team_user_id by user_id (removing team_user_id)?
I don't understand why this is needed.

@jgebel @lorenzomorandini could you review/test?

[lorenzomorandini]

Hi @CRogos , tested it and it does seem to fix yes (I tested it with the hr_holiday workflow).

[lorenzomorandini]

This probably means one thing: server side code that create activities will not have a team_id set by default, even if a matching team exists for that model and user. Is that wanted?

[CRogos]

I see what you mean. The current behavior is, that the default Team of the user triggering the activity is used, not the default team of the user assigned to the activity.
Hard to say what is the best/intended behavior. There is nothing mentioned in the description. All the other modules which might create activities are not aware of team_id, so leaving it empty would be the easiest solution.

On the other hand, we could also try to find the most suitable team for the assigned user, based on model-type and user, and only leave it empty if there is no match, and there is no explicit team set on the activity type.

When a special Activity Type is used, a Default Team can be set, and therefore the team_id would be set. (this could also lead to contraint errors, if the intended user_id is not member of the team.)

Personally I also see no need for the constraint, that the user_id must be member of the used team_id. The activity might be in the hands of a team, but there might be reasons that one activity is assigned to someone outside the team, without removing the team. (e.g. vacation replacement or a special customer should not be contacted by the Sales Team, but by the CEO itself [who is not member of the Sales Team])
Nevertheless removing the constraint, will only fix the symptom, but not the problem, that the wrong team is assigend.

I tend to the behavior, try to assign the team based of the assigned user/model and allow setting a team where the assigned user is not part of the team (and removing team_user_id).

[lorenzomorandini]

I agree. I don't think it really makes sense to set the team based on the user creating the activity, it should only be based on the assigned user. I also agree on removing the constraint, it can eventually be re-added by a custom module.

[StefanRijnhart]

Thanks! Slightly worried about the loss of functionality here, even though I have my own workaround for this which very much amounts to the current change. Would it make sense to update the team if a user is set that is not a member of the current team? That might be possible if we rewrite team_id to be a stored, readwrite computed field.

A small number of tests might help to determine the impact of the change. Would you be able to work this out a little further?

[CRogos]

@StefanRijnhart @lorenzomorandini I'll give it a try and keep you posted.

[CRogos]

@StefanRijnhart @lorenzomorandini Regarding the team_user_id I need to take a deeper look also in the tests. For now you could test the bugfix.

[lorenzomorandini]

Wouldn't it be safer to wrap model_id in a list?

[CRogos]

Agree... but it is also wrong in another way, because if there is a team with and without model set, the team with the model wasn't always returned.

[StefanRijnhart]

@StefanRijnhart @lorenzomorandini Regarding the team_user_id I need to take a deeper look also in the tests. For now you could test the bugfix.

I would prefer to take a look when it is finished

[CRogos]

Tests fail due to the use of odoo-test-helper are being fixed in OCA/odoo-test-helper#39

[CRogos]

@lorenzomorandini could you have a look?

[lorenzomorandini]

The new fallback branch in create auto-assigns a team whenever user_id is present and team_user_id is absent, but it does this even when callers explicitly pass team_id=False. That changes prior behavior where an explicit false value suppressed default assignment, so integrations creating non-team activities with {"team_id": False, "user_id": ...} will now get a team unexpectedly. This is a functional regression because it overrides caller intent and can alter downstream routing/visibility logic.

[StefanRijnhart]

@lorenzomorandini Thanks for observing. @CRogos This effect would be avoided if you went with the computed field option.

[CRogos]

@lorenzomorandini did I add the tests correctly?

[lorenzomorandini]

Nearly there. When create() receives a payload with team_id=False, a valid user_id, and no team_user_id, this new branch still runs and deletes user_id. That silently creates an unassigned activity even though the caller explicitly set an assignee, which is a regression from the previous if vals.get("team_id") behavior that only removed user_id when a real team was selected.

Regression test

def test_create_keeps_user_when_team_explicitly_false(self):
    """Explicit team_id=False must not drop an explicit assignee."""
    activity = (
        self.env["mail.activity"]
        .with_user(self.employee)
        .sudo()
        .create(
            {
                "activity_type_id": self.activity1.id,
                "note": "Regression check: keep explicit assignee.",
                "res_id": self.partner_client.id,
                "res_model_id": self.partner_ir_model.id,
                "user_id": self.employee2.id,
                "team_id": False,
            }
        )
    )

    self.assertFalse(activity.team_id)
    self.assertEqual(activity.user_id, self.employee2)

Proposed fix

if vals.get("team_id"):
    # team choosen: user_id must follow team_user_id
    if "user_id" in vals and not vals.get("team_user_id"):
        del vals["user_id"]
elif "team_id" not in vals and "user_id" in vals and "team_user_id" not in vals:
    # legacy: choose the team only if team_id has not been passed
    team_id = self._get_default_team_id(vals["user_id"], vals.get("res_model_id"))
    if team_id:
        vals["team_id"] = team_id.id

[lorenzomorandini]

LGTM

[CRogos]

@lorenzomorandini thanks for your support.

[lorenzomorandini]

Could we get this merged?

[CRogos]

@StefanRijnhart could you review and merge?

[CRogos]

@muaazsiddiq feel free to review ;)