From 5e30dda3c7220094738a60951ff871b0b3deb79e Mon Sep 17 00:00:00 2001 From: Martin Manelli Date: Tue, 22 Sep 2026 13:24:56 -0300 Subject: [PATCH 01/19] Implemented the learning-objective detach/delete workflow. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Association rows now use the Figma unlink icon with “Detach from learning objective” tooltip and distinct accessible naming. - Detaching preserves the sub-objective, course references, and other LO associations. - Added persisted objective_type so final-detached sub-objectives remain distinguishable and recoverable. - Add Existing now supports Status → Unassociated, searchable results, re-adding, and permanent deletion. - Permanent deletion is server-guarded against LO associations and page/activity/selection references. - Added explicit confirmation copy, success/error announcements, and dialog focus restoration. - Top-level delete tooltip is now exactly “Delete Learning Objective”. --- .../objectives/ObjectivesSelection.tsx | 5 +- .../components/resource/objectives/sort.ts | 2 +- assets/src/data/content/objective.ts | 1 + assets/src/hooks/modal.ts | 30 +- .../objectives/objectives_selection_test.ts | 2 + lib/oli/authoring/editing/objective_editor.ex | 48 ++- lib/oli/authoring/editing/page_editor.ex | 4 +- lib/oli/resources.ex | 1 + lib/oli/resources/revision.ex | 7 + lib/oli_web/icons.ex | 28 ++ .../course_author/objectives/listing.ex | 105 +++--- .../objectives/select_existing_sub_modal.ex | 197 ++++++++--- .../objectives/sub_objective_delete_modal.ex | 44 ++- .../course_author/objectives_live.ex | 329 ++++++++++-------- ...154029_add_objective_type_to_revisions.exs | 15 + test/oli/editing/objective_editor_test.exs | 6 + .../course_author/objectives_live_test.exs | 232 +++++++++--- 17 files changed, 739 insertions(+), 317 deletions(-) create mode 100644 priv/repo/migrations/20260922154029_add_objective_type_to_revisions.exs diff --git a/assets/src/components/resource/objectives/ObjectivesSelection.tsx b/assets/src/components/resource/objectives/ObjectivesSelection.tsx index 14f536e15b2..19ef4412bc6 100644 --- a/assets/src/components/resource/objectives/ObjectivesSelection.tsx +++ b/assets/src/components/resource/objectives/ObjectivesSelection.tsx @@ -35,7 +35,9 @@ export const objectivesForAttachment = ( switch (attachmentType) { case 'page': - return objectives.filter((objective) => !objective.parentIds?.length); + return objectives.filter( + (objective) => objective.objectiveType !== 'sub_objective' && !objective.parentIds?.length, + ); case 'activity': return objectives.map((objective) => ({ ...objective, @@ -187,6 +189,7 @@ export const ObjectivesSelection = (props: ObjectivesProps) => { id: result.resourceId, title: createdObjective.title, parentIds: null, + objectiveType: 'objective', }); // Use the newly created resource id instead of the id of diff --git a/assets/src/components/resource/objectives/sort.ts b/assets/src/components/resource/objectives/sort.ts index a1c9c609584..291230d147e 100644 --- a/assets/src/components/resource/objectives/sort.ts +++ b/assets/src/components/resource/objectives/sort.ts @@ -37,7 +37,7 @@ export function arrangeObjectives(objectives: Objective[]): Immutable.List { // Initialize top-level objectives (those that are not children of anyone) - if (!childIds.has(o.id)) { + if (!childIds.has(o.id) && o.objectiveType !== 'sub_objective') { if (bucketed[o.id] === undefined) { bucketed[o.id] = []; } diff --git a/assets/src/data/content/objective.ts b/assets/src/data/content/objective.ts index 4d2a4283eef..468301b3d43 100644 --- a/assets/src/data/content/objective.ts +++ b/assets/src/data/content/objective.ts @@ -6,4 +6,5 @@ export type Objective = { id: ResourceId; title: string; parentIds: ResourceId[] | null; + objectiveType?: 'objective' | 'sub_objective'; }; diff --git a/assets/src/hooks/modal.ts b/assets/src/hooks/modal.ts index 367121f552b..41449d81018 100644 --- a/assets/src/hooks/modal.ts +++ b/assets/src/hooks/modal.ts @@ -2,6 +2,9 @@ import { lockScroll, unlockScroll } from 'components/modal/utils'; export const ModalLaunch = { mounted(): void { + (this as any).trigger = + document.activeElement instanceof HTMLElement ? document.activeElement : null; + // initialize the bootstrap modal const id = this.el.getAttribute('id'); this.id = id; @@ -9,20 +12,45 @@ export const ModalLaunch = { // ($(this.el) as any).modal({}); this.modal = new (window as any).Modal(this.el, {}); + + const initialFocus = this.el.dataset.initialFocus; + if (initialFocus) { + $(`#${id}`).on('shown.bs.modal', () => { + const target = this.el.querySelector(initialFocus); + if (target instanceof HTMLElement) target.focus(); + }); + } + this.modal.show(); const scrollPosition = lockScroll(); // wire up server-side hide event (this as any).handleEvent('phx_modal.hide', () => { + (this as any).serverHiding = true; this.modal.hide(); }); // handle hiding of a modal as a result of many different methods // (modal close button, escape key, etc...) $(`#${id}`).on('hidden.bs.modal', () => { - (this as any).pushEvent('phx_modal.unmount'); + const dismissEvent = this.el.dataset.dismissEvent; + + if (dismissEvent && !(this as any).serverHiding) { + (this as any).pushEvent(dismissEvent, { + parent_slug: this.el.dataset.parentSlug, + focus_delete_slug: this.el.dataset.focusDeleteSlug, + }); + } else { + (this as any).pushEvent('phx_modal.unmount'); + } + unlockScroll(scrollPosition); + + const trigger = (this as any).trigger; + if (trigger?.isConnected) { + window.requestAnimationFrame(() => trigger.focus()); + } }); }, destroyed(): void { diff --git a/assets/test/components/resource/objectives/objectives_selection_test.ts b/assets/test/components/resource/objectives/objectives_selection_test.ts index 5c731dabb4c..9b871fa2fd7 100644 --- a/assets/test/components/resource/objectives/objectives_selection_test.ts +++ b/assets/test/components/resource/objectives/objectives_selection_test.ts @@ -9,6 +9,7 @@ const objectives: Objective[] = [ { id: 1, title: 'Parent', parentIds: null }, { id: 2, title: 'Another parent', parentIds: [] }, { id: 3, title: 'Child', parentIds: [1] }, + { id: 4, title: 'Unassociated child', parentIds: null, objectiveType: 'sub_objective' }, ]; describe('objective attachment restrictions', () => { @@ -21,6 +22,7 @@ describe('objective attachment restrictions', () => { { ...objectives[0], disabled: true }, { ...objectives[1], disabled: true }, { ...objectives[2], disabled: false }, + { ...objectives[3], disabled: true }, ]); }); diff --git a/lib/oli/authoring/editing/objective_editor.ex b/lib/oli/authoring/editing/objective_editor.ex index b985241a61f..9a1a613b299 100644 --- a/lib/oli/authoring/editing/objective_editor.ex +++ b/lib/oli/authoring/editing/objective_editor.ex @@ -14,11 +14,18 @@ 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` creates a sub-objective. The persisted type remains + stable if that final parent association is later removed. + """ def add_new(attrs, %Author{} = author, %Project{} = project, container_slug \\ nil) do attrs = Map.merge(attrs, %{ author_id: author.id, - resource_type_id: Oli.Resources.ResourceType.id_for_objective() + resource_type_id: Oli.Resources.ResourceType.id_for_objective(), + objective_type: objective_type(container_slug) }) result = @@ -203,24 +210,43 @@ defmodule Oli.Authoring.Editing.ObjectiveEditor do end) end + @doc """ + Removes only the association between a sub-objective and one parent objective. + + The sub-objective remains published in the project, retains its course-content + attachments and other parent associations, and is marked as a sub-objective even + when this was its final parent association. + """ def remove_sub_objective_from_parent( revision_slug, %Author{} = author, %Project{} = project, parent_objective ) do - resource = Resources.get_resource_from_slug(revision_slug) - - edit( - parent_objective.slug, - %{ - children: Enum.filter(parent_objective.children, fn id -> id != resource.id end) - }, - author, - project - ) + Repo.transaction(fn -> + with %{} = resource <- Resources.get_resource_from_slug(revision_slug), + {:ok, _sub_objective} <- + edit(revision_slug, %{objective_type: :sub_objective}, author, project), + {:ok, parent} <- + edit( + parent_objective.slug, + %{ + children: Enum.reject(parent_objective.children, &(&1 == resource.id)) + }, + author, + project + ) do + parent + else + nil -> Repo.rollback({:error, :not_found}) + error -> Repo.rollback(error) + end + end) end + defp objective_type(container_slug) when container_slug in [nil, ""], do: :objective + defp objective_type(_container_slug), do: :sub_objective + @doc """ Detaches an objective from all unlocked pages and activites that currently reference it. diff --git a/lib/oli/authoring/editing/page_editor.ex b/lib/oli/authoring/editing/page_editor.ex index 62ff2d705b2..5fbe0a9a18b 100644 --- a/lib/oli/authoring/editing/page_editor.ex +++ b/lib/oli/authoring/editing/page_editor.ex @@ -876,6 +876,7 @@ defmodule Oli.Authoring.Editing.PageEditor do # id: the slug of the objective # title: the title of the objective # parentIds: a list of parent objective ids, nil if no parent objectives + # objectiveType: whether the item is a top-level objective or sub-objective # } # # Note: This function returns a unique entry per objective, with parentIds as a list @@ -900,7 +901,8 @@ defmodule Oli.Authoring.Editing.PageEditor do %{ id: revision.resource_id, title: revision.title, - parentIds: parent_ids + parentIds: parent_ids, + objectiveType: Map.get(revision, :objective_type, :objective) } end) end diff --git a/lib/oli/resources.ex b/lib/oli/resources.ex index bae8a081f39..5c9395deacb 100644 --- a/lib/oli/resources.ex +++ b/lib/oli/resources.ex @@ -331,6 +331,7 @@ defmodule Oli.Resources do content: previous_revision.content, objectives: previous_revision.objectives, children: previous_revision.children, + objective_type: previous_revision.objective_type, deleted: previous_revision.deleted, ids_added: previous_revision.ids_added, slug: previous_revision.slug, diff --git a/lib/oli/resources/revision.ex b/lib/oli/resources/revision.ex index bf4b479410b..0a8b2a5ffcb 100644 --- a/lib/oli/resources/revision.ex +++ b/lib/oli/resources/revision.ex @@ -23,6 +23,7 @@ defmodule Oli.Resources.Revision do :scoring_strategy_id, :activity_type_id, :title, + :objective_type, :resource_id, :intro_video, :poster_image, @@ -50,6 +51,11 @@ defmodule Oli.Resources.Revision do # fields that apply to only a subset of the types field :content, :map, default: %{} field :children, {:array, :id}, default: [] + + field :objective_type, Ecto.Enum, + values: [:objective, :sub_objective], + default: :objective + field :tags, {:array, :id}, default: [] field :activity_refs, {:array, :id}, default: [] field :objectives, :map, default: %{} @@ -121,6 +127,7 @@ defmodule Oli.Resources.Revision do :resource_type_id, :content, :children, + :objective_type, :tags, :objectives, :graded, diff --git a/lib/oli_web/icons.ex b/lib/oli_web/icons.ex index f2cd4987945..6ea28478d35 100644 --- a/lib/oli_web/icons.ex +++ b/lib/oli_web/icons.ex @@ -976,6 +976,34 @@ defmodule OliWeb.Icons do attr :stroke_width, :string, default: "2" attr :variant, :string, default: "default" + 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" + attr :stroke_width, :string, default: "2" + attr :variant, :string, default: "default" + def trash(assigns) do ~H""" - + + + + <.link :if={@revision_history_link} @@ -263,8 +269,7 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.Listing do :if={!is_nil(sub_objective)} class={[ "group/item flex flex-wrap items-center gap-[10px] rounded-md border bg-Background-bg-secondary p-3", - issue_border_class(sub_objective.any_issue), - MapSet.member?(@pending_delete_slugs, sub_objective.slug) && "opacity-50" + issue_border_class(sub_objective.any_issue) ]} > <% child_expanded? = MapSet.member?(@expanded_slugs, sub_objective.slug) %> @@ -295,10 +300,7 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.Listing do ]} /> - + <.highlighted_title title={sub_objective.title} regex={@highlight_regex} @@ -306,15 +308,7 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.Listing do /> - <.loader - :if={MapSet.member?(@pending_delete_slugs, sub_objective.slug)} - class="ml-2" - icon_class="text-secondary" - /> -
+
- + + + +
assign( add: assigns.add, - filtered_sub_objectives: assigns.sub_objectives, + delete: assigns.delete, + focus_delete_slug: assigns.focus_delete_slug, id: assigns.id, parent_slug: assigns.parent_slug, + query: query, + status: status, sub_objectives: assigns.sub_objectives - )} + ) + |> assign_filtered_sub_objectives()} end attr(:add, :string, required: true) + attr(:delete, :string, required: true) attr(:filtered_sub_objectives, :list, default: []) + attr(:focus_delete_slug, :string, default: nil) attr(:id, :string) attr(:parent_slug, :string, required: true) attr(:query, :string, default: "") + attr(:status, :string, default: "all") attr(:sub_objectives, :list, default: []) def render(assigns) do @@ -29,43 +40,119 @@ 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"} + data-initial-focus={@focus_delete_slug && "#delete-sub-objective-#{@focus_delete_slug}"} phx-hook="ModalLaunch" > -
-
-
-
Select existing Sub-Objective
-
-
-
- -
- <%= for sub_objective <- @filtered_sub_objectives do %> -
-
- {sub_objective.title} -
- -
- <% end %> -
-
+ +
+
+ + + +
+ +

+ No sub-objectives match these filters. +

+ +
    +
  • + + {sub_objective.title} + +
    + + +
    +
    • +
@@ -73,26 +160,30 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.SelectExistingSubModal do """ end - def handle_event("text_search_change", %{"value" => query}, socket) do - query_str = String.downcase(query) + def handle_event("filters_changed", params, socket) do + {:noreply, + socket + |> assign( + query: Map.get(params, "query", socket.assigns.query), + status: Map.get(params, "status", socket.assigns.status) + ) + |> assign_filtered_sub_objectives()} + end + + defp assign_filtered_sub_objectives(socket) do + query = String.downcase(String.trim(socket.assigns.query)) filtered_sub_objectives = - Enum.filter(socket.assigns.sub_objectives, fn sub -> - String.contains?(String.downcase(sub.title), query_str) + socket.assigns.sub_objectives + |> Enum.filter(fn sub_objective -> + query == "" or String.contains?(String.downcase(sub_objective.title), query) end) + |> Enum.filter(&matches_status?(&1, socket.assigns.status)) - {:noreply, - assign(socket, - filtered_sub_objectives: filtered_sub_objectives, - query: query - )} + assign(socket, filtered_sub_objectives: filtered_sub_objectives) end - def handle_event("text_search_reset", _, socket) do - {:noreply, - assign(socket, - filtered_sub_objectives: socket.assigns.sub_objectives, - query: "" - )} - end + defp matches_status?(sub_objective, "associated"), do: sub_objective.association_count > 0 + defp matches_status?(sub_objective, "unassociated"), do: sub_objective.association_count == 0 + defp matches_status?(_sub_objective, _status), do: true end diff --git a/lib/oli_web/live/workspaces/course_author/objectives/sub_objective_delete_modal.ex b/lib/oli_web/live/workspaces/course_author/objectives/sub_objective_delete_modal.ex index 27f03e49828..abb65c98c5c 100644 --- a/lib/oli_web/live/workspaces/course_author/objectives/sub_objective_delete_modal.ex +++ b/lib/oli_web/live/workspaces/course_author/objectives/sub_objective_delete_modal.ex @@ -13,29 +13,47 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.SubObjectiveDeleteModal do id={@id} tabindex="-1" role="dialog" - aria-hidden="true" + aria-modal="true" + aria-labelledby={"#{@id}-title"} + aria-describedby={"#{@id}-description"} + data-dismiss-event="return_to_add_existing" + data-parent-slug={@parent_slug} + data-focus-delete-slug={@slug} phx-hook="ModalLaunch" > -
-
-
-
Delete Sub-Objective
-
-
- Are you sure you want to delete {@title}? +
+ “{@title}” is not associated with any learning objectives. Deleting it + will permanently remove it from this course. This action cannot be undone.
-
- +
+
diff --git a/lib/oli_web/live/workspaces/course_author/objectives_live.ex b/lib/oli_web/live/workspaces/course_author/objectives_live.ex index dd198e688ad..e2fcee0d6d9 100644 --- a/lib/oli_web/live/workspaces/course_author/objectives_live.ex +++ b/lib/oli_web/live/workspaces/course_author/objectives_live.ex @@ -78,7 +78,6 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLive do course_content_nodes_by_id: %{}, course_content_root_ids: [], course_content_expanded_ids: MapSet.new(), - pending_sub_objective_delete_slugs: MapSet.new(), query: "", search_matching_ids: nil, search_expansion_ids: nil, @@ -404,7 +403,6 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLive do } rows={@table_model.rows} expanded_slugs={@expanded_objective_slugs} - pending_delete_slugs={@pending_sub_objective_delete_slugs} project_slug={@project.slug} offset={@offset} query={@query} @@ -421,15 +419,12 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLive do |> ObjectiveEditor.fetch_objective_mappings() |> Enum.map(& &1.revision) - all_children = - all_objectives - |> Enum.reduce([], fn rev, acc -> rev.children ++ acc end) - |> Enum.uniq() + all_children = all_child_ids(all_objectives) objectives = Enum.reduce(all_objectives, [], fn rev, acc -> - case Enum.find(all_children, fn child_id -> child_id == rev.resource_id end) do - nil -> + case sub_objective?(rev, all_children) do + false -> mapped_children = Enum.map(rev.children, fn resource_id -> case Enum.find(all_objectives, fn rev -> rev.resource_id == resource_id end) do @@ -450,7 +445,7 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLive do }) ] ++ acc - _ -> + true -> acc end end) @@ -460,6 +455,24 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLive do {all_objectives, all_children, objectives, table_model} end + defp all_child_ids(objectives) do + objectives + |> Enum.flat_map(& &1.children) + |> Enum.uniq() + end + + defp sub_objective?(revision, child_ids) do + revision.objective_type == :sub_objective or revision.resource_id in child_ids + end + + defp objective_parent_counts(objectives) do + Enum.reduce(objectives, %{}, fn objective, counts -> + Enum.reduce(objective.children, counts, fn child_id, counts -> + Map.update(counts, child_id, 1, &(&1 + 1)) + end) + end) + end + defp base_coverage_fields do %{ page_attachments_count: 0, @@ -1042,35 +1055,15 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLive do end def handle_event("display_add_existing_sub_modal", %{"slug" => slug}, socket) do - %{project: project, all_children: all_children, all_objectives: all_objectives} = - socket.assigns - - %{children: objective_children} = AuthoringResolver.from_revision_slug(project.slug, slug) - - sub_objectives = - Enum.map(all_children -- objective_children, fn resource_id -> - Enum.find(all_objectives, fn obj -> obj.resource_id == resource_id end) - end) - - modal_assigns = %{ - id: "select_existing_sub_modal", - parent_slug: slug, - sub_objectives: sub_objectives, - add: "add_existing_sub" - } - - modal = fn assigns -> - ~H""" - <.live_component id="select_existing_sub_modal" module={SelectExistingSubModal} {@modal_assigns} /> - """ - end + show_add_existing_sub_modal(socket, slug) + end - {:noreply, - show_modal( - socket, - modal, - modal_assigns: modal_assigns - )} + def handle_event( + "return_to_add_existing", + %{"parent_slug" => parent_slug, "focus_delete_slug" => focus_delete_slug}, + socket + ) do + show_add_existing_sub_modal(socket, parent_slug, focus_delete_slug) end def handle_event("display_delete_modal", %{"slug" => slug}, socket) do @@ -1135,26 +1128,43 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLive do def handle_event( "display_sub_objective_delete_modal", - %{"slug" => slug, "parent_slug" => parent_slug} = params, + %{"slug" => slug, "parent_slug" => parent_slug}, socket ) do socket = clear_flash(socket) - title = Map.get(params, "title", "this sub-objective") - modal_assigns = %{ - id: "delete_sub_objective_modal", - slug: slug, - parent_slug: parent_slug, - title: title - } + case sub_objective_delete_eligibility(slug, socket.assigns.project) do + {:ok, sub_objective} -> + modal_assigns = %{ + id: "delete_sub_objective_modal", + slug: slug, + parent_slug: parent_slug, + title: sub_objective.title + } + + modal = fn assigns -> + ~H""" + + """ + end - modal = fn assigns -> - ~H""" - - """ - end + {:noreply, show_modal(socket, modal, modal_assigns: modal_assigns)} + + {:error, :associated} -> + {:noreply, + put_flash(socket, :error, "Associated sub-objectives cannot be permanently deleted")} - {:noreply, show_modal(socket, modal, modal_assigns: modal_assigns)} + {:error, :tagged} -> + {:noreply, + put_flash( + socket, + :error, + "This sub-objective cannot be deleted because it is tagged to course content" + )} + + {:error, :not_found} -> + {:noreply, put_flash(socket, :error, "Could not find that sub-objective")} + end end def handle_event("delete", %{"slug" => slug} = params, socket) do @@ -1169,35 +1179,67 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLive do end def handle_event( - "delete_sub_objective", + "detach_sub_objective", %{"slug" => slug, "parent_slug" => parent_slug}, socket ) do socket = clear_flash(socket) - %{project: %{slug: project_slug}, author: %{email: author_email}} = socket.assigns + %{project: project, author: author} = socket.assigns - if MapSet.member?(socket.assigns.pending_sub_objective_delete_slugs, slug) do - {:noreply, - socket - |> hide_modal(modal_assigns: nil) - |> put_flash(:error, "That sub-objective is already being deleted")} - else - socket = - socket - |> assign( - pending_sub_objective_delete_slugs: - MapSet.put(socket.assigns.pending_sub_objective_delete_slugs, slug) - ) - |> hide_modal(modal_assigns: nil) - |> start_async({:delete_sub_objective, slug}, fn -> - project = Course.get_project_by_slug(project_slug) - author = Accounts.get_author_by_email(author_email) + flash_fn = + case parent_objective(project, parent_slug) do + nil -> + fn socket -> put_flash(socket, :error, "Could not detach sub-objective") end - delete_objective_result(slug, parent_slug, project, author) - end) + parent -> + case ObjectiveEditor.remove_sub_objective_from_parent(slug, author, project, parent) do + {:ok, _revision} -> + fn socket -> + put_flash(socket, :info, "Sub-objective detached from learning objective") + end - {:noreply, socket} - end + {:error, _reason} -> + fn socket -> put_flash(socket, :error, "Could not detach sub-objective") end + end + end + + return_updated_data(project, flash_fn, socket) + end + + def handle_event("delete_sub_objective", %{"slug" => slug}, socket) do + socket = clear_flash(socket) + %{project: project, author: author} = socket.assigns + + flash_fn = + case sub_objective_delete_eligibility(slug, project) do + {:ok, _sub_objective} -> + case ObjectiveEditor.delete(slug, author, project) do + {:ok, _revision} -> + fn socket -> put_flash(socket, :info, "Sub-objective deleted") end + + {:error, _reason} -> + fn socket -> put_flash(socket, :error, "Could not delete sub-objective") end + end + + {:error, :associated} -> + fn socket -> + put_flash(socket, :error, "Associated sub-objectives cannot be permanently deleted") + end + + {:error, :tagged} -> + fn socket -> + put_flash( + socket, + :error, + "This sub-objective cannot be deleted because it is tagged to course content" + ) + end + + {:error, :not_found} -> + fn socket -> put_flash(socket, :error, "Could not find that sub-objective") end + end + + return_updated_data(project, flash_fn, socket) end def handle_event( @@ -1226,6 +1268,48 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLive do return_updated_data(project, flash_fn, socket) end + defp show_add_existing_sub_modal(socket, slug, focus_delete_slug \\ nil) do + %{project: project, all_objectives: all_objectives} = socket.assigns + + %{children: objective_children} = AuthoringResolver.from_revision_slug(project.slug, slug) + child_ids = all_child_ids(all_objectives) + parent_counts = objective_parent_counts(all_objectives) + + sub_objectives = + all_objectives + |> Enum.filter(&sub_objective?(&1, child_ids)) + |> Enum.reject(&(&1.resource_id in objective_children)) + |> Enum.map(fn sub_objective -> + Map.put( + sub_objective, + :association_count, + Map.get(parent_counts, sub_objective.resource_id, 0) + ) + end) + + modal_assigns = %{ + id: "select_existing_sub_modal", + parent_slug: slug, + sub_objectives: sub_objectives, + add: "add_existing_sub", + delete: "display_sub_objective_delete_modal", + focus_delete_slug: focus_delete_slug + } + + modal = fn assigns -> + ~H""" + <.live_component id="select_existing_sub_modal" module={SelectExistingSubModal} {@modal_assigns} /> + """ + end + + {:noreply, + show_modal( + socket, + modal, + modal_assigns: modal_assigns + )} + end + defp delete_objective(slug, parent_slug, project, author) do case delete_objective_result(slug, parent_slug, project, author) do :ok -> fn socket -> put_flash(socket, :info, "Objective successfully removed") end @@ -1238,57 +1322,48 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLive do ObjectiveEditor.detach_objective(resource_id, project, author) + case ObjectiveEditor.delete(slug, author, project, parent_objective(project, parent_slug)) do + {:ok, _} -> :ok + {:error, _error} -> :error + end + end + + defp sub_objective_delete_eligibility(slug, project) do objectives = project |> ObjectiveEditor.fetch_objective_mappings() |> Enum.map(& &1.revision) - {parents, parent_to_detach_slug} = - Enum.reduce(objectives, {[], ""}, fn objective, {parents, parent_to_detach_slug} -> - case Enum.member?(objective.children, resource_id) do - false -> - {parents, parent_to_detach_slug} + case Enum.find(objectives, &(&1.slug == slug)) do + nil -> + {:error, :not_found} - true -> - if objective.slug == parent_slug, - do: {[objective | parents], objective.slug}, - else: {[objective | parents], parent_to_detach_slug} - end - end) + %{objective_type: :sub_objective} = sub_objective -> + parent_counts = objective_parent_counts(objectives) - delete_fn = - if length(parents) <= 1 do - fn -> - ObjectiveEditor.delete( - slug, - author, - project, - parent_objective(project, parent_to_detach_slug) - ) - end - else - fn -> - case parent_objective(project, parent_to_detach_slug) do - nil -> - {:error, :not_found} - - parent_objective -> - ObjectiveEditor.remove_sub_objective_from_parent( - slug, - author, - project, - parent_objective - ) - end + cond do + Map.get(parent_counts, sub_objective.resource_id, 0) > 0 -> + {:error, :associated} + + objective_tagged?(sub_objective.resource_id, project) -> + {:error, :tagged} + + true -> + {:ok, sub_objective} end - end - case delete_fn.() do - {:ok, _} -> :ok - {:error, _error} -> :error + _objective -> + {:error, :not_found} end end + defp objective_tagged?(resource_id, project) do + publication_id = Oli.Publishing.get_unpublished_publication_id!(project.id) + + Oli.Publishing.find_objective_attachments(resource_id, publication_id) != [] or + Oli.Publishing.find_objective_in_selections(resource_id, publication_id) != [] + end + @impl Phoenix.LiveView def handle_async(:objective_coverage, {:ok, result}, socket), do: apply_coverage_result(result, socket) @@ -1308,36 +1383,6 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLive do apply_coverage_result({:error, :coverage_load_failed}, socket) end - def handle_async({:delete_sub_objective, slug}, result, socket) do - if MapSet.member?(socket.assigns.pending_sub_objective_delete_slugs, slug) do - flash_type = - case result do - {:ok, flash_type} -> - flash_type - - {:exit, reason} -> - Logger.error("Sub-objective deletion failed: #{inspect(reason)}") - :error - end - - flash_fn = - case flash_type do - :ok -> fn socket -> put_flash(socket, :info, "Objective successfully removed") end - :error -> fn socket -> put_flash(socket, :error, "Could not remove objective") end - end - - socket = - assign(socket, - pending_sub_objective_delete_slugs: - MapSet.delete(socket.assigns.pending_sub_objective_delete_slugs, slug) - ) - - return_updated_data(socket.assigns.project, flash_fn, socket) - else - {:noreply, socket} - end - end - # Kept for compatibility with in-flight messages from older LiveView # processes during a rolling deployment. New coverage loads use handle_async/3. @impl Phoenix.LiveView diff --git a/priv/repo/migrations/20260922154029_add_objective_type_to_revisions.exs b/priv/repo/migrations/20260922154029_add_objective_type_to_revisions.exs new file mode 100644 index 00000000000..c959f120aeb --- /dev/null +++ b/priv/repo/migrations/20260922154029_add_objective_type_to_revisions.exs @@ -0,0 +1,15 @@ +defmodule Oli.Repo.Migrations.AddObjectiveTypeToRevisions do + use Ecto.Migration + + def up do + alter table(:revisions) do + add :objective_type, :string, null: false, default: "objective" + end + end + + def down do + alter table(:revisions) do + remove :objective_type + end + end +end diff --git a/test/oli/editing/objective_editor_test.exs b/test/oli/editing/objective_editor_test.exs index ab390bcbaed..0d0e12e3e1a 100644 --- a/test/oli/editing/objective_editor_test.exs +++ b/test/oli/editing/objective_editor_test.exs @@ -400,6 +400,12 @@ defmodule Oli.Authoring.Editing.ObjectiveEditorTest do assert length(children) == 0 refute Enum.member?(children, subobjective12A_resource_id) assert Enum.member?(objective2_children, subobjective12A_resource_id) + + updated_sub_objective = + AuthoringResolver.from_revision_slug(project.slug, subobjective12A_slug) + + assert updated_sub_objective.objective_type == :sub_objective + refute updated_sub_objective.deleted end test "detach_objective/3 preserves tags when removing an objective from a banked activity", %{ diff --git a/test/oli_web/live/workspaces/course_author/objectives_live_test.exs b/test/oli_web/live/workspaces/course_author/objectives_live_test.exs index 420b4286071..ef83e74d55a 100644 --- a/test/oli_web/live/workspaces/course_author/objectives_live_test.exs +++ b/test/oli_web/live/workspaces/course_author/objectives_live_test.exs @@ -1377,8 +1377,8 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLiveTest do ) view - |> element("#select_existing_sub_modal #text-search-input") - |> render_hook("text_search_change", %{value: "testing"}) + |> element("#select_existing_sub_modal-filters") + |> render_change(%{"query" => "testing", "status" => "all"}) assert has_element?( view, @@ -1448,6 +1448,11 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLiveTest do assert has_element?(view, ".collapse", "#{title}") + assert Enum.any?(ObjectiveEditor.fetch_objective_mappings(project), fn mapping -> + mapping.revision.title == title and + mapping.revision.objective_type == :sub_objective + end) + wait_for_coverage(view) end @@ -1496,11 +1501,12 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLiveTest do wait_for_coverage(view) end - test "remove sub objective with one parent", %{ - conn: conn, - project: project, - publication: publication - } do + test "detaching a final association preserves the sub-objective and exposes it as unassociated", + %{ + conn: conn, + project: project, + publication: publication + } do {:ok, sub_obj} = create_objective(project, publication, "sub_obj", "Sub Objective") {:ok, obj} = @@ -1511,40 +1517,87 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLiveTest do assert has_element?(view, "##{obj.slug}") assert has_element?(view, ".collapse", "#{sub_obj.title}") - view - |> element( - "button[phx-click='display_sub_objective_delete_modal'][phx-value-slug=#{sub_obj.slug}]" - ) - |> render_click(%{"slug" => sub_obj.slug, "parent_slug" => obj.slug}) + assert has_element?( + view, + "button[phx-click='detach_sub_objective'][phx-value-slug=#{sub_obj.slug}]" + ) - assert has_element?(view, "#delete_sub_objective_modal", "Delete Sub-Objective") - assert has_element?(view, "#delete_sub_objective_modal", "#{sub_obj.title}") + assert has_element?( + view, + "[role='tooltip']", + "Detach from learning objective" + ) + + assert has_element?( + view, + "#delete-objective-tooltip-#{obj.resource_id}", + "Delete Learning Objective" + ) view - |> element("button[phx-click='delete_sub_objective'][phx-value-slug=#{sub_obj.slug}]") + |> element("button[phx-click='detach_sub_objective'][phx-value-slug=#{sub_obj.slug}]") |> render_click(%{"slug" => sub_obj.slug, "parent_slug" => obj.slug}) - assert has_element?(view, ".collapse .line-through", "#{sub_obj.title}") - - wait_until(fn -> - has_element?(view, ~s{div[role="alert"].alert-info}, "Objective successfully removed") - end) + assert has_element?( + view, + ~s{div[role="alert"].alert-info}, + "Sub-objective detached from learning objective" + ) - assert 1 == + assert 2 == project |> ObjectiveEditor.fetch_objective_mappings() |> length() refute has_element?(view, ".collapse", "#{sub_obj.title}") - wait_for_coverage(view) + detached = AuthoringResolver.from_revision_slug(project.slug, sub_obj.slug) + refute detached.deleted + assert detached.objective_type == :sub_objective + + view + |> element("button[phx-click='display_add_existing_sub_modal'][phx-value-slug=#{obj.slug}]") + |> render_click(%{"slug" => obj.slug}) + + view + |> element("#select_existing_sub_modal-filters") + |> render_change(%{"query" => "", "status" => "unassociated"}) + + assert has_element?(view, "#existing-sub-objective-#{sub_obj.resource_id}", sub_obj.title) + + assert has_element?( + view, + "button[phx-click='add_existing_sub'][phx-value-slug=#{sub_obj.slug}]", + "Add" + ) + + assert has_element?( + view, + "button[phx-click='display_sub_objective_delete_modal'][phx-value-slug=#{sub_obj.slug}]", + "Delete" + ) + + view + |> element("button[phx-click='add_existing_sub'][phx-value-slug=#{sub_obj.slug}]") + |> render_click(%{"slug" => sub_obj.slug, "parent_slug" => obj.slug}) + + view + |> element("button[phx-click='display_add_existing_sub_modal'][phx-value-slug=#{obj.slug}]") + |> render_click(%{"slug" => obj.slug}) + + view + |> element("#select_existing_sub_modal-filters") + |> render_change(%{"query" => "", "status" => "unassociated"}) + + refute has_element?(view, "#existing-sub-objective-#{sub_obj.resource_id}") end - test "remove sub objective with more than one parent", %{ - conn: conn, - project: project, - publication: publication - } do + test "detaching one of multiple associations preserves the other and does not expose delete", + %{ + conn: conn, + project: project, + publication: publication + } do {:ok, sub_obj} = create_objective(project, publication, "sub_obj", "Sub Objective") {:ok, obj_a} = @@ -1567,33 +1620,128 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLiveTest do view |> element( - "button[phx-click='display_sub_objective_delete_modal'][phx-value-slug=#{sub_obj.slug}][phx-value-parent_slug=#{obj_a.slug}]" + "##{obj_a.slug} button[phx-click='detach_sub_objective'][phx-value-slug=#{sub_obj.slug}]" ) |> render_click(%{"slug" => sub_obj.slug, "parent_slug" => obj_a.slug}) - assert has_element?(view, "#delete_sub_objective_modal", "Delete Sub-Objective") + assert 3 == + project + |> ObjectiveEditor.fetch_objective_mappings() + |> length() + + refute has_element?(view, "##{obj_a.slug} .collapse", "#{sub_obj.title}") + assert has_element?(view, "##{obj_b.slug} .collapse", "#{sub_obj.title}") view |> element( - "button[phx-click='delete_sub_objective'][phx-value-slug=#{sub_obj.slug}][phx-value-parent_slug=#{obj_a.slug}]" + "##{obj_a.slug} button[phx-click='display_add_existing_sub_modal'][phx-value-slug=#{obj_a.slug}]" ) - |> render_click(%{"slug" => sub_obj.slug, "parent_slug" => obj_a.slug}) + |> render_click(%{"slug" => obj_a.slug}) + + assert has_element?(view, "#existing-sub-objective-#{sub_obj.resource_id}", sub_obj.title) + + refute has_element?( + view, + "button[phx-click='display_sub_objective_delete_modal'][phx-value-slug=#{sub_obj.slug}]" + ) + end - assert has_element?(view, "##{obj_a.slug} .line-through", "#{sub_obj.title}") + test "permanently deletes only an unassociated, untagged sub-objective", %{ + conn: conn, + project: project, + publication: publication + } do + {:ok, sub_obj} = create_objective(project, publication, "sub_obj", "Sub Objective") - wait_until(fn -> - has_element?(view, ~s{div[role="alert"].alert-info}, "Objective successfully removed") - end) + {:ok, obj} = + create_objective(project, publication, "obj", "Objective", [sub_obj.resource_id]) - assert 3 == - project - |> ObjectiveEditor.fetch_objective_mappings() - |> length() + {:ok, view, _html} = live(conn, live_view_route(project.slug, %{selected: obj.slug})) - refute has_element?(view, "##{obj_a.slug} .collapse", "#{sub_obj.title}") - assert has_element?(view, "##{obj_b.slug} .collapse", "#{sub_obj.title}") + view + |> element("button[phx-click='detach_sub_objective'][phx-value-slug=#{sub_obj.slug}]") + |> render_click(%{"slug" => sub_obj.slug, "parent_slug" => obj.slug}) - wait_for_coverage(view) + view + |> element("button[phx-click='display_add_existing_sub_modal'][phx-value-slug=#{obj.slug}]") + |> render_click(%{"slug" => obj.slug}) + + view + |> element( + "button[phx-click='display_sub_objective_delete_modal'][phx-value-slug=#{sub_obj.slug}]" + ) + |> render_click(%{"slug" => sub_obj.slug, "parent_slug" => obj.slug}) + + assert has_element?(view, "#delete_sub_objective_modal", "Delete sub-objective?") + assert has_element?(view, "#delete_sub_objective_modal", sub_obj.title) + assert has_element?(view, "#delete_sub_objective_modal", "This action cannot be undone") + + refute AuthoringResolver.from_revision_slug(project.slug, sub_obj.slug).deleted + + render_hook(view, "return_to_add_existing", %{ + "parent_slug" => obj.slug, + "focus_delete_slug" => sub_obj.slug + }) + + assert has_element?( + view, + "#select_existing_sub_modal[data-initial-focus='#delete-sub-objective-#{sub_obj.slug}']" + ) + + view + |> element( + "button[phx-click='display_sub_objective_delete_modal'][phx-value-slug=#{sub_obj.slug}]" + ) + |> render_click(%{"slug" => sub_obj.slug, "parent_slug" => obj.slug}) + + view + |> element("button[phx-click='delete_sub_objective'][phx-value-slug=#{sub_obj.slug}]") + |> render_click(%{"slug" => sub_obj.slug}) + + assert has_element?(view, ~s{div[role="alert"].alert-info}, "Sub-objective deleted") + assert 1 == length(ObjectiveEditor.fetch_objective_mappings(project)) + + refute Enum.any?(ObjectiveEditor.fetch_objective_mappings(project), fn mapping -> + mapping.revision.slug == sub_obj.slug + end) + end + + test "blocks permanent deletion when an unassociated sub-objective is tagged", %{ + conn: conn, + project: project, + publication: publication + } do + {:ok, sub_obj} = create_objective(project, publication, "sub_obj", "Sub Objective") + + {:ok, obj} = + create_objective(project, publication, "obj", "Objective", [sub_obj.resource_id]) + + {:ok, _page} = create_page_with_objective(project, publication, [sub_obj.resource_id]) + {:ok, view, _html} = live(conn, live_view_route(project.slug, %{selected: obj.slug})) + + view + |> element("button[phx-click='detach_sub_objective'][phx-value-slug=#{sub_obj.slug}]") + |> render_click(%{"slug" => sub_obj.slug, "parent_slug" => obj.slug}) + + view + |> element("button[phx-click='display_add_existing_sub_modal'][phx-value-slug=#{obj.slug}]") + |> render_click(%{"slug" => obj.slug}) + + view + |> element( + "button[phx-click='display_sub_objective_delete_modal'][phx-value-slug=#{sub_obj.slug}]" + ) + |> render_click(%{"slug" => sub_obj.slug, "parent_slug" => obj.slug}) + + refute has_element?(view, "#delete_sub_objective_modal") + + assert has_element?( + view, + ~s{div[role="alert"].alert-danger}, + "cannot be deleted because it is tagged to course content" + ) + + refute AuthoringResolver.from_revision_slug(project.slug, sub_obj.slug).deleted end test "renders links to revision history if #show_links is added to the url (being an admin)", From 69c96e860cc8a3979e0eb00398cd5113ac654cd1 Mon Sep 17 00:00:00 2001 From: Martin Manelli Date: Wed, 23 Sep 2026 14:40:11 -0300 Subject: [PATCH 02/19] fix styles --- .../objectives/select_existing_sub_modal.ex | 38 +++++++++++-------- .../course_author/objectives_live_test.exs | 5 +++ 2 files changed, 27 insertions(+), 16 deletions(-) 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 f05486cd4ad..bc336405982 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,6 +1,7 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.SelectExistingSubModal do use OliWeb, :live_component + alias OliWeb.Components.DesignTokens.Primitives.Button alias OliWeb.Icons def update(assigns, socket) do @@ -47,30 +48,29 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.SelectExistingSubModal do >
-
+

Select Existing Sub-Objective

- + />
-
+
-
@@ -169,10 +122,7 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.SelectExistingSubModal do def handle_event("filters_changed", params, socket) do {:noreply, socket - |> assign( - query: Map.get(params, "query", socket.assigns.query), - status: Map.get(params, "status", socket.assigns.status) - ) + |> assign(query: Map.get(params, "query", socket.assigns.query)) |> assign_filtered_sub_objectives()} end @@ -184,12 +134,7 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.SelectExistingSubModal do |> Enum.filter(fn sub_objective -> query == "" or String.contains?(String.downcase(sub_objective.title), query) end) - |> Enum.filter(&matches_status?(&1, socket.assigns.status)) assign(socket, filtered_sub_objectives: filtered_sub_objectives) end - - defp matches_status?(sub_objective, "associated"), do: sub_objective.association_count > 0 - defp matches_status?(sub_objective, "unassociated"), do: sub_objective.association_count == 0 - defp matches_status?(_sub_objective, _status), do: true end diff --git a/lib/oli_web/live/workspaces/course_author/objectives/sub_objective_delete_modal.ex b/lib/oli_web/live/workspaces/course_author/objectives/sub_objective_delete_modal.ex index abb65c98c5c..25a41621244 100644 --- a/lib/oli_web/live/workspaces/course_author/objectives/sub_objective_delete_modal.ex +++ b/lib/oli_web/live/workspaces/course_author/objectives/sub_objective_delete_modal.ex @@ -16,9 +16,7 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.SubObjectiveDeleteModal do aria-modal="true" aria-labelledby={"#{@id}-title"} aria-describedby={"#{@id}-description"} - data-dismiss-event="return_to_add_existing" - data-parent-slug={@parent_slug} - data-focus-delete-slug={@slug} + data-initial-focus="#cancel-sub-objective-delete" phx-hook="ModalLaunch" >
@@ -36,12 +34,13 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.SubObjectiveDeleteModal do
- “{@title}” is not associated with any learning objectives. Deleting it - will permanently remove it from this course. This action cannot be undone. + “{@title}” is associated only with this learning objective. Removing + its final association will permanently delete it from this course. This action cannot be undone.
- - - - + <.link :if={@revision_history_link} @@ -389,59 +382,49 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.Listing do class="shrink-0 text-current" /> - - - - +
+

+ {results_status(@filtered_sub_objectives)} +

+

+ case self() == owner and String.contains?(metadata.query, "FOR UPDATE") do + true -> send(owner, {:deletion_lock_query, metadata.params}) + false -> :ok + end + end, + self() + ) + + result = + try do + ObjectiveEditor.delete_sub_objective(child.slug, author, project, parent.slug) + after + :telemetry.detach(handler_id) + end + + assert {:ok, deleted} = result + + assert_receive {:deletion_lock_query, params} + assert [child.slug, parent.slug] in params + refute_receive {:deletion_lock_query, _params} assert deleted.deleted assert AuthoringResolver.from_resource_id(project.slug, parent.resource_id).children == [] diff --git a/test/oli_web/live/workspaces/course_author/objectives_live_test.exs b/test/oli_web/live/workspaces/course_author/objectives_live_test.exs index 200aa027e15..083a0c98c8b 100644 --- a/test/oli_web/live/workspaces/course_author/objectives_live_test.exs +++ b/test/oli_web/live/workspaces/course_author/objectives_live_test.exs @@ -1372,6 +1372,12 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLiveTest do "Add" ) + assert has_element?( + view, + "#select_existing_sub_modal-results-status[role='status'][aria-live='polite'][aria-atomic='true']", + "2 sub-objectives available." + ) + assert has_element?( view, "button[phx-click='add_existing_sub'][phx-value-slug=#{sub_obj_b.slug}]", @@ -1388,6 +1394,12 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLiveTest do |> element("#select_existing_sub_modal-filters") |> render_change(%{"query" => "testing", "status" => "all"}) + assert has_element?( + view, + "#select_existing_sub_modal-results-status", + "1 sub-objective available." + ) + assert has_element?( view, "button[phx-click='add_existing_sub'][phx-value-slug=#{sub_obj_b.slug}]", @@ -1524,11 +1536,13 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLiveTest do assert has_element?( view, - "#delete-objective-tooltip-#{obj.resource_id}", - "Delete Learning Objective" + "#delete-objective-action-#{obj.resource_id}[phx-hook='GlobalTooltip'][data-tooltip='Delete Learning Objective']" ) - assert has_element?(view, "[role='tooltip']", "Delete sub-objective") + assert has_element?( + view, + "#sub-objective-action-#{obj.resource_id}-#{sub_obj.resource_id}[phx-hook='GlobalTooltip'][data-tooltip='Delete sub-objective']" + ) view |> element( @@ -1600,7 +1614,10 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLiveTest do live_view_route(project.slug, %{expanded: Enum.join([obj_a.slug, obj_b.slug], ",")}) ) - assert has_element?(view, "[role='tooltip']", "Detach from learning objective") + assert has_element?( + view, + "#sub-objective-action-#{obj_a.resource_id}-#{sub_obj.resource_id}[phx-hook='GlobalTooltip'][data-tooltip='Detach from learning objective']" + ) refute has_element?( view, @@ -1652,11 +1669,23 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLiveTest do assert has_element?(view, "#select_existing_sub_modal-empty") + assert has_element?( + view, + "#select_existing_sub_modal-results-status[role='status'][aria-live='polite']", + "No sub-objectives match these filters." + ) + view |> element("#select_existing_sub_modal-filters") |> render_submit(%{"query" => "Sub Objective"}) assert has_element?(view, "#existing-sub-objective-#{sub_obj.resource_id}") + + assert has_element?( + view, + "#select_existing_sub_modal-results-status", + "1 sub-objective available." + ) end test "tagged final association cannot be deleted or detached", From eb4261e516d916bdc80ec0e3d37ef10f64d1b218 Mon Sep 17 00:00:00 2001 From: Martin Manelli Date: Tue, 6 Oct 2026 22:30:02 -0300 Subject: [PATCH 10/19] addressed review comments: fix modal focus fallback and share tooltip Escape listener --- assets/src/hooks/global_tooltip.ts | 14 +++--- assets/src/hooks/modal.ts | 5 +- assets/test/hooks/global_tooltip_test.ts | 63 ++++++++++++++++++++++++ assets/test/hooks/modal_test.ts | 15 ++++++ 4 files changed, 88 insertions(+), 9 deletions(-) diff --git a/assets/src/hooks/global_tooltip.ts b/assets/src/hooks/global_tooltip.ts index a60795b7416..38da92d9422 100644 --- a/assets/src/hooks/global_tooltip.ts +++ b/assets/src/hooks/global_tooltip.ts @@ -8,6 +8,7 @@ const clearActiveTooltip = () => { if (wrapper) wrapper.remove(); if (activeDescribedElement) { + document.removeEventListener('keydown', handleTooltipKeydown); if (activePreviousAriaDescribedBy) { activeDescribedElement.setAttribute('aria-describedby', activePreviousAriaDescribedBy); } else { @@ -19,6 +20,10 @@ const clearActiveTooltip = () => { activePreviousAriaDescribedBy = null; }; +const handleTooltipKeydown = (event: KeyboardEvent) => { + if (event.key === 'Escape') clearActiveTooltip(); +}; + export const GlobalTooltip = { mounted(this: { el: HTMLElement }) { const el = this.el; @@ -90,6 +95,7 @@ export const GlobalTooltip = { [activePreviousAriaDescribedBy, tooltipId].filter(Boolean).join(' '), ); activeDescribedElement = el; + document.addEventListener('keydown', handleTooltipKeydown); tooltipVisible = true; shownAt = Date.now(); @@ -140,18 +146,11 @@ export const GlobalTooltip = { } }; - const handleKeydown = (event: KeyboardEvent) => { - if (event.key === 'Escape' && activeDescribedElement === el) { - hideTooltip(); - } - }; - el.addEventListener('mouseenter', showTooltip); el.addEventListener('focus', showTooltip); el.addEventListener('click', handleClick); el.addEventListener('mouseleave', hideTooltip); el.addEventListener('blur', hideTooltip); - document.addEventListener('keydown', handleKeydown); document.addEventListener('click', handleDocumentClick); cleanupByElement.set(el, () => { @@ -161,7 +160,6 @@ export const GlobalTooltip = { el.removeEventListener('click', handleClick); el.removeEventListener('mouseleave', hideTooltip); el.removeEventListener('blur', hideTooltip); - document.removeEventListener('keydown', handleKeydown); document.removeEventListener('click', handleDocumentClick); }); }, diff --git a/assets/src/hooks/modal.ts b/assets/src/hooks/modal.ts index f9b131e2d09..4bf1a773c4a 100644 --- a/assets/src/hooks/modal.ts +++ b/assets/src/hooks/modal.ts @@ -23,8 +23,11 @@ export const ModalLaunch = { // A malformed custom selector must not prevent the autofocus fallback. } } - target ??= this.el.querySelector('[autofocus]'); if (target instanceof HTMLElement) target.focus(); + if (!(target instanceof HTMLElement) || document.activeElement !== target) { + const autofocus = this.el.querySelector('[autofocus]'); + if (autofocus instanceof HTMLElement) autofocus.focus(); + } }); this.modal.show(); diff --git a/assets/test/hooks/global_tooltip_test.ts b/assets/test/hooks/global_tooltip_test.ts index 01e6a372c61..3c7d04ace4a 100644 --- a/assets/test/hooks/global_tooltip_test.ts +++ b/assets/test/hooks/global_tooltip_test.ts @@ -175,4 +175,67 @@ describe('GlobalTooltip', () => { expect(second).toHaveFocus(); expect(second.hasAttribute('aria-describedby')).toBe(false); }); + + test('shares one Escape listener across mounted tooltips and removes it on dismissal', () => { + const addListener = jest.spyOn(document, 'addEventListener'); + const removeListener = jest.spyOn(document, 'removeEventListener'); + const keydownAdds = () => addListener.mock.calls.filter(([type]) => type === 'keydown'); + const keydownRemovals = () => removeListener.mock.calls.filter(([type]) => type === 'keydown'); + try { + const first = buildTrigger(); + const second = document.createElement('button'); + second.dataset.tooltip = 'Second help text'; + document.body.append(second); + mount(first); + mount(second); + + expect(keydownAdds()).toHaveLength(0); + + first.dispatchEvent(new Event('mouseenter')); + expect(keydownAdds()).toHaveLength(1); + + second.dispatchEvent(new Event('mouseenter')); + expect(keydownAdds()).toHaveLength(2); + expect(keydownRemovals()).toHaveLength(1); + expect(keydownAdds()[0][1]).toBe(keydownAdds()[1][1]); + expect(keydownRemovals()[0][1]).toBe(keydownAdds()[0][1]); + + GlobalTooltip.destroyed!.call({ el: first } as any); + expect(keydownRemovals()).toHaveLength(1); + + document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter' })); + expect(document.getElementById(WRAPPER_ID)).not.toBeNull(); + document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape' })); + expect(document.getElementById(WRAPPER_ID)).toBeNull(); + expect(keydownRemovals()).toHaveLength(2); + expect(keydownRemovals()[1][1]).toBe(keydownAdds()[1][1]); + } finally { + addListener.mockRestore(); + removeListener.mockRestore(); + } + }); + + test('cleans up the active Escape listener on destruction and supports remounting', () => { + const el = buildTrigger(); + el.setAttribute('aria-describedby', 'existing-description'); + mount(el); + el.dispatchEvent(new Event('mouseenter')); + const removeListener = jest.spyOn(document, 'removeEventListener'); + try { + GlobalTooltip.destroyed!.call({ el } as any); + + expect(document.getElementById(WRAPPER_ID)).toBeNull(); + expect(el.getAttribute('aria-describedby')).toBe('existing-description'); + expect(removeListener).toHaveBeenCalledWith('keydown', expect.any(Function)); + + mount(el); + el.dispatchEvent(new Event('mouseenter')); + document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape' })); + + expect(document.getElementById(WRAPPER_ID)).toBeNull(); + expect(el.getAttribute('aria-describedby')).toBe('existing-description'); + } finally { + removeListener.mockRestore(); + } + }); }); diff --git a/assets/test/hooks/modal_test.ts b/assets/test/hooks/modal_test.ts index 467bcd51f9b..bece052fc53 100644 --- a/assets/test/hooks/modal_test.ts +++ b/assets/test/hooks/modal_test.ts @@ -107,4 +107,19 @@ describe('ModalLaunch confirmation focus', () => { expect(hook.pushEvent).toHaveBeenCalledWith('phx_modal.unmount'); expect(document.body).not.toHaveClass('fix-position'); }); + + test.each([ + '', + '

Not focusable
', + '', + ])('falls back to autofocus when the custom target cannot receive focus: %s', (markup) => { + const modal = document.getElementById('confirmation') as HTMLElement; + modal.dataset.initialFocus = '#custom-target'; + modal.insertAdjacentHTML('beforeend', markup); + document.getElementById('cancel')?.setAttribute('autofocus', ''); + + expect(() => mount()).not.toThrow(); + + expect(document.getElementById('cancel')).toHaveFocus(); + }); }); From 2b91a11ff482596ad8ef5f6ae28c4b9c1d874301 Mon Sep 17 00:00:00 2001 From: Martin Manelli Date: Wed, 7 Oct 2026 01:19:44 -0300 Subject: [PATCH 11/19] remove tooltip and modal changes --- assets/src/hooks/global_tooltip.ts | 14 +++--- assets/src/hooks/modal.ts | 5 +- assets/test/hooks/global_tooltip_test.ts | 63 ------------------------ assets/test/hooks/modal_test.ts | 15 ------ 4 files changed, 9 insertions(+), 88 deletions(-) diff --git a/assets/src/hooks/global_tooltip.ts b/assets/src/hooks/global_tooltip.ts index 38da92d9422..a60795b7416 100644 --- a/assets/src/hooks/global_tooltip.ts +++ b/assets/src/hooks/global_tooltip.ts @@ -8,7 +8,6 @@ const clearActiveTooltip = () => { if (wrapper) wrapper.remove(); if (activeDescribedElement) { - document.removeEventListener('keydown', handleTooltipKeydown); if (activePreviousAriaDescribedBy) { activeDescribedElement.setAttribute('aria-describedby', activePreviousAriaDescribedBy); } else { @@ -20,10 +19,6 @@ const clearActiveTooltip = () => { activePreviousAriaDescribedBy = null; }; -const handleTooltipKeydown = (event: KeyboardEvent) => { - if (event.key === 'Escape') clearActiveTooltip(); -}; - export const GlobalTooltip = { mounted(this: { el: HTMLElement }) { const el = this.el; @@ -95,7 +90,6 @@ export const GlobalTooltip = { [activePreviousAriaDescribedBy, tooltipId].filter(Boolean).join(' '), ); activeDescribedElement = el; - document.addEventListener('keydown', handleTooltipKeydown); tooltipVisible = true; shownAt = Date.now(); @@ -146,11 +140,18 @@ export const GlobalTooltip = { } }; + const handleKeydown = (event: KeyboardEvent) => { + if (event.key === 'Escape' && activeDescribedElement === el) { + hideTooltip(); + } + }; + el.addEventListener('mouseenter', showTooltip); el.addEventListener('focus', showTooltip); el.addEventListener('click', handleClick); el.addEventListener('mouseleave', hideTooltip); el.addEventListener('blur', hideTooltip); + document.addEventListener('keydown', handleKeydown); document.addEventListener('click', handleDocumentClick); cleanupByElement.set(el, () => { @@ -160,6 +161,7 @@ export const GlobalTooltip = { el.removeEventListener('click', handleClick); el.removeEventListener('mouseleave', hideTooltip); el.removeEventListener('blur', hideTooltip); + document.removeEventListener('keydown', handleKeydown); document.removeEventListener('click', handleDocumentClick); }); }, diff --git a/assets/src/hooks/modal.ts b/assets/src/hooks/modal.ts index 4bf1a773c4a..f9b131e2d09 100644 --- a/assets/src/hooks/modal.ts +++ b/assets/src/hooks/modal.ts @@ -23,11 +23,8 @@ export const ModalLaunch = { // A malformed custom selector must not prevent the autofocus fallback. } } + target ??= this.el.querySelector('[autofocus]'); if (target instanceof HTMLElement) target.focus(); - if (!(target instanceof HTMLElement) || document.activeElement !== target) { - const autofocus = this.el.querySelector('[autofocus]'); - if (autofocus instanceof HTMLElement) autofocus.focus(); - } }); this.modal.show(); diff --git a/assets/test/hooks/global_tooltip_test.ts b/assets/test/hooks/global_tooltip_test.ts index 3c7d04ace4a..01e6a372c61 100644 --- a/assets/test/hooks/global_tooltip_test.ts +++ b/assets/test/hooks/global_tooltip_test.ts @@ -175,67 +175,4 @@ describe('GlobalTooltip', () => { expect(second).toHaveFocus(); expect(second.hasAttribute('aria-describedby')).toBe(false); }); - - test('shares one Escape listener across mounted tooltips and removes it on dismissal', () => { - const addListener = jest.spyOn(document, 'addEventListener'); - const removeListener = jest.spyOn(document, 'removeEventListener'); - const keydownAdds = () => addListener.mock.calls.filter(([type]) => type === 'keydown'); - const keydownRemovals = () => removeListener.mock.calls.filter(([type]) => type === 'keydown'); - try { - const first = buildTrigger(); - const second = document.createElement('button'); - second.dataset.tooltip = 'Second help text'; - document.body.append(second); - mount(first); - mount(second); - - expect(keydownAdds()).toHaveLength(0); - - first.dispatchEvent(new Event('mouseenter')); - expect(keydownAdds()).toHaveLength(1); - - second.dispatchEvent(new Event('mouseenter')); - expect(keydownAdds()).toHaveLength(2); - expect(keydownRemovals()).toHaveLength(1); - expect(keydownAdds()[0][1]).toBe(keydownAdds()[1][1]); - expect(keydownRemovals()[0][1]).toBe(keydownAdds()[0][1]); - - GlobalTooltip.destroyed!.call({ el: first } as any); - expect(keydownRemovals()).toHaveLength(1); - - document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter' })); - expect(document.getElementById(WRAPPER_ID)).not.toBeNull(); - document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape' })); - expect(document.getElementById(WRAPPER_ID)).toBeNull(); - expect(keydownRemovals()).toHaveLength(2); - expect(keydownRemovals()[1][1]).toBe(keydownAdds()[1][1]); - } finally { - addListener.mockRestore(); - removeListener.mockRestore(); - } - }); - - test('cleans up the active Escape listener on destruction and supports remounting', () => { - const el = buildTrigger(); - el.setAttribute('aria-describedby', 'existing-description'); - mount(el); - el.dispatchEvent(new Event('mouseenter')); - const removeListener = jest.spyOn(document, 'removeEventListener'); - try { - GlobalTooltip.destroyed!.call({ el } as any); - - expect(document.getElementById(WRAPPER_ID)).toBeNull(); - expect(el.getAttribute('aria-describedby')).toBe('existing-description'); - expect(removeListener).toHaveBeenCalledWith('keydown', expect.any(Function)); - - mount(el); - el.dispatchEvent(new Event('mouseenter')); - document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape' })); - - expect(document.getElementById(WRAPPER_ID)).toBeNull(); - expect(el.getAttribute('aria-describedby')).toBe('existing-description'); - } finally { - removeListener.mockRestore(); - } - }); }); diff --git a/assets/test/hooks/modal_test.ts b/assets/test/hooks/modal_test.ts index bece052fc53..467bcd51f9b 100644 --- a/assets/test/hooks/modal_test.ts +++ b/assets/test/hooks/modal_test.ts @@ -107,19 +107,4 @@ describe('ModalLaunch confirmation focus', () => { expect(hook.pushEvent).toHaveBeenCalledWith('phx_modal.unmount'); expect(document.body).not.toHaveClass('fix-position'); }); - - test.each([ - '', - '
Not focusable
', - '', - ])('falls back to autofocus when the custom target cannot receive focus: %s', (markup) => { - const modal = document.getElementById('confirmation') as HTMLElement; - modal.dataset.initialFocus = '#custom-target'; - modal.insertAdjacentHTML('beforeend', markup); - document.getElementById('cancel')?.setAttribute('autofocus', ''); - - expect(() => mount()).not.toThrow(); - - expect(document.getElementById('cancel')).toHaveFocus(); - }); }); From 1ef40d18024f945a58be41d7ec52e2d6ff3c202c Mon Sep 17 00:00:00 2001 From: Martin Manelli Date: Wed, 7 Oct 2026 12:16:06 -0300 Subject: [PATCH 12/19] remove tooltip changes --- assets/src/hooks/global_tooltip.ts | 42 ++---- assets/src/hooks/modal.ts | 24 --- assets/test/hooks/global_tooltip_test.ts | 178 ----------------------- assets/test/hooks/modal_test.ts | 110 -------------- 4 files changed, 12 insertions(+), 342 deletions(-) delete mode 100644 assets/test/hooks/global_tooltip_test.ts delete mode 100644 assets/test/hooks/modal_test.ts diff --git a/assets/src/hooks/global_tooltip.ts b/assets/src/hooks/global_tooltip.ts index a60795b7416..c8fb85b4df7 100644 --- a/assets/src/hooks/global_tooltip.ts +++ b/assets/src/hooks/global_tooltip.ts @@ -36,8 +36,6 @@ export const GlobalTooltip = { const showTooltip = () => { clearActiveTooltip(); - const position = el.dataset.tooltipPosition === 'bottom' ? 'bottom' : 'top'; - const wrapper = document.createElement('div'); wrapper.id = tooltipId; wrapper.setAttribute('role', 'tooltip'); @@ -61,27 +59,14 @@ export const GlobalTooltip = { tooltip.textContent = el.dataset.tooltip || ''; const caret = document.createElement('div'); - caret.className = - position === 'bottom' - ? ` - w-2 h-2 bg-Surface-surface-background - border-l-[0.5px] border-b-[0.5px] border-Border-border-default - rotate-[135deg] -mb-1 - ` - : ` - w-2 h-2 bg-Surface-surface-background - border-l-[0.5px] border-b-[0.5px] border-Border-border-default - -rotate-45 -mt-1 - `; - - if (position === 'bottom') { - wrapper.appendChild(caret); - wrapper.appendChild(tooltip); - } else { - wrapper.appendChild(tooltip); - wrapper.appendChild(caret); - } - + caret.className = ` + w-2 h-2 bg-Surface-surface-background + border-l-[0.5px] border-b-[0.5px] border-Border-border-default + -rotate-45 -mt-1 + `; + + wrapper.appendChild(tooltip); + wrapper.appendChild(caret); document.body.appendChild(wrapper); activePreviousAriaDescribedBy = el.getAttribute('aria-describedby'); @@ -108,10 +93,7 @@ export const GlobalTooltip = { wrapper.style.transform = 'translateX(-50%)'; } - wrapper.style.top = - position === 'bottom' - ? `${rect.bottom + 4}px` - : `${rect.top - wrapperRect.height - 4}px`; + wrapper.style.top = `${rect.top - wrapperRect.height - 4}px`; wrapper.style.visibility = 'visible'; // Show after positioning }); @@ -141,7 +123,7 @@ export const GlobalTooltip = { }; const handleKeydown = (event: KeyboardEvent) => { - if (event.key === 'Escape' && activeDescribedElement === el) { + if (event.key === 'Escape') { hideTooltip(); } }; @@ -151,7 +133,7 @@ export const GlobalTooltip = { el.addEventListener('click', handleClick); el.addEventListener('mouseleave', hideTooltip); el.addEventListener('blur', hideTooltip); - document.addEventListener('keydown', handleKeydown); + el.addEventListener('keydown', handleKeydown); document.addEventListener('click', handleDocumentClick); cleanupByElement.set(el, () => { @@ -161,7 +143,7 @@ export const GlobalTooltip = { el.removeEventListener('click', handleClick); el.removeEventListener('mouseleave', hideTooltip); el.removeEventListener('blur', hideTooltip); - document.removeEventListener('keydown', handleKeydown); + el.removeEventListener('keydown', handleKeydown); document.removeEventListener('click', handleDocumentClick); }); }, diff --git a/assets/src/hooks/modal.ts b/assets/src/hooks/modal.ts index f9b131e2d09..367121f552b 100644 --- a/assets/src/hooks/modal.ts +++ b/assets/src/hooks/modal.ts @@ -2,9 +2,6 @@ import { lockScroll, unlockScroll } from 'components/modal/utils'; export const ModalLaunch = { mounted(): void { - this.triggerElement = - document.activeElement instanceof HTMLElement ? document.activeElement : null; - // initialize the bootstrap modal const id = this.el.getAttribute('id'); this.id = id; @@ -12,21 +9,6 @@ export const ModalLaunch = { // ($(this.el) as any).modal({}); this.modal = new (window as any).Modal(this.el, {}); - - const initialFocus = this.el.dataset.initialFocus; - $(`#${id}`).on('shown.bs.modal', () => { - let target: Element | null = null; - if (initialFocus) { - try { - target = this.el.querySelector(initialFocus); - } catch { - // A malformed custom selector must not prevent the autofocus fallback. - } - } - target ??= this.el.querySelector('[autofocus]'); - if (target instanceof HTMLElement) target.focus(); - }); - this.modal.show(); const scrollPosition = lockScroll(); @@ -40,13 +22,7 @@ export const ModalLaunch = { // (modal close button, escape key, etc...) $(`#${id}`).on('hidden.bs.modal', () => { (this as any).pushEvent('phx_modal.unmount'); - unlockScroll(scrollPosition); - - const trigger = this.triggerElement; - if (trigger?.isConnected) { - window.requestAnimationFrame(() => trigger.focus()); - } }); }, destroyed(): void { diff --git a/assets/test/hooks/global_tooltip_test.ts b/assets/test/hooks/global_tooltip_test.ts deleted file mode 100644 index 01e6a372c61..00000000000 --- a/assets/test/hooks/global_tooltip_test.ts +++ /dev/null @@ -1,178 +0,0 @@ -import { GlobalTooltip } from '../../src/hooks/global_tooltip'; - -const WRAPPER_ID = 'global-tooltip-wrapper'; - -function buildTrigger(position?: string): HTMLElement { - document.body.innerHTML = ` - - `; - return document.getElementById('trigger') as HTMLElement; -} - -describe('GlobalTooltip', () => { - const originalRequestAnimationFrame = window.requestAnimationFrame; - const mountedElements: HTMLElement[] = []; - - beforeAll(() => { - window.requestAnimationFrame = ((callback: FrameRequestCallback) => { - callback(0); - return 1; - }) as typeof window.requestAnimationFrame; - - jest - .spyOn(HTMLElement.prototype, 'getBoundingClientRect') - .mockImplementation(function (this: HTMLElement) { - if (this.id === WRAPPER_ID) { - return { - width: 220, - height: 44, - top: 0, - left: 0, - bottom: 44, - right: 220, - x: 0, - y: 0, - toJSON() {}, - } as DOMRect; - } - - return { - width: 40, - height: 30, - top: 100, - left: 50, - bottom: 130, - right: 90, - x: 50, - y: 100, - toJSON() {}, - } as DOMRect; - }); - }); - - afterAll(() => { - window.requestAnimationFrame = originalRequestAnimationFrame; - jest.restoreAllMocks(); - }); - - afterEach(() => { - mountedElements.forEach((el) => GlobalTooltip.destroyed!.call({ el } as any)); - mountedElements.length = 0; - document.getElementById(WRAPPER_ID)?.remove(); - }); - - function mount(el: HTMLElement) { - GlobalTooltip.mounted!.call({ el } as any); - mountedElements.push(el); - } - - test('positions a bottom tooltip below the trigger and orders the caret before the tooltip', () => { - const el = buildTrigger('bottom'); - mount(el); - - el.dispatchEvent(new Event('mouseenter')); - - const wrapper = document.getElementById(WRAPPER_ID) as HTMLElement; - expect(wrapper.style.top).toBe('134px'); // trigger's rect.bottom (130) + 4 - - const [first, second] = Array.from(wrapper.children) as HTMLElement[]; - expect(first.textContent).toBe(''); // caret comes first for a bottom tooltip - expect(first.className).toContain('rotate-[135deg]'); - expect(second.textContent).toBe('Some help text'); - }); - - test('positions a default (top) tooltip above the trigger and orders the tooltip before the caret', () => { - const el = buildTrigger(); - mount(el); - - el.dispatchEvent(new Event('mouseenter')); - - const wrapper = document.getElementById(WRAPPER_ID) as HTMLElement; - expect(wrapper.style.top).toBe('52px'); // trigger's rect.top (100) - wrapper height (44) - 4 - - const [first, second] = Array.from(wrapper.children) as HTMLElement[]; - expect(first.textContent).toBe('Some help text'); // tooltip comes first by default - expect(second.className).toContain('-rotate-45'); - }); - - test('treats any non-"bottom" value the same as the default top position', () => { - const el = buildTrigger('top'); - mount(el); - - el.dispatchEvent(new Event('mouseenter')); - - const wrapper = document.getElementById(WRAPPER_ID) as HTMLElement; - expect(wrapper.style.top).toBe('52px'); - }); - - test('opens on keyboard focus, describes the trigger, and closes on blur', () => { - const el = buildTrigger('bottom'); - mount(el); - - el.dispatchEvent(new Event('focus')); - - const wrapper = document.getElementById(WRAPPER_ID) as HTMLElement; - expect(wrapper).not.toBeNull(); - expect(wrapper.textContent).toBe('Some help text'); - expect(el.getAttribute('aria-describedby')).toBe(WRAPPER_ID); - - el.dispatchEvent(new Event('blur')); - - expect(document.getElementById(WRAPPER_ID)).toBeNull(); - expect(el.hasAttribute('aria-describedby')).toBe(false); - }); - - test('Escape dismisses a focused tooltip without moving focus and restores its description', () => { - const el = buildTrigger(); - el.setAttribute('aria-describedby', 'existing-description'); - mount(el); - el.focus(); - - expect(document.getElementById(WRAPPER_ID)).not.toBeNull(); - expect(el.getAttribute('aria-describedby')).toBe(`existing-description ${WRAPPER_ID}`); - - el.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true })); - - expect(document.getElementById(WRAPPER_ID)).toBeNull(); - expect(el).toHaveFocus(); - expect(el.getAttribute('aria-describedby')).toBe('existing-description'); - }); - - test('Escape dismisses a hovered tooltip while focus remains on another control', () => { - const el = buildTrigger(); - const focusedControl = document.createElement('button'); - document.body.append(focusedControl); - focusedControl.focus(); - mount(el); - el.dispatchEvent(new Event('mouseenter')); - - expect(document.getElementById(WRAPPER_ID)).not.toBeNull(); - - focusedControl.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true })); - - expect(document.getElementById(WRAPPER_ID)).toBeNull(); - expect(focusedControl).toHaveFocus(); - expect(el.hasAttribute('aria-describedby')).toBe(false); - }); - - test('Escape dismisses only the currently active tooltip when multiple hooks are mounted', () => { - const first = buildTrigger(); - const second = document.createElement('button'); - second.dataset.tooltip = 'Second help text'; - document.body.append(second); - mount(first); - mount(second); - first.focus(); - second.focus(); - - expect(document.getElementById(WRAPPER_ID)?.textContent).toBe('Second help text'); - - document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape' })); - - expect(document.getElementById(WRAPPER_ID)).toBeNull(); - expect(second).toHaveFocus(); - expect(second.hasAttribute('aria-describedby')).toBe(false); - }); -}); diff --git a/assets/test/hooks/modal_test.ts b/assets/test/hooks/modal_test.ts deleted file mode 100644 index 467bcd51f9b..00000000000 --- a/assets/test/hooks/modal_test.ts +++ /dev/null @@ -1,110 +0,0 @@ -import { ModalLaunch } from '../../src/hooks/modal'; - -describe('ModalLaunch confirmation focus', () => { - const originalDollar = (global as any).$; - const originalModal = (window as any).Modal; - const events = new Map void>(); - const serverEvents = new Map void>(); - - beforeEach(() => { - events.clear(); - serverEvents.clear(); - document.body.innerHTML = ` - -
- -
- `; - (global as any).$ = jest.fn(() => ({ - on: (name: string, handler: () => void) => events.set(name, handler), - })); - (window as any).Modal = class { - show() { - document.getElementById('confirmation')?.focus(); - events.get('shown.bs.modal')?.(); - } - hide() { - events.get('hidden.bs.modal')?.(); - } - }; - jest.spyOn(window, 'requestAnimationFrame').mockImplementation((callback) => { - callback(0); - return 0; - }); - }); - - afterEach(() => { - jest.restoreAllMocks(); - (global as any).$ = originalDollar; - (window as any).Modal = originalModal; - document.body.innerHTML = ''; - document.body.classList.remove('fix-position'); - }); - - function mount() { - const trigger = document.getElementById('trigger') as HTMLElement; - trigger.focus(); - const hook = { - el: document.getElementById('confirmation') as HTMLElement, - pushEvent: jest.fn(), - handleEvent: (name: string, handler: () => void) => serverEvents.set(name, handler), - }; - ModalLaunch.mounted.call(hook); - return { hook, trigger }; - } - - test('focuses Cancel on opening and returns focus to the trigger on dismissal', () => { - const { hook, trigger } = mount(); - expect(document.getElementById('cancel')).toHaveFocus(); - expect(document.body).toHaveClass('fix-position'); - - events.get('hidden.bs.modal')?.(); - - expect(hook.pushEvent).toHaveBeenCalledWith('phx_modal.unmount'); - expect(trigger).toHaveFocus(); - expect(document.body).not.toHaveClass('fix-position'); - }); - - test('focuses the autofocus element when no explicit initial-focus target is set', () => { - const modal = document.getElementById('confirmation') as HTMLElement; - delete modal.dataset.initialFocus; - document.getElementById('cancel')?.setAttribute('autofocus', ''); - - mount(); - - expect(document.getElementById('cancel')).toHaveFocus(); - }); - - test('prefers the explicit initial-focus target over an autofocus element', () => { - const autofocus = document.createElement('button'); - autofocus.setAttribute('autofocus', ''); - document.getElementById('confirmation')?.append(autofocus); - - mount(); - - expect(document.getElementById('cancel')).toHaveFocus(); - expect(autofocus).not.toHaveFocus(); - }); - - test.each(['[', '#missing'])( - 'falls back to autofocus when the initial-focus selector is invalid or missing: %s', - (selector) => { - const modal = document.getElementById('confirmation') as HTMLElement; - modal.dataset.initialFocus = selector; - document.getElementById('cancel')?.setAttribute('autofocus', ''); - - expect(() => mount()).not.toThrow(); - - expect(document.getElementById('cancel')).toHaveFocus(); - }, - ); - - test('unmounts on server dismissal even if the triggering control was removed', () => { - const { hook, trigger } = mount(); - trigger.remove(); - serverEvents.get('phx_modal.hide')?.(); - - expect(hook.pushEvent).toHaveBeenCalledWith('phx_modal.unmount'); - expect(document.body).not.toHaveClass('fix-position'); - }); -}); From 846899f7019417dd59a87ed31d9fcd6877e65e7e Mon Sep 17 00:00:00 2001 From: Martin Manelli Date: Wed, 7 Oct 2026 12:29:47 -0300 Subject: [PATCH 13/19] reset files --- assets/src/hooks/global_tooltip.ts | 36 +++++-- assets/src/hooks/modal.ts | 7 ++ assets/test/hooks/global_tooltip_test.ts | 122 +++++++++++++++++++++++ 3 files changed, 156 insertions(+), 9 deletions(-) create mode 100644 assets/test/hooks/global_tooltip_test.ts diff --git a/assets/src/hooks/global_tooltip.ts b/assets/src/hooks/global_tooltip.ts index c8fb85b4df7..3209e28d924 100644 --- a/assets/src/hooks/global_tooltip.ts +++ b/assets/src/hooks/global_tooltip.ts @@ -36,6 +36,8 @@ export const GlobalTooltip = { const showTooltip = () => { clearActiveTooltip(); + const position = el.dataset.tooltipPosition === 'bottom' ? 'bottom' : 'top'; + const wrapper = document.createElement('div'); wrapper.id = tooltipId; wrapper.setAttribute('role', 'tooltip'); @@ -59,14 +61,27 @@ export const GlobalTooltip = { tooltip.textContent = el.dataset.tooltip || ''; const caret = document.createElement('div'); - caret.className = ` - w-2 h-2 bg-Surface-surface-background - border-l-[0.5px] border-b-[0.5px] border-Border-border-default - -rotate-45 -mt-1 - `; - - wrapper.appendChild(tooltip); - wrapper.appendChild(caret); + caret.className = + position === 'bottom' + ? ` + w-2 h-2 bg-Surface-surface-background + border-l-[0.5px] border-b-[0.5px] border-Border-border-default + rotate-[135deg] -mb-1 + ` + : ` + w-2 h-2 bg-Surface-surface-background + border-l-[0.5px] border-b-[0.5px] border-Border-border-default + -rotate-45 -mt-1 + `; + + if (position === 'bottom') { + wrapper.appendChild(caret); + wrapper.appendChild(tooltip); + } else { + wrapper.appendChild(tooltip); + wrapper.appendChild(caret); + } + document.body.appendChild(wrapper); activePreviousAriaDescribedBy = el.getAttribute('aria-describedby'); @@ -93,7 +108,10 @@ export const GlobalTooltip = { wrapper.style.transform = 'translateX(-50%)'; } - wrapper.style.top = `${rect.top - wrapperRect.height - 4}px`; + wrapper.style.top = + position === 'bottom' + ? `${rect.bottom + 4}px` + : `${rect.top - wrapperRect.height - 4}px`; wrapper.style.visibility = 'visible'; // Show after positioning }); diff --git a/assets/src/hooks/modal.ts b/assets/src/hooks/modal.ts index 367121f552b..5fa534d955b 100644 --- a/assets/src/hooks/modal.ts +++ b/assets/src/hooks/modal.ts @@ -8,9 +8,15 @@ export const ModalLaunch = { // ($('#' + id) as any).modal({}); // ($(this.el) as any).modal({}); + this.triggerElement = document.activeElement as HTMLElement; this.modal = new (window as any).Modal(this.el, {}); this.modal.show(); + $(`#${id}`).on('shown.bs.modal', () => { + const autofocus = this.el.querySelector('[autofocus]') as HTMLElement | null; + autofocus?.focus(); + }); + const scrollPosition = lockScroll(); // wire up server-side hide event @@ -23,6 +29,7 @@ export const ModalLaunch = { $(`#${id}`).on('hidden.bs.modal', () => { (this as any).pushEvent('phx_modal.unmount'); unlockScroll(scrollPosition); + this.triggerElement?.focus(); }); }, destroyed(): void { diff --git a/assets/test/hooks/global_tooltip_test.ts b/assets/test/hooks/global_tooltip_test.ts new file mode 100644 index 00000000000..837127b65e2 --- /dev/null +++ b/assets/test/hooks/global_tooltip_test.ts @@ -0,0 +1,122 @@ +import { GlobalTooltip } from '../../src/hooks/global_tooltip'; + +const WRAPPER_ID = 'global-tooltip-wrapper'; + +function buildTrigger(position?: string): HTMLElement { + document.body.innerHTML = ` + + `; + return document.getElementById('trigger') as HTMLElement; +} + +describe('GlobalTooltip', () => { + const originalRequestAnimationFrame = window.requestAnimationFrame; + + beforeAll(() => { + window.requestAnimationFrame = ((callback: FrameRequestCallback) => { + callback(0); + return 1; + }) as typeof window.requestAnimationFrame; + + jest + .spyOn(HTMLElement.prototype, 'getBoundingClientRect') + .mockImplementation(function (this: HTMLElement) { + if (this.id === WRAPPER_ID) { + return { + width: 220, + height: 44, + top: 0, + left: 0, + bottom: 44, + right: 220, + x: 0, + y: 0, + toJSON() {}, + } as DOMRect; + } + + return { + width: 40, + height: 30, + top: 100, + left: 50, + bottom: 130, + right: 90, + x: 50, + y: 100, + toJSON() {}, + } as DOMRect; + }); + }); + + afterAll(() => { + window.requestAnimationFrame = originalRequestAnimationFrame; + jest.restoreAllMocks(); + }); + + afterEach(() => { + document.getElementById(WRAPPER_ID)?.remove(); + }); + + function mount(el: HTMLElement) { + GlobalTooltip.mounted!.call({ el } as any); + } + + test('positions a bottom tooltip below the trigger and orders the caret before the tooltip', () => { + const el = buildTrigger('bottom'); + mount(el); + + el.dispatchEvent(new Event('mouseenter')); + + const wrapper = document.getElementById(WRAPPER_ID) as HTMLElement; + expect(wrapper.style.top).toBe('134px'); // trigger's rect.bottom (130) + 4 + + const [first, second] = Array.from(wrapper.children) as HTMLElement[]; + expect(first.textContent).toBe(''); // caret comes first for a bottom tooltip + expect(first.className).toContain('rotate-[135deg]'); + expect(second.textContent).toBe('Some help text'); + }); + + test('positions a default (top) tooltip above the trigger and orders the tooltip before the caret', () => { + const el = buildTrigger(); + mount(el); + + el.dispatchEvent(new Event('mouseenter')); + + const wrapper = document.getElementById(WRAPPER_ID) as HTMLElement; + expect(wrapper.style.top).toBe('52px'); // trigger's rect.top (100) - wrapper height (44) - 4 + + const [first, second] = Array.from(wrapper.children) as HTMLElement[]; + expect(first.textContent).toBe('Some help text'); // tooltip comes first by default + expect(second.className).toContain('-rotate-45'); + }); + + test('treats any non-"bottom" value the same as the default top position', () => { + const el = buildTrigger('top'); + mount(el); + + el.dispatchEvent(new Event('mouseenter')); + + const wrapper = document.getElementById(WRAPPER_ID) as HTMLElement; + expect(wrapper.style.top).toBe('52px'); + }); + + test('opens on keyboard focus, describes the trigger, and closes on blur', () => { + const el = buildTrigger('bottom'); + mount(el); + + el.dispatchEvent(new Event('focus')); + + const wrapper = document.getElementById(WRAPPER_ID) as HTMLElement; + expect(wrapper).not.toBeNull(); + expect(wrapper.textContent).toBe('Some help text'); + expect(el.getAttribute('aria-describedby')).toBe(WRAPPER_ID); + + el.dispatchEvent(new Event('blur')); + + expect(document.getElementById(WRAPPER_ID)).toBeNull(); + expect(el.hasAttribute('aria-describedby')).toBe(false); + }); +}); From a9c44346f3b9f8d757d38de4578854de0c5e94a0 Mon Sep 17 00:00:00 2001 From: Martin Manelli Date: Wed, 7 Oct 2026 12:53:05 -0300 Subject: [PATCH 14/19] remove serializable transaction --- lib/oli/authoring/editing/objective_editor.ex | 31 +++---------------- .../course_author/objectives_live.ex | 10 ------ test/oli/editing/objective_editor_test.exs | 3 -- test/oli_web/live/objectives_live_test.exs | 3 -- .../course_author/objectives_live_test.exs | 3 -- .../directives/objectives_handler_test.exs | 3 -- test/scenarios/objectives/objectives_test.exs | 3 -- test/scenarios/run_all_test.exs | 3 -- test/scenarios/scenario_runner_test.exs | 3 -- 9 files changed, 4 insertions(+), 58 deletions(-) diff --git a/lib/oli/authoring/editing/objective_editor.ex b/lib/oli/authoring/editing/objective_editor.ex index 67cedec2050..8d0c6518692 100644 --- a/lib/oli/authoring/editing/objective_editor.ex +++ b/lib/oli/authoring/editing/objective_editor.ex @@ -14,8 +14,6 @@ defmodule Oli.Authoring.Editing.ObjectiveEditor do import Oli.Utils - @serializable_retries 1 - @doc """ Creates a learning objective or sub-objective and optionally associates it with a parent. @@ -77,7 +75,7 @@ defmodule Oli.Authoring.Editing.ObjectiveEditor do """ def add_new_parent_for_sub_objective(slug, container_slug, project_slug, author) do result = - serializable_transaction(fn -> + Repo.transaction(fn -> publication = Publishing.project_working_publication(project_slug) revisions = objective_revisions(publication.id, slugs: [slug, container_slug], lock: true) @@ -234,7 +232,6 @@ defmodule Oli.Authoring.Editing.ObjectiveEditor do :last_association | :not_associated | :not_found - | :transaction_conflict | Ecto.Changeset.t()} def remove_sub_objective_from_parent( revision_slug, @@ -245,7 +242,7 @@ defmodule Oli.Authoring.Editing.ObjectiveEditor do parent_slug = objective_slug(parent_objective_or_slug) result = - serializable_transaction(fn -> + Repo.transaction(fn -> publication = Publishing.project_working_publication(project.slug) revisions = @@ -297,7 +294,7 @@ defmodule Oli.Authoring.Editing.ObjectiveEditor do 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 a serializable + 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()) :: @@ -307,11 +304,10 @@ defmodule Oli.Authoring.Editing.ObjectiveEditor do | :not_associated | :not_found | :tagged - | :transaction_conflict | Ecto.Changeset.t()} def delete_sub_objective(revision_slug, %Author{} = author, %Project{} = project, parent_slug) do result = - serializable_transaction(fn -> + Repo.transaction(fn -> publication = Publishing.project_working_publication(project.slug) revisions = @@ -387,25 +383,6 @@ defmodule Oli.Authoring.Editing.ObjectiveEditor do |> Enum.map(& &1.revision) end - defp serializable_transaction(fun, retries \\ @serializable_retries) do - Repo.transaction(fn -> - Repo.query!("SET TRANSACTION ISOLATION LEVEL SERIALIZABLE") - fun.() - end) - rescue - error in Postgrex.Error -> - case {serialization_failure?(error), retries} do - {true, retries} when retries > 0 -> serializable_transaction(fun, retries - 1) - {true, _retries} -> {:error, :transaction_conflict} - {false, _retries} -> reraise error, __STACKTRACE__ - end - end - - defp serialization_failure?(%Postgrex.Error{postgres: %{code: :serialization_failure}}), - do: true - - defp serialization_failure?(_error), do: false - defp objective_slug(%{slug: slug}), do: slug defp objective_slug(slug) when is_binary(slug), do: slug diff --git a/lib/oli_web/live/workspaces/course_author/objectives_live.ex b/lib/oli_web/live/workspaces/course_author/objectives_live.ex index d4ef61d97db..7bfb3605a61 100644 --- a/lib/oli_web/live/workspaces/course_author/objectives_live.ex +++ b/lib/oli_web/live/workspaces/course_author/objectives_live.ex @@ -1347,16 +1347,6 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLive do defp sub_objective_delete_flash({:error, :not_found}), do: fn socket -> put_flash(socket, :error, "Could not find that sub-objective") end - defp sub_objective_delete_flash({:error, :transaction_conflict}) do - fn socket -> - put_flash( - socket, - :error, - "The sub-objective changed while it was being deleted. Please try again" - ) - end - end - defp sub_objective_delete_flash({:error, _reason}), do: fn socket -> put_flash(socket, :error, "Could not delete sub-objective") end diff --git a/test/oli/editing/objective_editor_test.exs b/test/oli/editing/objective_editor_test.exs index 3a0cb735ba3..d78a6f81d11 100644 --- a/test/oli/editing/objective_editor_test.exs +++ b/test/oli/editing/objective_editor_test.exs @@ -6,9 +6,6 @@ defmodule Oli.Authoring.Editing.ObjectiveEditorTest do alias Oli.Publishing.AuthoringResolver alias Oli.Resources.Revision - @moduletag isolation: "serializable" - setup :setup_tags - describe "objective editing" do setup do map = diff --git a/test/oli_web/live/objectives_live_test.exs b/test/oli_web/live/objectives_live_test.exs index c580531d0e2..bd3f6db1143 100644 --- a/test/oli_web/live/objectives_live_test.exs +++ b/test/oli_web/live/objectives_live_test.exs @@ -16,9 +16,6 @@ defmodule OliWeb.ObjectivesLiveTest do alias Oli.Authoring.Editing.ObjectiveEditor alias Oli.Resources.ResourceType - @moduletag isolation: "serializable" - setup :setup_tags - defp live_view_route(project_slug, params \\ %{}), do: Routes.live_path(OliWeb.Endpoint, OliWeb.ObjectivesLive.Objectives, project_slug, params) diff --git a/test/oli_web/live/workspaces/course_author/objectives_live_test.exs b/test/oli_web/live/workspaces/course_author/objectives_live_test.exs index 083a0c98c8b..8b92dd955d7 100644 --- a/test/oli_web/live/workspaces/course_author/objectives_live_test.exs +++ b/test/oli_web/live/workspaces/course_author/objectives_live_test.exs @@ -35,9 +35,6 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLiveTest do alias Oli.Resources.Revision alias Oli.Resources.ResourceType - @moduletag isolation: "serializable" - setup :setup_tags - defp live_view_route(project_slug, params \\ %{}), do: ~p"/workspaces/course_author/#{project_slug}/objectives?#{params}" diff --git a/test/scenarios/directives/objectives_handler_test.exs b/test/scenarios/directives/objectives_handler_test.exs index a9f36cc63c2..cd30e1791f9 100644 --- a/test/scenarios/directives/objectives_handler_test.exs +++ b/test/scenarios/directives/objectives_handler_test.exs @@ -4,9 +4,6 @@ defmodule Oli.Scenarios.Directives.ObjectivesHandlerTest do alias Oli.Scenarios.DirectiveParser alias Oli.Scenarios.Engine - @moduletag isolation: "serializable" - setup :setup_tags - test "objectives directive creates and removes objective hierarchy entries" do yaml = """ - project: diff --git a/test/scenarios/objectives/objectives_test.exs b/test/scenarios/objectives/objectives_test.exs index 5723b7f4c1b..fce41947da4 100644 --- a/test/scenarios/objectives/objectives_test.exs +++ b/test/scenarios/objectives/objectives_test.exs @@ -4,9 +4,6 @@ defmodule Oli.Scenarios.Objectives.ObjectivesTest do alias Oli.Scenarios alias Oli.Scenarios.RuntimeOpts - @moduletag isolation: "serializable" - setup :setup_tags - @scenario_path Path.join(__DIR__, "authoring_objectives.scenario.yaml") test "objective authoring scenario" do diff --git a/test/scenarios/run_all_test.exs b/test/scenarios/run_all_test.exs index ea07643e087..dc52c77f39c 100644 --- a/test/scenarios/run_all_test.exs +++ b/test/scenarios/run_all_test.exs @@ -1,6 +1,3 @@ defmodule Oli.Scenarios.RunAllTest do use Oli.Scenarios.ScenarioRunner - - @moduletag isolation: "serializable" - setup :setup_tags end diff --git a/test/scenarios/scenario_runner_test.exs b/test/scenarios/scenario_runner_test.exs index a1415394535..7b325c1884c 100644 --- a/test/scenarios/scenario_runner_test.exs +++ b/test/scenarios/scenario_runner_test.exs @@ -23,7 +23,4 @@ defmodule Oli.Scenarios.ScenarioRunnerTest do end) end end - - @moduletag isolation: "serializable" - setup :setup_tags end From 6dbfc4c7b1fd5b79d210086e593579ef25e096d0 Mon Sep 17 00:00:00 2001 From: Martin Manelli Date: Wed, 7 Oct 2026 13:07:18 -0300 Subject: [PATCH 15/19] add tests --- test/oli/publishing_test.exs | 308 +++++++++++++++++++++++++++++++++++ 1 file changed, 308 insertions(+) diff --git a/test/oli/publishing_test.exs b/test/oli/publishing_test.exs index 3877857488a..6297941f815 100644 --- a/test/oli/publishing_test.exs +++ b/test/oli/publishing_test.exs @@ -51,6 +51,298 @@ defmodule Oli.PublishingTest do revision end + describe "get_objective_mappings_by_publication/2" do + setup do + %{publication: insert(:publication)} + end + + test "returns only active objectives in resource order with their associations preloaded", %{ + publication: publication + } do + first_resource = insert(:resource) + second_resource = insert(:resource) + second = insert_published_mapping(publication, %{resource: second_resource}) + first = insert_published_mapping(publication, %{resource: first_resource}) + insert_published_mapping(publication, %{deleted: true}) + insert_published_mapping(publication, %{resource_type_id: ResourceType.id_for_page()}) + insert_published_mapping(insert(:publication)) + + assert [first_result, second_result] = + Publishing.get_objective_mappings_by_publication(publication.id) + + assert first_result.id == first.id + assert second_result.id == second.id + assert first_result.resource.id == first_resource.id + assert first_result.revision.id == first.revision_id + assert second_result.resource.id == second_resource.id + assert second_result.revision.id == second.revision_id + end + + test "filters by revision slugs without including deleted or foreign-publication objectives", + %{ + publication: publication + } do + selected = insert_published_mapping(publication) + insert_published_mapping(publication) + deleted = insert_published_mapping(publication, %{deleted: true}) + foreign = insert_published_mapping(insert(:publication)) + slugs = [selected.revision.slug, deleted.revision.slug, foreign.revision.slug] + + assert [%PublishedResource{id: id}] = + Publishing.get_objective_mappings_by_publication(publication.id, slugs: slugs) + + assert id == selected.id + assert [] == Publishing.get_objective_mappings_by_publication(publication.id, slugs: []) + + assert [] == + Publishing.get_objective_mappings_by_publication(publication.id, + slugs: ["missing_objective"] + ) + end + + test "locks only the selected mappings when requested inside a transaction", %{ + publication: publication + } do + selected = insert_published_mapping(publication) + insert_published_mapping(publication) + handler_id = {__MODULE__, :objective_mapping_locks, make_ref()} + + :ok = + :telemetry.attach( + handler_id, + [:oli, :repo, :query], + fn _event, _measurements, metadata, owner -> + case self() == owner and String.contains?(metadata.query, "FOR UPDATE") do + true -> send(owner, {:objective_mapping_lock, metadata.params}) + false -> :ok + end + end, + self() + ) + + result = + try do + Repo.transaction(fn -> + Publishing.get_objective_mappings_by_publication(publication.id, + slugs: [selected.revision.slug], + lock: true + ) + end) + after + :telemetry.detach(handler_id) + end + + assert {:ok, [%PublishedResource{id: id}]} = result + assert id == selected.id + assert_receive {:objective_mapping_lock, params} + assert [selected.revision.slug] in params + refute_receive {:objective_mapping_lock, _params} + end + end + + describe "objective_parent_ids/2" do + setup do + %{publication: insert(:publication), child: insert(:resource)} + end + + test "returns each active parent resource ID once", %{publication: publication, child: child} do + first = insert_published_mapping(publication, %{children: [child.id, child.id]}) + second = insert_published_mapping(publication, %{children: [child.id]}) + insert_published_mapping(publication) + + assert Enum.sort(Publishing.objective_parent_ids(child.id, publication.id)) == + Enum.sort([first.resource_id, second.resource_id]) + end + + test "ignores deleted parents, non-objectives, and other publications", %{ + publication: publication, + child: child + } do + insert_published_mapping(publication, %{children: [child.id], deleted: true}) + + insert_published_mapping(publication, %{ + children: [child.id], + resource_type_id: ResourceType.id_for_container() + }) + + insert_published_mapping(insert(:publication), %{children: [child.id]}) + + assert Publishing.objective_parent_ids(child.id, publication.id) == [] + assert Publishing.objective_parent_ids(child.id, insert(:publication).id) == [] + end + + test "uses the currently mapped parent revision rather than historical children", %{ + publication: publication, + child: child + } do + parent = insert_published_mapping(publication, %{children: [child.id]}) + assert Publishing.objective_parent_ids(child.id, publication.id) == [parent.resource_id] + + assert {:ok, revision} = + Resources.create_revision_from_previous(parent.revision, %{children: []}) + + assert {:ok, _mapping} = + Publishing.update_published_resource(parent, %{revision_id: revision.id}) + + assert Publishing.objective_parent_ids(child.id, publication.id) == [] + assert Resources.get_revision!(parent.revision_id).children == [child.id] + end + end + + describe "objective_referenced?/2" do + setup do + publication = insert(:publication) + objective_id = insert(:resource).id + + references = %{ + page: %{ + resource_type_id: ResourceType.id_for_page(), + objectives: %{"attached" => [objective_id]} + }, + activity: %{ + resource_type_id: ResourceType.id_for_activity(), + objectives: %{"first_part" => [], "second_part" => [objective_id]} + }, + selection: %{ + resource_type_id: ResourceType.id_for_page(), + content: %{ + "model" => [ + %{ + "type" => "selection", + "logic" => %{ + "conditions" => %{ + "all" => [ + %{ + "any" => [ + %{ + "fact" => "objectives", + "operator" => "contains", + "value" => [objective_id] + } + ] + } + ] + } + } + } + ] + } + } + } + + %{publication: publication, objective_id: objective_id, references: references} + end + + for source <- [:page, :activity, :selection] do + test "detects an objective referenced only by #{source}", %{ + publication: publication, + objective_id: objective_id, + references: references + } do + insert_published_mapping(publication, Map.fetch!(references, unquote(source))) + + assert Publishing.objective_referenced?(objective_id, publication.id) + refute Publishing.objective_referenced?(insert(:resource).id, publication.id) + end + + test "ignores deleted #{source} references", %{ + publication: publication, + objective_id: objective_id, + references: references + } do + attrs = Map.fetch!(references, unquote(source)) |> Map.put(:deleted, true) + insert_published_mapping(publication, attrs) + + refute Publishing.objective_referenced?(objective_id, publication.id) + end + + test "does not count #{source} references from another publication", %{ + publication: publication, + objective_id: objective_id, + references: references + } do + other_publication = insert(:publication) + insert_published_mapping(other_publication, Map.fetch!(references, unquote(source))) + + refute Publishing.objective_referenced?(objective_id, publication.id) + assert Publishing.objective_referenced?(objective_id, other_publication.id) + end + + test "ignores #{source} references in superseded revisions", %{ + publication: publication, + objective_id: objective_id, + references: references + } do + mapping = insert_published_mapping(publication, Map.fetch!(references, unquote(source))) + assert Publishing.objective_referenced?(objective_id, publication.id) + + assert {:ok, revision} = + Resources.create_revision_from_previous(mapping.revision, %{ + objectives: %{}, + content: %{"model" => []} + }) + + assert {:ok, _mapping} = + Publishing.update_published_resource(mapping, %{revision_id: revision.id}) + + refute Publishing.objective_referenced?(objective_id, publication.id) + end + end + + test "returns false when there are no mapped references", %{ + publication: publication, + objective_id: objective_id + } do + insert(:revision, + resource_type_id: ResourceType.id_for_page(), + objectives: %{"attached" => [objective_id]} + ) + + refute Publishing.objective_referenced?(objective_id, publication.id) + end + + test "ignores references on resource types other than pages and activities", %{ + publication: publication, + objective_id: objective_id, + references: references + } do + insert_published_mapping(publication, references.page |> Map.delete(:resource_type_id)) + + insert_published_mapping( + publication, + Map.put(references.selection, :resource_type_id, ResourceType.id_for_container()) + ) + + refute Publishing.objective_referenced?(objective_id, publication.id) + end + + test "does not confuse other facts, page objective keys, or string IDs with objective tags", + %{ + publication: publication, + objective_id: objective_id + } do + insert_published_mapping(publication, %{ + resource_type_id: ResourceType.id_for_page(), + objectives: %{"attached" => [Integer.to_string(objective_id)], "other" => [objective_id]}, + content: %{ + "model" => [ + %{ + "type" => "selection", + "logic" => %{"conditions" => %{"fact" => "tags", "value" => [objective_id]}} + } + ] + } + }) + + insert_published_mapping(publication, %{ + resource_type_id: ResourceType.id_for_activity(), + objectives: %{"part" => [Integer.to_string(objective_id)]} + }) + + refute Publishing.objective_referenced?(objective_id, publication.id) + end + end + describe "learning-model parameter publication resolution" do setup do Oli.Seeder.base_project_with_resource2() @@ -1657,6 +1949,22 @@ defmodule Oli.PublishingTest do end end + defp insert_published_mapping(publication, attrs \\ %{}) do + revision = + attrs + |> Map.put_new_lazy(:resource, fn -> insert(:resource) end) + |> Map.put_new_lazy(:author, fn -> insert(:author) end) + |> Map.put_new(:resource_type_id, ResourceType.id_for_objective()) + |> then(&insert(:revision, &1)) + + insert(:published_resource, + publication: publication, + resource: revision.resource, + revision: revision, + author: revision.author + ) + end + defp activity_parameters(part_id, beta_difficulty) do %Parameters{ schema_version: 1, From 1d6390e992d13a95d2f96ec6a1255f8dcbb1147e Mon Sep 17 00:00:00 2001 From: Martin Manelli Date: Wed, 7 Oct 2026 13:27:31 -0300 Subject: [PATCH 16/19] [MER-5786] Clean up objective modal contracts and action handling --- lib/oli/authoring/editing/objective_editor.ex | 2 + lib/oli_web/icons.ex | 3 ++ .../objectives/sub_objective_delete_modal.ex | 2 +- .../course_author/objectives_live.ex | 46 +++++-------------- .../course_author/objectives_live_test.exs | 4 +- 5 files changed, 20 insertions(+), 37 deletions(-) diff --git a/lib/oli/authoring/editing/objective_editor.ex b/lib/oli/authoring/editing/objective_editor.ex index 8d0c6518692..ed3df7ef0bd 100644 --- a/lib/oli/authoring/editing/objective_editor.ex +++ b/lib/oli/authoring/editing/objective_editor.ex @@ -73,6 +73,8 @@ defmodule Oli.Authoring.Editing.ObjectiveEditor do 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 -> diff --git a/lib/oli_web/icons.ex b/lib/oli_web/icons.ex index 52cc1e44712..8a4b2c97e23 100644 --- a/lib/oli_web/icons.ex +++ b/lib/oli_web/icons.ex @@ -975,6 +975,9 @@ defmodule OliWeb.Icons do 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"""
@@ -41,6 +40,7 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.SubObjectiveDeleteModal do +
diff --git a/test/oli_web/live/workspaces/course_author/objectives_live_test.exs b/test/oli_web/live/workspaces/course_author/objectives_live_test.exs index 334fd93ded9..fbf458e653b 100644 --- a/test/oli_web/live/workspaces/course_author/objectives_live_test.exs +++ b/test/oli_web/live/workspaces/course_author/objectives_live_test.exs @@ -1360,7 +1360,22 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLiveTest do assert has_element?( view, - "#select_existing_sub_modal button[aria-label='Close Select Existing Sub-Objective dialog'][class~='!size-11'] svg" + "#select_existing_sub_modal button[aria-label='Close Select Existing Sub-Objective dialog'][class~='!h-11'][class~='!w-11'] svg" + ) + + assert has_element?( + view, + "#select_existing_sub_modal .modal-content[class~='!border'][class~='!border-Border-border-default']" + ) + + assert has_element?( + view, + "#select_existing_sub_modal-title[class~='leading-8']" + ) + + assert has_element?( + view, + "#select_existing_sub_modal-filters[class~='mb-2.5']" ) refute has_element?( @@ -1655,7 +1670,7 @@ defmodule OliWeb.Workspaces.CourseAuthor.ObjectivesLiveTest do assert has_element?( view, - "#select_existing_sub_modal button[phx-click='add_existing_sub'][class~='h-11']" + "#select_existing_sub_modal button[phx-click='add_existing_sub'][class~='h-8'][class~='border-Border-border-bold']" ) refute has_element?(view, "#select_existing_sub_modal button", "Delete") From 7db84a1240a6269e817703eace8786ad68fcc1fc Mon Sep 17 00:00:00 2001 From: Anders Weinstein Date: Fri, 9 Oct 2026 11:33:25 -0400 Subject: [PATCH 18/19] [MER-5786] Align delete button with Figma and improve contrast --- .../objectives/sub_objective_delete_modal.ex | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/oli_web/live/workspaces/course_author/objectives/sub_objective_delete_modal.ex b/lib/oli_web/live/workspaces/course_author/objectives/sub_objective_delete_modal.ex index d2a292bb9b3..7b8a57bb4d3 100644 --- a/lib/oli_web/live/workspaces/course_author/objectives/sub_objective_delete_modal.ex +++ b/lib/oli_web/live/workspaces/course_author/objectives/sub_objective_delete_modal.ex @@ -1,6 +1,8 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.SubObjectiveDeleteModal do use OliWeb, :html + alias OliWeb.Components.DesignTokens.Primitives.Button + attr(:id, :string, required: true) attr(:parent_slug, :string, required: true) attr(:slug, :string, required: true) @@ -46,15 +48,14 @@ defmodule OliWeb.Workspaces.CourseAuthor.Objectives.SubObjectiveDeleteModal do > Cancel - +
From d848c1f925ef01b119ce29c0e24d2d3ca858844b Mon Sep 17 00:00:00 2001 From: Anders Weinstein Date: Fri, 9 Oct 2026 13:35:51 -0400 Subject: [PATCH 19/19] addressed review comments: scope objective locking order --- lib/oli/authoring/editing/objective_editor.ex | 5 +-- lib/oli/publishing.ex | 3 +- test/oli/publishing_test.exs | 31 ++++++++++++------- 3 files changed, 24 insertions(+), 15 deletions(-) diff --git a/lib/oli/authoring/editing/objective_editor.ex b/lib/oli/authoring/editing/objective_editor.ex index ed3df7ef0bd..2e0265d011d 100644 --- a/lib/oli/authoring/editing/objective_editor.ex +++ b/lib/oli/authoring/editing/objective_editor.ex @@ -296,8 +296,9 @@ defmodule Oli.Authoring.Editing.ObjectiveEditor do 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. + 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{}} diff --git a/lib/oli/publishing.ex b/lib/oli/publishing.ex index 34ce4d72b55..717d69c3ada 100644 --- a/lib/oli/publishing.ex +++ b/lib/oli/publishing.ex @@ -828,7 +828,6 @@ defmodule Oli.Publishing do 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 = @@ -839,7 +838,7 @@ defmodule Oli.Publishing do query = case Keyword.get(opts, :lock, false) do - true -> from mapping in query, lock: "FOR UPDATE" + true -> from mapping in query, order_by: mapping.resource_id, lock: "FOR UPDATE" false -> query end diff --git a/test/oli/publishing_test.exs b/test/oli/publishing_test.exs index 6297941f815..79c4b778ae2 100644 --- a/test/oli/publishing_test.exs +++ b/test/oli/publishing_test.exs @@ -56,7 +56,7 @@ defmodule Oli.PublishingTest do %{publication: insert(:publication)} end - test "returns only active objectives in resource order with their associations preloaded", %{ + test "returns only active objectives with their associations preloaded", %{ publication: publication } do first_resource = insert(:resource) @@ -67,8 +67,12 @@ defmodule Oli.PublishingTest do insert_published_mapping(publication, %{resource_type_id: ResourceType.id_for_page()}) insert_published_mapping(insert(:publication)) - assert [first_result, second_result] = - Publishing.get_objective_mappings_by_publication(publication.id) + results = Publishing.get_objective_mappings_by_publication(publication.id) + + assert MapSet.new(Enum.map(results, & &1.id)) == MapSet.new([first.id, second.id]) + + first_result = Enum.find(results, &(&1.id == first.id)) + second_result = Enum.find(results, &(&1.id == second.id)) assert first_result.id == first.id assert second_result.id == second.id @@ -103,7 +107,10 @@ defmodule Oli.PublishingTest do test "locks only the selected mappings when requested inside a transaction", %{ publication: publication } do - selected = insert_published_mapping(publication) + first_resource = insert(:resource) + second_resource = insert(:resource) + second = insert_published_mapping(publication, %{resource: second_resource}) + first = insert_published_mapping(publication, %{resource: first_resource}) insert_published_mapping(publication) handler_id = {__MODULE__, :objective_mapping_locks, make_ref()} @@ -113,7 +120,7 @@ defmodule Oli.PublishingTest do [:oli, :repo, :query], fn _event, _measurements, metadata, owner -> case self() == owner and String.contains?(metadata.query, "FOR UPDATE") do - true -> send(owner, {:objective_mapping_lock, metadata.params}) + true -> send(owner, {:objective_mapping_lock, metadata.query, metadata.params}) false -> :ok end end, @@ -124,7 +131,7 @@ defmodule Oli.PublishingTest do try do Repo.transaction(fn -> Publishing.get_objective_mappings_by_publication(publication.id, - slugs: [selected.revision.slug], + slugs: [second.revision.slug, first.revision.slug], lock: true ) end) @@ -132,11 +139,13 @@ defmodule Oli.PublishingTest do :telemetry.detach(handler_id) end - assert {:ok, [%PublishedResource{id: id}]} = result - assert id == selected.id - assert_receive {:objective_mapping_lock, params} - assert [selected.revision.slug] in params - refute_receive {:objective_mapping_lock, _params} + assert {:ok, [%PublishedResource{id: first_id}, %PublishedResource{id: second_id}]} = result + assert first_id == first.id + assert second_id == second.id + assert_receive {:objective_mapping_lock, query, params} + assert query =~ "ORDER BY" + assert [second.revision.slug, first.revision.slug] in params + refute_receive {:objective_mapping_lock, _, _} end end