Merge branch 'fixes' into 'main'

Make sure every relation of actor is loaded when operating on it

Closes #1049, #1016 et #996

See merge request framasoft/mobilizon!1196

js/src/views/Group/GroupSettings.vue

@@ -259,6 +259,7 @@ export default class GroupSettings extends mixins(GroupMixin) {
 
   @Watch("group")
   async watchUpdateGroup(oldGroup: IGroup, newGroup: IGroup): Promise<void> {
+    try {
       if (
         oldGroup?.avatar !== undefined &&
         oldGroup?.avatar !== newGroup?.avatar
@@ -271,6 +272,10 @@ export default class GroupSettings extends mixins(GroupMixin) {
       ) {
         this.bannerFile = await buildFileFromIMedia(this.group.banner);
       }
+    } catch (e) {
+      // Catch errors while building media
+      console.error(e);
+    }
     this.editableGroup = { ...this.group };
   }
 

js/src/views/Search.vue

@@ -93,6 +93,7 @@
             </b-select>
           </b-field>
           <b-field
+            v-if="config"
             expanded
             :label="$t('Category')"
             label-for="category"

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()

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
 

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
 

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

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
 

lib/web/templates/email/participation/card/_metadata.html.heex

@@ -26,7 +26,7 @@
                   </td>
                 </tr>
                 <!-- row for datetime end -->
-                <%= if @event.physical_address do %>
+                <%= if not is_nil(@event.physical_address) and not is_nil(render_address(@event.physical_address)) do %>
                   <!-- venue block -->
                   <tr style="vertical-align: baseline; margin: 0; padding: 0; border: 0;" align="left">
                     <td style="word-wrap: break-word; border-collapse: collapse; color: rgb(10,10,10); font-family: Helvetica,Arial,sans-serif; font-weight: normal; line-height: 1.3; font-size: 16px; margin: 0; padding: 0 12px 0 0; border: 0;" valign="baseline" align="left">

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 %>

lib/web/templates/export/event_participants.html.heex

@@ -125,7 +125,7 @@ th {
                 <dt><%= gettext("Ends on") %></dt>
                 <dd><%= datetime_to_string(@event.ends_on, @locale, :long) %></dd>
             <% end %>
-            <%= if @event.physical_address do %>
+            <%= if not is_nil(@event.physical_address) and not is_nil(render_address(@event.physical_address)) do %>
                 <dt><%= gettext("Location") %></dt>
                 <dd><%= render_address(@event.physical_address) %></dd>
             <% end %>

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