From 7f5c5b11a5baeddec36ccc01b4954ac8aa9f8590 Mon Sep 17 00:00:00 2001 From: lain Date: Wed, 3 Jun 2020 14:26:50 +0200 Subject: [PATCH] Chats: Remove `unread` from the db, calculate from unseen messages. --- lib/pleroma/chat.ex | 13 +++---------- lib/pleroma/chat_message_reference.ex | 16 ++++++++++++++++ lib/pleroma/web/activity_pub/side_effects.ex | 9 ++------- .../pleroma_api/controllers/chat_controller.ex | 2 +- lib/pleroma/web/pleroma_api/views/chat_view.ex | 2 +- .../20200603120448_remove_unread_from_chats.exs | 9 +++++++++ test/chat_test.exs | 6 +----- test/web/activity_pub/side_effects_test.exs | 2 -- .../controllers/chat_controller_test.exs | 11 +++++++---- 9 files changed, 40 insertions(+), 30 deletions(-) create mode 100644 priv/repo/migrations/20200603120448_remove_unread_from_chats.exs diff --git a/lib/pleroma/chat.ex b/lib/pleroma/chat.ex index 65938c7a4..5aefddc5e 100644 --- a/lib/pleroma/chat.ex +++ b/lib/pleroma/chat.ex @@ -19,14 +19,13 @@ defmodule Pleroma.Chat do schema "chats" do belongs_to(:user, User, type: FlakeId.Ecto.CompatType) field(:recipient, :string) - field(:unread, :integer, default: 0, read_after_writes: true) timestamps() end def creation_cng(struct, params) do struct - |> cast(params, [:user_id, :recipient, :unread]) + |> cast(params, [:user_id, :recipient]) |> validate_change(:recipient, fn :recipient, recipient -> case User.get_cached_by_ap_id(recipient) do @@ -61,16 +60,10 @@ def get_or_create(user_id, recipient) do def bump_or_create(user_id, recipient) do %__MODULE__{} - |> creation_cng(%{user_id: user_id, recipient: recipient, unread: 1}) + |> creation_cng(%{user_id: user_id, recipient: recipient}) |> Repo.insert( - on_conflict: [set: [updated_at: NaiveDateTime.utc_now()], inc: [unread: 1]], + on_conflict: [set: [updated_at: NaiveDateTime.utc_now()]], conflict_target: [:user_id, :recipient] ) end - - def mark_as_read(chat) do - chat - |> change(%{unread: 0}) - |> Repo.update() - end end diff --git a/lib/pleroma/chat_message_reference.ex b/lib/pleroma/chat_message_reference.ex index 6808d1365..ad174b294 100644 --- a/lib/pleroma/chat_message_reference.ex +++ b/lib/pleroma/chat_message_reference.ex @@ -84,4 +84,20 @@ def create(chat, object, seen) do |> changeset(params) |> Repo.insert() end + + def unread_count_for_chat(chat) do + chat + |> for_chat_query() + |> where([cmr], cmr.seen == false) + |> Repo.aggregate(:count) + end + + def set_all_seen_for_chat(chat) do + chat + |> for_chat_query() + |> exclude(:order_by) + |> exclude(:preload) + |> where([cmr], cmr.seen == false) + |> Repo.update_all(set: [seen: true]) + end end diff --git a/lib/pleroma/web/activity_pub/side_effects.ex b/lib/pleroma/web/activity_pub/side_effects.ex index cda52b00e..884d399d0 100644 --- a/lib/pleroma/web/activity_pub/side_effects.ex +++ b/lib/pleroma/web/activity_pub/side_effects.ex @@ -139,13 +139,8 @@ def handle_object_creation(%{"type" => "ChatMessage"} = object, meta) do [[actor, recipient], [recipient, actor]] |> Enum.each(fn [user, other_user] -> if user.local do - if user.ap_id == actor.ap_id do - {:ok, chat} = Chat.get_or_create(user.id, other_user.ap_id) - ChatMessageReference.create(chat, object, true) - else - {:ok, chat} = Chat.bump_or_create(user.id, other_user.ap_id) - ChatMessageReference.create(chat, object, false) - end + {:ok, chat} = Chat.bump_or_create(user.id, other_user.ap_id) + ChatMessageReference.create(chat, object, user.ap_id == actor.ap_id) end end) diff --git a/lib/pleroma/web/pleroma_api/controllers/chat_controller.ex b/lib/pleroma/web/pleroma_api/controllers/chat_controller.ex index f22f33de9..29922da99 100644 --- a/lib/pleroma/web/pleroma_api/controllers/chat_controller.ex +++ b/lib/pleroma/web/pleroma_api/controllers/chat_controller.ex @@ -90,7 +90,7 @@ def post_chat_message( def mark_as_read(%{assigns: %{user: %{id: user_id}}} = conn, %{id: id}) do with %Chat{} = chat <- Repo.get_by(Chat, id: id, user_id: user_id), - {:ok, chat} <- Chat.mark_as_read(chat) do + {_n, _} <- ChatMessageReference.set_all_seen_for_chat(chat) do conn |> put_view(ChatView) |> render("show.json", chat: chat) diff --git a/lib/pleroma/web/pleroma_api/views/chat_view.ex b/lib/pleroma/web/pleroma_api/views/chat_view.ex index 331c1d282..c903a71fd 100644 --- a/lib/pleroma/web/pleroma_api/views/chat_view.ex +++ b/lib/pleroma/web/pleroma_api/views/chat_view.ex @@ -20,7 +20,7 @@ def render("show.json", %{chat: %Chat{} = chat} = opts) do %{ id: chat.id |> to_string(), account: AccountView.render("show.json", Map.put(opts, :user, recipient)), - unread: chat.unread, + unread: ChatMessageReference.unread_count_for_chat(chat), last_message: last_message && ChatMessageReferenceView.render("show.json", chat_message_reference: last_message), diff --git a/priv/repo/migrations/20200603120448_remove_unread_from_chats.exs b/priv/repo/migrations/20200603120448_remove_unread_from_chats.exs new file mode 100644 index 000000000..6322137d5 --- /dev/null +++ b/priv/repo/migrations/20200603120448_remove_unread_from_chats.exs @@ -0,0 +1,9 @@ +defmodule Pleroma.Repo.Migrations.RemoveUnreadFromChats do + use Ecto.Migration + + def change do + alter table(:chats) do + remove(:unread, :integer, default: 0) + end + end +end diff --git a/test/chat_test.exs b/test/chat_test.exs index 42e01fe27..332f2180a 100644 --- a/test/chat_test.exs +++ b/test/chat_test.exs @@ -6,7 +6,6 @@ defmodule Pleroma.ChatTest do use Pleroma.DataCase, async: true alias Pleroma.Chat - alias Pleroma.Web.CommonAPI import Pleroma.Factory @@ -35,7 +34,6 @@ test "it returns and bumps a chat for a user and recipient if it already exists" {:ok, chat_two} = Chat.bump_or_create(user.id, other_user.ap_id) assert chat.id == chat_two.id - assert chat_two.unread == 2 end test "it returns a chat for a user and recipient if it already exists" do @@ -48,15 +46,13 @@ test "it returns a chat for a user and recipient if it already exists" do assert chat.id == chat_two.id end - test "a returning chat will have an updated `update_at` field and an incremented unread count" do + test "a returning chat will have an updated `update_at` field" do user = insert(:user) other_user = insert(:user) {:ok, chat} = Chat.bump_or_create(user.id, other_user.ap_id) - assert chat.unread == 1 :timer.sleep(1500) {:ok, chat_two} = Chat.bump_or_create(user.id, other_user.ap_id) - assert chat_two.unread == 2 assert chat.id == chat_two.id assert chat.updated_at != chat_two.updated_at diff --git a/test/web/activity_pub/side_effects_test.exs b/test/web/activity_pub/side_effects_test.exs index ff6b3ac15..f2fa062b4 100644 --- a/test/web/activity_pub/side_effects_test.exs +++ b/test/web/activity_pub/side_effects_test.exs @@ -346,7 +346,6 @@ test "it creates a Chat and ChatMessageReferences for the local users and bumps SideEffects.handle(create_activity, local: false, object_data: chat_message_data) chat = Chat.get(author.id, recipient.ap_id) - assert chat.unread == 0 [cm_ref] = ChatMessageReference.for_chat_query(chat) |> Repo.all() @@ -354,7 +353,6 @@ test "it creates a Chat and ChatMessageReferences for the local users and bumps assert cm_ref.seen == true chat = Chat.get(recipient.id, author.ap_id) - assert chat.unread == 1 [cm_ref] = ChatMessageReference.for_chat_query(chat) |> Repo.all() diff --git a/test/web/pleroma_api/controllers/chat_controller_test.exs b/test/web/pleroma_api/controllers/chat_controller_test.exs index bd4024c09..e62b71799 100644 --- a/test/web/pleroma_api/controllers/chat_controller_test.exs +++ b/test/web/pleroma_api/controllers/chat_controller_test.exs @@ -19,9 +19,12 @@ defmodule Pleroma.Web.PleromaAPI.ChatControllerTest do test "it marks all messages in a chat as read", %{conn: conn, user: user} do other_user = insert(:user) - {:ok, chat} = Chat.bump_or_create(user.id, other_user.ap_id) + {:ok, create} = CommonAPI.post_chat_message(other_user, user, "sup") + {:ok, chat} = Chat.get_or_create(user.id, other_user.ap_id) + object = Object.normalize(create, false) + cm_ref = ChatMessageReference.for_chat_and_object(chat, object) - assert chat.unread == 1 + assert cm_ref.seen == false result = conn @@ -30,9 +33,9 @@ test "it marks all messages in a chat as read", %{conn: conn, user: user} do assert result["unread"] == 0 - {:ok, chat} = Chat.get_or_create(user.id, other_user.ap_id) + cm_ref = ChatMessageReference.for_chat_and_object(chat, object) - assert chat.unread == 0 + assert cm_ref.seen == true end end