[BUG FIX] [MER-4055] Fix section update after re-adding page earlier removed from curriculum

[andersweinstein]

This PR fixes MER-4055, where a page removed from curriculum would disappear from a section after a publication
update, but later re-adding that same page to the curriculum and applying a subsequent publication update would
not make it reappear in student delivery.

The root cause was in section update processing, so it could affect both automatically pushed ("force push")
updates and manually applied section updates.When a page became unreachable, its section_resource could be
culled from the section. If that same page was later reintroduced, the update logic did not recreate the missing
section_resource, so delivery still had nothing to render even though the page was back in the authoring project
and publication. This PR updates the section update flow to recreate missing section resources for resources
present in the new publication before reconciling hierarchy, which allows reintroduced pages to appear again in
sections.

This branch retains an authoring-side safeguard committed in an earlier PR: when a deleted page is reattached
to a container, the code now restores its deleted flag. That is not the primary fix for MER-4055, but it remains a
reasonable defensive improvement for exceptional case of admin page recovery workflows.

Testing:

• Added a regression test covering the bug workflow: remove a page from curriculum, publish and apply the
  section update, re-add the same page, publish and apply the next update, and confirm it reappears in delivery.
• Re-ran the nearby section update regression and the ContainerEditor tests.

[github-actions]

Risk score: 7 → risk/medium

Generated by 🚫 dangerJS against d2579e0

[github-actions]

AI Review — security

No issues found

[github-actions]

AI Review — performance

Full revision structs are loaded unnecessarily

file: lib/oli/delivery/sections/updates.ex
line: 331
Description: select: rev loads full Revision structs for every missing resource, but bulk_create_section_resources/3 uses only a subset of fields. On large publications this increases DB payload size, decoding CPU, and memory pressure.
Suggestion: Replace select: rev with a projected map containing only fields used for inserts (for example resource_id, scoring_strategy_id, title, collab_space_config, batch_scoring, replacement_strategy, max_attempts, retake_mode, assessment_mode), and update bulk_create_section_resources/3 to consume that map shape.

Major update now always does an extra full publication scan

file: lib/oli/delivery/sections/updates.ex
line: 357
Description: Calling create_missing_section_resources/3 unconditionally adds an additional query over all PublishedResource rows for the new publication (plus join to SectionResource) on every major update, even when no resources are missing. This adds avoidable latency for large courses.
Suggestion: Gate this call behind a cheap precondition (for example only when diff indicates potential adds/moves), or fold missing-resource creation into the existing add/remove path to avoid an extra full scan pass.

[github-actions]

AI Review — elixir

Full revision rows loaded unnecessarily in missing-resource query

file: lib/oli/delivery/sections/updates.ex
line: 331
Description: select: rev loads entire revision structs for every missing resource, which can be expensive in major updates (large courses) and increases memory/DB payload even though only a small subset of fields is used by bulk_create_section_resources/3.
Suggestion: Select only the fields needed for section resource inserts (e.g., resource_id, title, scoring_strategy_id, collab_space_config, batch_scoring, replacement_strategy, max_attempts, retake_mode, assessment_mode) and update bulk_create_section_resources/3 to consume that slimmer shape.

Test can crash with unclear failure if expected child is missing

file: test/oli/sections_test.exs
line: 1174
Description: Enum.find(...).uuid will raise if the page is not found, producing a less actionable error than an assertion and making the test failure harder to diagnose.
Suggestion: Split this into an explicit assertion before dereferencing, e.g. page1_node = Enum.find(...); refute is_nil(page1_node); page1_uuid = page1_node.uuid.

[andersweinstein]

AI Review — performance

Full revision structs loaded for bulk insert path

file: lib/oli/delivery/sections/updates.ex line: 331 Description: select: rev loads full revision structs for every missing published resource. In Torus, revisions can carry large fields that are not needed for bulk_create_section_resources, increasing DB payload, decoding CPU, and BEAM memory pressure on large publications. Suggestion: Change the query to project only the fields used in bulk_create_section_resources (for example with select: map(rev, [:resource_id, :title, :graded, :purpose, :max_attempts, :retake_mode, :assessment_mode, :scoring_strategy_id, :collab_space_config, :batch_scoring, :replacement_strategy])), and update bulk_create_section_resources to consume that shape.

Because missing resources arise from instructor manually restoring previously removed pages to the curriculum, we expect the size of this set to be small (in most cases empty), so current simpler code should be acceptable.

Major update now adds a full publication-wide reconciliation query

file: lib/oli/delivery/sections/updates.ex line: 357 Description: Running create_missing_section_resources/3 on every major update introduces an additional publication-wide query and insert attempt pass, even when only a small diff changed. This increases update latency proportionally to publication size. Suggestion: Restrict the reconciliation to candidate resource IDs from the already computed PublicationDiff (or gate execution when the diff indicates possible missing records), so the query and insert scope is bounded to changed resources instead of the full publication.

Scoping this reconciliation to only PublicationDiff additions would miss
already-bugged sections. If a page was restored in an earlier publish but
failed to create its section_resource, then on a later unrelated publish that
page is present in both publications and no longer appears as :added in the
diff. The broader reconciliation query is what allows those previously missed
restored pages to be repaired on subsequent updates.

[darrensiegel]

This scares me a bit. I think we need to ensure that there is specifically a unit or scenario test somewhere (and if there isn't one, create it) that clearly demonstrates these operations:

1. Project created and published, course section created
2. Course section remixed to REMOVE page A from section
3. Change made in project (new page B added) and MAJOR publication made and applied to section

Assert that B is added but A remains removed from section

1. Project created and published, course section created
2. Course section remixed to REMOVE page A from section
3. Change made in project to REMOVE page A. Publication applied to section
4. Assert Page A is not present in section
5. Change made in project to RE-ADD page A to curriculum. Publication applied to section
6. Assert Page A is not present in section

[darrensiegel]

See additional tests that we need

[andersweinstein]

See additional tests that we need

The implementation passes the first test as is for the following reason:

During a major update, update_container_children/3 does a three-way merge between:
  - base: prior published container children
  - source: new published container children
  - target: current section container children

If the section removed page A locally and the project later adds unrelated page B, the merge
reapplies the section-side removal to the new project list, so A stays out and B is appended.

So although A is detected as a missing resource in this case and (temporarily) restored to section_resources, the existing three-way merge logic does not add it to curriculum in this situation. After merge, the provisionally added row gets culled once again as unreachable since not in curriculum.

But the second case would require more work to support:

The second desired case is different. Once the project itself removes page A and that update is applied, the section no longer retains any persisted “local removal” tombstone for A. rebuild_section_curriculum/3 deletes removed section-resource rows, so when the project later re-adds A, the update path only sees “A is back in the publication” and has no durable record that the section had previously removed it on purpose. In other words, with the current data model, that sticky-removal intent is lost after the project-side removal is applied.

Basically we would need some way to persist the information that A was removed from section even after A is removed from project and published to handle the case where A is added back into project and published. Otherwise code has no way to detect that there was a prior remix removal of A that needs to take precedence. This would require new database support, most naturally a table to remember remix-removed resources. (could also add column to flag removed section resources, but then removed resources would have to be filtered in queries where appropriate)

[github-actions]

PrivSignal Report

Top-level result: NONE with PR label privacy-none

Score	High	Medium	Low	Total
NONE	0	0	0	0

Links: artifact bundle

Detail	Value
Base ref used	origin/master
Scan exit	0
Diff exit	0
Score exit	0
Lockfile changed	true
Reason events	0
Sample events shown	0

Reason Events

None

Sample Of All Events

None

[github-actions]

Preview deployed to: https://pr-6283.plasma.oli.cmu.edu

[andersweinstein]

Added test for Darren's case 1. Case 2 we are allowing to fail rather than add new database support to handle it.