Merge branch 'issue/2069' into 'develop'

[#2069] unread_conversation_count

See merge request pleroma/pleroma!2939

lib/pleroma/conversation.ex

@@ -43,7 +43,7 @@ def get_for_ap_id(ap_id) do
   def maybe_create_recipientships(participation, activity) do
     participation = Repo.preload(participation, :recipients)
 
-    if participation.recipients |> Enum.empty?() do
+    if Enum.empty?(participation.recipients) do
       recipients = User.get_all_by_ap_id(activity.recipients)
       RecipientShip.create(recipients, participation)
     end
@@ -69,10 +69,6 @@ def create_or_bump_for(activity, opts \\ []) do
         Enum.map(users, fn user ->
           invisible_conversation = Enum.any?(users, &User.blocks?(user, &1))
 
-          unless invisible_conversation do
-            User.increment_unread_conversation_count(conversation, user)
-          end
-
           opts = Keyword.put(opts, :invisible_conversation, invisible_conversation)
 
           {:ok, participation} =

lib/pleroma/conversation/participation.ex

@@ -63,21 +63,10 @@ def mark_as_read(%User{} = user, %Conversation{} = conversation) do
     end
   end
 
-  def mark_as_read(participation) do
+  def mark_as_read(%__MODULE__{} = participation) do
-    __MODULE__
+    participation
-    |> where(id: ^participation.id)
+    |> change(read: true)
-    |> update(set: [read: true])
+    |> Repo.update()
-    |> select([p], p)
-    |> Repo.update_all([])
-    |> case do
-      {1, [participation]} ->
-        participation = Repo.preload(participation, :user)
-        User.set_unread_conversation_count(participation.user)
-        {:ok, participation}
-
-      error ->
-        error
-    end
   end
 
   def mark_all_as_read(%User{local: true} = user, %User{} = target_user) do
@@ -93,7 +82,6 @@ def mark_all_as_read(%User{local: true} = user, %User{} = target_user) do
     |> update([p], set: [read: true])
     |> Repo.update_all([])
 
-    {:ok, user} = User.set_unread_conversation_count(user)
     {:ok, user, []}
   end
 
@@ -108,7 +96,6 @@ def mark_all_as_read(%User{} = user) do
       |> select([p], p)
       |> Repo.update_all([])
 
-    {:ok, user} = User.set_unread_conversation_count(user)
     {:ok, user, participations}
   end
 
@@ -220,6 +207,12 @@ def set_recipients(participation, user_ids) do
     {:ok, Repo.preload(participation, :recipients, force: true)}
   end
 
+  @spec unread_count(User.t()) :: integer()
+  def unread_count(%User{id: user_id}) do
+    from(q in __MODULE__, where: q.user_id == ^user_id and q.read == false)
+    |> Repo.aggregate(:count, :id)
+  end
+
   def unread_conversation_count_for_user(user) do
     from(p in __MODULE__,
       where: p.user_id == ^user.id,

lib/pleroma/user.ex

@@ -128,7 +128,6 @@ defmodule Pleroma.User do
     field(:hide_followers, :boolean, default: false)
     field(:hide_follows, :boolean, default: false)
     field(:hide_favorites, :boolean, default: true)
-    field(:unread_conversation_count, :integer, default: 0)
     field(:pinned_activities, {:array, :string}, default: [])
     field(:email_notifications, :map, default: %{"digest" => false})
     field(:mascot, :map, default: nil)
@@ -1305,47 +1304,6 @@ def update_following_count(%User{local: true} = user) do
     |> update_and_set_cache()
   end
 
-  def set_unread_conversation_count(%User{local: true} = user) do
-    unread_query = Participation.unread_conversation_count_for_user(user)
-
-    User
-    |> join(:inner, [u], p in subquery(unread_query))
-    |> update([u, p],
-      set: [unread_conversation_count: p.count]
-    )
-    |> where([u], u.id == ^user.id)
-    |> select([u], u)
-    |> Repo.update_all([])
-    |> case do
-      {1, [user]} -> set_cache(user)
-      _ -> {:error, user}
-    end
-  end
-
-  def set_unread_conversation_count(user), do: {:ok, user}
-
-  def increment_unread_conversation_count(conversation, %User{local: true} = user) do
-    unread_query =
-      Participation.unread_conversation_count_for_user(user)
-      |> where([p], p.conversation_id == ^conversation.id)
-
-    User
-    |> join(:inner, [u], p in subquery(unread_query))
-    |> update([u, p],
-      inc: [unread_conversation_count: 1]
-    )
-    |> where([u], u.id == ^user.id)
-    |> where([u, p], p.count == 0)
-    |> select([u], u)
-    |> Repo.update_all([])
-    |> case do
-      {1, [user]} -> set_cache(user)
-      _ -> {:error, user}
-    end
-  end
-
-  def increment_unread_conversation_count(_, user), do: {:ok, user}
-
   @spec get_users_from_set([String.t()], keyword()) :: [User.t()]
   def get_users_from_set(ap_ids, opts \\ []) do
     local_only = Keyword.get(opts, :local_only, true)

lib/pleroma/web/mastodon_api/views/account_view.ex

@@ -388,7 +388,7 @@ defp maybe_put_unread_conversation_count(data, %User{id: user_id} = user, %User{
     data
     |> Kernel.put_in(
       [:pleroma, :unread_conversation_count],
-      user.unread_conversation_count
+      Pleroma.Conversation.Participation.unread_count(user)
     )
   end
 

priv/repo/migrations/20200831114918_remove_unread_conversation_count_from_user.exs

@@ -0,0 +1,38 @@
+defmodule Pleroma.Repo.Migrations.RemoveUnreadConversationCountFromUser do
+  use Ecto.Migration
+  import Ecto.Query
+  alias Pleroma.Repo
+
+  def up do
+    alter table(:users) do
+      remove_if_exists(:unread_conversation_count, :integer)
+    end
+  end
+
+  def down do
+    alter table(:users) do
+      add_if_not_exists(:unread_conversation_count, :integer, default: 0)
+    end
+
+    flush()
+    recalc_unread_conversation_count()
+  end
+
+  defp recalc_unread_conversation_count do
+    participations_subquery =
+      from(
+        p in "conversation_participations",
+        where: p.read == false,
+        group_by: p.user_id,
+        select: %{user_id: p.user_id, unread_conversation_count: count(p.id)}
+      )
+
+    from(
+      u in "users",
+      join: p in subquery(participations_subquery),
+      on: p.user_id == u.id,
+      update: [set: [unread_conversation_count: p.unread_conversation_count]]
+    )
+    |> Repo.update_all([])
+  end
+end

priv/repo/migrations/20200831115854_add_unread_index_to_conversation_participation.exs

@@ -0,0 +1,12 @@
+defmodule Pleroma.Repo.Migrations.AddUnreadIndexToConversationParticipation do
+  use Ecto.Migration
+
+  def change do
+    create(
+      index(:conversation_participations, [:user_id],
+        where: "read = false",
+        name: "unread_conversation_participation_count_index"
+      )
+    )
+  end
+end

test/pleroma/conversation/participation_test.exs

@@ -37,9 +37,8 @@ test "for a new conversation or a reply, it doesn't mark the author's participat
 
     [%{read: true}] = Participation.for_user(user)
     [%{read: false} = participation] = Participation.for_user(other_user)
-
+    assert Participation.unread_count(user) == 0
-    assert User.get_cached_by_id(user.id).unread_conversation_count == 0
+    assert Participation.unread_count(other_user) == 1
-    assert User.get_cached_by_id(other_user.id).unread_conversation_count == 1
 
     {:ok, _} =
       CommonAPI.post(other_user, %{
@@ -54,8 +53,8 @@ test "for a new conversation or a reply, it doesn't mark the author's participat
     [%{read: false}] = Participation.for_user(user)
     [%{read: true}] = Participation.for_user(other_user)
 
-    assert User.get_cached_by_id(user.id).unread_conversation_count == 1
+    assert Participation.unread_count(user) == 1
-    assert User.get_cached_by_id(other_user.id).unread_conversation_count == 0
+    assert Participation.unread_count(other_user) == 0
   end
 
   test "for a new conversation, it sets the recipents of the participation" do
@@ -264,7 +263,7 @@ test "when the user blocks a recipient, the existing conversations with them are
       assert [%{read: false}, %{read: false}, %{read: false}, %{read: false}] =
                Participation.for_user(blocker)
 
-      assert User.get_cached_by_id(blocker.id).unread_conversation_count == 4
+      assert Participation.unread_count(blocker) == 4
 
       {:ok, _user_relationship} = User.block(blocker, blocked)
 
@@ -272,15 +271,15 @@ test "when the user blocks a recipient, the existing conversations with them are
       assert [%{read: true}, %{read: true}, %{read: true}, %{read: false}] =
                Participation.for_user(blocker)
 
-      assert User.get_cached_by_id(blocker.id).unread_conversation_count == 1
+      assert Participation.unread_count(blocker) == 1
 
       # The conversation is not marked as read for the blocked user
       assert [_, _, %{read: false}] = Participation.for_user(blocked)
-      assert User.get_cached_by_id(blocked.id).unread_conversation_count == 1
+      assert Participation.unread_count(blocker) == 1
 
       # The conversation is not marked as read for the third user
       assert [%{read: false}, _, _] = Participation.for_user(third_user)
-      assert User.get_cached_by_id(third_user.id).unread_conversation_count == 1
+      assert Participation.unread_count(third_user) == 1
     end
 
     test "the new conversation with the blocked user is not marked as unread " do
@@ -298,7 +297,7 @@ test "the new conversation with the blocked user is not marked as unread " do
         })
 
       assert [%{read: true}] = Participation.for_user(blocker)
-      assert User.get_cached_by_id(blocker.id).unread_conversation_count == 0
+      assert Participation.unread_count(blocker) == 0
 
       # When the blocked user is a recipient
       {:ok, _direct2} =
@@ -308,10 +307,10 @@ test "the new conversation with the blocked user is not marked as unread " do
         })
 
       assert [%{read: true}, %{read: true}] = Participation.for_user(blocker)
-      assert User.get_cached_by_id(blocker.id).unread_conversation_count == 0
+      assert Participation.unread_count(blocker) == 0
 
       assert [%{read: false}, _] = Participation.for_user(blocked)
-      assert User.get_cached_by_id(blocked.id).unread_conversation_count == 1
+      assert Participation.unread_count(blocked) == 1
     end
 
     test "the conversation with the blocked user is not marked as unread on a reply" do
@@ -327,8 +326,8 @@ test "the conversation with the blocked user is not marked as unread on a reply"
 
       {:ok, _user_relationship} = User.block(blocker, blocked)
       assert [%{read: true}] = Participation.for_user(blocker)
-      assert User.get_cached_by_id(blocker.id).unread_conversation_count == 0
 
+      assert Participation.unread_count(blocker) == 0
       assert [blocked_participation] = Participation.for_user(blocked)
 
       # When it's a reply from the blocked user
@@ -340,8 +339,8 @@ test "the conversation with the blocked user is not marked as unread on a reply"
         })
 
       assert [%{read: true}] = Participation.for_user(blocker)
-      assert User.get_cached_by_id(blocker.id).unread_conversation_count == 0
 
+      assert Participation.unread_count(blocker) == 0
       assert [third_user_participation] = Participation.for_user(third_user)
 
       # When it's a reply from the third user
@@ -353,11 +352,12 @@ test "the conversation with the blocked user is not marked as unread on a reply"
         })
 
       assert [%{read: true}] = Participation.for_user(blocker)
-      assert User.get_cached_by_id(blocker.id).unread_conversation_count == 0
+      assert Participation.unread_count(blocker) == 0
 
       # Marked as unread for the blocked user
       assert [%{read: false}] = Participation.for_user(blocked)
-      assert User.get_cached_by_id(blocked.id).unread_conversation_count == 1
+
+      assert Participation.unread_count(blocked) == 1
     end
   end
 end

test/pleroma/web/mastodon_api/controllers/conversation_controller_test.exs

@@ -5,6 +5,7 @@
 defmodule Pleroma.Web.MastodonAPI.ConversationControllerTest do
   use Pleroma.Web.ConnCase
 
+  alias Pleroma.Conversation.Participation
   alias Pleroma.User
   alias Pleroma.Web.CommonAPI
 
@@ -28,10 +29,10 @@ test "returns correct conversations", %{
       user_three: user_three,
       conn: conn
     } do
-      assert User.get_cached_by_id(user_two.id).unread_conversation_count == 0
+      assert Participation.unread_count(user_two) == 0
       {:ok, direct} = create_direct_message(user_one, [user_two, user_three])
 
-      assert User.get_cached_by_id(user_two.id).unread_conversation_count == 1
+      assert Participation.unread_count(user_two) == 1
 
       {:ok, _follower_only} =
         CommonAPI.post(user_one, %{
@@ -59,7 +60,7 @@ test "returns correct conversations", %{
       assert is_binary(res_id)
       assert unread == false
       assert res_last_status["id"] == direct.id
-      assert User.get_cached_by_id(user_one.id).unread_conversation_count == 0
+      assert Participation.unread_count(user_one) == 0
     end
 
     test "observes limit params", %{
@@ -134,8 +135,8 @@ test "the user marks a conversation as read", %{user: user_one, conn: conn} do
     user_two = insert(:user)
     {:ok, direct} = create_direct_message(user_one, [user_two])
 
-    assert User.get_cached_by_id(user_one.id).unread_conversation_count == 0
+    assert Participation.unread_count(user_one) == 0
-    assert User.get_cached_by_id(user_two.id).unread_conversation_count == 1
+    assert Participation.unread_count(user_two) == 1
 
     user_two_conn =
       build_conn()
@@ -155,8 +156,8 @@ test "the user marks a conversation as read", %{user: user_one, conn: conn} do
       |> post("/api/v1/conversations/#{direct_conversation_id}/read")
       |> json_response_and_validate_schema(200)
 
-    assert User.get_cached_by_id(user_one.id).unread_conversation_count == 0
+    assert Participation.unread_count(user_one) == 0
-    assert User.get_cached_by_id(user_two.id).unread_conversation_count == 0
+    assert Participation.unread_count(user_two) == 0
 
     # The conversation is marked as unread on reply
     {:ok, _} =
@@ -171,8 +172,8 @@ test "the user marks a conversation as read", %{user: user_one, conn: conn} do
       |> get("/api/v1/conversations")
       |> json_response_and_validate_schema(200)
 
-    assert User.get_cached_by_id(user_one.id).unread_conversation_count == 1
+    assert Participation.unread_count(user_one) == 1
-    assert User.get_cached_by_id(user_two.id).unread_conversation_count == 0
+    assert Participation.unread_count(user_two) == 0
 
     # A reply doesn't increment the user's unread_conversation_count if the conversation is unread
     {:ok, _} =
@@ -182,8 +183,8 @@ test "the user marks a conversation as read", %{user: user_one, conn: conn} do
         in_reply_to_status_id: direct.id
       })
 
-    assert User.get_cached_by_id(user_one.id).unread_conversation_count == 1
+    assert Participation.unread_count(user_one) == 1
-    assert User.get_cached_by_id(user_two.id).unread_conversation_count == 0
+    assert Participation.unread_count(user_two) == 0
   end
 
   test "(vanilla) Mastodon frontend behaviour", %{user: user_one, conn: conn} do

test/pleroma/web/pleroma_api/controllers/conversation_controller_test.exs

@@ -121,7 +121,7 @@ test "POST /api/v1/pleroma/conversations/read" do
     [participation2, participation1] = Participation.for_user(other_user)
     assert Participation.get(participation2.id).read == false
     assert Participation.get(participation1.id).read == false
-    assert User.get_cached_by_id(other_user.id).unread_conversation_count == 2
+    assert Participation.unread_count(other_user) == 2
 
     [%{"unread" => false}, %{"unread" => false}] =
       conn
@@ -131,6 +131,6 @@ test "POST /api/v1/pleroma/conversations/read" do
     [participation2, participation1] = Participation.for_user(other_user)
     assert Participation.get(participation2.id).read == true
     assert Participation.get(participation1.id).read == true
-    assert User.get_cached_by_id(other_user.id).unread_conversation_count == 0
+    assert Participation.unread_count(other_user) == 0
   end
 end