From b1ac997f8ab8884f902c7dc532f8cac520666304 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Fri, 1 Apr 2022 10:03:43 +0200 Subject: [PATCH 1/5] Make sure every relation of actor is loaded when operating on it Closes #1049 Signed-off-by: Thomas Citharel --- lib/mobilizon/actors/actors.ex | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/mobilizon/actors/actors.ex b/lib/mobilizon/actors/actors.ex index 4b4344c89..9058ec482 100644 --- a/lib/mobilizon/actors/actors.ex +++ b/lib/mobilizon/actors/actors.ex @@ -58,6 +58,8 @@ defmodule Mobilizon.Actors do @moderator_roles [:moderator] ++ @administrator_roles @member_roles [:member] ++ @moderator_roles + @associations_to_preload [:organized_events, :followers, :followings, :user, :physical_address] + @doc """ Gets a single actor. """ @@ -169,7 +171,7 @@ defmodule Mobilizon.Actors do def get_local_actor_by_name_with_preload(name) do name |> get_local_actor_by_name() - |> Repo.preload([:organized_events, :followers, :followings]) + |> Repo.preload(@associations_to_preload) end @doc """ @@ -179,7 +181,7 @@ defmodule Mobilizon.Actors do def get_actor_by_name_with_preload(name, type \\ nil) do name |> get_actor_by_name(type) - |> Repo.preload([:organized_events, :user, :physical_address]) + |> Repo.preload(@associations_to_preload) end @doc """ @@ -246,7 +248,7 @@ defmodule Mobilizon.Actors do @spec update_actor(Actor.t(), map) :: {:ok, Actor.t()} | {:error, Ecto.Changeset.t()} def update_actor(%Actor{} = actor, attrs) do actor - |> Repo.preload([:physical_address]) + |> Repo.preload(@associations_to_preload) |> Actor.update_changeset(attrs) |> delete_files_if_media_changed() |> Repo.update() @@ -271,7 +273,7 @@ defmodule Mobilizon.Actors do case insert do {:ok, actor} -> - actor = if preload, do: Repo.preload(actor, [:followers]), else: actor + actor = if preload, do: Repo.preload(actor, @associations_to_preload), else: actor {:ok, actor} @@ -1304,7 +1306,7 @@ defmodule Mobilizon.Actors do defp actor_with_preload_query(actor_id, true) do Actor |> where([a], a.id == ^actor_id) - |> preload([a], [:organized_events, :followers, :followings]) + |> preload([a], ^@associations_to_preload) end @spec actor_by_username_query(String.t()) :: Ecto.Query.t() From c0ef41cb71747cfe811ee3ece60341214ff4fac2 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Fri, 1 Apr 2022 10:04:22 +0200 Subject: [PATCH 2/5] Fix accessing explore page without config Signed-off-by: Thomas Citharel --- js/src/views/Search.vue | 1 + 1 file changed, 1 insertion(+) diff --git a/js/src/views/Search.vue b/js/src/views/Search.vue index 2ecdec35d..6231c94a4 100644 --- a/js/src/views/Search.vue +++ b/js/src/views/Search.vue @@ -93,6 +93,7 @@ Date: Fri, 1 Apr 2022 12:08:53 +0200 Subject: [PATCH 3/5] Handle errors when loading group pictures Signed-off-by: Thomas Citharel --- js/src/views/Group/GroupSettings.vue | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/js/src/views/Group/GroupSettings.vue b/js/src/views/Group/GroupSettings.vue index da70ab3ef..da5479e33 100644 --- a/js/src/views/Group/GroupSettings.vue +++ b/js/src/views/Group/GroupSettings.vue @@ -259,17 +259,22 @@ export default class GroupSettings extends mixins(GroupMixin) { @Watch("group") async watchUpdateGroup(oldGroup: IGroup, newGroup: IGroup): Promise { - if ( - oldGroup?.avatar !== undefined && - oldGroup?.avatar !== newGroup?.avatar - ) { - this.avatarFile = await buildFileFromIMedia(this.group.avatar); - } - if ( - oldGroup?.banner !== undefined && - oldGroup?.banner !== newGroup?.banner - ) { - this.bannerFile = await buildFileFromIMedia(this.group.banner); + try { + if ( + oldGroup?.avatar !== undefined && + oldGroup?.avatar !== newGroup?.avatar + ) { + this.avatarFile = await buildFileFromIMedia(this.group.avatar); + } + if ( + oldGroup?.banner !== undefined && + oldGroup?.banner !== newGroup?.banner + ) { + this.bannerFile = await buildFileFromIMedia(this.group.banner); + } + } catch (e) { + // Catch errors while building media + console.error(e); } this.editableGroup = { ...this.group }; } From a99d66b68f4df157a205b0cbb634aefd3039eea0 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Fri, 1 Apr 2022 12:09:23 +0200 Subject: [PATCH 4/5] Fix group deletion caused by foreign keys issue with comments & discussions Closes #1016 Signed-off-by: Thomas Citharel --- lib/service/actor_suspension.ex | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/lib/service/actor_suspension.ex b/lib/service/actor_suspension.ex index 1b6272a67..96977ead0 100644 --- a/lib/service/actor_suspension.ex +++ b/lib/service/actor_suspension.ex @@ -190,6 +190,29 @@ defmodule Mobilizon.Service.ActorSuspension do defp delete_discussions(%Multi{} = multi, %Actor{type: :Group, id: actor_id}) do Logger.debug("Delete group's discussions") + + multi = + Multi.run(multi, :group_discussion_comments, fn _, _ -> + group_comments_ids = + Comment + |> join(:inner, [c], d in Discussion, on: c.discussion_id == d.id) + |> where([_c, d], d.actor_id == ^actor_id) + |> select([c], [c.id]) + |> Repo.all() + |> Enum.concat() + + {:ok, group_comments_ids} + end) + + multi = + Multi.delete_all( + multi, + :delete_discussions_comments, + fn %{group_discussion_comments: group_discussion_comments} -> + where(Comment, [c], c.id in ^group_discussion_comments) + end + ) + Multi.delete_all(multi, :delete_discussions, where(Discussion, [e], e.actor_id == ^actor_id)) end From 2bdce8b2fc6bfc5285a27567d2cbff7e2393a7b8 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Fri, 1 Apr 2022 13:54:16 +0200 Subject: [PATCH 5/5] Handle address is invalid while rendering event preview Closes #996 Signed-off-by: Thomas Citharel --- lib/service/address/address.ex | 13 ++++++++++++- lib/service/metadata/event.ex | 10 ++++++++-- lib/service/metadata/utils.ex | 2 +- .../email/participation/card/_metadata.html.heex | 2 +- .../email/participation/card/_metadata.text.eex | 2 +- .../templates/export/event_participants.html.heex | 2 +- test/service/address/address_test.exs | 4 +++- 7 files changed, 27 insertions(+), 8 deletions(-) diff --git a/lib/service/address/address.ex b/lib/service/address/address.ex index 97392712b..12f7a396e 100644 --- a/lib/service/address/address.ex +++ b/lib/service/address/address.ex @@ -7,7 +7,7 @@ defmodule Mobilizon.Service.Address do @type address :: %{name: String.t(), alternative_name: String.t()} - @spec render_address(AddressModel.t()) :: String.t() | no_return + @spec render_address(AddressModel.t()) :: String.t() | nil def render_address(%AddressModel{} = address) do %{name: name, alternative_name: alternative_name} = render_names(address) @@ -22,7 +22,18 @@ defmodule Mobilizon.Service.Address do alternative_name true -> + nil + end + end + + @spec render_address!(AddressModel.t()) :: String.t() | no_return + def render_address!(%AddressModel{} = address) do + case render_address(address) do + nil -> raise ArgumentError, message: "Invalid address" + + address -> + address end end diff --git a/lib/service/metadata/event.ex b/lib/service/metadata/event.ex index 0e361f41d..aa6011e93 100644 --- a/lib/service/metadata/event.ex +++ b/lib/service/metadata/event.ex @@ -9,7 +9,7 @@ defimpl Mobilizon.Service.Metadata, for: Mobilizon.Events.Event do alias Mobilizon.Web.Router.Helpers, as: Routes import Mobilizon.Service.Metadata.Utils, - only: [process_description: 2, strip_tags: 1, datetime_to_string: 2, render_address: 1] + only: [process_description: 2, strip_tags: 1, datetime_to_string: 2, render_address!: 1] def build_tags(%Event{} = event, locale \\ "en") do formatted_description = description(event, locale) @@ -150,7 +150,13 @@ defimpl Mobilizon.Service.Metadata, for: Mobilizon.Events.Event do @spec maybe_build_address(list(String.t()), Address.t() | nil) :: list(String.t()) defp maybe_build_address(elements, %Address{} = address) do - elements ++ [render_address(address)] + elements ++ [render_address!(address)] + rescue + # If the address is not renderable + e in ArgumentError -> + require Logger + Logger.error(Exception.format(:error, e, __STACKTRACE__)) + elements end defp maybe_build_address(elements, _address), do: elements diff --git a/lib/service/metadata/utils.ex b/lib/service/metadata/utils.ex index c57990988..a819f194c 100644 --- a/lib/service/metadata/utils.ex +++ b/lib/service/metadata/utils.ex @@ -54,7 +54,7 @@ defmodule Mobilizon.Service.Metadata.Utils do end defdelegate datetime_to_string(datetime, locale \\ "en", format \\ :medium), to: DateTime - defdelegate render_address(address), to: Address + defdelegate render_address!(address), to: Address defp maybe_slice(description, nil), do: description diff --git a/lib/web/templates/email/participation/card/_metadata.html.heex b/lib/web/templates/email/participation/card/_metadata.html.heex index 05316b591..af7b78e2b 100644 --- a/lib/web/templates/email/participation/card/_metadata.html.heex +++ b/lib/web/templates/email/participation/card/_metadata.html.heex @@ -26,7 +26,7 @@ - <%= if @event.physical_address do %> + <%= if not is_nil(@event.physical_address) and not is_nil(render_address(@event.physical_address)) do %> diff --git a/lib/web/templates/email/participation/card/_metadata.text.eex b/lib/web/templates/email/participation/card/_metadata.text.eex index 6694d741b..1caa89e8c 100644 --- a/lib/web/templates/email/participation/card/_metadata.text.eex +++ b/lib/web/templates/email/participation/card/_metadata.text.eex @@ -1,2 +1,2 @@ <%= gettext("Date:") %> <%= render("date/event_tz_date_range.text", event: @event, start_date: datetime_tz_convert(@event.begins_on, @event.options.timezone), end_date: datetime_tz_convert(@event.ends_on, @event.options.timezone), timezone: @timezone, locale: @locale) %> -<%= gettext("Address:") %> <%= if @event.physical_address do %><%= render_address(@event.physical_address) %><% end %><%= if @event.options.is_online do %><%= gettext "Online event" %><% end %> +<%= gettext("Address:") %> <%= if not is_nil(@event.physical_address) and not is_nil(render_address(@event.physical_address)) do %><%= render_address(@event.physical_address) %><% end %><%= if @event.options.is_online do %><%= gettext "Online event" %><% end %> diff --git a/lib/web/templates/export/event_participants.html.heex b/lib/web/templates/export/event_participants.html.heex index 597806728..7079404e8 100644 --- a/lib/web/templates/export/event_participants.html.heex +++ b/lib/web/templates/export/event_participants.html.heex @@ -125,7 +125,7 @@ th {
<%= gettext("Ends on") %>
<%= datetime_to_string(@event.ends_on, @locale, :long) %>
<% end %> - <%= if @event.physical_address do %> + <%= if not is_nil(@event.physical_address) and not is_nil(render_address(@event.physical_address)) do %>
<%= gettext("Location") %>
<%= render_address(@event.physical_address) %>
<% end %> diff --git a/test/service/address/address_test.exs b/test/service/address/address_test.exs index 45a607547..492379025 100644 --- a/test/service/address/address_test.exs +++ b/test/service/address/address_test.exs @@ -50,8 +50,10 @@ defmodule Mobilizon.Service.AddressTest do end test "with no data" do + assert AddressRenderer.render_address(%Address{}) == nil + assert_raise ArgumentError, "Invalid address", fn -> - AddressRenderer.render_address(%Address{}) + AddressRenderer.render_address!(%Address{}) end end end