Skip to content

Conversation

@sayomaki
Copy link
Contributor

No description provided.

@sayomaki sayomaki requested a review from RichDom2185 November 11, 2025 11:28
@sayomaki sayomaki self-assigned this Nov 11, 2025
Comment on lines 2998 to 3006

Repo.insert(
answer_changeset,
on_conflict: [
set: [answer: get_change(answer_changeset, :answer), last_modified_at: Timex.now()]
],
conflict_target: [:submission_id, :question_id]
)
end
Copy link

Choose a reason for hiding this comment

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

Bug: Removing on_conflict from Repo.insert with conflict_target causes Ecto.ConstraintError on update attempts.
Severity: CRITICAL | Confidence: 1.00

🔍 Detailed Analysis

The code change removes the on_conflict clause from Repo.insert while retaining conflict_target: [:submission_id, :question_id]. According to Ecto's behavior, this defaults to :raise on unique constraint violation. When a student attempts to update an existing answer, the unique constraint on (submission_id, question_id) will be violated, raising an Ecto.ConstraintError and crashing the assessment-saving endpoint.

💡 Suggested Fix

Restore the on_conflict clause with a set action to correctly handle updates for existing answers, ensuring the insert_or_update_answer function behaves as intended.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: lib/cadet/assessments/assessments.ex#L2998-L3003

Potential issue: The code change removes the `on_conflict` clause from `Repo.insert`
while retaining `conflict_target: [:submission_id, :question_id]`. According to Ecto's
behavior, this defaults to `:raise` on unique constraint violation. When a student
attempts to update an existing answer, the unique constraint on `(submission_id,
question_id)` will be violated, raising an `Ecto.ConstraintError` and crashing the
assessment-saving endpoint.

Did we get this right? 👍 / 👎 to inform future reviews.

@sayomaki sayomaki force-pushed the logging-answer-deploy branch from e7b8dc6 to 621cc42 Compare November 11, 2025 11:34
@RichDom2185 RichDom2185 changed the title Include submission logging as well as remove conflict handling Include submission logging Nov 11, 2025
@RichDom2185 RichDom2185 merged commit 973b9ca into deploy Nov 11, 2025
1 of 3 checks passed
@RichDom2185 RichDom2185 deleted the logging-answer-deploy branch November 11, 2025 11:51
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.

3 participants