diff --git a/lib/oli/authoring/editing/objective_editor.ex b/lib/oli/authoring/editing/objective_editor.ex index b985241a61f..2e0265d011d 100644 --- a/lib/oli/authoring/editing/objective_editor.ex +++ b/lib/oli/authoring/editing/objective_editor.ex @@ -3,10 +3,10 @@ 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 @@ -14,6 +14,11 @@ defmodule Oli.Authoring.Editing.ObjectiveEditor do 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,178 @@ 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. + Parent associations and course-content references are rechecked within the + transaction before deletion. Objective-association writers share the child + mapping lock; course-content reference writers do not. + """ + @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. diff --git a/lib/oli/publishing.ex b/lib/oli/publishing.ex index ce0a2d43ea2..717d69c3ada 100644 --- a/lib/oli/publishing.ex +++ b/lib/oli/publishing.ex @@ -808,10 +808,19 @@ 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, @@ -820,6 +829,38 @@ defmodule Oli.Publishing do mapping.publication_id == ^publication_id, select: mapping, 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, order_by: mapping.resource_id, 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 +1495,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. diff --git a/lib/oli/scenarios/directives/objectives_handler.ex b/lib/oli/scenarios/directives/objectives_handler.ex index 64b1a2e8db6..5996da57b60 100644 --- a/lib/oli/scenarios/directives/objectives_handler.ex +++ b/lib/oli/scenarios/directives/objectives_handler.ex @@ -4,6 +4,7 @@ defmodule Oli.Scenarios.Directives.ObjectivesHandler do """ alias Oli.Authoring.Editing.ObjectiveEditor + alias Oli.Publishing.AuthoringResolver alias Oli.Scenarios.DirectiveTypes.{ExecutionState, ObjectivesDirective} @max_objective_ops 25 @@ -84,14 +85,12 @@ defmodule Oli.Scenarios.Directives.ObjectivesHandler do with {:ok, parent} <- get_objective(built_project, parent_title), {:ok, child} <- get_objective(built_project, title), :ok <- validate_child(parent, child), - {:ok, updated_parent} <- - ObjectiveEditor.remove_sub_objective_from_parent( - child.slug, - author, - built_project.project, - parent - ) do - {:ok, put_objective(built_project, parent_title, updated_parent)} + {:ok, updated_parent, updated_child} <- + remove_sub_objective(child, parent, author, built_project.project) do + {:ok, + built_project + |> put_objective(parent_title, updated_parent) + |> put_objective(title, updated_child)} else {:error, reason} -> {:error, "Could not remove sub-objective '#{title}': #{inspect(reason)}"} @@ -103,6 +102,23 @@ defmodule Oli.Scenarios.Directives.ObjectivesHandler do "Unsupported objective operation #{inspect(op)}. Expected create, create_sub, or remove_sub"} end + defp remove_sub_objective(child, parent, author, project) do + case ObjectiveEditor.remove_sub_objective_from_parent(child.slug, author, project, parent) do + {:ok, updated_parent} -> + {:ok, updated_parent, child} + + {:error, :last_association} -> + with {:ok, deleted_child} <- + ObjectiveEditor.delete_sub_objective(child.slug, author, project, parent.slug) do + updated_parent = AuthoringResolver.from_resource_id(project.slug, parent.resource_id) + {:ok, updated_parent, deleted_child} + end + + {:error, reason} -> + {:error, reason} + end + end + defp get_objective(built_project, title) do case Map.get(built_project.objectives_by_title || %{}, title) do nil -> {:error, "Objective '#{title}' not found"} diff --git a/lib/oli_web/icons.ex b/lib/oli_web/icons.ex index f03fd7d7ef7..8a4b2c97e23 100644 --- a/lib/oli_web/icons.ex +++ b/lib/oli_web/icons.ex @@ -970,6 +970,36 @@ defmodule OliWeb.Icons do """ end + attr :class, :string, default: "stroke-black dark:stroke-white" + attr :width, :string, default: "24" + attr :height, :string, default: "24" + attr :stroke_width, :string, default: "2" + + @doc """ + Renders a decorative unlink icon; the triggering control must provide its accessible name. + """ + def unlink(assigns) do + ~H""" + + """ + end + attr :class, :string, default: "stroke-black dark:stroke-white" attr :width, :string, default: "24" attr :height, :string, default: "24" diff --git a/lib/oli_web/live/workspaces/course_author/objectives/listing.ex b/lib/oli_web/live/workspaces/course_author/objectives/listing.ex index d2407b21143..feac61d38f2 100644 --- a/lib/oli_web/live/workspaces/course_author/objectives/listing.ex +++ b/lib/oli_web/live/workspaces/course_author/objectives/listing.ex @@ -1,8 +1,6 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.Listing do use OliWeb, :html - import OliWeb.Components.Common - alias OliWeb.Icons alias OliWeb.Workspaces.CourseAuthor.Objectives.Actions @@ -10,7 +8,9 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.Listing do attr(:revision_history_link, :boolean, required: true) attr(:rows, :list, required: true) attr(:expanded_slugs, :any, default: MapSet.new()) - attr(:pending_delete_slugs, :any, default: MapSet.new()) + attr(:pending_detaches, :any, default: MapSet.new()) + attr(:pending_deletes, :any, default: MapSet.new()) + attr(:objective_parents, :map, default: %{}) attr(:offset, :integer, default: 0) attr(:query, :string, default: "") @@ -87,12 +87,14 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.Listing do - <.loader - :if={MapSet.member?(@pending_delete_slugs, sub_objective.slug)} - class="ml-2" - icon_class="text-secondary" - /> -
+
diff --git a/lib/oli_web/live/workspaces/course_author/objectives/select_existing_sub_modal.ex b/lib/oli_web/live/workspaces/course_author/objectives/select_existing_sub_modal.ex index c9ff5e7c022..8d81b3a50cb 100644 --- a/lib/oli_web/live/workspaces/course_author/objectives/select_existing_sub_modal.ex +++ b/lib/oli_web/live/workspaces/course_author/objectives/select_existing_sub_modal.ex @@ -1,17 +1,21 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.SelectExistingSubModal do use OliWeb, :live_component - alias OliWeb.Common.TextSearch + alias OliWeb.Components.DesignTokens.Primitives.Button def update(assigns, socket) do + query = Map.get(socket.assigns, :query, "") + {:ok, - assign(socket, + socket + |> assign( add: assigns.add, - filtered_sub_objectives: assigns.sub_objectives, id: assigns.id, parent_slug: assigns.parent_slug, + query: query, sub_objectives: assigns.sub_objectives - )} + ) + |> assign_filtered_sub_objectives()} end attr(:add, :string, required: true) @@ -29,43 +33,95 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.SelectExistingSubModal do style="display: block" tabindex="-1" role="dialog" - aria-labelledby="show-existing-sub-modal" - aria-hidden="true" + aria-modal="true" + aria-labelledby={"#{@id}-title"} phx-hook="ModalLaunch" > -