Skip to content

[FEATURE] [MER-4082] Adaptive page duplication#6491

Merged
rgachuhi merged 3 commits intomasterfrom
adaptive-page-duplication
Apr 28, 2026
Merged

[FEATURE] [MER-4082] Adaptive page duplication#6491
rgachuhi merged 3 commits intomasterfrom
adaptive-page-duplication

Conversation

@darrensiegel
Copy link
Copy Markdown
Contributor

This PR implements page duplication for adaptive pages. Given that this is a high risk feature, the entire feature is hidden behind a canary feature flag that defaults to 100% available - so if after deployment we realize something isn't fully correct we can easily toggle it back to 0% available to remove access.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 23, 2026

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

Note: risk rules not loaded — micromatch is not a function

Generated by 🚫 dangerJS against 8878230

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 23, 2026

AI Review — security

Missing edit authorization in adaptive duplication entry point

file: lib/oli/authoring/editing/adaptive_duplication.ex
line: 65
Description: duplicate/3 performs a state-changing copy after only checking that the author has feature-flag access. Feature-flag access is not the same as project edit authorization, so any caller that reaches this public function with a valid %Author{} and %Project{} could duplicate content in a project without an explicit object-level permission check.
Suggestion: Add a server-side authorization check before duplication, using the project authoring policy/collaboration permission used by other curriculum edit actions, and return an authorization error before loading or mutating revisions when the author cannot edit the project.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 23, 2026

AI Review — performance

Per-row feature flag checks during render

file: lib/oli_web/live/curriculum/entries/actions.ex
line: 109
Description: ScopedFeatureFlags.can_access?/3 is now called from the row action component for each rendered entry/table row. If this hits scoped flag state storage, rendering pages with many rows can turn into repeated flag lookups on every LiveView render.
Suggestion: Compute adaptive duplication access once in the parent LiveView/table model, pass a boolean assign such as adaptive_duplication_enabled, and have show_duplicate_action?/3 use that precomputed value instead of calling can_access?/3 per row.

DB updates executed in a loop

file: lib/oli/authoring/editing/adaptive_duplication.ex
line: 478
Description: update_revision_references/2 runs Repo.update_all/2 inside Enum.reduce_while/3, issuing one database update per duplicated screen plus the page. Torus performance guidelines explicitly disallow DB queries inside Enum loops because this scales linearly with the number of copied screens.
Suggestion: Replace the per-row updates with a bulk update, for example a single PostgreSQL UPDATE revisions ... FROM (VALUES ...) query carrying revision_id, content, children, and activity_refs, and verify the affected row count matches length(update_rows).

Quadratic list accumulation while extracting screen refs

file: lib/oli/authoring/editing/adaptive_duplication.ex
line: 122
Description: refs ++ [screen_ref] copies the accumulated list on every child, making extraction O(n²) for adaptive pages with many deck children.
Suggestion: Prepend refs with [screen_ref | refs] during reduce and Enum.reverse/1 once at the end before returning.

Quadratic list accumulation while deduplicating screen ids

file: lib/oli/authoring/editing/adaptive_duplication.ex
line: 143
Description: ids ++ [activity_id] copies the accumulated id list for every new screen id, making the deduplication path O(n²).
Suggestion: Accumulate with [activity_id | ids], then reverse the final list before returning to preserve order.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 23, 2026

AI Review — ui

No issues found

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 23, 2026

AI Review — elixir

Avoid repeated feature-flag checks per row

file: lib/oli_web/live/curriculum/entries/actions.ex
line: 109
Description: ScopedFeatureFlags.can_access?/3 is called from the row action render path, so large curricula/page tables will evaluate the same project/author flag once per rendered entry. In this codebase, render paths should avoid repeated data/cache lookups when the value is invariant for the list.
Suggestion: Compute adaptive_duplication_enabled once in the parent LiveView/table model for the current project and author, pass that boolean into Actions.render, and have show_duplicate_action?/3 use the precomputed value.

Validate the source revision is an adaptive page

file: lib/oli/authoring/editing/adaptive_duplication.ex
line: 73
Description: duplicate/3 treats any revision whose content looks like an adaptive deck as duplicable, then copies its original resource_type_id and can attach it to a container. A non-page revision with adaptive-shaped content could therefore be duplicated and attached as a curriculum child.
Suggestion: After fetching source_page, require ResourceType.is_adaptive_page(source_page) before extracting screen refs, returning {:error, {:adaptive_duplication, :not_adaptive_page}} otherwise.

Validate screen activity type

file: lib/oli/authoring/editing/adaptive_duplication.ex
line: 160
Description: load_source_screen_revisions/2 accepts any activity revision as an adaptive screen. If malformed adaptive page content references a non-adaptive activity, this path will duplicate it as a screen and rewire it into the adaptive page, which can break adaptive authoring/delivery assumptions.
Suggestion: Replace the generic ResourceType.is_activity/1 check with validation that the revision is an adaptive activity, for example by comparing activity_type_id to the oli_adaptive activity registration id.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 23, 2026

PrivSignal Risk: MEDIUM

Scoring guide

Top 10 contributing items

  1. [R-MEDIUM-BEHAVIORAL-SIGNAL-PERSISTED] sensitivity_changed (medium, flow confidence: n/a) for conversation_message_content (+4 related refs) at lib/oli/authoring/editing/adaptive_duplication.ex:529
  2. [R-MEDIUM-BEHAVIORAL-SIGNAL-PERSISTED] sensitivity_changed (medium, flow confidence: n/a) for dialogue_user_input_content (+4 related refs) at lib/oli/authoring/editing/adaptive_duplication.ex:529
  3. [R-MEDIUM-BEHAVIORAL-SIGNAL-PERSISTED] sensitivity_changed (medium, flow confidence: n/a) for resource_attempt_content (+4 related refs) at lib/oli/authoring/editing/adaptive_duplication.ex:529
  4. [R-MEDIUM-BEHAVIORAL-SIGNAL-PERSISTED] sensitivity_changed (medium, flow confidence: n/a) for activity_attempt_save_file_content (+4 related refs) at lib/oli/authoring/editing/adaptive_duplication.ex:529
  5. [R-MEDIUM-INTERNAL-FLOW-ADDED] edge_added (medium, flow confidence: 0.7) for conversation_message_content (+4 related refs) at lib/oli/authoring/editing/adaptive_duplication.ex:529
  6. [R-MEDIUM-INTERNAL-FLOW-ADDED] edge_added (medium, flow confidence: 0.7) for activity_attempt_save_file_content (+4 related refs) at lib/oli/authoring/editing/adaptive_duplication.ex:529
  7. [R-MEDIUM-INTERNAL-FLOW-ADDED] edge_added (medium, flow confidence: 0.7) for resource_attempt_content (+4 related refs) at lib/oli/authoring/editing/adaptive_duplication.ex:529
  8. [R-MEDIUM-INTERNAL-FLOW-ADDED] edge_added (medium, flow confidence: 0.7) for dialogue_user_input_content (+4 related refs) at lib/oli/authoring/editing/adaptive_duplication.ex:529

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2e223b7839

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +472 to +473
true ->
new_page_attrs =
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Fail closed when adaptive payload cannot be parsed

This fallback sends any adaptive page that fails extract_adaptive_screen_refs/1 through the generic duplication path instead of returning an adaptive-duplication error. In those cases (for example malformed or non-canonical adaptive structures), we skip the adaptive remapping logic and can create copies whose internal adaptive references still point at original screens, which violates the fail-closed requirement and can leave broken duplicates behind.

Useful? React with 👍 / 👎.

Comment on lines +24 to +25
ON CONFLICT (feature_name, scope_type, scope_id)
DO UPDATE SET
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid NULL-based ON CONFLICT for global rollout seed

This upsert relies on ON CONFLICT (feature_name, scope_type, scope_id) even though scope_id is NULL for global rows; in Postgres, NULL values do not conflict under a normal unique index, so an existing global rollout row can be duplicated instead of updated. That can later break rollout reads (which use single-row lookups) with multiple-results errors for adaptive_duplication in long-lived environments where the row already exists.

Useful? React with 👍 / 👎.

@darrensiegel
Copy link
Copy Markdown
Contributor Author

Addressed review comments

@rgachuhi rgachuhi merged commit cecd8bd into master Apr 28, 2026
13 checks passed
@rgachuhi rgachuhi deleted the adaptive-page-duplication branch April 28, 2026 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants