Skip to content

OMR Projection Now Order-Proof: Map by question_set_id, Not Position (Closes #148) - #150

Open
Kanika0306 wants to merge 2 commits into
avantifellows:mainfrom
Kanika0306:fix/omr-option-projection-qsetid
Open

OMR Projection Now Order-Proof: Map by question_set_id, Not Position (Closes #148)#150
Kanika0306 wants to merge 2 commits into
avantifellows:mainfrom
Kanika0306:fix/omr-option-projection-qsetid

Conversation

@Kanika0306

Copy link
Copy Markdown

PR Description

Problem

OMR option projection relied on positional alignment between quiz question sets and aggregated results.
When ordering differed, mapping became incorrect and could produce wrong option placeholders.

What this PR changes

  1. Replaced positional/index-based mapping with deterministic mapping by question_set_id.
  2. Added strict duplicate-ID protection to prevent silent overwrites.
  3. Added safe handling for missing/partial aggregated results to avoid crashes.
  4. Preserved existing API shape and behavior while fixing correctness.
  5. Added targeted tests for order independence and duplicate-ID detection.

Files Changed

  1. quizzes.py
  2. test_quizzes.py

Validation

  1. Targeted tests pass (test_quizzes).
  2. Full suite passes locally: 61 passed.
  3. Local endpoints healthy:

Issue

Fixes #148.

Copilot AI review requested due to automatic review settings April 18, 2026 14:36

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes incorrect OMR option placeholder projection when question_sets order differs from aggregation output by switching from index-based alignment to deterministic mapping via question_set_id (closes #148).

Changes:

  • Added project_omr_option_counts() helper to project OMR option placeholders by question_set_id (order-independent).
  • Updated the aggregation pipeline output to include question_set_id and wired the helper into GET /quiz/{quiz_id} OMR flow.
  • Added tests for order independence and duplicate question_set_id detection.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
app/routers/quizzes.py Introduces deterministic question_set_id mapping for OMR option projection and updates aggregation projection accordingly.
app/tests/test_quizzes.py Adds targeted regression tests for reordered aggregation output and duplicate-ID protection.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/routers/quizzes.py
Comment thread app/routers/quizzes.py Outdated
Comment thread app/tests/test_quizzes.py
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Kanika0306

Copy link
Copy Markdown
Author

@copilot apply changes based on the comments in this thread

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect OMR Option Mapping Due to Index-Based Alignment

2 participants