[ENHANCEMENT] [MER-5786] Subobjective deletion checks

[manelli]

Summary

Clarifies sub-objective removal by deriving parent associations from the objective graph, without adding a Revision.objective_type field or migration.

• Shared sub-objectives display an unlink action with the tooltip “Detach from learning objective.” Removing one association preserves the sub-objective, its course-content references, and its other learning-objective associations.
• Sub-objectives with one remaining parent display a distinct delete action. A confirmation dialog identifies the sub-objective and explains that removing its final association permanently deletes it from the course.
• Final-association deletion is blocked when the sub-objective is tagged to course content. Eligibility is checked both before showing the dialog and again when deletion is confirmed.
• Parent unlinking and child deletion are atomic. Transactional checks handle stale associations and concurrent edits; shared unlinking locks only the selected parent/child mappings.
• Add Existing lists active sub-objectives that still have a parent association. Search submission is handled without a page reload, and Add buttons have 44px targets.
• Distinct accessible action names, hover/focus tooltips, confirmation focus handling, and success/error messages support the updated workflow. Existing top-level learning-objective deletion remains unchanged.

Scope clarification

Per Darren’s review clarification, removing the final association deletes the sub-objective after confirmation; detached, unassociated sub-objectives are not retained for later reattachment.

This intentionally supersedes MER-5786’s original criteria for preserving the final detached sub-objective, offering an Unassociated status filter, and restricting permanent deletion to the unassociated list. The Unassociated workflow and its delete action have been removed. The ticket still contains the original criteria; this PR documents the agreed implementation scope.

[github-actions]

	Warnings
⚠️	PR is large (1915 LOC changed). Consider splitting.

Risk score: 11 → risk/high

Generated by 🚫 dangerJS against 2b91a11

[github-actions]

AI Review — performance

Avoid unconditional sorting of objective mappings

file: lib/oli/publishing.ex
line: 831
Description: The added order_by applies to every call, including existing unlocked reads that do not require deterministic lock acquisition. This can add an unnecessary sort across all objective mappings in a publication.
Suggestion: Add order_by: mapping.resource_id only when lock: true, where stable lock ordering is needed, or back it with a matching publication/resource index if all callers genuinely require this order.

Recursive JSONPath lookup creates a publication-wide scan

file: lib/oli/publishing.ex
line: 1532
Description: The recursive jsonb_path_exists over rev.content is evaluated across active page revisions in the publication. When the objective is not referenced—the normal prerequisite for deletion—the EXISTS query must inspect every candidate and recursively traverse each content document, increasing interactive deletion latency with course size.
Suggestion: Use an indexed/normalized objective-reference lookup maintained when revisions are written. If JSONB must remain the source, rewrite to an index-supported predicate and add the corresponding GIN index, then verify the absent-reference case with EXPLAIN ANALYZE.

[github-actions]

AI Review — security

No issues found

[github-actions]

AI Review — ui

Add action allows duplicate submissions

file: lib/oli_web/live/workspaces/course_author/objectives/select_existing_sub_modal.ex
line: 119
Description: The Add button remains enabled with no pending feedback after activation. Under network latency, users can click repeatedly, sending duplicate association requests and receiving no indication that work is underway.
Suggestion: Track the pending sub-objective, disable its button immediately, set aria-busy, and replace “Add” with a loading state until the server responds.

Deleting state exposes the wrong tooltip

file: lib/oli_web/live/workspaces/course_author/objectives/listing.ex
line: 402
Description: When deleting? is true, the control is disabled and displays a loader, but its tooltip still says “Delete sub-objective.” This gives inconsistent status feedback, especially for pointer users.
Suggestion: Set the tooltip to “Deleting…” whenever deleting? is true, analogous to the existing detaching state.

[github-actions]

AI Review — elixir

Call the SQL adapter for the isolation statement

file: lib/oli/authoring/editing/objective_editor.ex
line: 394
Description: Ecto.Repo does not expose query!/1; this transaction will fail when attempting to set the isolation level.
Suggestion: Use Ecto.Adapters.SQL.query!(Repo, "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE", []).

Declare the class attribute before the component

file: lib/oli_web/icons.ex
line: 1000
Description: Phoenix attr declarations apply to the next component function. Placing attr :class after unlink/1 leaves @class without its declared default and incorrectly applies the attribute to the following component.
Suggestion: Move attr :class, :string, default: "stroke-black dark:stroke-white" above def unlink(assigns) with the other unlink attributes.

[github-actions]

AI Review — typescript

Document listener scales with every tooltip

file: assets/src/hooks/global_tooltip.ts
line: 154
Description: Each tooltip instance now registers a document-level keydown listener, so every key press invokes every tooltip handler. Pages with many tooltip elements can incur substantial unnecessary work.
Suggestion: Use one shared document listener for the active tooltip, or attach this listener only while the tooltip is active and remove it when hidden.

[manelli]

Elixir review follow-up: the objective-type migration is present at priv/repo/migrations/20260922154029_add_objective_type_to_revisions.exs with explicit up/0 and down/0, and the Icons.unlink/1 attributes are declared immediately before the function. Commit 8c912c1d00 also flattens the detach transaction error contract and adds coverage for the resulting domain behavior.

[manelli]

TypeScript review follow-up: no code change was needed for the reported modal focus-listener ordering. In the current PR, assets/src/hooks/modal.ts registers the shown.bs.modal listener before calling this.modal.show(), so the initial focus event cannot be missed for that reason.

[darrensiegel]

There is no need to add a new Revision field for objective_type (:objective or :sub_objective) here to address this ticket.

All that needs to be done in the ObjectiveLive view is an upfront traversal of the graph of objectives. For each objective, store a list of parents. When a child being "removed" has more than one parents we simply unlink. When a child being removed has only one parent, we show a warning message indicating that it is about to be deleted.

[manelli]

Hi @darrensiegel the field was added in order to fulfill the following acceptance criteria:

If the removed association was the sub-objective’s final learning-objective association, the sub-objective becomes unassociated.
A detached sub-objective remains available in the Add Existing modal and can be associated with a learning objective again.

When the final association is removed both an unassociated subobjective and a top level objective have zero parents, so using the graph alone I don't think i can distinguish them to populate the Add Existing modal.

[darrensiegel]

@manelli I still do not believe that a new Revision field is necessary to accomplish this.

All we are requesting is that when we are removing the ONLY instance of a sub objective, we delete that sub objective. That tracking can be strictly accomplished by maintaining an in-memory "graph" of the learning objectives.

"A detached sub-objective remains available in the Add Existing modal and can be associated with a learning objective again." This is an INCORRECT acceptance criteria. We cannot - and do not want - to support detaching sub objectives and later reattaching them.

[manelli]

Hi @darrensiegel, updated this to follow your clarification. The new Revision field and its migration have been removed. ObjectivesLive now builds a parent map from the objective graph: shared sub-objectives use the unlink action, while removing the final association requires an explicit permanent-deletion confirmation.

Final-association deletion is blocked if the sub-objective is tagged to course content, and the server rechecks both references and parent associations when confirmation is submitted. Shared unlinking preserves the child and its other associations.

I also removed the Unassociated workflow from Add Existing and updated the PR description to explicitly document which original ticket criteria this clarification supersedes.