From 519c5abce5cbf7168d2e6adfa8ef19157738abd6 Mon Sep 17 00:00:00 2001 From: Benjamin Piouffle Date: Fri, 15 Mar 2019 22:52:49 +0100 Subject: [PATCH] chore(Notifications): Properly subscribe and add graphql queries --- apps/cf/lib/comments/comments.ex | 44 +++++++++++++++---- apps/cf/lib/notifications/subscriptions.ex | 20 +++++++-- .../cf_graphql/lib/resolvers/notifications.ex | 38 ++++++++++++++++ apps/cf_graphql/lib/router.ex | 6 +-- apps/cf_graphql/lib/schema/schema.ex | 10 +++++ .../lib/schema/types/user_action.ex | 6 +++ apps/db/lib/db_schema/subscription.ex | 7 ++- apps/db/lib/db_type/subscription_reason.ex | 6 ++- 8 files changed, 118 insertions(+), 19 deletions(-) diff --git a/apps/cf/lib/comments/comments.ex b/apps/cf/lib/comments/comments.ex index 828a9007..0b7f3fe3 100644 --- a/apps/cf/lib/comments/comments.ex +++ b/apps/cf/lib/comments/comments.ex @@ -5,6 +5,7 @@ defmodule CF.Comments do import CF.Actions.ActionCreator alias Ecto.Multi + alias Kaur.Result alias DB.Repo alias DB.Schema.Source @@ -14,6 +15,7 @@ defmodule CF.Comments do alias DB.Schema.UserAction alias CF.Accounts.UserPermissions + alias CF.Notifications.Subscriptions alias CF.Sources # ---- Public API ---- @@ -28,6 +30,12 @@ defmodule CF.Comments do |> Repo.all() end + @doc """ + Add a new comment. + + [!] This function is very bad and should be refactored, especially the async + source fetcher should be moved to a job. + """ def add_comment(user, video_id, params, source_url \\ nil, source_fetch_callback \\ nil) do # TODO [Security] What if reply_to_id refer to a comment that is on a different statement ? UserPermissions.check!(user, :create, :comment) @@ -38,26 +46,44 @@ defmodule CF.Comments do source_url && (Sources.get_by_url(source_url) || Source.changeset(%Source{}, %{url: source_url})) - # Insert comment in DB - full_comment = + comment_changeset = user |> Ecto.build_assoc(:comments) |> Ecto.Changeset.change() |> Ecto.Changeset.put_assoc(:source, source) |> Comment.changeset(params) + + Multi.new() + |> Multi.run(:comment, fn _ -> + comment_changeset |> Repo.insert!() |> Map.put(:user, user) - |> Repo.preload(:source) + |> Repo.preload([:source, :statement]) |> Map.put(:score, 0) + |> Result.ok() + end) + |> Multi.run(:action, fn %{comment: comment} -> + Repo.insert(action_create(user.id, video_id, comment, source_url)) + end) + |> Multi.run(:suscription, fn %{comment: comment} -> + Subscriptions.subscribe(user, comment, :is_author) + end) + |> Repo.transaction() + |> case do + {:error, _operation, reason, _changes} -> + {:error, reason} - # Record action - Repo.insert(action_create(user.id, video_id, full_comment, source_url)) + {:ok, %{comment: comment}} -> + # Set default on comment + full_comment = comment - # If new source, fetch metadata - unless is_nil(source) || !is_nil(Map.get(source, :id)), - do: fetch_source_metadata_and_update_comment(full_comment, source_fetch_callback) + # If new source, fetch metadata + if source && is_nil(Map.get(source, :id)), + do: fetch_source_metadata_and_update_comment(comment, source_fetch_callback) - full_comment + # Return comment + full_comment + end end # Delete diff --git a/apps/cf/lib/notifications/subscriptions.ex b/apps/cf/lib/notifications/subscriptions.ex index 870b0905..3fdd280b 100644 --- a/apps/cf/lib/notifications/subscriptions.ex +++ b/apps/cf/lib/notifications/subscriptions.ex @@ -32,12 +32,18 @@ defmodule CF.Notifications.Subscriptions do """ def is_subscribed(%User{id: user_id}, %Video{id: video_id}) do Subscription - |> select([:id]) + |> select([:id, :is_subscribed]) |> where([s], s.scope == ^:video) |> where([s], s.user_id == ^user_id) |> where([s], s.video_id == ^video_id) |> DB.Repo.one() - |> Kernel.!=(nil) + |> case do + nil -> + false + + %{is_subscribed: is_subscribed} = a -> + is_subscribed + end end @doc """ @@ -56,8 +62,14 @@ defmodule CF.Notifications.Subscriptions do """ @spec unsubscribe(User.t(), subscribable_entities()) :: {:ok, Subscription.t()} | nil def unsubscribe(user, entity) do - with subscription when not is_nil(subscription) <- load_subscription(user, entity) do - DB.Repo.delete(subscription) + user + |> load_subscription(entity) + |> case do + nil -> + {:error, "not_found"} + + subscription -> + DB.Repo.update(Subscription.changeset_subscribed(subscription, false)) end end diff --git a/apps/cf_graphql/lib/resolvers/notifications.ex b/apps/cf_graphql/lib/resolvers/notifications.ex index 9a1dcad2..46ae97db 100644 --- a/apps/cf_graphql/lib/resolvers/notifications.ex +++ b/apps/cf_graphql/lib/resolvers/notifications.ex @@ -46,4 +46,42 @@ defmodule CF.Graphql.Resolvers.Notifications do |> elem(1) |> Result.ok() end + + @doc """ + Update the given subscription + """ + def update_subscription( + _, + %{ + scope: scope, + entity_id: entity_id, + is_subscribed: is_subscribed + } = params, + %{context: %{user: loggedin_user}} + ) do + entity = load_entity(scope, entity_id) + + cond do + is_nil(entity) -> + {:error, "not_found"} + + is_subscribed -> + Subscriptions.subscribe(loggedin_user, entity, params[:reason]) + + true -> + Subscriptions.unsubscribe(loggedin_user, entity) + end + end + + defp load_entity("comment", entity_id) do + DB.Repo.get(DB.Schema.Comment, entity_id) + end + + defp load_entity("statement", entity_id) do + DB.Repo.get(DB.Schema.Statement, entity_id) + end + + defp load_entity("video", entity_id) do + DB.Repo.get(DB.Schema.Video, entity_id) + end end diff --git a/apps/cf_graphql/lib/router.ex b/apps/cf_graphql/lib/router.ex index cf360de4..ab4ae2ad 100644 --- a/apps/cf_graphql/lib/router.ex +++ b/apps/cf_graphql/lib/router.ex @@ -27,8 +27,7 @@ defmodule CF.GraphQLWeb.Router do Absinthe.Plug.GraphiQL, schema: CF.Graphql.Schema, analyze_complexity: true, - # (6 joins = 300) + 20 fields - max_complexity: 320 + max_complexity: 400 ) end @@ -37,8 +36,7 @@ defmodule CF.GraphQLWeb.Router do Absinthe.Plug, schema: CF.Graphql.Schema, analyze_complexity: true, - # (6 joins = 300) + 20 fields - max_complexity: 320 + max_complexity: 400 ) end end diff --git a/apps/cf_graphql/lib/schema/schema.ex b/apps/cf_graphql/lib/schema/schema.ex index e2814eb7..3117fb20 100644 --- a/apps/cf_graphql/lib/schema/schema.ex +++ b/apps/cf_graphql/lib/schema/schema.ex @@ -68,5 +68,15 @@ defmodule CF.Graphql.Schema do resolve(&Resolvers.Notifications.update/3) end + + @desc "Use this to (un)subscribe from an item notifications" + field :update_subscription, :notifications_subscription do + arg(:scope, non_null(:string)) + arg(:entity_id, non_null(:id)) + arg(:is_subscribed, non_null(:boolean)) + arg(:reason, :string) + + resolve(&Resolvers.Notifications.update_subscription/3) + end end end diff --git a/apps/cf_graphql/lib/schema/types/user_action.ex b/apps/cf_graphql/lib/schema/types/user_action.ex index 70162eac..3c260566 100644 --- a/apps/cf_graphql/lib/schema/types/user_action.ex +++ b/apps/cf_graphql/lib/schema/types/user_action.ex @@ -67,6 +67,12 @@ defmodule CF.Graphql.Schema.Types.UserAction do resolve(assoc(:comment)) end + @desc "Associated speaker" + field :speaker, :speaker do + complexity(join_complexity()) + resolve(assoc(:speaker)) + end + @desc "A map with all the changes made by this action" field( :changes, diff --git a/apps/db/lib/db_schema/subscription.ex b/apps/db/lib/db_schema/subscription.ex index 55700f4e..3ee84944 100644 --- a/apps/db/lib/db_schema/subscription.ex +++ b/apps/db/lib/db_schema/subscription.ex @@ -63,10 +63,15 @@ defmodule DB.Schema.Subscription do changeset(struct, %{ video_id: video.id, scope: :video, - reason: reason + reason: reason, + is_subscribed: true }) end + def changeset_subscribed(struct, is_subscribed) do + change(struct, is_subscribed: is_subscribed) + end + defp validate_required_by_scope(changeset) do case get_field(changeset, :scope) do nil -> diff --git a/apps/db/lib/db_type/subscription_reason.ex b/apps/db/lib/db_type/subscription_reason.ex index 1de6bab2..3d14e4b2 100644 --- a/apps/db/lib/db_type/subscription_reason.ex +++ b/apps/db/lib/db_type/subscription_reason.ex @@ -3,5 +3,9 @@ import EctoEnum defenum( DB.Type.SubscriptionReason, :subscription_reason, - [:is_author] + [ + :is_author, + :manual, + :suggestion + ] )