[FIX] im_livechat: fix operator assignation

[tsm-odoo]

Before this PR, trying to get an available operator could result in an
error.

When a live chat is created, an operator is selected based on various
criteria such as language, country, number of ongoing chats and
availability for calls. This selection process involves the
_get_less_active_operator method, which receives the status of all
operators and a list of operators to choose from.

Prior to this fix, an operator not included in the list of operators
to choose from could still be selected, resulting in errors when
trying to locate them in the operator list.

This PR resolves the issue by ensuring that operators are only
selected from the provided list.

opw-3874872

[robodoo]

Pull request status dashboard.

[phenix-factory]

@robodoo r+

[odony]

@tsm-odoo thanks for the quick fix and the test!

@robodoo delegate+ (an update has discarded @phenix-factory's review)

Additionally, may I offer the following nitpicking for free? ;-)

🧦 The code of _get_less_active_operator() seems too "clever" and (prematurely) optimised, and this tends to make it harder to read, and possibly more difficult to get right / maintain.

Considering that it's not designed to deal with large volumes of records (it's bounded by the number of connected operators), I think it might be good to make the code more straightforward and explicit, even if less optimized.

Here's a suggestion for such a rewrite, assuming I got it right.
I've slightly altered the parameter names to increase readability, as they seem to always be passed positionally. To be strictly conservative with API stability we might want to preserve the original ones.

def _get_less_active_operator(self, active_operator_statuses, available_operators):
    """ Retrieve the most available operator based on the following criteria:
    - Lowest number of active chats.
    - Not in  a call.
    - If an operator is in a call and has two or more active chats, don't
      give priority over an operator with more conversations who is not in a
      call.

    :param active_operator_statuses: list of dictionaries containing the operator's
        id, the number of active chats and a boolean indicating if the
        operator is in a call. The list is ordered by the number of active
        chats (ascending) and whether the operator is in a call
        (descending).
    :param available_operators: recordset of :class:`ResUsers` operators to choose from.
    :return: the :class:`ResUsers` record for the chosen operator
    """
    if not available_operators:
        return False

    # 1) first try to select an inactive op, i.e. one w/ no active status (no recent chat)
    active_op_partner_ids = set(s['livechat_operator_id'] for s in active_operator_statuses)
    candidates = available_operators.filtered(lambda o: o.partner_id.id not in active_op_partner_ids)
    if candidates:
        return random.choice(candidates)

    # 2) otherwise select least active ops, based on status ordering (count + in_call)
    best_status = active_operator_statuses[0]  # best means "least active"
    best_status_op_partner_ids = set(
         s['livechat_operator_id']
         for s in active_operator_statuses
         if (s['count'], s['in_call']) == (best_status['count'], best_status['in_call'])
    )
    candidates = available_operators.filtered(lambda o: o.partner_id.id in best_status_op_partner_ids)
    return random.choice(candidates)

What do you think? It doesn't have optimal run time complexity for some operations, but considering that we're dealing with small lists (likely less than 10 or 100 items), it won't make any measurable difference.
And the code simplicity and readability is worth it, IMO.

I leave it up to you, your current fix is fine too ;-)

[alexkuhn]

@odony Also the code would also be much simpler in general if available_operator_ids was a many on res.partner rather than res.users.

[odony]

@odony Also the code would also be much simpler in general if available_operator_ids was a many on res.partner rather than res.users.

Certainly, but operators are supposed to be users, not just partners, so that m2m seems correctly defined, I think. An alternative is to retrieve the user IDs as well as part of the operator_statuses, for a (hopefully small) extra SQL cost.

[tsm-odoo]

Hello @odony,

Thank you for your advice; it's definitely simpler! I noticed a small mistake in the algorithm's second part. It initially retrieves the best_status from the list of all available operators, whereas it should only consider the best status for operators we want to choose from. To solve this, I filtered the list of operator_statuses to only include operators we are allowed to choose from, and I updated the test to reflect this scenario.

[alexkuhn]

@robodoo r+

[robodoo]

@alexkuhn you may want to rebuild or fix this PR as it has failed CI.

[tsm-odoo]

@robodoo r+

Rebased to latest 17.0 to fix already fixed runbot error.