From 0d5088c2b83fafd9d8da1f1b04936f831ac5ee87 Mon Sep 17 00:00:00 2001 From: Maksim Pechnikov Date: Tue, 1 Sep 2020 09:37:08 +0300 Subject: [PATCH] remove `unread_conversation_count` from User --- lib/pleroma/conversation.ex | 6 +-- lib/pleroma/conversation/participation.ex | 27 +++++------- lib/pleroma/user.ex | 42 ------------------- .../web/mastodon_api/views/account_view.ex | 2 +- ...ve_unread_conversation_count_from_user.exs | 38 +++++++++++++++++ ...ad_index_to_conversation_participation.exs | 12 ++++++ test/conversation/participation_test.exs | 32 +++++++------- .../conversation_controller_test.exs | 23 +++++----- .../conversation_controller_test.exs | 4 +- 9 files changed, 92 insertions(+), 94 deletions(-) create mode 100644 priv/repo/migrations/20200831114918_remove_unread_conversation_count_from_user.exs create mode 100644 priv/repo/migrations/20200831115854_add_unread_index_to_conversation_participation.exs diff --git a/lib/pleroma/conversation.ex b/lib/pleroma/conversation.ex index e76eb0087..77933f0be 100644 --- a/lib/pleroma/conversation.ex +++ b/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} = diff --git a/lib/pleroma/conversation/participation.ex b/lib/pleroma/conversation/participation.ex index 8bc3e85d6..4c32b273a 100644 --- a/lib/pleroma/conversation/participation.ex +++ b/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 - __MODULE__ - |> where(id: ^participation.id) - |> update(set: [read: true]) - |> 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 + def mark_as_read(%__MODULE__{} = participation) do + participation + |> change(read: true) + |> Repo.update() 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, diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index d2ad9516f..7fc7a533e 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -129,7 +129,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) @@ -1295,47 +1294,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) diff --git a/lib/pleroma/web/mastodon_api/views/account_view.ex b/lib/pleroma/web/mastodon_api/views/account_view.ex index 864c0417f..1bf53600c 100644 --- a/lib/pleroma/web/mastodon_api/views/account_view.ex +++ b/lib/pleroma/web/mastodon_api/views/account_view.ex @@ -386,7 +386,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 diff --git a/priv/repo/migrations/20200831114918_remove_unread_conversation_count_from_user.exs b/priv/repo/migrations/20200831114918_remove_unread_conversation_count_from_user.exs new file mode 100644 index 000000000..b7bdb9166 --- /dev/null +++ b/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 diff --git a/priv/repo/migrations/20200831115854_add_unread_index_to_conversation_participation.exs b/priv/repo/migrations/20200831115854_add_unread_index_to_conversation_participation.exs new file mode 100644 index 000000000..68771c655 --- /dev/null +++ b/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 diff --git a/test/conversation/participation_test.exs b/test/conversation/participation_test.exs index 59a1b6492..5a603dcc1 100644 --- a/test/conversation/participation_test.exs +++ b/test/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 User.get_cached_by_id(user.id).unread_conversation_count == 0 - assert User.get_cached_by_id(other_user.id).unread_conversation_count == 1 + assert Participation.unread_count(user) == 0 + assert Participation.unread_count(other_user) == 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 User.get_cached_by_id(other_user.id).unread_conversation_count == 0 + assert Participation.unread_count(user) == 1 + 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 diff --git a/test/web/mastodon_api/controllers/conversation_controller_test.exs b/test/web/mastodon_api/controllers/conversation_controller_test.exs index 3e21e6bf1..b23b22752 100644 --- a/test/web/mastodon_api/controllers/conversation_controller_test.exs +++ b/test/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 User.get_cached_by_id(user_two.id).unread_conversation_count == 1 + assert Participation.unread_count(user_one) == 0 + 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 User.get_cached_by_id(user_two.id).unread_conversation_count == 0 + assert Participation.unread_count(user_one) == 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 User.get_cached_by_id(user_two.id).unread_conversation_count == 0 + assert Participation.unread_count(user_one) == 1 + 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 User.get_cached_by_id(user_two.id).unread_conversation_count == 0 + assert Participation.unread_count(user_one) == 1 + assert Participation.unread_count(user_two) == 0 end test "(vanilla) Mastodon frontend behaviour", %{user: user_one, conn: conn} do diff --git a/test/web/pleroma_api/controllers/conversation_controller_test.exs b/test/web/pleroma_api/controllers/conversation_controller_test.exs index e6d0b3e37..f2feeaaef 100644 --- a/test/web/pleroma_api/controllers/conversation_controller_test.exs +++ b/test/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