From c8104687834ff688ad4abfffb44988cad8e68972 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Mon, 18 Nov 2019 18:40:03 +0100 Subject: [PATCH] Validate ends_on being after begins_on Closes #315 Signed-off-by: Thomas Citharel --- lib/mobilizon/events/event.ex | 18 ++++- lib/mobilizon_web/resolvers/event.ex | 9 ++- lib/service/activity_pub/activity_pub.ex | 30 ++++----- lib/service/activity_pub/transmogrifier.ex | 6 +- .../resolvers/event_resolver_test.exs | 67 +++++++++++++++++++ 5 files changed, 104 insertions(+), 26 deletions(-) diff --git a/lib/mobilizon/events/event.ex b/lib/mobilizon/events/event.ex index ce0b2b5ad..9cbe0023b 100644 --- a/lib/mobilizon/events/event.ex +++ b/lib/mobilizon/events/event.ex @@ -128,7 +128,6 @@ defmodule Mobilizon.Events.Event do |> common_changeset(attrs) |> put_creator_if_published(:create) |> validate_required(@required_attrs) - |> validate_lengths() end @doc false @@ -139,7 +138,6 @@ defmodule Mobilizon.Events.Event do |> common_changeset(attrs) |> put_creator_if_published(:update) |> validate_required(@update_required_attrs) - |> validate_lengths() end @spec common_changeset(Changeset.t(), map) :: Changeset.t() @@ -149,6 +147,8 @@ defmodule Mobilizon.Events.Event do |> put_tags(attrs) |> put_address(attrs) |> put_picture(attrs) + |> validate_lengths() + |> validate_end_time() end @spec validate_lengths(Changeset.t()) :: Changeset.t() @@ -162,6 +162,20 @@ defmodule Mobilizon.Events.Event do |> validate_length(:slug, min: 3, max: 200) end + defp validate_end_time(%Changeset{} = changeset) do + case fetch_field(changeset, :begins_on) do + {_, begins_on} -> + validate_change(changeset, :ends_on, fn :ends_on, ends_on -> + if begins_on > ends_on, + do: [ends_on: "ends_on cannot be set before begins_on"], + else: [] + end) + + :error -> + changeset + end + end + @doc """ Checks whether an event can be managed. """ diff --git a/lib/mobilizon_web/resolvers/event.ex b/lib/mobilizon_web/resolvers/event.ex index 630326abd..2efc53830 100644 --- a/lib/mobilizon_web/resolvers/event.ex +++ b/lib/mobilizon_web/resolvers/event.ex @@ -275,6 +275,9 @@ defmodule MobilizonWeb.Resolvers.Event do {:is_owned, nil} -> {:error, "Organizer actor id is not owned by the user"} + {:error, _, %Ecto.Changeset{} = error, _} -> + {:error, error} + {:error, %Ecto.Changeset{} = error} -> {:error, error} end @@ -292,15 +295,12 @@ defmodule MobilizonWeb.Resolvers.Event do %{event_id: event_id} = args, %{context: %{current_user: user}} = _resolution ) do - require Logger - Logger.error(inspect(args)) # See https://github.com/absinthe-graphql/absinthe/issues/490 with args <- Map.put(args, :options, args[:options] || %{}), {:ok, %Event{} = event} <- Events.get_event_with_preload(event_id), organizer_actor_id <- args |> Map.get(:organizer_actor_id, event.organizer_actor_id), {:is_owned, %Actor{} = organizer_actor} <- User.owns_actor(user, organizer_actor_id), - :ok <- Logger.error(inspect(organizer_actor)), args <- Map.put(args, :organizer_actor, organizer_actor), {:ok, %Activity{data: %{"object" => %{"type" => "Event"}}}, %Event{} = event} <- MobilizonWeb.API.Events.update_event(args, event) do @@ -311,6 +311,9 @@ defmodule MobilizonWeb.Resolvers.Event do {:is_owned, nil} -> {:error, "User doesn't own actor"} + + {:error, _, %Ecto.Changeset{} = error, _} -> + {:error, error} end end diff --git a/lib/service/activity_pub/activity_pub.ex b/lib/service/activity_pub/activity_pub.ex index 428394a27..778e7b1d3 100644 --- a/lib/service/activity_pub/activity_pub.ex +++ b/lib/service/activity_pub/activity_pub.ex @@ -93,8 +93,6 @@ defmodule Mobilizon.Service.ActivityPub do {:ok, Actors.get_actor_by_url!(actor_url, true)} e -> - require Logger - Logger.error(inspect(e)) {:error, e} end end @@ -135,14 +133,13 @@ defmodule Mobilizon.Service.ActivityPub do Logger.debug("creating an activity") Logger.debug(inspect(args)) - {:ok, entity, create_data} = - case type do - :event -> create_event(args, additional) - :comment -> create_comment(args, additional) - :group -> create_group(args, additional) - end - - with {:ok, activity} <- create_activity(create_data, local), + with {:ok, entity, create_data} <- + (case type do + :event -> create_event(args, additional) + :comment -> create_comment(args, additional) + :group -> create_group(args, additional) + end), + {:ok, activity} <- create_activity(create_data, local), :ok <- maybe_federate(activity) do {:ok, activity, entity} else @@ -167,13 +164,12 @@ defmodule Mobilizon.Service.ActivityPub do Logger.debug("updating an activity") Logger.debug(inspect(args)) - {:ok, entity, update_data} = - case type do - :event -> update_event(old_entity, args, additional) - :actor -> update_actor(old_entity, args, additional) - end - - with {:ok, activity} <- create_activity(update_data, local), + with {:ok, entity, update_data} <- + (case type do + :event -> update_event(old_entity, args, additional) + :actor -> update_actor(old_entity, args, additional) + end), + {:ok, activity} <- create_activity(update_data, local), :ok <- maybe_federate(activity) do {:ok, activity, entity} else diff --git a/lib/service/activity_pub/transmogrifier.ex b/lib/service/activity_pub/transmogrifier.ex index c14148b49..1f7cd7f76 100644 --- a/lib/service/activity_pub/transmogrifier.ex +++ b/lib/service/activity_pub/transmogrifier.ex @@ -339,8 +339,7 @@ defmodule Mobilizon.Service.ActivityPub.Transmogrifier do ActivityPub.update(:event, old_event, object_data, false) do {:ok, activity, new_event} else - e -> - Logger.error(inspect(e)) + _e -> :error end end @@ -442,8 +441,7 @@ defmodule Mobilizon.Service.ActivityPub.Transmogrifier do :error - e -> - Logger.error(inspect(e)) + _e -> :error end end diff --git a/test/mobilizon_web/resolvers/event_resolver_test.exs b/test/mobilizon_web/resolvers/event_resolver_test.exs index 01f62fd87..512c59cd0 100644 --- a/test/mobilizon_web/resolvers/event_resolver_test.exs +++ b/test/mobilizon_web/resolvers/event_resolver_test.exs @@ -95,6 +95,40 @@ defmodule MobilizonWeb.Resolvers.EventResolverTest do "Organizer actor id is not owned by the user" end + test "create_event/3 should check that end time is after start time", %{ + conn: conn, + actor: actor, + user: user + } do + begins_on = DateTime.utc_now() |> DateTime.truncate(:second) + ends_on = Timex.shift(begins_on, hours: -2) + + mutation = """ + mutation { + createEvent( + title: "come to my event", + description: "it will be fine", + begins_on: "#{DateTime.to_iso8601(begins_on)}", + ends_on: "#{DateTime.to_iso8601(ends_on)}", + organizer_actor_id: "#{actor.id}", + category: "birthday" + ) { + id, + title, + uuid + } + } + """ + + res = + conn + |> auth_conn(user) + |> post("/api", AbsintheHelpers.mutation_skeleton(mutation)) + + assert hd(json_response(res, 200)["errors"])["message"] == + "ends_on cannot be set before begins_on" + end + test "create_event/3 creates an event", %{conn: conn, actor: actor, user: user} do mutation = """ mutation { @@ -661,6 +695,39 @@ defmodule MobilizonWeb.Resolvers.EventResolverTest do assert hd(json_response(res, 200)["errors"])["message"] == "User doesn't own actor" end + test "update_event/3 should check end time is after the beginning time", %{ + conn: conn, + actor: actor, + user: user + } do + event = insert(:event, organizer_actor: actor) + + mutation = """ + mutation { + updateEvent( + title: "my event updated", + ends_on: "#{Timex.shift(event.begins_on, hours: -2)}", + event_id: #{event.id} + ) { + title, + uuid, + tags { + title, + slug + } + } + } + """ + + res = + conn + |> auth_conn(user) + |> post("/api", AbsintheHelpers.mutation_skeleton(mutation)) + + assert hd(json_response(res, 200)["errors"])["message"] == + "ends_on cannot be set before begins_on" + end + test "update_event/3 updates an event", %{conn: conn, actor: actor, user: user} do event = insert(:event, organizer_actor: actor) _creator = insert(:participant, event: event, actor: actor, role: :creator)