Forum: add instant push notifications

[iszmais]

https://docu.ilias.de/go/wiki/wpage_9000_1357

[mjansenDatabay]

Hi @iszmais

Thank you for wiring Forum into the new push provider mechanism.

Please answer the following questions:

• How should PushQueueResult::UNKNOWN be treated? NotificationsPushProvider::push() only returns true for PushQueueResult::SUCCEEDED. Unknown is treated as failure, so mail is sent as well. That can duplicate a notification that the endpoint already accepted.

Please consider the following suggestions. You do not need to follow those, but please indicate shortly why you prefer to do otherwise:

• Cache user objects / languages or load users in bulk. ilObjectFactory::getInstanceByObjId() plus ilLanguageFactory::_getLanguageOfUser() per recipient, then a synchronous Web Push request, will sit on the poster’s HTTP request. On forums with many subscribers this can make posting and instant notification slow enough that installations will leave the feature off.
• Log push failures the same way you log success. Failures currently disappear into the mail fallback.
• The new language key forums_forum_push_notification_desc follows the old forums_* pattern. Current language rules prefer module prefix frm_ and _info for setting bylines. Of course, existing Forum keys are inconsistent, so this is optional.
• If forum_notification is disabled, filterMailRecipients() returns []. That is redundant with the mail sender, but it is also easy to misread as "suppress all mail". Please prefer returning the original $recipients and skipping push.

Please implement the following changes:

• Rename $pushProvider to $push_provider in ilForumAppEventListener.
• Fix the action title on ilNotificationLink. ilNotificationLink stores ilNotificationParameter::getName() as title, and ilNotificationPushHandler::buildContent() puts that string into the payload actions[].title. You pass forums_notification_show_post with no parameters. The English translation is "Show post: %s" (the %s is the URL in mail).
• If somehow possible, please do not construct NotificationsPushProvider (ilNotificationPushHandler) unless push can actually run. getPushProvider() always does new NotificationsPushProvider($this), and the handler constructor does file_get_contents($settings->get('private_key_path')). ilSetting::get() declares ?string, so file_get_contents(null) results in a type error if no private key path can be found. That runs for every immediate Forum notification with recipients, even when enable_push is off or keys were never configured. The handler returns early in notify() when disabled, but the constructor already ran. A failed constructor would abort sendNotification() before mail is sent. Please guard on enable_push (and a usable key path), or make the handler construction null-safe, before this is merged.

Best regards,
Michael

[iszmais]

@mjansenDatabay Thank you for your feedback.

I updated the code. Most i implemented without any further addition from my side. Here are the exceptions:

How should PushQueueResult::UNKNOWN be treated? NotificationsPushProvider::push() only returns true for PushQueueResult::SUCCEEDED. Unknown is treated as failure, so mail is sent as well. That can duplicate a notification that the endpoint already accepted.

I would prefer to send a duplicate notification (especially since it is over 2 channels) than missing out on one.

Cache user objects / languages or load users in bulk. ilObjectFactory::getInstanceByObjId() plus ilLanguageFactory::_getLanguageOfUser() per recipient, then a synchronous Web Push request, will sit on the poster’s HTTP request. On forums with many subscribers this can make posting and instant notification slow enough that installations will leave the feature off.

I cached the languages, but I don't see any angle to cache the users, especially since every user should only be called once per request by concept.

Greetings,
@iszmais

[iszmais]

@mjansenDatabay I added a line to the privacy.md of the Forum. Please another (final?) look!