search

glpi-project / glpi

GLPI is a Free Asset and IT Management Software package, Data center management, ITIL Service Desk, licenses tracking and software auditing.
https://glpi-project.org
GNU General Public License v3.0
4.29k stars 1.29k forks source link

sending multiple notifications after-hook #12773

Open glebsts opened 2 years ago

glebsts commented 2 years ago

Code of Conduct

Is there an existing issue for this?

Version

9.5.5

Bug description

Plugin subscribes to "add_ticket" or "add_followup" event. Plugin creates and adds to queue multiple notifications (i.e. to ticket requester, ticket assignee, ticket observer). Adding forceSendFor() is called: https://github.com/glpi-project/glpi/blob/32179c568ae9ab382e047ef8974cb03fd37efce9/src/QueuedNotification.php#L648-L655

Comment for forceSendFor() says "sending all emails", but line 663 actually calls getPendings() with limit = 1. So if plugin added notifications, how are they all sent after hook call?

Expected behavior: all notifications created for entity (for example ticket or followup) are sent after hook call.

Actual behavior: only first notification is picked from database.

What's the expected flow in case of multiple notifications?

Relevant log output

No response

Page URL

No response

Steps To reproduce

  1. add some plugin with hooks to add_followup that adds multiple notifications to notification queue
  2. enable sql output (optionally can just enable debug in running query in getPendings())
  3. add followup
  4. watch only single notification being selected from database (and sent)

Your GLPI setup information

GLPI 9.5.5 ( => glpi) Installation mode: TARBALL PHP 7.4.3

Anything else?

No response

cedric-anne commented 2 years ago

Hi,

Indeed the QueuedNotification::forceSendFor() only send the first found notification for the given item.

Method was made to force sending of the notification that was just created, but as query is ordered by send_time ASC, it may send a notification that was already queued. This logic is buggy by design, and should be replaced by a $bypass_queue argument in NotificationEvent::raiseEvent() method, in order to force sending of notification synchronously.

Anyway, it will not be done in GLPI 9.5.x as this version is not supported anymore.

To bypass this problem, you may try to call QueuedNotification::forceSendFor() after each notification creation.

glebsts commented 2 years ago

Thank you for quick reply! Do I get it right that $bypass_queue does not exists yet? :) I also found that for 9.5.5 if notification object contains field notificationqueueonaction, it is being handled in CommonDBTM->add() calling forceSendFor() with notification item type, therefor causing force send for new notification. Mb could be used as workaround :)

cedric-anne commented 2 years ago

Thank you for quick reply! Do I get it right that $bypass_queue does not exists yet? :)

Indeed, it does not exists.

I also found that for 9.5.5 if notification object contains field notificationqueueonaction, it is being handled in CommonDBTM->add() calling forceSendFor() with notification item type, therefor causing force send for new notification. Mb could be used as workaround :)

notificationqueueonaction property has been removed in GLPI 10.0. You could still use it in GLPI 9.5.x.