fix: refuse review writes while a test attempt is open

WR-02: Confirm and SetScore answer ErrAttemptOpen (409 attempt_open,
an admin refusal) for an answer of a running test, ConfirmBulk skips
such attempts, and the default open queue leaves them out, so
AnswerReviewed never fires before the test ends.
This commit is contained in:
Jakub Zych
2026-11-09 22:47:47 +01:00
parent adc02d7e0c
commit 34b0966819
6 changed files with 37 additions and 9 deletions

View File

@@ -29,7 +29,8 @@ import (
// Review status filters besides the grading statuses themselves.
const (
// ReviewStatusOpen is every answer the owner has not reviewed yet:
// pending, graded and failed. It is the default filter.
// pending, graded and failed, except those of a test attempt that is
// still running. It is the default filter.
ReviewStatusOpen = "open"
// ReviewStatusAll lists every open answer, reviewed ones included.
ReviewStatusAll = "all"
@@ -52,6 +53,10 @@ var (
// ErrGraderManual is a regrade on a quiz whose grader is manual or not
// registered by any activated plugin.
ErrGraderManual = errors.New("quizzes: the quiz has no grader that can grade again")
// ErrAttemptOpen is a review write on an answer of a test attempt that
// is still running: a test's results, the owner's review included, come
// only after it ends (Phase 7 D-07, D-08).
ErrAttemptOpen = errors.New("quizzes: the test attempt is still open")
)
// ReviewFilter narrows the review queue. Zero values do not filter; Status
@@ -287,6 +292,8 @@ func (r reviewScan) detail() *ReviewDetail {
}
// reviewWhere turns a filter into predicates appended to reviewFromSQL.
// The default open filter leaves out the answers of a test attempt that is
// still running: they cannot be reviewed until it ends (ErrAttemptOpen).
func reviewWhere(f ReviewFilter, statuses []string) (string, []any) {
var b strings.Builder
var args []any
@@ -294,6 +301,10 @@ func reviewWhere(f ReviewFilter, statuses []string) (string, []any) {
b.WriteString(" AND aa.grading_status IN ?")
args = append(args, statuses)
}
if status := strings.TrimSpace(f.Status); status == "" || status == ReviewStatusOpen {
b.WriteString(" AND NOT (qz.mode = ? AND a.completed_at IS NULL)")
args = append(args, models.ModeTest)
}
if slug := strings.TrimSpace(f.QuizSlug); slug != "" {
b.WriteString(" AND qz.slug = ?")
args = append(args, slug)
@@ -369,7 +380,8 @@ type reviewTarget struct {
// lockReviewTarget locks the attempt of an open answer and reads the
// answer again under the lock, so a grading job or a re-answer cannot
// change it in between. A missing answer or attempt, or an answer that is
// not open, is ErrNotFound.
// not open, is ErrNotFound; an answer of a test attempt that is still
// running is ErrAttemptOpen.
func lockReviewTarget(ctx context.Context, tx *gorm.DB, answerID uint) (*reviewTarget, error) {
tx = tx.WithContext(ctx)
var row models.AttemptAnswer
@@ -388,6 +400,9 @@ func lockReviewTarget(ctx context.Context, tx *gorm.DB, answerID uint) (*reviewT
return nil, notFound(err)
}
attempt.Quiz = &quiz
if Withheld(attempt) {
return nil, ErrAttemptOpen
}
var question models.Question
if err := tx.Where("id = ?", row.QuestionID).Take(&question).Error; err != nil {
return nil, notFound(err)
@@ -583,9 +598,10 @@ func (s *Service) SetScore(ctx context.Context, answerID uint, points float64, c
// confirmed. Pending, failed and reviewed answers are left alone. The
// affected attempts are locked in id order, recalculated once each, and
// every changed answer and every completed attempt that is now fully
// reviewed fire their events after the commit. A missing quiz or attempt
// is ErrNotFound; a scope that is not exactly one of the two is a
// *ValidationError.
// reviewed fire their events after the commit. Answers of a test attempt
// that is still running are skipped; a scope of one such attempt is
// ErrAttemptOpen. A missing quiz or attempt is ErrNotFound; a scope that
// is not exactly one of the two is a *ValidationError.
func (s *Service) ConfirmBulk(ctx context.Context, scope ReviewScope, by Reviewer) (int, error) {
if problems := CheckReviewScope(scope, false); len(problems) > 0 {
return 0, &ValidationError{Problems: problems}
@@ -607,6 +623,12 @@ func (s *Service) ConfirmBulk(ctx context.Context, scope ReviewScope, by Reviewe
}
touched := map[uint]bool{}
for _, row := range rows {
if Withheld(attempts[row.AttemptID]) {
if scope.AttemptID != 0 {
return ErrAttemptOpen
}
continue
}
t := &reviewTarget{Answer: row, Question: questions[row.QuestionID], Attempt: attempts[row.AttemptID]}
done, err := s.confirmTarget(ctx, tx, t, by)
if err != nil {