From 692a647650b15d1b6c4298ad3b30b6e1376b6140 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Mon, 9 Nov 2026 18:06:02 +0100 Subject: [PATCH] feat: set scores, confirm in bulk and regrade open answers over MCP - SetScore (0..points in 0.5 steps, comment shown to the student), ConfirmBulk per quiz or attempt and Regrade through the quiz's grader - close() announces an attempt reviewed while it was open - quizzes_review_set_score, quizzes_review_confirm_bulk, quizzes_review_regrade; 409 already_reviewed and grader_manual --- classes/review.go | 385 +++++++++++++++++++++++++++++++- classes/review_test.go | 75 +++++++ classes/service.go | 8 +- controllers/api/reviews.go | 101 +++++++++ controllers/api/reviews_test.go | 54 ++++- mcp_reviews.go | 50 +++++ mcp_test.go | 46 ++++ plugin_test.go | 3 + routes.go | 4 + 9 files changed, 717 insertions(+), 9 deletions(-) diff --git a/classes/review.go b/classes/review.go index 9ea953f..5f3e5ac 100644 --- a/classes/review.go +++ b/classes/review.go @@ -13,9 +13,12 @@ package classes import ( "context" "errors" + "math" "slices" + "strconv" "strings" "time" + "unicode/utf8" "git.golem15.com/golem15/sm-quizzes-plugin/events" "git.golem15.com/golem15/sm-quizzes-plugin/models" @@ -35,9 +38,21 @@ const ( // ReviewExcerptRunes is how much of a student's text a queue row shows. const ReviewExcerptRunes = 300 -// ErrNoGraderScore is a confirm of an answer that has no grader score: it -// is pending or failed, so the owner sets the score instead. -var ErrNoGraderScore = errors.New("quizzes: the answer has no grader score to confirm") +// MaxReviewComment is the longest comment to the student, in runes. +const MaxReviewComment = 2000 + +// Errors of the review actions, answered as 409s with a code. +var ( + // ErrNoGraderScore is a confirm of an answer that has no grader score: + // it is pending or failed, so the owner sets the score instead. + ErrNoGraderScore = errors.New("quizzes: the answer has no grader score to confirm") + // ErrAlreadyReviewed is a regrade of one answer the owner already + // reviewed: owner decisions are final (D-12). + ErrAlreadyReviewed = errors.New("quizzes: the answer is already reviewed") + // 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") +) // ReviewFilter narrows the review queue. Zero values do not filter; Status // is one CheckReviewStatus accepts. @@ -68,6 +83,68 @@ func (r Reviewer) ids() (user, admin *uint) { return user, admin } +// ReviewScope picks the open answers of a bulk action: one answer, one +// attempt or one quiz (by slug). Exactly one is set; see CheckReviewScope. +type ReviewScope struct { + AnswerID uint + AttemptID uint + QuizSlug string +} + +// CheckReviewScope requires exactly one of the scope's keys: answer_id, +// attempt_id or quiz when allowAnswer (regrade), attempt_id or quiz +// otherwise (bulk confirm). +func CheckReviewScope(scope ReviewScope, allowAnswer bool) []string { + n := 0 + if scope.AttemptID != 0 { + n++ + } + if strings.TrimSpace(scope.QuizSlug) != "" { + n++ + } + if scope.AnswerID != 0 { + if !allowAnswer { + return []string{"exactly one of attempt_id and quiz is required"} + } + n++ + } + if n == 1 { + return nil + } + if allowAnswer { + return []string{"exactly one of answer_id, attempt_id and quiz is required"} + } + return []string{"exactly one of attempt_id and quiz is required"} +} + +// CheckReviewPoints reports whether points is a score the owner may give a +// question worth maxPoints: finite, 0..maxPoints (a negative maxPoints +// counts as 0) and a multiple of 0.5. +func CheckReviewPoints(points float64, maxPoints int) bool { + if math.IsNaN(points) || math.IsInf(points, 0) { + return false + } + full := float64(max(maxPoints, 0)) + return points >= 0 && points <= full && points*2 == math.Trunc(points*2) +} + +// CheckReviewComment cleans the owner's comment to the student: trimmed, +// NUL bytes dropped, and nil when nothing is left. More than +// MaxReviewComment runes is a problem. +func CheckReviewComment(comment *string) (*string, []string) { + if comment == nil { + return nil, nil + } + c := strings.TrimSpace(strings.ReplaceAll(*comment, "\x00", "")) + if c == "" { + return nil, nil + } + if utf8.RuneCountInString(c) > MaxReviewComment { + return nil, []string{"comment must be at most " + strconv.Itoa(MaxReviewComment) + " characters"} + } + return &c, nil +} + // unreviewedStatuses are the grading statuses the owner still has to act // on. func unreviewedStatuses() []string { @@ -454,3 +531,305 @@ func (s *Service) fireAttemptReviewedIfDone(ctx context.Context, tx *gorm.DB, at }) return nil } + +// SetScore gives one open answer the owner's final score, on any status +// (D-12): status reviewed, points as given, is_correct for full marks, and +// the comment to the student replacing any earlier one (nil or blank +// clears it). grader_points and grader_feedback stay for the record. +// Points outside 0..question points or not a multiple of 0.5, and a +// comment over MaxReviewComment runes, are a *ValidationError. A +// completed attempt is recalculated; AnswerReviewedEvent, and +// AttemptReviewedEvent when the attempt is fully reviewed, fire after the +// commit. +func (s *Service) SetScore(ctx context.Context, answerID uint, points float64, comment *string, by Reviewer) (*ReviewDetail, error) { + comment, problems := CheckReviewComment(comment) + if len(problems) > 0 { + return nil, &ValidationError{Problems: problems} + } + var detail *ReviewDetail + err := lagoon.Transaction(ctx, s.DB, func(ctx context.Context, tx *gorm.DB) error { + t, err := lockReviewTarget(ctx, tx, answerID) + if err != nil { + return err + } + if !CheckReviewPoints(points, t.Question.Points) { + return &ValidationError{Problems: []string{ + "points must be between 0 and " + strconv.Itoa(max(t.Question.Points, 0)) + " in steps of 0.5", + }} + } + if points == 0 { + points = 0 // no negative zero in JSON + } + if err := s.storeReview(ctx, tx, t, points, comment, true, by); err != nil { + return err + } + if err := s.recalcAttempt(ctx, tx, t.Attempt.ID); err != nil { + return err + } + if err := s.fireAttemptReviewedIfDone(ctx, tx, t.Attempt.ID); err != nil { + return err + } + detail, err = reviewItem(ctx, tx, answerID) + return err + }) + if err != nil { + return nil, err + } + return detail, nil +} + +// ConfirmBulk confirms every graded answer of one quiz (QuizSlug) or one +// attempt (AttemptID), exactly as Confirm does, and returns how many it +// 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. +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} + } + confirmed := 0 + err := lagoon.Transaction(ctx, s.DB, func(ctx context.Context, tx *gorm.DB) error { + confirmed = 0 + quiz, err := scopeQuiz(ctx, tx, scope) + if err != nil { + return err + } + rows, attempts, err := lockScope(ctx, tx, scope, quiz, []string{models.GradingGraded}) + if err != nil { + return err + } + questions, err := scopeQuestions(ctx, tx, rows) + if err != nil { + return err + } + touched := map[uint]bool{} + for _, row := range rows { + t := &reviewTarget{Answer: row, Question: questions[row.QuestionID], Attempt: attempts[row.AttemptID]} + done, err := s.confirmTarget(ctx, tx, t, by) + if err != nil { + return err + } + if done { + confirmed++ + touched[row.AttemptID] = true + } + } + for _, id := range sortedIDs(touched) { + if err := s.recalcAttempt(ctx, tx, id); err != nil { + return err + } + if err := s.fireAttemptReviewedIfDone(ctx, tx, id); err != nil { + return err + } + } + return nil + }) + if err != nil { + return 0, err + } + return confirmed, nil +} + +// Regrade sends open answers back to the quiz's grader, e.g. after a +// rubric change (D-12): every pending, graded or failed answer of one +// answer, attempt or quiz goes back to pending with the next grading_seq +// (so a job for the old text is ignored), its grade, error and points +// cleared, and is enqueued again. Answers of a test attempt that is still +// open are not enqueued: closing the attempt enqueues them (UD-2). +// Reviewed answers are skipped, as owner decisions are final; a single +// reviewed answer is ErrAlreadyReviewed. A quiz whose grader is manual or +// not registered is ErrGraderManual. Completed attempts are recalculated. +// It returns how many answers were reset. +func (s *Service) Regrade(ctx context.Context, scope ReviewScope) (int, error) { + if problems := CheckReviewScope(scope, true); len(problems) > 0 { + return 0, &ValidationError{Problems: problems} + } + regraded := 0 + err := lagoon.Transaction(ctx, s.DB, func(ctx context.Context, tx *gorm.DB) error { + regraded = 0 + tx = tx.WithContext(ctx) + quiz, err := scopeQuiz(ctx, tx, scope) + if err != nil { + return err + } + if _, ok := GraderFor(s.Graders, quiz); !ok { + return ErrGraderManual + } + if scope.AnswerID != 0 { + if err := refuseReviewed(ctx, tx, scope.AnswerID); err != nil { + return err + } + } + rows, attempts, err := lockScope(ctx, tx, scope, quiz, unreviewedStatuses()) + if err != nil { + return err + } + if scope.AnswerID != 0 && len(rows) == 0 { + // Reviewed between the first read and the lock. + return refuseReviewed(ctx, tx, scope.AnswerID) + } + now := s.now().UTC() + seqs := make(map[uint]int, len(rows)) + for _, row := range rows { + seq := row.GradingSeq + 1 + if err := tx.Model(&models.AttemptAnswer{}).Where("id = ?", row.ID).Updates(map[string]any{ + "grading_status": models.GradingPending, + "grading_seq": seq, + "grading_attempts": 0, + "grader": nil, + "grader_points": nil, + "grader_feedback": nil, + "grading_error": nil, + "graded_at": nil, + "points": 0, + "is_correct": false, + "updated_at": now, + }).Error; err != nil { + return err + } + seqs[row.ID] = seq + regraded++ + } + touched := map[uint]bool{} + for _, row := range rows { + touched[row.AttemptID] = true + } + for _, id := range sortedIDs(touched) { + if err := s.recalcAttempt(ctx, tx, id); err != nil { + return err + } + } + for _, row := range rows { + a := attempts[row.AttemptID] + if a.CompletedAt == nil && IsTest(quiz) { + continue + } + if err := s.enqueueGrading(ctx, tx, row.ID, seqs[row.ID], a); err != nil { + return err + } + } + return nil + }) + if err != nil { + return 0, err + } + return regraded, nil +} + +// refuseReviewed is ErrAlreadyReviewed for a reviewed answer and +// ErrNotFound for one that is missing or not open. +func refuseReviewed(ctx context.Context, tx *gorm.DB, answerID uint) error { + var row models.AttemptAnswer + if err := tx.WithContext(ctx).Where("id = ? AND grading_status IS NOT NULL", answerID).Take(&row).Error; err != nil { + return notFound(err) + } + if *row.GradingStatus == models.GradingReviewed { + return ErrAlreadyReviewed + } + return nil +} + +// scopeQuiz resolves the quiz of a review scope: by slug, from the +// attempt, or from the open answer's attempt. Anything missing is +// ErrNotFound. +func scopeQuiz(ctx context.Context, tx *gorm.DB, scope ReviewScope) (*models.Quiz, error) { + tx = tx.WithContext(ctx) + var quiz models.Quiz + var err error + switch { + case scope.AnswerID != 0: + err = tx.Where(`id = (SELECT a.quiz_id FROM golem15_quizzes_attempt_answers aa +JOIN golem15_quizzes_attempts a ON a.id = aa.attempt_id +WHERE aa.id = ? AND aa.grading_status IS NOT NULL)`, scope.AnswerID).Take(&quiz).Error + case scope.AttemptID != 0: + err = tx.Where("id = (SELECT quiz_id FROM golem15_quizzes_attempts WHERE id = ?)", scope.AttemptID).Take(&quiz).Error + default: + err = tx.Where("slug = ?", strings.TrimSpace(scope.QuizSlug)).Take(&quiz).Error + } + if err != nil { + return nil, notFound(err) + } + return &quiz, nil +} + +// scopeRows narrows a query on the attempt answers to a scope of quiz. +func scopeRows(q *gorm.DB, scope ReviewScope, quiz *models.Quiz) *gorm.DB { + switch { + case scope.AnswerID != 0: + return q.Where("id = ?", scope.AnswerID) + case scope.AttemptID != 0: + return q.Where("attempt_id = ?", scope.AttemptID) + } + return q.Where("attempt_id IN (SELECT id FROM golem15_quizzes_attempts WHERE quiz_id = ?)", quiz.ID) +} + +// lockScope locks, in id order, the attempts that have open answers with +// one of statuses in scope, then selects those answers again under the +// locks (only of the locked attempts, so nothing changes underneath). It +// returns the answers by attempt and id, and the locked attempts by id +// with their quiz set. +func lockScope(ctx context.Context, tx *gorm.DB, scope ReviewScope, quiz *models.Quiz, statuses []string) ([]models.AttemptAnswer, map[uint]*models.Attempt, error) { + tx = tx.WithContext(ctx) + var ids []uint + if err := scopeRows(tx.Model(&models.AttemptAnswer{}), scope, quiz). + Where("grading_status IN ?", statuses).Distinct("attempt_id").Order("attempt_id ASC"). + Pluck("attempt_id", &ids).Error; err != nil { + return nil, nil, err + } + attempts := make(map[uint]*models.Attempt, len(ids)) + locked := make([]uint, 0, len(ids)) + for _, id := range ids { + a, err := lockAttempt(ctx, tx, id) + if errors.Is(err, gorm.ErrRecordNotFound) { + continue + } + if err != nil { + return nil, nil, err + } + a.Quiz = quiz + attempts[id] = a + locked = append(locked, id) + } + if len(locked) == 0 { + return nil, attempts, nil + } + var rows []models.AttemptAnswer + if err := scopeRows(tx, scope, quiz).Where("grading_status IN ? AND attempt_id IN ?", statuses, locked). + Order("attempt_id ASC, id ASC").Find(&rows).Error; err != nil { + return nil, nil, err + } + return rows, attempts, nil +} + +// scopeQuestions loads the questions of the given answers by id. +func scopeQuestions(ctx context.Context, tx *gorm.DB, rows []models.AttemptAnswer) (map[uint]models.Question, error) { + out := map[uint]models.Question{} + if len(rows) == 0 { + return out, nil + } + ids := make([]uint, 0, len(rows)) + for _, r := range rows { + ids = append(ids, r.QuestionID) + } + var questions []models.Question + if err := tx.WithContext(ctx).Where("id IN ?", ids).Find(&questions).Error; err != nil { + return nil, err + } + for _, q := range questions { + out[q.ID] = q + } + return out, nil +} + +// sortedIDs returns the keys of set in ascending order. +func sortedIDs(set map[uint]bool) []uint { + out := make([]uint, 0, len(set)) + for id := range set { + out = append(out, id) + } + slices.Sort(out) + return out +} diff --git a/classes/review_test.go b/classes/review_test.go index 57f2b54..2ef3f7f 100644 --- a/classes/review_test.go +++ b/classes/review_test.go @@ -1,6 +1,7 @@ package classes_test import ( + "math" "slices" "strings" "testing" @@ -38,3 +39,77 @@ func TestCheckReviewStatus(t *testing.T) { } } } + +// TestCheckReviewPoints: 0..points in steps of 0.5, finite. +func TestCheckReviewPoints(t *testing.T) { + for _, tc := range []struct { + p float64 + max int + want bool + }{ + {0, 2, true}, {1.5, 2, true}, {2, 2, true}, {0, 0, true}, {0.5, 1, true}, + {-0.5, 2, false}, {2.5, 2, false}, {1.25, 2, false}, {0.1, 2, false}, + {math.NaN(), 2, false}, {math.Inf(1), 2, false}, {math.Inf(-1), 2, false}, + {0.5, 0, false}, {0, -1, true}, {1, -1, false}, + } { + if got := classes.CheckReviewPoints(tc.p, tc.max); got != tc.want { + t.Errorf("CheckReviewPoints(%v, %d) = %v, want %v", tc.p, tc.max, got, tc.want) + } + } +} + +// TestCheckReviewComment: trimmed, blank is no comment, NUL dropped, +// at most 2000 runes. +func TestCheckReviewComment(t *testing.T) { + str := func(s string) *string { return &s } + if got, p := classes.CheckReviewComment(nil); got != nil || p != nil { + t.Errorf("nil = %v %v", got, p) + } + if got, p := classes.CheckReviewComment(str(" \n ")); got != nil || p != nil { + t.Errorf("spaces = %v %v, want no comment", got, p) + } + if got, p := classes.CheckReviewComment(str(" Dobrze, ale\x00 brak przykładu. ")); got == nil || *got != "Dobrze, ale brak przykładu." || p != nil { + t.Errorf("trimmed = %v %v", got, p) + } + max := strings.Repeat("ł", classes.MaxReviewComment) + if got, p := classes.CheckReviewComment(&max); got == nil || *got != max || p != nil { + t.Errorf("2000 runes refused: %v", p) + } + over := max + "x" + if got, p := classes.CheckReviewComment(&over); got != nil || len(p) != 1 || p[0] != "comment must be at most 2000 characters" { + t.Errorf("2001 runes = %v %v, want one problem", got, p) + } +} + +// TestCheckReviewScope: exactly one key; answer_id only for regrade. +func TestCheckReviewScope(t *testing.T) { + regrade := "exactly one of answer_id, attempt_id and quiz is required" + bulk := "exactly one of attempt_id and quiz is required" + for name, tc := range map[string]struct { + scope classes.ReviewScope + allow bool + want string + }{ + "regrade answer": {classes.ReviewScope{AnswerID: 1}, true, ""}, + "regrade attempt": {classes.ReviewScope{AttemptID: 1}, true, ""}, + "regrade quiz": {classes.ReviewScope{QuizSlug: "q"}, true, ""}, + "regrade none": {classes.ReviewScope{}, true, regrade}, + "regrade blank quiz": {classes.ReviewScope{QuizSlug: " "}, true, regrade}, + "regrade two": {classes.ReviewScope{AnswerID: 1, QuizSlug: "q"}, true, regrade}, + "regrade three": {classes.ReviewScope{AnswerID: 1, AttemptID: 2, QuizSlug: "q"}, true, regrade}, + "bulk attempt": {classes.ReviewScope{AttemptID: 1}, false, ""}, + "bulk quiz": {classes.ReviewScope{QuizSlug: "q"}, false, ""}, + "bulk none": {classes.ReviewScope{}, false, bulk}, + "bulk two": {classes.ReviewScope{AttemptID: 1, QuizSlug: "q"}, false, bulk}, + "bulk answer": {classes.ReviewScope{AnswerID: 1}, false, bulk}, + "bulk answer and quiz": {classes.ReviewScope{AnswerID: 1, QuizSlug: "q"}, false, bulk}, + } { + got := classes.CheckReviewScope(tc.scope, tc.allow) + switch { + case tc.want == "" && got != nil: + t.Errorf("%s = %v, want no problem", name, got) + case tc.want != "" && (len(got) != 1 || got[0] != tc.want): + t.Errorf("%s = %v, want %q", name, got, tc.want) + } + } +} diff --git a/classes/service.go b/classes/service.go index a21d7e7..670b192 100644 --- a/classes/service.go +++ b/classes/service.go @@ -662,7 +662,8 @@ func (s *Service) Finish(ctx context.Context, userID, attemptID uint) (*AttemptR // answers, floors the total at 0 and stores the score. Finish closes at the // current time, an expired test closes at its deadline. A test then // enqueues one grading job per pending open answer (UD-2); every caller -// sets attempt.Quiz. +// sets attempt.Quiz. An attempt whose open answers were all reviewed +// while it was open is announced as reviewed once it closes. func (s *Service) close(ctx context.Context, tx *gorm.DB, attempt *models.Attempt, at time.Time) error { tx = tx.WithContext(ctx) var points []float64 @@ -685,7 +686,10 @@ func (s *Service) close(ctx context.Context, tx *gorm.DB, attempt *models.Attemp }).Error; err != nil { return err } - return s.enqueuePending(ctx, tx, attempt) + if err := s.enqueuePending(ctx, tx, attempt); err != nil { + return err + } + return s.fireAttemptReviewedIfDone(ctx, tx, attempt.ID) } // AttemptSummary is an attempt with its quiz and answered question count. diff --git a/controllers/api/reviews.go b/controllers/api/reviews.go index 4748102..7a158bf 100644 --- a/controllers/api/reviews.go +++ b/controllers/api/reviews.go @@ -29,6 +29,10 @@ func writeReview(w http.ResponseWriter, status int, data any, err error) { switch { case errors.Is(err, classes.ErrNoGraderScore): reviewConflict(w, "The answer has no grader score to confirm; set the score instead.", "no_grader_score") + case errors.Is(err, classes.ErrAlreadyReviewed): + reviewConflict(w, "The answer is already reviewed; set the score instead.", "already_reviewed") + case errors.Is(err, classes.ErrGraderManual): + reviewConflict(w, "The quiz's grader is manual or not available, so nothing can be graded again.", "grader_manual") case errors.Is(err, classes.ErrNotFound): wire.WriteJSON(w, http.StatusNotFound, map[string]any{"error": true, "message": "Open answer, attempt or quiz not found."}) default: @@ -143,3 +147,100 @@ func answerReview(w http.ResponseWriter, d *classes.ReviewDetail, err error) { } writeReview(w, http.StatusOK, reviewDetailJSON(*d), nil) } + +// setScoreBody is the POST /{id}/score body. +type setScoreBody struct { + Points *float64 `json:"points"` + Comment *string `json:"comment"` +} + +// ReviewSetScore answers POST /api/v1/quiz-reviews/{id}/score with +// {points, comment}: the owner's final score on any status, with an +// optional comment the student sees. Points outside 0..question points or +// off the 0.5 steps, and a comment over 2000 characters, are a 422. +func (h *Handlers) ReviewSetScore(w http.ResponseWriter, r *http.Request) { + id, ok := reviewID(w, r) + if !ok { + return + } + var body setScoreBody + if !readManaged(w, r, &body) { + return + } + comment, problems := classes.CheckReviewComment(body.Comment) + if body.Points == nil { + problems = append([]string{"points is required"}, problems...) + } + if refuse(w, problems) { + return + } + by, ok := h.reviewer(w, r) + if !ok { + return + } + svc, ok := h.service(w) + if !ok { + return + } + d, err := svc.SetScore(r.Context(), id, *body.Points, comment, by) + answerReview(w, d, err) +} + +// confirmBulkBody is the POST /confirm body. +type confirmBulkBody struct { + Quiz string `json:"quiz"` + AttemptID uint `json:"attempt_id"` +} + +// ReviewConfirmBulk answers POST /api/v1/quiz-reviews/confirm with +// exactly one of {quiz} and {attempt_id}: every graded answer in scope is +// confirmed; it answers {confirmed: n}. +func (h *Handlers) ReviewConfirmBulk(w http.ResponseWriter, r *http.Request) { + var body confirmBulkBody + if !readManaged(w, r, &body) { + return + } + scope := classes.ReviewScope{AttemptID: body.AttemptID, QuizSlug: strings.TrimSpace(body.Quiz)} + if refuse(w, classes.CheckReviewScope(scope, false)) { + return + } + by, ok := h.reviewer(w, r) + if !ok { + return + } + svc, ok := h.service(w) + if !ok { + return + } + n, err := svc.ConfirmBulk(r.Context(), scope, by) + writeReview(w, http.StatusOK, map[string]any{"confirmed": n}, err) +} + +// regradeBody is the POST /regrade body. +type regradeBody struct { + AnswerID uint `json:"answer_id"` + AttemptID uint `json:"attempt_id"` + Quiz string `json:"quiz"` +} + +// ReviewRegrade answers POST /api/v1/quiz-reviews/regrade with exactly one +// of {answer_id}, {attempt_id} and {quiz}: the unreviewed open answers in +// scope go back to the quiz's grader; it answers {regraded: n}. 409 +// grader_manual when the quiz has no grader, already_reviewed for a single +// reviewed answer. +func (h *Handlers) ReviewRegrade(w http.ResponseWriter, r *http.Request) { + var body regradeBody + if !readManaged(w, r, &body) { + return + } + scope := classes.ReviewScope{AnswerID: body.AnswerID, AttemptID: body.AttemptID, QuizSlug: strings.TrimSpace(body.Quiz)} + if refuse(w, classes.CheckReviewScope(scope, true)) { + return + } + svc, ok := h.service(w) + if !ok { + return + } + n, err := svc.Regrade(r.Context(), scope) + writeReview(w, http.StatusOK, map[string]any{"regraded": n}, err) +} diff --git a/controllers/api/reviews_test.go b/controllers/api/reviews_test.go index cc9312a..48346f3 100644 --- a/controllers/api/reviews_test.go +++ b/controllers/api/reviews_test.go @@ -107,10 +107,12 @@ func TestReviewErrorMapping(t *testing.T) { code int key string }{ - "no grader score": {classes.ErrNoGraderScore, http.StatusConflict, "no_grader_score"}, - "not found": {classes.ErrNotFound, http.StatusNotFound, ""}, - "validation": {&classes.ValidationError{Problems: []string{"status must be one of"}}, http.StatusUnprocessableEntity, ""}, - "other": {errors.New("boom"), http.StatusInternalServerError, ""}, + "no grader score": {classes.ErrNoGraderScore, http.StatusConflict, "no_grader_score"}, + "already reviewed": {classes.ErrAlreadyReviewed, http.StatusConflict, "already_reviewed"}, + "grader manual": {classes.ErrGraderManual, http.StatusConflict, "grader_manual"}, + "not found": {classes.ErrNotFound, http.StatusNotFound, ""}, + "validation": {&classes.ValidationError{Problems: []string{"status must be one of"}}, http.StatusUnprocessableEntity, ""}, + "other": {errors.New("boom"), http.StatusInternalServerError, ""}, } { rec := httptest.NewRecorder() writeReview(rec, 0, nil, tc.err) @@ -167,3 +169,47 @@ func TestReviewHandlersBeforeDatabase(t *testing.T) { t.Errorf("confirm without a user = %d, want 401", code) } } + +// TestReviewWritesBeforeDatabase: the bodies of set_score, confirm_bulk +// and regrade are checked before the database: unknown keys, a missing +// points, a long comment and a scope that is not exactly one key are 422s. +func TestReviewWritesBeforeDatabase(t *testing.T) { + h := New(backpack.New(nil)) + long := strings.Repeat("x", 2001) + id := map[string]string{"id": "3"} + for name, tc := range map[string]struct { + fn http.HandlerFunc + body string + want string + }{ + "score empty body": {h.ReviewSetScore, "", "invalid JSON"}, + "score unknown key": {h.ReviewSetScore, `{"points":1,"status":"reviewed"}`, "invalid JSON"}, + "score without points": {h.ReviewSetScore, `{"comment":"ok"}`, "points is required"}, + "score long comment": {h.ReviewSetScore, `{"points":1,"comment":"` + long + `"}`, "comment must be at most 2000 characters"}, + "bulk none": {h.ReviewConfirmBulk, `{}`, "exactly one of attempt_id and quiz is required"}, + "bulk two": {h.ReviewConfirmBulk, `{"quiz":"q","attempt_id":4}`, "exactly one of attempt_id and quiz is required"}, + "bulk answer_id": {h.ReviewConfirmBulk, `{"answer_id":4}`, "invalid JSON"}, + "regrade none": {h.ReviewRegrade, `{}`, "exactly one of answer_id, attempt_id and quiz is required"}, + "regrade two": {h.ReviewRegrade, `{"answer_id":3,"quiz":"q"}`, "exactly one of answer_id, attempt_id and quiz is required"}, + } { + code, out := call(t, tc.fn, "POST", "/", tc.body, 7, id) + if msg, _ := out["message"].(string); code != http.StatusUnprocessableEntity || !strings.Contains(msg, tc.want) { + t.Errorf("%s = %d %v, want 422 with %q", name, code, out, tc.want) + } + } + for name, tc := range map[string]struct { + fn http.HandlerFunc + body string + }{ + "score": {h.ReviewSetScore, `{"points":1.5,"comment":null}`}, + "bulk": {h.ReviewConfirmBulk, `{"quiz":"q"}`}, + "regrade": {h.ReviewRegrade, `{"attempt_id":4}`}, + } { + if code, _ := call(t, tc.fn, "POST", "/", tc.body, 7, id); code != http.StatusServiceUnavailable { + t.Errorf("%s without a database = %d, want 503", name, code) + } + } + if code, _ := call(t, h.ReviewSetScore, "POST", "/", `{"points":1}`, 7, map[string]string{"id": "x"}); code != http.StatusNotFound { + t.Errorf("score with a bad id = %d, want 404", code) + } +} diff --git a/mcp_reviews.go b/mcp_reviews.go index ebbe5a1..4e4a413 100644 --- a/mcp_reviews.go +++ b/mcp_reviews.go @@ -21,6 +21,26 @@ type reviewAnswerInput struct { ID uint `json:"id" path:"id" jsonschema:"the answer_id, as listed by quizzes_review_list"` } +// reviewSetScoreInput is the input of quizzes_review_set_score. +type reviewSetScoreInput struct { + ID uint `json:"id" path:"id" jsonschema:"the answer_id, as listed by quizzes_review_list"` + Points float64 `json:"points" jsonschema:"0 up to the question's points, in steps of 0.5"` + Comment *string `json:"comment,omitempty" jsonschema:"optional comment the student sees, at most 2000 characters; leaving it out clears an earlier comment"` +} + +// reviewConfirmBulkInput is the input of quizzes_review_confirm_bulk. +type reviewConfirmBulkInput struct { + Quiz string `json:"quiz,omitempty" jsonschema:"confirm the graded answers of this quiz (its slug); give either quiz or attempt_id"` + AttemptID uint `json:"attempt_id,omitempty" jsonschema:"confirm the graded answers of this attempt; give either quiz or attempt_id"` +} + +// reviewRegradeInput is the input of quizzes_review_regrade. +type reviewRegradeInput struct { + AnswerID uint `json:"answer_id,omitempty" jsonschema:"grade this one answer again; give exactly one of answer_id, attempt_id and quiz"` + AttemptID uint `json:"attempt_id,omitempty" jsonschema:"grade the unreviewed answers of this attempt again"` + Quiz string `json:"quiz,omitempty" jsonschema:"grade the unreviewed answers of this quiz (its slug) again"` +} + // reviewTools are the owner's tools over the review queue of open answers // (D-11). They bind the routes under ReviewPrefix, which check the same // permission as the quiz tools (D-13). @@ -54,5 +74,35 @@ func reviewTools() []pact.MCPTool { Permissions: quizPermissions, Idempotent: &yes, }, + { + Name: "review_set_score", + Title: "Set the score of an open answer", + Description: "Sets your own final score on one open answer, whatever its status (pending, graded, failed or reviewed): points from 0 up to the question's points in steps of 0.5 (anything else is refused with 422), and an optional comment of at most 2000 characters that the student sees with the answer; leaving the comment out clears an earlier one. status becomes reviewed and the score is final over any later grading; the grader's score and feedback stay for the record, and a finished attempt's score is recalculated. Returns the answer as quizzes_review_show does." + needsWrite, + Method: http.MethodPost, + Path: ReviewPrefix + "/{id}/score", + Input: reviewSetScoreInput{}, + Permissions: quizPermissions, + Idempotent: &yes, + }, + { + Name: "review_confirm_bulk", + Title: "Confirm grader scores in bulk", + Description: "Confirms the grader's score of every graded answer of one quiz (quiz, its slug) or of one attempt (attempt_id); give exactly one of the two. Each becomes reviewed with points = grader_points, as quizzes_review_confirm does; pending, failed and reviewed answers are left alone, so check failed ones with quizzes_review_list first. Finished attempts are recalculated. Returns {confirmed: the number of answers confirmed}; a second call confirms nothing new." + needsWrite, + Method: http.MethodPost, + Path: ReviewPrefix + "/confirm", + Input: reviewConfirmBulkInput{}, + Permissions: quizPermissions, + Idempotent: &yes, + }, + { + Name: "review_regrade", + Title: "Grade open answers again", + Description: "Re-runs the quiz's grader on one answer (answer_id), one attempt (attempt_id) or a whole quiz (quiz, its slug); give exactly one. Use it after changing a rubric or when the grader failed. Every pending, graded or failed answer in scope goes back to pending: the current grader scores, feedback and errors are discarded and the points drop to 0 until the grader scores it again (a finished attempt's score is recalculated). Answers of a test attempt that is still open are graded when it ends. Reviewed answers are skipped, and a single reviewed answer is refused with 409 code already_reviewed. Refused with 409 code grader_manual when the quiz's grader is manual or not available. Returns {regraded: the number of answers sent back}." + needsWrite, + Method: http.MethodPost, + Path: ReviewPrefix + "/regrade", + Input: reviewRegradeInput{}, + Permissions: quizPermissions, + Destructive: &yes, + }, } } diff --git a/mcp_test.go b/mcp_test.go index 31d249e..8762143 100644 --- a/mcp_test.go +++ b/mcp_test.go @@ -85,7 +85,10 @@ var pinnedTools = []string{ "quizzes_quiz_stats", "quizzes_reorder_questions", "quizzes_review_confirm", + "quizzes_review_confirm_bulk", "quizzes_review_list", + "quizzes_review_regrade", + "quizzes_review_set_score", "quizzes_review_show", "quizzes_show_quiz", "quizzes_unpublish", @@ -160,6 +163,49 @@ func TestQuizzesMCP(t *testing.T) { } }) + t.Run("review annotations", func(t *testing.T) { + destructive := map[string]bool{"review_regrade": true} + idempotent := map[string]bool{"review_confirm": true, "review_set_score": true, "review_confirm_bulk": true} + n := 0 + for _, tool := range tools { + if !strings.HasPrefix(tool.Name, "review_") { + continue + } + n++ + if !strings.HasPrefix(tool.Path, ReviewPrefix) { + t.Errorf("%s: path %s is not under %s", tool.Name, tool.Path, ReviewPrefix) + } + if got := tool.Destructive != nil && *tool.Destructive; got != destructive[tool.Name] { + t.Errorf("%s: Destructive = %v, want %v", tool.Name, got, destructive[tool.Name]) + } + if got := tool.Idempotent != nil && *tool.Idempotent; got != idempotent[tool.Name] { + t.Errorf("%s: Idempotent = %v, want %v", tool.Name, got, idempotent[tool.Name]) + } + suffix := needsWrite + if tool.Method == http.MethodGet { + suffix = needsRead + } + if !strings.HasSuffix(tool.Description, suffix) { + t.Errorf("%s: description does not end with %q", tool.Name, suffix) + } + } + if n != 6 { + t.Errorf("%d review tools, want 6", n) + } + }) + + t.Run("no vendor names", func(t *testing.T) { + for _, tool := range tools { + // The golem15 namespace of the permission is not the bridge. + text := strings.ReplaceAll(strings.ToLower(tool.Name+" "+tool.Title+" "+tool.Description+" "+tool.Path), "golem15.", "") + for _, word := range []string{" ai ", "ai.", "openai", "anthropic", "claude", "gpt", "gemini", "llm", "language model", "bridge", "golem"} { + if strings.Contains(" "+text+" ", word) { + t.Errorf("%s mentions %q", tool.Name, word) + } + } + } + }) + t.Run("descriptions", func(t *testing.T) { for _, tool := range tools { if n := utf8.RuneCountInString(tool.Description); n > pier.MaxDescriptionRunes { diff --git a/plugin_test.go b/plugin_test.go index 61325d5..7a54e7c 100644 --- a/plugin_test.go +++ b/plugin_test.go @@ -84,6 +84,9 @@ func TestRoutesRegistered(t *testing.T) { "GET /api/v1/quiz-reviews", "GET /api/v1/quiz-reviews/{id}", "POST /api/v1/quiz-reviews/{id}/confirm", + "POST /api/v1/quiz-reviews/{id}/score", + "POST /api/v1/quiz-reviews/confirm", + "POST /api/v1/quiz-reviews/regrade", } var got []string for _, r := range router.Routes() { diff --git a/routes.go b/routes.go index f917bf0..f16ac56 100644 --- a/routes.go +++ b/routes.go @@ -90,6 +90,10 @@ func (p *Plugin) Routes(r pact.Router) error { g.Where("id", "[0-9]+") g.Post("/{id}/confirm", h.ReviewConfirm, "user.scope:write", perm) g.Where("id", "[0-9]+") + g.Post("/{id}/score", h.ReviewSetScore, "user.scope:write", perm) + g.Where("id", "[0-9]+") + g.Post("/confirm", h.ReviewConfirmBulk, "user.scope:write", perm) + g.Post("/regrade", h.ReviewRegrade, "user.scope:write", perm) }) return nil }