[FEATURE] [MER-4082] Adaptive page duplication#6491
Conversation
AI Review — securityMissing edit authorization in adaptive duplication entry pointfile: lib/oli/authoring/editing/adaptive_duplication.ex |
AI Review — performancePer-row feature flag checks during renderfile: lib/oli_web/live/curriculum/entries/actions.ex DB updates executed in a loopfile: lib/oli/authoring/editing/adaptive_duplication.ex Quadratic list accumulation while extracting screen refsfile: lib/oli/authoring/editing/adaptive_duplication.ex Quadratic list accumulation while deduplicating screen idsfile: lib/oli/authoring/editing/adaptive_duplication.ex |
AI Review — uiNo issues found |
AI Review — elixirAvoid repeated feature-flag checks per rowfile: lib/oli_web/live/curriculum/entries/actions.ex Validate the source revision is an adaptive pagefile: lib/oli/authoring/editing/adaptive_duplication.ex Validate screen activity typefile: lib/oli/authoring/editing/adaptive_duplication.ex |
PrivSignal Risk:
|
There was a problem hiding this comment.
💡 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".
| true -> | ||
| new_page_attrs = |
There was a problem hiding this comment.
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 👍 / 👎.
| ON CONFLICT (feature_name, scope_type, scope_id) | ||
| DO UPDATE SET |
There was a problem hiding this comment.
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 👍 / 👎.
|
Addressed review comments |
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.