[ENHANCEMENT] [MER-5786] Subobjective deletion checks

lib/oli/authoring/editing/objective_editor.ex

@@ -3,17 +3,22 @@ defmodule Oli.Authoring.Editing.ObjectiveEditor do
 
   alias Oli.Repo
   alias Oli.Resources
+  alias Oli.Resources.Revision
   alias Oli.Publishing
   alias Oli.Accounts.Author
   alias Oli.Authoring.Course.Project
-  alias Oli.Repo
   alias Oli.Authoring.Broadcaster
   alias Oli.Authoring.Editing.PageEditor
   alias Oli.Authoring.Editing.ActivityEditor
   alias Oli.Publishing.AuthoringResolver
 
   import Oli.Utils
 
+  @doc """
+  Creates a learning objective or sub-objective and optionally associates it with a parent.
+
+  Passing a non-empty `container_slug` associates the new objective with that parent.
+  """
   def add_new(attrs, %Author{} = author, %Project{} = project, container_slug \\ nil) do
     attrs =
       Map.merge(attrs, %{
@@ -62,14 +67,22 @@ defmodule Oli.Authoring.Editing.ObjectiveEditor do
     end
   end
 
+  @doc """
+  Associates an active sub-objective with another objective in the working publication.
+
+  The selected mappings are locked to prevent adding a reference to a child that
+  is being deleted concurrently.
+  """
+  @spec add_new_parent_for_sub_objective(binary(), binary(), binary(), %Author{}) ::
+          {:ok, %Revision{}} | {:error, :not_found | {:not_found}}
   def add_new_parent_for_sub_objective(slug, container_slug, project_slug, author) do
     result =
       Repo.transaction(fn ->
-        with {:ok, resource} <- Resources.get_resource_from_slug(slug) |> trap_nil(),
-             publication <- Publishing.project_working_publication(project_slug),
-             {:ok, revision_to_attach} <-
-               Publishing.get_published_revision(publication.id, resource.id)
-               |> trap_nil(),
+        publication = Publishing.project_working_publication(project_slug)
+        revisions = objective_revisions(publication.id, slugs: [slug, container_slug], lock: true)
+
+        with %{} = revision_to_attach <- Enum.find(revisions, &(&1.slug == slug)),
+             %{} <- Enum.find(revisions, &(&1.slug == container_slug)),
              {:ok, revision} <-
                append_to_container(
                  container_slug,
@@ -80,7 +93,8 @@ defmodule Oli.Authoring.Editing.ObjectiveEditor do
                ) do
           revision
         else
-          error -> Repo.rollback(error)
+          nil -> Repo.rollback(:not_found)
+          {:error, reason} -> Repo.rollback(reason)
         end
       end)
 
@@ -89,8 +103,8 @@ defmodule Oli.Authoring.Editing.ObjectiveEditor do
         Broadcaster.broadcast_revision(revision, project_slug)
         {:ok, revision}
 
-      e ->
-        e
+      error ->
+        error
     end
   end
 
@@ -203,24 +217,177 @@ defmodule Oli.Authoring.Editing.ObjectiveEditor do
     end)
   end
 
+  @doc """
+  Unlinks a shared sub-objective from the selected parent.
+
+  Only the selected parent and child mappings are locked. The transaction checks
+  that another active parent remains, so unlinking cannot create an orphan.
+  """
+  @spec remove_sub_objective_from_parent(
+          binary(),
+          %Author{},
+          %Project{},
+          binary() | %{required(:slug) => binary()}
+        ) ::
+          {:ok, %Revision{}}
+          | {:error,
+             :last_association
+             | :not_associated
+             | :not_found
+             | Ecto.Changeset.t()}
   def remove_sub_objective_from_parent(
         revision_slug,
         %Author{} = author,
         %Project{} = project,
-        parent_objective
+        parent_objective_or_slug
       ) do
-    resource = Resources.get_resource_from_slug(revision_slug)
+    parent_slug = objective_slug(parent_objective_or_slug)
 
-    edit(
-      parent_objective.slug,
-      %{
-        children: Enum.filter(parent_objective.children, fn id -> id != resource.id end)
-      },
-      author,
-      project
-    )
+    result =
+      Repo.transaction(fn ->
+        publication = Publishing.project_working_publication(project.slug)
+
+        revisions =
+          objective_revisions(publication.id, slugs: [revision_slug, parent_slug], lock: true)
+
+        with {:ok, sub_objective, parent} <-
+               associated_revisions(revisions, revision_slug, parent_slug),
+             parent_ids <-
+               Publishing.objective_parent_ids(sub_objective.resource_id, publication.id),
+             true <- Enum.any?(parent_ids, &(&1 != parent.resource_id)),
+             {:ok, updated_parent} <-
+               unlink_from_parent(parent, sub_objective, publication, author) do
+          updated_parent
+        else
+          false -> Repo.rollback(:last_association)
+          {:error, reason} -> Repo.rollback(reason)
+        end
+      end)
+
+    case result do
+      {:ok, parent} ->
+        Broadcaster.broadcast_resource(parent, project.slug)
+        {:ok, parent}
+
+      {:error, reason} ->
+        {:error, reason}
+    end
   end
 
+  @doc """
+  Checks whether removing a sub-objective from the given parent would delete its
+  final association. Tagged sub-objectives cannot be deleted.
+  """
+  @spec sub_objective_delete_eligibility(binary(), %Project{}, binary()) ::
+          {:ok, %Revision{}} | {:error, :associated | :not_associated | :not_found | :tagged}
+  def sub_objective_delete_eligibility(revision_slug, %Project{} = project, parent_slug) do
+    publication = Publishing.project_working_publication(project.slug)
+    revisions = objective_revisions(publication.id, slugs: [revision_slug, parent_slug])
+
+    with {:ok, sub_objective, parent} <-
+           associated_revisions(revisions, revision_slug, parent_slug),
+         :ok <- final_association_eligibility(sub_objective, parent, publication.id) do
+      {:ok, sub_objective}
+    end
+  end
+
+  @doc """
+  Deletes an untagged sub-objective and removes its final parent association.
+
+  Only the selected child and parent mappings are locked, sharing the child lock
+  with association creation so unrelated objective edits are not blocked.
+  Association and course-content references are checked again within the
+  transaction, including when they changed after the confirmation dialog opened.
+  """
+  @spec delete_sub_objective(binary(), %Author{}, %Project{}, binary()) ::
+          {:ok, %Revision{}}
+          | {:error,
+             :associated
+             | :not_associated
+             | :not_found
+             | :tagged
+             | Ecto.Changeset.t()}
+  def delete_sub_objective(revision_slug, %Author{} = author, %Project{} = project, parent_slug) do
+    result =
+      Repo.transaction(fn ->
+        publication = Publishing.project_working_publication(project.slug)
+
+        revisions =
+          objective_revisions(publication.id, slugs: [revision_slug, parent_slug], lock: true)
+
+        with {:ok, sub_objective, parent} <-
+               associated_revisions(revisions, revision_slug, parent_slug),
+             :ok <- final_association_eligibility(sub_objective, parent, publication.id),
+             {:ok, deleted_sub_objective} <-
+               Resources.create_revision_from_previous(sub_objective, %{
+                 author_id: author.id,
+                 deleted: true
+               }),
+             {:ok, _mapping} <-
+               Publishing.upsert_published_resource(publication, deleted_sub_objective),
+             {:ok, updated_parent} <-
+               unlink_from_parent(parent, sub_objective, publication, author) do
+          %{sub_objective: deleted_sub_objective, parent: updated_parent}
+        else
+          {:error, reason} -> Repo.rollback(reason)
+        end
+      end)
+
+    case result do
+      {:ok, %{sub_objective: sub_objective, parent: parent}} ->
+        Broadcaster.broadcast_resource(sub_objective, project.slug)
+        Broadcaster.broadcast_resource(parent, project.slug)
+        {:ok, sub_objective}
+
+      {:error, reason} ->
+        {:error, reason}
+    end
+  end
+
+  defp associated_revisions(revisions, revision_slug, parent_slug) do
+    with %{} = sub_objective <- Enum.find(revisions, &(&1.slug == revision_slug)),
+         %{} = parent <- Enum.find(revisions, &(&1.slug == parent_slug)),
+         true <- sub_objective.resource_id in parent.children do
+      {:ok, sub_objective, parent}
+    else
+      nil -> {:error, :not_found}
+      false -> {:error, :not_associated}
+    end
+  end
+
+  defp final_association_eligibility(sub_objective, parent, publication_id) do
+    case Publishing.objective_parent_ids(sub_objective.resource_id, publication_id) do
+      [parent_id] when parent_id == parent.resource_id ->
+        case Publishing.objective_referenced?(sub_objective.resource_id, publication_id) do
+          true -> {:error, :tagged}
+          false -> :ok
+        end
+
+      _parents ->
+        {:error, :associated}
+    end
+  end
+
+  defp unlink_from_parent(parent, sub_objective, publication, author) do
+    with {:ok, updated_parent} <-
+           Resources.create_revision_from_previous(parent, %{
+             author_id: author.id,
+             children: Enum.reject(parent.children, &(&1 == sub_objective.resource_id))
+           }),
+         {:ok, _mapping} <- Publishing.upsert_published_resource(publication, updated_parent) do
+      {:ok, updated_parent}
+    end
+  end
+
+  defp objective_revisions(publication_id, opts) do
+    publication_id
+    |> Publishing.get_objective_mappings_by_publication(opts)
+    |> Enum.map(& &1.revision)
+  end
+
+  defp objective_slug(%{slug: slug}), do: slug
+  defp objective_slug(slug) when is_binary(slug), do: slug
+
   @doc """
   Detaches an objective from all unlocked pages and activites that currently reference it.
 

lib/oli/publishing.ex

@@ -808,18 +808,60 @@ defmodule Oli.Publishing do
     end)
   end
 
-  def get_objective_mappings_by_publication(publication_id) do
+  @doc """
+  Returns active objective mappings for a publication.
+
+  Pass `lock: true` only inside a transaction to lock the current mapping and
+  revision rows while enforcing a cross-revision invariant. Use `slugs: [...]`
+  to restrict the query to selected objective revisions.
+  """
+  @spec get_objective_mappings_by_publication(integer(), keyword()) ::
+          [%PublishedResource{}]
+  def get_objective_mappings_by_publication(publication_id, opts \\ []) do
     objective = ResourceType.id_for_objective()
 
-    Repo.all(
+    query =
       from mapping in PublishedResource,
         join: rev in Revision,
         on: mapping.revision_id == rev.id,
         where:
           rev.deleted == false and rev.resource_type_id == ^objective and
             mapping.publication_id == ^publication_id,
         select: mapping,
+        order_by: mapping.resource_id,
         preload: [:resource, :revision]
+
+    query =
+      case Keyword.fetch(opts, :slugs) do
+        {:ok, slugs} -> from [mapping, rev] in query, where: rev.slug in ^slugs
+        :error -> query
+      end
+
+    query =
+      case Keyword.get(opts, :lock, false) do
+        true -> from mapping in query, lock: "FOR UPDATE"
+        false -> query
+      end
+
+    Repo.all(query)
+  end
+
+  @doc """
+  Returns resource IDs of active objectives that contain the given child in a publication.
+  """
+  @spec objective_parent_ids(integer(), integer()) :: [integer()]
+  def objective_parent_ids(resource_id, publication_id) do
+    objective = ResourceType.id_for_objective()
+
+    Repo.all(
+      from mapping in PublishedResource,
+        join: rev in Revision,
+        on: mapping.revision_id == rev.id,
+        where:
+          mapping.publication_id == ^publication_id and rev.deleted == false and
+            rev.resource_type_id == ^objective and ^resource_id in rev.children,
+        select: rev.resource_id,
+        distinct: true
     )
   end
 
@@ -1454,6 +1496,60 @@ defmodule Oli.Publishing do
     end)
   end
 
+  @doc """
+  Returns whether an objective is referenced by any active page, activity, or
+  selection in a publication without materializing the matching revisions.
+  """
+  @spec objective_referenced?(integer(), integer()) :: boolean()
+  def objective_referenced?(resource_id, publication_id) do
+    page_id = ResourceType.id_for_page()
+    activity_id = ResourceType.id_for_activity()
+
+    sql = """
+    SELECT
+      EXISTS (
+        SELECT 1
+        FROM published_resources AS mapping
+        JOIN revisions AS rev ON mapping.revision_id = rev.id
+        WHERE mapping.publication_id = $1
+          AND rev.deleted IS FALSE
+          AND (
+            (
+              rev.resource_type_id = $2
+              AND jsonb_path_exists(
+                rev.objectives,
+                '$.*[*] ? (@ == $objective)',
+                jsonb_build_object('objective', $4::bigint)
+              )
+            )
+            OR
+            (
+              rev.resource_type_id = $3
+              AND (
+                rev.objectives->'attached' @> jsonb_build_array($4::bigint)
+                OR jsonb_path_exists(
+                  rev.content,
+                  '$.**.conditions.** ? (@.fact == "objectives").value ? (@ == $objective)',
+                  jsonb_build_object('objective', $4::bigint)
+                )
+              )
+            )
+          )
+        LIMIT 1
+      )
+    """
+
+    %{rows: [[referenced?]]} =
+      Ecto.Adapters.SQL.query!(Repo, sql, [
+        publication_id,
+        activity_id,
+        page_id,
+        resource_id
+      ])
+
+    referenced?
+  end
+
   @doc """
     For a given project's publication id, this function will find all pages and
     activities that have an objective attached to it.