WIP: Notification System

[aquibbaig]

This commit is WIP. Currently aimed at fixing #373 #374 #379

[aquibbaig]

Will be coming up with a design for the UI today at the earliest, but this is what is being achievable rn, with all the configuration as discussed @imphil @agathver

[aquibbaig]

Status

• Added Mark as Read and Remove Notifications with their specific handlers. Note that these features do not depend on other parameters than the notification itself such as user, project, etc. So to make it independent, it is wiser to create another Controller with specific routes to handle it.
• Also, the color and styles are subject to change and to be moved to an independent file after Migrate to webpack  #353
• So currently, we just need to create a project at our site to call up the producer(NotificationProducerService) and the consumers would handle it.

TODO

• Email notifications
• Notification Settings

[imphil]

Please do a separate PR for changes like this, they should be merged independent of this PR, which is more experimental.

[imphil]

Thanks @aquibbaig for this starting point!

Let's do this project in multiple steps, starting with the backend queuing system. Therefore I'm only reviewing the commit " Set up RabbitMQ for notifications and created one " for now.

What I'd like to see there is the following:

• Show the class structure and the common functions within these classes.
• Show two consumers: one for web/app notifications, one for email notifications
• Implement the methods as stubs: the functions should be there, but they can always return a dummy value. For example, if you have a function which checks "should this notification be sent out as email", it can always return true or false. But it must be there.

Some comments to achieve that:

• You currently do not show in your RabbitMQ setup that a single message from the queue is forwarded to two consumers. I'd like to see that happening. That forwarding should happen through RabbitMQ, not by calling two handlers from a single consumer.
• Introduce parent/child classes where appropriate.
• Notifications displayed in the web interface are not push notifications; You can call them app notifications or web notifications for example.
• Double-check your naming for consistency. A "send notification consumer" is a confusing, "notification sender" and "notification receiver" works, as does "notification producer" and "notification consumer".
• If you make changes to this commit, please update the existing commit (with rebase) and do not add fixup commits to the series.

[imphil]

Type would not be "push notification", but "crawling error". So you need to do a routing decision here, and not when creating the message.

[imphil]

To clarify what I meant with "That forwarding should happen through RabbitMQ, not by calling two handlers from a single consumer." please have a look at this picture:

That's the same setup we have for our repo crawler currently.

[aquibbaig]

@imphil Found some time out and made the required architectural changes. Probably have left out some PhPDocs comments. will look it up when they allow me to. Have a look

[imphil]

Thanks @aquibbaig for the updates to this PR. The queue structure looks good now! A couple comments:

• Please create smaller and more focused PRs for the individual actionable work items. If you only have a single large PR, you're gating the merge of subfeatures by the full implementation, which isn't necessary. Also, it's very hard to review a large PR as this one and keep the discussion focused. (You've seen that already in the webpack PR.) So these are the PRs I'd like to see whenever things are ready:
  • Architecture with RabbitMQ
  • Add Web notifications
  • Add Email notifications
  • All other stuff are other PRs
    (e.g. https://github.com/librecores/librecores-web/pull/381/files#r294060214, which already went in through 04edf9d. That shows how important it is to keep unrelated things in separate PRs and get them merged independently.)
• Use a common base class for WebNotificationConsumer and EmailNotificationConsumer (e.g. NotificationConsumer). The two functions shouldBeHandledAsEmailNotification() and shouldBeHandledAsWebNotification() then need to have a generic name to be overwritten from the base class, e.g. shouldHandle(). Please look again at the picture in [GSoC 2019] Decide a routing mechanism for notifications #379 (comment), it's all shown there.
• We had this before: type of a notification is not email_notification or web_notification, it's something like repo_crawling_failed or site_announcement. Within the shouldBeHandledAsEmailNotification() functions (as it is called currently) you need to add logic answering the question: "Should this notification be sent out though email, given this type of notification, and the user's preferences (stored somewhere in the DB), and possibly other constraints." For now, it's sufficient to "stub" these shouldBeHandled...() methods by just returning true always. (I.e. all notifications are sent out by email and web and what else.)

[aquibbaig]

This Pull Request will be open to discussions only

[imphil]

This has now been merged through individual PRs, closing.