fix: prevent duplicated queued webhooks

[flegastelois]

Checklist before requesting a review

• I have read the CONTRIBUTING document.
• I have performed a self-review of my code.
• I have added tests that prove my fix is effective or that my feature works.

Description

Prevent duplicate queued webhooks for the same webhook and the same item/event.

Example case:

• when adding a solution to a ticket with set_solution_tech
• this triggers the update event twice within nearly the same second
• without this PR, two webhooks would be queued for the same ticket update event

[cconard96]

I have some doubts that deduplication can be done so easily for webhooks, and I think there were similar issues experienced with notifications which led to deduplication being removed for them.

I cannot say that the important update will always be the first one within a request or that it is the only one the user will want a webhook for.

With your example:

• A solution is added triggering a ticket update
• A webhook is queued with the current ticket data (solved, no technician assigned)
• The technician is auto-assigned and triggers another ticket update without a webhook
  The only webhook that gets sent has incomplete data as the assigned technician is missing.

Updating the content for the existing queued webhook may yield better results but I am not sure at the moment if that introduces other issues.

I'd like to get other people's opinions.

[flegastelois]

I have some doubts that deduplication can be done so easily for webhooks, and I think there were similar issues experienced with notifications which led to deduplication being removed for them.

I cannot say that the important update will always be the first one within a request or that it is the only one the user will want a webhook for.

With your example:

* A solution is added triggering a ticket update

* A webhook is queued with the current ticket data (solved, no technician assigned)

* The technician is auto-assigned and triggers another ticket update without a webhook
  The only webhook that gets sent has incomplete data as the assigned technician is missing.

Updating the content for the existing queued webhook may yield better results but I am not sure at the moment if that introduces other issues.

I'd like to get other people's opinions.

It seems to me that we have a deduplication mechanism on the email notification side (to avoid sending two emails to the same person for the same event).

What you say is true: my change may not be the best approach, and I also realize that the body of the first webhook does not correctly reflect the state of the ticket (since there is no technician yet).

Maybe adding an additional filter in the webhook would prevent the first trigger (for example, when the technician is not empty).

We can close this PR if we don’t find a reliable solution, no problem.

[orthagh]

@cconard96 how it could be hard to add specific events such as "resolve ticket" or "assign technician" like done in classic notifications ?

[cconard96]

@cconard96 how it could be hard to add specific events such as "resolve ticket" or "assign technician" like done in classic notifications ?

I think the global triggering of webhooks in CommonDBTM would need removed and moved to each supported itemtype if you want to have more specific events that can replace the update event.

[orthagh]

For ticket's notifications, global "update" event and specific events exists, and it's not an issue as notification target a trigger.
So, I don't think it's needed to remove global events, just add new specific ones

[cconard96]

For ticket's notifications, global "update" event and specific events exists, and it's not an issue as notification target a trigger. So, I don't think it's needed to remove global events, just add new specific ones

The event still exists, but the logic in Ticket::post_updateItem() changes the event of the raised notification if the input matches a more specific event. If the global webhook events are raised in CommonDBTM::update(), the child classes won't be able to override that.

glpi/src/Ticket.php

Lines 1468 to 1502 in 9ea3c75

	if ($donotif && $CFG_GLPI["use_notifications"]) {
	$mailtype = "update";
	if (
	isset($this->input["status"])
	&& $this->input["status"]
	&& in_array("status", $this->updates)
	&& in_array($this->input["status"], static::getSolvedStatusArray())
	) {
	$mailtype = "solved";
	}
	if (
	isset($this->input["status"])
	&& $this->input["status"]
	&& in_array("status", $this->updates)
	&& in_array($this->input["status"], static::getClosedStatusArray())
	) {
	$mailtype = "closed";
	}
	// to know if a solution is approved or not
	if (
	(isset($this->input['solvedate']) && ($this->input['solvedate'] == 'NULL')
	&& isset($this->oldvalues['solvedate']) && $this->oldvalues['solvedate'])
	&& (isset($this->input['status'])
	&& ($this->input['status'] != $this->oldvalues['status'])
	&& ($this->oldvalues['status'] == self::SOLVED))
	) {
	$mailtype = "rejectsolution";
	}
	// Read again ticket to be sure that all data are up to date
	$this->getFromDB($this->fields['id']);
	NotificationEvent::raiseEvent($mailtype, $this);
	}