Merge pull request #342 from philomena-dev/notifications-cleanups

Notifications cleanups

lib/philomena/comments.ex

@@ -15,7 +15,6 @@ defmodule Philomena.Comments do
   alias Philomena.Images.Image
   alias Philomena.Images
   alias Philomena.Notifications
-  alias Philomena.NotificationWorker
   alias Philomena.Versions
   alias Philomena.Reports
 
@@ -63,21 +62,13 @@ defmodule Philomena.Comments do
     |> Multi.one(:image, image_lock_query)
     |> Multi.insert(:comment, comment)
     |> Multi.update_all(:update_image, image_query, inc: [comments_count: 1])
+    |> Multi.run(:notification, &notify_comment/2)
     |> Images.maybe_subscribe_on(:image, attribution[:user], :watch_on_reply)
     |> Repo.transaction()
   end
 
-  def notify_comment(comment) do
+  defp notify_comment(_repo, %{image: image, comment: comment}) do
-    Exq.enqueue(Exq, "notifications", NotificationWorker, ["Comments", comment.id])
+    Notifications.create_image_comment_notification(comment.user, image, comment)
-  end
-
-  def perform_notify(comment_id) do
-    comment =
-      comment_id
-      |> get_comment!()
-      |> Repo.preload([:user, :image])
-
-    Notifications.create_image_comment_notification(comment.user, comment.image, comment)
   end
 
   @doc """
@@ -177,7 +168,6 @@ defmodule Philomena.Comments do
     |> Repo.transaction()
     |> case do
       {:ok, %{comment: comment, reports: {_count, reports}}} ->
-        notify_comment(comment)
         UserStatistics.inc_stat(comment.user, :comments_posted)
         Reports.reindex_reports(reports)
         reindex_comment(comment)

lib/philomena/galleries.ex

@@ -14,7 +14,6 @@ defmodule Philomena.Galleries do
   alias Philomena.IndexWorker
   alias Philomena.GalleryReorderWorker
   alias Philomena.Notifications
-  alias Philomena.NotificationWorker
   alias Philomena.Images
 
   use Philomena.Subscriptions,
@@ -163,7 +162,7 @@ defmodule Philomena.Galleries do
 
   def add_image_to_gallery(gallery, image) do
     Multi.new()
-    |> Multi.run(:lock, fn repo, %{} ->
+    |> Multi.run(:gallery, fn repo, %{} ->
       gallery =
         Gallery
         |> where(id: ^gallery.id)
@@ -179,7 +178,7 @@ defmodule Philomena.Galleries do
       |> Interaction.changeset(%{"image_id" => image.id, "position" => position})
       |> repo.insert()
     end)
-    |> Multi.run(:gallery, fn repo, %{} ->
+    |> Multi.run(:image_count, fn repo, %{} ->
       now = DateTime.utc_now()
 
       {count, nil} =
@@ -189,11 +188,11 @@ defmodule Philomena.Galleries do
 
       {:ok, count}
     end)
+    |> Multi.run(:notification, &notify_gallery/2)
     |> Repo.transaction()
     |> case do
       {:ok, result} ->
         Images.reindex_image(image)
-        notify_gallery(gallery, image)
         reindex_gallery(gallery)
 
         {:ok, result}
@@ -205,7 +204,7 @@ defmodule Philomena.Galleries do
 
   def remove_image_from_gallery(gallery, image) do
     Multi.new()
-    |> Multi.run(:lock, fn repo, %{} ->
+    |> Multi.run(:gallery, fn repo, %{} ->
       gallery =
         Gallery
         |> where(id: ^gallery.id)
@@ -222,7 +221,7 @@ defmodule Philomena.Galleries do
 
       {:ok, count}
     end)
-    |> Multi.run(:gallery, fn repo, %{interaction: interaction_count} ->
+    |> Multi.run(:image_count, fn repo, %{interaction: interaction_count} ->
       now = DateTime.utc_now()
 
       {count, nil} =
@@ -245,22 +244,16 @@ defmodule Philomena.Galleries do
     end
   end
 
+  defp notify_gallery(_repo, %{gallery: gallery}) do
+    Notifications.create_gallery_image_notification(gallery)
+  end
+
   defp last_position(gallery_id) do
     Interaction
     |> where(gallery_id: ^gallery_id)
     |> Repo.aggregate(:max, :position)
   end
 
-  def notify_gallery(gallery, image) do
-    Exq.enqueue(Exq, "notifications", NotificationWorker, ["Galleries", [gallery.id, image.id]])
-  end
-
-  def perform_notify([gallery_id, _image_id]) do
-    gallery = get_gallery!(gallery_id)
-
-    Notifications.create_gallery_image_notification(gallery)
-  end
-
   def reorder_gallery(gallery, image_ids) do
     Exq.enqueue(Exq, "indexing", GalleryReorderWorker, [gallery.id, image_ids])
   end

lib/philomena/images.ex

@@ -24,7 +24,6 @@ defmodule Philomena.Images do
   alias Philomena.SourceChanges.SourceChange
   alias Philomena.Notifications.ImageCommentNotification
   alias Philomena.Notifications.ImageMergeNotification
-  alias Philomena.NotificationWorker
   alias Philomena.TagChanges.Limits
   alias Philomena.TagChanges.TagChange
   alias Philomena.Tags
@@ -593,13 +592,13 @@ defmodule Philomena.Images do
     |> Multi.run(:migrate_interactions, fn _, %{} ->
       {:ok, Interactions.migrate_interactions(image, duplicate_of_image)}
     end)
+    |> Multi.run(:notification, &notify_merge(&1, &2, image, duplicate_of_image))
     |> Repo.transaction()
     |> process_after_hide()
     |> case do
       {:ok, result} ->
         reindex_image(duplicate_of_image)
         Comments.reindex_comments(duplicate_of_image)
-        notify_merge(image, duplicate_of_image)
 
         {:ok, result}
 
@@ -945,14 +944,7 @@ defmodule Philomena.Images do
     |> Repo.update()
   end
 
-  def notify_merge(source, target) do
+  defp notify_merge(_repo, _changes, source, target) do
-    Exq.enqueue(Exq, "notifications", NotificationWorker, ["Images", [source.id, target.id]])
-  end
-
-  def perform_notify([source_id, target_id]) do
-    source = get_image!(source_id)
-    target = get_image!(target_id)
-
     Notifications.create_image_merge_notification(target, source)
   end
 

lib/philomena/notifications.ex

@@ -4,6 +4,7 @@ defmodule Philomena.Notifications do
   """
 
   import Ecto.Query, warn: false
+  alias Philomena.Repo
 
   alias Philomena.Channels.Subscription, as: ChannelSubscription
   alias Philomena.Forums.Subscription, as: ForumSubscription
@@ -78,12 +79,11 @@ defmodule Philomena.Notifications do
 
   """
   def create_channel_live_notification(channel) do
-    Creator.create_single(
+    Creator.broadcast_notification(
-      where(ChannelSubscription, channel_id: ^channel.id),
+      from: {ChannelSubscription, channel_id: channel.id},
-      ChannelLiveNotification,
+      into: ChannelLiveNotification,
-      nil,
+      select: [channel_id: channel.id],
-      :channel_id,
+      unique_key: :channel_id
-      channel
     )
   end
 
@@ -97,14 +97,12 @@ defmodule Philomena.Notifications do
 
   """
   def create_forum_post_notification(user, topic, post) do
-    Creator.create_double(
+    Creator.broadcast_notification(
-      where(TopicSubscription, topic_id: ^topic.id),
+      notification_author: user,
-      ForumPostNotification,
+      from: {TopicSubscription, topic_id: topic.id},
-      user,
+      into: ForumPostNotification,
-      :topic_id,
+      select: [topic_id: topic.id, post_id: post.id],
-      topic,
+      unique_key: :topic_id
-      :post_id,
-      post
     )
   end
 
@@ -118,12 +116,12 @@ defmodule Philomena.Notifications do
 
   """
   def create_forum_topic_notification(user, topic) do
-    Creator.create_single(
+    Creator.broadcast_notification(
-      where(ForumSubscription, forum_id: ^topic.forum_id),
+      notification_author: user,
-      ForumTopicNotification,
+      from: {ForumSubscription, forum_id: topic.forum_id},
-      user,
+      into: ForumTopicNotification,
-      :topic_id,
+      select: [topic_id: topic.id],
-      topic
+      unique_key: :topic_id
     )
   end
 
@@ -137,12 +135,11 @@ defmodule Philomena.Notifications do
 
   """
   def create_gallery_image_notification(gallery) do
-    Creator.create_single(
+    Creator.broadcast_notification(
-      where(GallerySubscription, gallery_id: ^gallery.id),
+      from: {GallerySubscription, gallery_id: gallery.id},
-      GalleryImageNotification,
+      into: GalleryImageNotification,
-      nil,
+      select: [gallery_id: gallery.id],
-      :gallery_id,
+      unique_key: :gallery_id
-      gallery
     )
   end
 
@@ -156,14 +153,12 @@ defmodule Philomena.Notifications do
 
   """
   def create_image_comment_notification(user, image, comment) do
-    Creator.create_double(
+    Creator.broadcast_notification(
-      where(ImageSubscription, image_id: ^image.id),
+      notification_author: user,
-      ImageCommentNotification,
+      from: {ImageSubscription, image_id: image.id},
-      user,
+      into: ImageCommentNotification,
-      :image_id,
+      select: [image_id: image.id, comment_id: comment.id],
-      image,
+      unique_key: :image_id
-      :comment_id,
-      comment
     )
   end
 
@@ -177,14 +172,11 @@ defmodule Philomena.Notifications do
 
   """
   def create_image_merge_notification(target, source) do
-    Creator.create_double(
+    Creator.broadcast_notification(
-      where(ImageSubscription, image_id: ^target.id),
+      from: {ImageSubscription, image_id: target.id},
-      ImageMergeNotification,
+      into: ImageMergeNotification,
-      nil,
+      select: [target_id: target.id, source_id: source.id],
-      :target_id,
+      unique_key: :target_id
-      target,
-      :source_id,
-      source
     )
   end
 
@@ -201,7 +193,7 @@ defmodule Philomena.Notifications do
   def clear_channel_live_notification(channel, user) do
     ChannelLiveNotification
     |> where(channel_id: ^channel.id)
-    |> Creator.clear(user)
+    |> delete_all_for_user(user)
   end
 
   @doc """
@@ -217,7 +209,7 @@ defmodule Philomena.Notifications do
   def clear_forum_post_notification(topic, user) do
     ForumPostNotification
     |> where(topic_id: ^topic.id)
-    |> Creator.clear(user)
+    |> delete_all_for_user(user)
   end
 
   @doc """
@@ -233,7 +225,7 @@ defmodule Philomena.Notifications do
   def clear_forum_topic_notification(topic, user) do
     ForumTopicNotification
     |> where(topic_id: ^topic.id)
-    |> Creator.clear(user)
+    |> delete_all_for_user(user)
   end
 
   @doc """
@@ -249,7 +241,7 @@ defmodule Philomena.Notifications do
   def clear_gallery_image_notification(gallery, user) do
     GalleryImageNotification
     |> where(gallery_id: ^gallery.id)
-    |> Creator.clear(user)
+    |> delete_all_for_user(user)
   end
 
   @doc """
@@ -265,7 +257,7 @@ defmodule Philomena.Notifications do
   def clear_image_comment_notification(image, user) do
     ImageCommentNotification
     |> where(image_id: ^image.id)
-    |> Creator.clear(user)
+    |> delete_all_for_user(user)
   end
 
   @doc """
@@ -281,6 +273,24 @@ defmodule Philomena.Notifications do
   def clear_image_merge_notification(image, user) do
     ImageMergeNotification
     |> where(target_id: ^image.id)
-    |> Creator.clear(user)
+    |> delete_all_for_user(user)
+  end
+
+  #
+  # Clear all unread notifications using the given query.
+  #
+  # Returns `{:ok, count}`, where `count` is the number of affected rows.
+  #
+  defp delete_all_for_user(query, user) do
+    if user do
+      {count, nil} =
+        query
+        |> where(user_id: ^user.id)
+        |> Repo.delete_all()
+
+      {:ok, count}
+    else
+      {:ok, 0}
+    end
   end
 end

lib/philomena/notifications/creator.ex

@@ -1,98 +1,61 @@
 defmodule Philomena.Notifications.Creator do
   @moduledoc """
   Internal notifications creation logic.
-
-  Supports two formats for notification creation:
-  - Key-only (`create_single/4`): The object's id is the only other component inserted.
-  - Non-key (`create_double/6`): The object's id plus another object's id are inserted.
-
-  See the respective documentation for each function for more details.
   """
 
   import Ecto.Query, warn: false
   alias Philomena.Repo
 
   @doc """
-  Propagate notifications for a notification table type containing a single reference column.
+  Propagate notifications for a notification table type.
-
-  The single reference column (`name`, `object`) is also part of the unique key for the table,
-  and is used to select which object to act on.
 
   Returns `{:ok, count}`, where `count` is the number of affected rows.
 
-  ## Example
+  ## Examples
 
-      iex> create_single(
+      iex> broadcast_notification(
-      ...>   where(GallerySubscription, gallery_id: ^gallery.id),
+      ...>   from: {GallerySubscription, gallery_id: gallery.id},
-      ...>   GalleryImageNotification,
+      ...>   into: GalleryImageNotification,
-      ...>   nil,
+      ...>   select: [gallery_id: gallery.id],
-      ...>   :gallery_id,
+      ...>   unique_key: :gallery_id
-      ...>   gallery
+      ...> )
+      {:ok, 2}
+
+      iex> broadcast_notification(
+      ...>   notification_author: user,
+      ...>   from: {ImageSubscription, image_id: image.id},
+      ...>   into: ImageCommentNotification,
+      ...>   select: [image_id: image.id, comment_id: comment.id],
+      ...>   unique_key: :image_id
       ...> )
       {:ok, 2}
 
   """
-  def create_single(subscription, notification, user, name, object) do
+  def broadcast_notification(opts) do
-    subscription
+    opts = Keyword.validate!(opts, [:notification_author, :from, :into, :select, :unique_key])
-    |> create_notification_query(user, name, object)
-    |> create_notification(notification, name)
-  end
 
-  @doc """
+    notification_author = Keyword.get(opts, :notification_author, nil)
-  Propagate notifications for a notification table type containing two reference columns.
+    {subscription_schema, filters} = Keyword.fetch!(opts, :from)
+    notification_schema = Keyword.fetch!(opts, :into)
+    select_keywords = Keyword.fetch!(opts, :select)
+    unique_key = Keyword.fetch!(opts, :unique_key)
 
-  The first reference column (`name1`, `object1`) is also part of the unique key for the table,
+    subscription_schema
-  and is used to select which object to act on.
+    |> subscription_query(notification_author)
-
+    |> where(^filters)
-  Returns `{:ok, count}`, where `count` is the number of affected rows.
+    |> convert_to_notification(select_keywords)
-
+    |> insert_notifications(notification_schema, unique_key)
-  ## Example
-
-      iex> create_double(
-      ...>   where(ImageSubscription, image_id: ^image.id),
-      ...>   ImageCommentNotification,
-      ...>   user,
-      ...>   :image_id,
-      ...>   image,
-      ...>   :comment_id,
-      ...>   comment
-      ...> )
-      {:ok, 2}
-
-  """
-  def create_double(subscription, notification, user, name1, object1, name2, object2) do
-    subscription
-    |> create_notification_query(user, name1, object1, name2, object2)
-    |> create_notification(notification, name1)
-  end
-
-  @doc """
-  Clear all unread notifications using the given query.
-
-  Returns `{:ok, count}`, where `count` is the number of affected rows.
-  """
-  def clear(query, user) do
-    if user do
-      {count, nil} =
-        query
-        |> where(user_id: ^user.id)
-        |> Repo.delete_all()
-
-      {:ok, count}
-    else
-      {:ok, 0}
-    end
   end
 
   # TODO: the following cannot be accomplished with a single query expression
   # due to this Ecto bug: https://github.com/elixir-ecto/ecto/issues/4430
 
-  defp create_notification_query(subscription, user, name, object) do
+  defp convert_to_notification(subscription, [{name, object_id}]) do
     now = DateTime.utc_now(:second)
 
-    from s in subscription_query(subscription, user),
+    from s in subscription,
       select: %{
-        ^name => type(^object.id, :integer),
+        ^name => type(^object_id, :integer),
         user_id: s.user_id,
         created_at: ^now,
         updated_at: ^now,
@@ -100,13 +63,13 @@ defmodule Philomena.Notifications.Creator do
       }
   end
 
-  defp create_notification_query(subscription, user, name1, object1, name2, object2) do
+  defp convert_to_notification(subscription, [{name1, object_id1}, {name2, object_id2}]) do
     now = DateTime.utc_now(:second)
 
-    from s in subscription_query(subscription, user),
+    from s in subscription,
       select: %{
-        ^name1 => type(^object1.id, :integer),
+        ^name1 => type(^object_id1, :integer),
-        ^name2 => type(^object2.id, :integer),
+        ^name2 => type(^object_id2, :integer),
         user_id: s.user_id,
         created_at: ^now,
         updated_at: ^now,
@@ -114,8 +77,8 @@ defmodule Philomena.Notifications.Creator do
       }
   end
 
-  defp subscription_query(subscription, user) do
+  defp subscription_query(subscription, notification_author) do
-    case user do
+    case notification_author do
       %{id: user_id} ->
         # Avoid sending notifications to the user which performed the action.
         from s in subscription,
@@ -127,13 +90,13 @@ defmodule Philomena.Notifications.Creator do
     end
   end
 
-  defp create_notification(query, notification, name) do
+  defp insert_notifications(query, notification, unique_key) do
     {count, nil} =
       Repo.insert_all(
         notification,
         query,
         on_conflict: {:replace_all_except, [:created_at]},
-        conflict_target: [name, :user_id]
+        conflict_target: [unique_key, :user_id]
       )
 
     {:ok, count}

lib/philomena/posts.ex

@@ -16,7 +16,6 @@ defmodule Philomena.Posts do
   alias Philomena.IndexWorker
   alias Philomena.Forums.Forum
   alias Philomena.Notifications
-  alias Philomena.NotificationWorker
   alias Philomena.Versions
   alias Philomena.Reports
 
@@ -93,6 +92,7 @@ defmodule Philomena.Posts do
 
       {:ok, count}
     end)
+    |> Multi.run(:notification, &notify_post/2)
     |> Topics.maybe_subscribe_on(:topic, attributes[:user], :watch_on_reply)
     |> Repo.transaction()
     |> case do
@@ -106,8 +106,8 @@ defmodule Philomena.Posts do
     end
   end
 
-  def notify_post(post) do
+  defp notify_post(_repo, %{post: post, topic: topic}) do
-    Exq.enqueue(Exq, "notifications", NotificationWorker, ["Posts", post.id])
+    Notifications.create_forum_post_notification(post.user, topic, post)
   end
 
   def report_non_approved(%Post{approved: true}), do: false
@@ -120,15 +120,6 @@ defmodule Philomena.Posts do
     )
   end
 
-  def perform_notify(post_id) do
-    post =
-      post_id
-      |> get_post!()
-      |> Repo.preload([:user, :topic])
-
-    Notifications.create_forum_post_notification(post.user, post.topic, post)
-  end
-
   @doc """
   Updates a post.
 
@@ -241,7 +232,6 @@ defmodule Philomena.Posts do
     |> Repo.transaction()
     |> case do
       {:ok, %{post: post, reports: {_count, reports}}} ->
-        notify_post(post)
         UserStatistics.inc_stat(post.user, :forum_posts)
         Reports.reindex_reports(reports)
         reindex_post(post)

lib/philomena/topics.ex

@@ -11,7 +11,6 @@ defmodule Philomena.Topics do
   alias Philomena.Forums.Forum
   alias Philomena.Posts
   alias Philomena.Notifications
-  alias Philomena.NotificationWorker
 
   use Philomena.Subscriptions,
     on_delete: :clear_topic_notification,
@@ -73,6 +72,7 @@ defmodule Philomena.Topics do
 
       {:ok, count}
     end)
+    |> Multi.run(:notification, &notify_topic/2)
     |> maybe_subscribe_on(:topic, attribution[:user], :watch_on_new_topic)
     |> Repo.transaction()
     |> case do
@@ -87,16 +87,7 @@ defmodule Philomena.Topics do
     end
   end
 
-  def notify_topic(topic, post) do
+  defp notify_topic(_repo, %{topic: topic}) do
-    Exq.enqueue(Exq, "notifications", NotificationWorker, ["Topics", [topic.id, post.id]])
-  end
-
-  def perform_notify([topic_id, _post_id]) do
-    topic =
-      topic_id
-      |> get_topic!()
-      |> Repo.preload(:user)
-
     Notifications.create_forum_topic_notification(topic.user, topic)
   end
 

lib/philomena/workers/notification_worker.ex

@@ -1,13 +0,0 @@
-defmodule Philomena.NotificationWorker do
-  @modules %{
-    "Comments" => Philomena.Comments,
-    "Galleries" => Philomena.Galleries,
-    "Images" => Philomena.Images,
-    "Posts" => Philomena.Posts,
-    "Topics" => Philomena.Topics
-  }
-
-  def perform(module, args) do
-    @modules[module].perform_notify(args)
-  end
-end

lib/philomena_web/controllers/image/comment_controller.ex

@@ -82,7 +82,6 @@ defmodule PhilomenaWeb.Image.CommentController do
         Images.reindex_image(conn.assigns.image)
 
         if comment.approved do
-          Comments.notify_comment(comment)
           UserStatistics.inc_stat(conn.assigns.current_user, :comments_posted)
         else
           Comments.report_non_approved(comment)

lib/philomena_web/controllers/topic/post_controller.ex

@@ -36,7 +36,6 @@ defmodule PhilomenaWeb.Topic.PostController do
     case Posts.create_post(topic, attributes, post_params) do
       {:ok, %{post: post}} ->
         if post.approved do
-          Posts.notify_post(post)
           UserStatistics.inc_stat(conn.assigns.current_user, :forum_posts)
         else
           Posts.report_non_approved(post)

lib/philomena_web/controllers/topic_controller.ex

@@ -111,7 +111,6 @@ defmodule PhilomenaWeb.TopicController do
     case Topics.create_topic(forum, attributes, topic_params) do
       {:ok, %{topic: topic}} ->
         post = hd(topic.posts)
-        Topics.notify_topic(topic, post)
 
         if forum.access_level == "normal" do
           PhilomenaWeb.Endpoint.broadcast!(