From 2fa8aa417847ad6cc5a8485879917a694ee0315a Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Mon, 9 Nov 2026 18:02:35 +0100 Subject: [PATCH] feat: list, show and confirm open answers over MCP - classes/review.go: review queue, detail and Confirm, shared by MCP and the admin screen; AnswerReviewed and AttemptReviewed after commit - /api/v1/quiz-reviews routes behind the quizzes permission - quizzes_review_list, quizzes_review_show, quizzes_review_confirm --- classes/review.go | 456 ++++++++++++++++++++++++++++++++ classes/review_test.go | 40 +++ controllers/api/review_json.go | 175 ++++++++++++ controllers/api/reviews.go | 145 ++++++++++ controllers/api/reviews_test.go | 169 ++++++++++++ mcp.go | 5 +- mcp_reviews.go | 58 ++++ mcp_test.go | 3 + plugin_test.go | 6 +- routes.go | 16 ++ 10 files changed, 1070 insertions(+), 3 deletions(-) create mode 100644 classes/review.go create mode 100644 classes/review_test.go create mode 100644 controllers/api/review_json.go create mode 100644 controllers/api/reviews.go create mode 100644 controllers/api/reviews_test.go create mode 100644 mcp_reviews.go diff --git a/classes/review.go b/classes/review.go new file mode 100644 index 0000000..9ea953f --- /dev/null +++ b/classes/review.go @@ -0,0 +1,456 @@ +package classes + +// The owner's review queue of open answers: one service shared by the MCP +// review routes (/api/v1/quiz-reviews) and the admin "Do oceny" screen. +// Reviewers hold the golem15.quizzes.access_quizzes permission (D-13); the +// routes and the admin controller check it, this file trusts its caller. +// +// Every write runs in lagoon.Transaction on s.DB, so a Service whose DB is +// an outer transaction (an admin save) nests as a savepoint. A write locks +// the answer's attempt first, as grading does, recalculates a completed +// attempt and fires its events only after the commit (D-14). + +import ( + "context" + "errors" + "slices" + "strings" + "time" + + "git.golem15.com/golem15/sm-quizzes-plugin/events" + "git.golem15.com/golem15/sm-quizzes-plugin/models" + "git.golem15.com/golem15/summercms/modules/lagoon" + "gorm.io/gorm" +) + +// 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. + ReviewStatusOpen = "open" + // ReviewStatusAll lists every open answer, reviewed ones included. + ReviewStatusAll = "all" +) + +// 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") + +// ReviewFilter narrows the review queue. Zero values do not filter; Status +// is one CheckReviewStatus accepts. +type ReviewFilter struct { + QuizSlug string + Status string + UserID uint + AttemptID uint +} + +// Reviewer is who reviews: a frontend user over MCP (UserID, the API +// token's user) or a backend administrator (AdminID). A zero id is stored +// as NULL (Pitfall 7: the two live in different tables). +type Reviewer struct { + UserID uint + AdminID uint +} + +func (r Reviewer) ids() (user, admin *uint) { + if r.UserID != 0 { + id := r.UserID + user = &id + } + if r.AdminID != 0 { + id := r.AdminID + admin = &id + } + return user, admin +} + +// unreviewedStatuses are the grading statuses the owner still has to act +// on. +func unreviewedStatuses() []string { + return []string{models.GradingPending, models.GradingGraded, models.GradingFailed} +} + +// CheckReviewStatus maps a status filter to the grading statuses to list: +// "" and open give pending, graded and failed, a grading status gives +// itself and all gives nil (no filter). Anything else is a problem naming +// the valid values. +func CheckReviewStatus(status string) ([]string, []string) { + status = strings.TrimSpace(status) + switch status { + case "", ReviewStatusOpen: + return unreviewedStatuses(), nil + case ReviewStatusAll: + return nil, nil + } + if slices.Contains(models.GradingStatuses, status) { + return []string{status}, nil + } + return nil, []string{"status must be one of open, pending, graded, failed, reviewed, all"} +} + +// ReviewRow is one open answer of the review queue. In a queue row Text is +// cut to ReviewExcerptRunes+1 runes, enough to tell that it was cut; the +// detail carries the whole text. +type ReviewRow struct { + AnswerID uint + Status string + QuizID uint + QuizSlug string + QuizTitle string + QuizMode string + QuestionID uint + QuestionText string + QuestionPoints int + AttemptID uint + AttemptCompletedAt *time.Time + UserID uint + Email string + Name *string + Surname *string + Text string + GraderPoints *float64 + Points float64 + GradedAt *time.Time + ReviewedAt *time.Time + UpdatedAt time.Time +} + +// ReviewDetail is one open answer with everything the owner reviews it +// against: the model answer and the rubric (owner-only), the grader's +// result or error and the review so far. MaxLength is the question's +// effective limit. +type ReviewDetail struct { + ReviewRow + AnswerText *string + Rubric *string + MaxLength int + Grader *string + GraderFeedback *string + GradingError *string + ReviewComment *string + GradingAttempts int + IsCorrect bool + ReviewedByUserID *uint + ReviewedByAdminID *uint +} + +// reviewFromSQL joins an open answer to its question, attempt, quiz and +// student. Only open answers (a grading status) are in the queue. +const reviewFromSQL = ` +FROM golem15_quizzes_attempt_answers aa +JOIN golem15_quizzes_questions q ON q.id = aa.question_id +JOIN golem15_quizzes_attempts a ON a.id = aa.attempt_id +JOIN golem15_quizzes_quizzes qz ON qz.id = a.quiz_id +JOIN users u ON u.id = a.user_id +WHERE aa.grading_status IS NOT NULL` + +// reviewRowColumnsSQL are the columns of a ReviewRow but the text. +const reviewRowColumnsSQL = `aa.id AS answer_id, aa.grading_status AS status, + qz.id AS quiz_id, qz.slug AS quiz_slug, qz.title AS quiz_title, qz.mode AS quiz_mode, + q.id AS question_id, q.text AS question_text, q.points AS question_points, + a.id AS attempt_id, a.completed_at AS attempt_completed_at, + a.user_id AS user_id, COALESCE(u.email, '') AS email, u.name AS name, u.surname AS surname, + aa.grader_points AS grader_points, aa.points AS points, + aa.graded_at AS graded_at, aa.reviewed_at AS reviewed_at, aa.updated_at AS updated_at` + +// reviewQueueSQL selects the queue rows; the filter predicates, the order +// and the page are appended as bound parameters. +const reviewQueueSQL = `SELECT ` + reviewRowColumnsSQL + `, + LEFT(COALESCE(aa.text_answer, ''), 301) AS text` + reviewFromSQL + +// reviewDetailSQL selects one open answer by id with the detail columns. +const reviewDetailSQL = `SELECT ` + reviewRowColumnsSQL + `, + COALESCE(aa.text_answer, '') AS text, + q.answer_text AS answer_text, q.rubric AS rubric, q.max_length AS max_length, + aa.grader AS grader, aa.grader_feedback AS grader_feedback, aa.grading_error AS grading_error, + aa.review_comment AS review_comment, aa.grading_attempts AS grading_attempts, + aa.is_correct AS is_correct, + aa.reviewed_by_user_id AS reviewed_by_user_id, aa.reviewed_by_admin_id AS reviewed_by_admin_id` + + reviewFromSQL + ` AND aa.id = ?` + +// reviewScan is what the review queries scan into. +type reviewScan struct { + ReviewRow + AnswerText *string + Rubric *string + MaxLength *int + Grader *string + GraderFeedback *string + GradingError *string + ReviewComment *string + GradingAttempts int + IsCorrect bool + ReviewedByUserID *uint + ReviewedByAdminID *uint +} + +func (r reviewScan) detail() *ReviewDetail { + maxLength := models.DefaultMaxLength + if r.MaxLength != nil { + maxLength = *r.MaxLength + } + return &ReviewDetail{ + ReviewRow: r.ReviewRow, + AnswerText: r.AnswerText, + Rubric: r.Rubric, + MaxLength: maxLength, + Grader: r.Grader, + GraderFeedback: r.GraderFeedback, + GradingError: r.GradingError, + ReviewComment: r.ReviewComment, + GradingAttempts: r.GradingAttempts, + IsCorrect: r.IsCorrect, + ReviewedByUserID: r.ReviewedByUserID, + ReviewedByAdminID: r.ReviewedByAdminID, + } +} + +// reviewWhere turns a filter into predicates appended to reviewFromSQL. +func reviewWhere(f ReviewFilter, statuses []string) (string, []any) { + var b strings.Builder + var args []any + if statuses != nil { + b.WriteString(" AND aa.grading_status IN ?") + args = append(args, statuses) + } + if slug := strings.TrimSpace(f.QuizSlug); slug != "" { + b.WriteString(" AND qz.slug = ?") + args = append(args, slug) + } + if f.UserID != 0 { + b.WriteString(" AND a.user_id = ?") + args = append(args, f.UserID) + } + if f.AttemptID != 0 { + b.WriteString(" AND aa.attempt_id = ?") + args = append(args, f.AttemptID) + } + return b.String(), args +} + +// ReviewQueue lists the open answers matching f, oldest change first, one +// page at a time (perPage 1..100, default 20), with the total count. An +// invalid status is a *ValidationError. +func (s *Service) ReviewQueue(ctx context.Context, f ReviewFilter, page, perPage int) ([]ReviewRow, int64, error) { + statuses, problems := CheckReviewStatus(f.Status) + if len(problems) > 0 { + return nil, 0, &ValidationError{Problems: problems} + } + if page < 1 { + page = 1 + } + if perPage < 1 { + perPage = 20 + } + perPage = min(perPage, 100) + where, args := reviewWhere(f, statuses) + var total int64 + if err := s.db(ctx).Raw("SELECT COUNT(*)"+reviewFromSQL+where, args...).Scan(&total).Error; err != nil { + return nil, 0, err + } + var scans []reviewScan + q := reviewQueueSQL + where + " ORDER BY aa.updated_at ASC, aa.id ASC LIMIT ? OFFSET ?" + if err := s.db(ctx).Raw(q, append(args, perPage, (page-1)*perPage)...).Scan(&scans).Error; err != nil { + return nil, 0, err + } + out := make([]ReviewRow, 0, len(scans)) + for _, sc := range scans { + out = append(out, sc.ReviewRow) + } + return out, total, nil +} + +// ReviewItem loads one open answer with its detail. A missing answer, or +// one that is not an open answer, is ErrNotFound. +func (s *Service) ReviewItem(ctx context.Context, answerID uint) (*ReviewDetail, error) { + return reviewItem(ctx, s.db(ctx), answerID) +} + +func reviewItem(ctx context.Context, db *gorm.DB, answerID uint) (*ReviewDetail, error) { + var scans []reviewScan + if err := db.WithContext(ctx).Raw(reviewDetailSQL, answerID).Scan(&scans).Error; err != nil { + return nil, err + } + if len(scans) == 0 { + return nil, ErrNotFound + } + return scans[0].detail(), nil +} + +// reviewTarget is an open answer loaded for a review write, with its +// question and its attempt (quiz set); the attempt row is locked. +type reviewTarget struct { + Answer models.AttemptAnswer + Question models.Question + Attempt *models.Attempt +} + +// 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. +func lockReviewTarget(ctx context.Context, tx *gorm.DB, answerID uint) (*reviewTarget, error) { + tx = tx.WithContext(ctx) + var row models.AttemptAnswer + if err := tx.Where("id = ? AND grading_status IS NOT NULL", answerID).Take(&row).Error; err != nil { + return nil, notFound(err) + } + attempt, err := lockAttempt(ctx, tx, row.AttemptID) + if err != nil { + return nil, notFound(err) + } + if err := tx.Where("id = ? AND grading_status IS NOT NULL", answerID).Take(&row).Error; err != nil { + return nil, notFound(err) + } + var quiz models.Quiz + if err := tx.Where("id = ?", attempt.QuizID).Take(&quiz).Error; err != nil { + return nil, notFound(err) + } + attempt.Quiz = &quiz + var question models.Question + if err := tx.Where("id = ?", row.QuestionID).Take(&question).Error; err != nil { + return nil, notFound(err) + } + return &reviewTarget{Answer: row, Question: question, Attempt: attempt}, nil +} + +// notFound maps gorm.ErrRecordNotFound to ErrNotFound. +func notFound(err error) error { + if errors.Is(err, gorm.ErrRecordNotFound) { + return ErrNotFound + } + return err +} + +// Confirm makes the grader's score of a graded answer final: status +// reviewed, points = grader_points (D-12). A reviewed answer is returned +// unchanged, without a write or an event; a pending or failed answer has +// no score to confirm (ErrNoGraderScore). A completed attempt is +// recalculated, and AnswerReviewedEvent, plus AttemptReviewedEvent when +// the attempt is now fully reviewed, fire after the commit. +func (s *Service) Confirm(ctx context.Context, answerID uint, by Reviewer) (*ReviewDetail, error) { + 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 done, err := s.confirmTarget(ctx, tx, t, by); err != nil { + return err + } else if done { + 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 +} + +// confirmTarget confirms one locked answer and reports whether it wrote: +// false for a reviewed answer, ErrNoGraderScore without a grader score. +// The caller recalculates the attempt. +func (s *Service) confirmTarget(ctx context.Context, tx *gorm.DB, t *reviewTarget, by Reviewer) (bool, error) { + status := "" + if t.Answer.GradingStatus != nil { + status = *t.Answer.GradingStatus + } + switch { + case status == models.GradingReviewed: + return false, nil + case status != models.GradingGraded || t.Answer.GraderPoints == nil: + return false, ErrNoGraderScore + } + return true, s.storeReview(ctx, tx, t, *t.Answer.GraderPoints, t.Answer.ReviewComment, false, by) +} + +// storeReview writes the owner's decision on one locked open answer and +// registers its AnswerReviewedEvent. setComment replaces review_comment +// with comment (nil clears it); otherwise the comment stays. grader_points +// and grader_feedback stay for the record. +func (s *Service) storeReview(ctx context.Context, tx *gorm.DB, t *reviewTarget, points float64, comment *string, setComment bool, by Reviewer) error { + now := s.now().UTC() + user, admin := by.ids() + cols := map[string]any{ + "grading_status": models.GradingReviewed, + "points": points, + "is_correct": OpenCorrect(points, t.Question.Points), + "reviewed_at": now, + "reviewed_by_user_id": user, + "reviewed_by_admin_id": admin, + "updated_at": now, + } + if setComment { + cols["review_comment"] = comment + } else { + comment = t.Answer.ReviewComment + } + if err := tx.WithContext(ctx).Model(&models.AttemptAnswer{}).Where("id = ?", t.Answer.ID).Updates(cols).Error; err != nil { + return err + } + fireAfterCommit(ctx, s, tx, events.AnswerReviewedEventName, &events.AnswerReviewedEvent{ + QuizID: t.Attempt.QuizID, + QuestionID: t.Question.ID, + AttemptID: t.Attempt.ID, + AnswerID: t.Answer.ID, + UserID: t.Attempt.UserID, + Points: points, + MaxPoints: t.Question.Points, + GraderPoints: t.Answer.GraderPoints, + Comment: comment, + ReviewerUserID: user, + ReviewerAdminID: admin, + }) + return nil +} + +// fireAttemptReviewedIfDone registers AttemptReviewedEvent for a completed +// attempt that has at least one open answer and none left pending, graded +// or failed, with the attempt's stored score; passed compares its percent +// with the quiz's passing score. Call it after the attempt is +// recalculated, inside the transaction that holds its lock. Without an +// event bus it reads nothing. +func (s *Service) fireAttemptReviewedIfDone(ctx context.Context, tx *gorm.DB, attemptID uint) error { + if s == nil || s.Events == nil { + return nil + } + tx = tx.WithContext(ctx) + var a models.Attempt + if err := tx.Preload("Quiz").Where("id = ?", attemptID).Take(&a).Error; err != nil { + if errors.Is(err, gorm.ErrRecordNotFound) { + return nil + } + return err + } + if a.CompletedAt == nil || a.Quiz == nil { + return nil + } + counts, err := answerCounts(ctx, tx, []uint{attemptID}) + if err != nil { + return err + } + if c := counts[attemptID]; c.OpenN == 0 || c.UnreviewedN > 0 { + return nil + } + fireAfterCommit(ctx, s, tx, events.AttemptReviewedEventName, &events.AttemptReviewedEvent{ + QuizID: a.QuizID, + AttemptID: a.ID, + UserID: a.UserID, + ScorePoints: a.ScorePoints, + MaxPoints: a.MaxPoints, + ScorePercent: a.ScorePercent, + Passed: a.ScorePercent != nil && *a.ScorePercent >= float64(a.Quiz.PassingScore), + }) + return nil +} diff --git a/classes/review_test.go b/classes/review_test.go new file mode 100644 index 0000000..57f2b54 --- /dev/null +++ b/classes/review_test.go @@ -0,0 +1,40 @@ +package classes_test + +import ( + "slices" + "strings" + "testing" + + "git.golem15.com/golem15/sm-quizzes-plugin/classes" +) + +// TestCheckReviewStatus: "" and open list the three unreviewed statuses, +// a grading status lists itself, all lists everything (nil), and any +// other value is a problem naming the valid values. +func TestCheckReviewStatus(t *testing.T) { + open := []string{"pending", "graded", "failed"} + for in, want := range map[string][]string{ + "": open, + "open": open, + " open ": open, + "pending": {"pending"}, + "graded": {"graded"}, + "failed": {"failed"}, + "reviewed": {"reviewed"}, + "all": nil, + } { + got, problems := classes.CheckReviewStatus(in) + if len(problems) != 0 { + t.Errorf("%q: problems %v", in, problems) + } + if !slices.Equal(got, want) || (want == nil) != (got == nil) { + t.Errorf("%q = %v, want %v", in, got, want) + } + } + for _, in := range []string{"done", "Open", "pending,graded"} { + got, problems := classes.CheckReviewStatus(in) + if got != nil || len(problems) != 1 || !strings.Contains(problems[0], "open, pending, graded, failed, reviewed, all") { + t.Errorf("%q = %v %v, want one problem listing the valid values", in, got, problems) + } + } +} diff --git a/controllers/api/review_json.go b/controllers/api/review_json.go new file mode 100644 index 0000000..e203fee --- /dev/null +++ b/controllers/api/review_json.go @@ -0,0 +1,175 @@ +package api + +// The owner shapes of the review routes. They carry student emails, the +// model answer, the rubric and grading errors, and are never used by a +// student route; the student shapes live in json.go. Built field by field, +// so a new column never leaks. + +import ( + "time" + "unicode/utf8" + + "git.golem15.com/golem15/sm-quizzes-plugin/classes" + "git.golem15.com/golem15/sm-quizzes-plugin/models" + "git.golem15.com/golem15/summercms/modules/wire" +) + +type reviewQuizShape struct { + ID uint `json:"id"` + Slug string `json:"slug"` + Title string `json:"title"` + Mode string `json:"mode"` +} + +type reviewQuestionShape struct { + ID uint `json:"id"` + Text string `json:"text"` + Points int `json:"points"` +} + +type reviewDetailQuestionShape struct { + reviewQuestionShape + AnswerText *string `json:"answer_text"` + Rubric *string `json:"rubric"` + MaxLength int `json:"max_length"` +} + +type reviewAttemptShape struct { + ID uint `json:"id"` + Completed bool `json:"completed"` + CompletedAt *wire.Time `json:"completed_at"` +} + +type reviewStudentShape struct { + UserID uint `json:"user_id"` + Email string `json:"email"` + Name *string `json:"name"` + Surname *string `json:"surname"` +} + +type reviewerShape struct { + UserID *uint `json:"user_id"` + AdminID *uint `json:"admin_id"` +} + +// reviewRowShape is one row of GET /api/v1/quiz-reviews. +type reviewRowShape struct { + AnswerID uint `json:"answer_id"` + Status string `json:"status"` + Quiz reviewQuizShape `json:"quiz"` + Question reviewQuestionShape `json:"question"` + Attempt reviewAttemptShape `json:"attempt"` + Student reviewStudentShape `json:"student"` + Text string `json:"text"` + TextTruncated bool `json:"text_truncated"` + GraderPoints *float64 `json:"grader_points"` + Points float64 `json:"points"` + GradedAt *wire.Time `json:"graded_at"` + ReviewedAt *wire.Time `json:"reviewed_at"` + UpdatedAt wire.Time `json:"updated_at"` +} + +// reviewDetailShape is GET /api/v1/quiz-reviews/{id} and every review +// write: the row keys with the whole text, plus the solution and the +// grading and review record. +type reviewDetailShape struct { + AnswerID uint `json:"answer_id"` + Status string `json:"status"` + Quiz reviewQuizShape `json:"quiz"` + Question reviewDetailQuestionShape `json:"question"` + Attempt reviewAttemptShape `json:"attempt"` + Student reviewStudentShape `json:"student"` + Text string `json:"text"` + GraderPoints *float64 `json:"grader_points"` + Points float64 `json:"points"` + GradedAt *wire.Time `json:"graded_at"` + ReviewedAt *wire.Time `json:"reviewed_at"` + UpdatedAt wire.Time `json:"updated_at"` + Grader *string `json:"grader"` + GraderFeedback *string `json:"grader_feedback"` + GradingError *string `json:"grading_error"` + GradingAttempts int `json:"grading_attempts"` + IsCorrect bool `json:"is_correct"` + ReviewComment *string `json:"review_comment"` + ReviewedBy reviewerShape `json:"reviewed_by"` +} + +func wireTimePtr(t *time.Time) *wire.Time { + if t == nil { + return nil + } + w := wireTime(*t) + return &w +} + +func reviewQuiz(r classes.ReviewRow) reviewQuizShape { + return reviewQuizShape{ID: r.QuizID, Slug: r.QuizSlug, Title: r.QuizTitle, Mode: quizMode(&models.Quiz{Mode: r.QuizMode})} +} + +func reviewAttempt(r classes.ReviewRow) reviewAttemptShape { + return reviewAttemptShape{ID: r.AttemptID, Completed: r.AttemptCompletedAt != nil, CompletedAt: wireTimePtr(r.AttemptCompletedAt)} +} + +func reviewStudent(r classes.ReviewRow) reviewStudentShape { + return reviewStudentShape{UserID: r.UserID, Email: r.Email, Name: r.Name, Surname: r.Surname} +} + +// excerpt keeps the first n runes of s and reports whether it cut. +func excerpt(s string, n int) (string, bool) { + if utf8.RuneCountInString(s) <= n { + return s, false + } + return string([]rune(s)[:n]), true +} + +// reviewRowJSON is one queue row; the text is cut to the first +// classes.ReviewExcerptRunes runes with text_truncated set. +func reviewRowJSON(r classes.ReviewRow) reviewRowShape { + text, cut := excerpt(r.Text, classes.ReviewExcerptRunes) + return reviewRowShape{ + AnswerID: r.AnswerID, + Status: r.Status, + Quiz: reviewQuiz(r), + Question: reviewQuestionShape{ID: r.QuestionID, Text: r.QuestionText, Points: r.QuestionPoints}, + Attempt: reviewAttempt(r), + Student: reviewStudent(r), + Text: text, + TextTruncated: cut, + GraderPoints: r.GraderPoints, + Points: r.Points, + GradedAt: wireTimePtr(r.GradedAt), + ReviewedAt: wireTimePtr(r.ReviewedAt), + UpdatedAt: wireTime(r.UpdatedAt), + } +} + +// reviewDetailJSON is one answer for review, owner view. +func reviewDetailJSON(d classes.ReviewDetail) reviewDetailShape { + r := d.ReviewRow + return reviewDetailShape{ + AnswerID: r.AnswerID, + Status: r.Status, + Quiz: reviewQuiz(r), + Question: reviewDetailQuestionShape{ + reviewQuestionShape: reviewQuestionShape{ID: r.QuestionID, Text: r.QuestionText, Points: r.QuestionPoints}, + AnswerText: d.AnswerText, + Rubric: d.Rubric, + MaxLength: d.MaxLength, + }, + Attempt: reviewAttempt(r), + Student: reviewStudent(r), + Text: r.Text, + GraderPoints: r.GraderPoints, + Points: r.Points, + GradedAt: wireTimePtr(r.GradedAt), + ReviewedAt: wireTimePtr(r.ReviewedAt), + UpdatedAt: wireTime(r.UpdatedAt), + Grader: d.Grader, + GraderFeedback: d.GraderFeedback, + GradingError: d.GradingError, + GradingAttempts: d.GradingAttempts, + IsCorrect: d.IsCorrect, + ReviewComment: d.ReviewComment, + ReviewedBy: reviewerShape{UserID: d.ReviewedByUserID, AdminID: d.ReviewedByAdminID}, + } +} diff --git a/controllers/api/reviews.go b/controllers/api/reviews.go new file mode 100644 index 0000000..4748102 --- /dev/null +++ b/controllers/api/reviews.go @@ -0,0 +1,145 @@ +package api + +// The owner review routes under /api/v1/quiz-reviews, behind user.api_token, +// the golem15.quizzes.manage throttle, a token scope and the access_quizzes +// permission (D-13). The quizzes_review_* MCP tools bind them. Answers use +// the owner shapes of review_json.go. + +import ( + "errors" + "net/http" + "net/url" + "strconv" + "strings" + + "git.golem15.com/golem15/sm-quizzes-plugin/classes" + "git.golem15.com/golem15/summercms/modules/lagoon" + "git.golem15.com/golem15/summercms/modules/wire" +) + +// reviewConflict answers a review action the answer's state refuses: 409 +// with a stable code for clients to branch on. +func reviewConflict(w http.ResponseWriter, message, code string) { + wire.WriteJSON(w, http.StatusConflict, map[string]any{"error": true, "message": message, "code": code}) +} + +// writeReview answers {data: data}, or maps err: the review conflicts are +// 409s with a code, ErrNotFound a 404, everything else as writeManaged. +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.ErrNotFound): + wire.WriteJSON(w, http.StatusNotFound, map[string]any{"error": true, "message": "Open answer, attempt or quiz not found."}) + default: + writeManaged(w, status, data, err) + } +} + +// queryID reads an optional id query value: empty is 0, anything but a +// positive integer is a problem. +func queryID(q url.Values, name string) (uint, []string) { + raw := strings.TrimSpace(q.Get(name)) + if raw == "" { + return 0, nil + } + id, err := strconv.ParseUint(raw, 10, 32) + if err != nil || id == 0 { + return 0, []string{name + " must be a positive integer"} + } + return uint(id), nil +} + +// reviewID reads the {id} path value, the answer id: anything but a +// positive integer is the 404 of a missing answer. +func reviewID(w http.ResponseWriter, r *http.Request) (uint, bool) { + id, err := strconv.ParseUint(r.PathValue("id"), 10, 32) + if err != nil || id == 0 { + writeReview(w, 0, nil, classes.ErrNotFound) + return 0, false + } + return uint(id), true +} + +// reviewer is the API token's user, who signs a review made over MCP. +func (h *Handlers) reviewer(w http.ResponseWriter, r *http.Request) (classes.Reviewer, bool) { + id, ok := h.user(w, r) + return classes.Reviewer{UserID: id}, ok +} + +// ReviewList answers GET /api/v1/quiz-reviews?quiz=&status=&user_id= +// &attempt_id=&page=&per_page=: the open answers, oldest change first, +// paginated. status defaults to open (pending, graded and failed). +func (h *Handlers) ReviewList(w http.ResponseWriter, r *http.Request) { + q := r.URL.Query() + f := classes.ReviewFilter{QuizSlug: strings.TrimSpace(q.Get("quiz")), Status: strings.TrimSpace(q.Get("status"))} + _, problems := classes.CheckReviewStatus(f.Status) + var p []string + f.UserID, p = queryID(q, "user_id") + problems = append(problems, p...) + f.AttemptID, p = queryID(q, "attempt_id") + problems = append(problems, p...) + if refuse(w, problems) { + return + } + svc, ok := h.service(w) + if !ok { + return + } + page := queryInt(r, "page", 1, 1, 100000) + perPage := queryInt(r, "per_page", 20, 1, 100) + rows, total, err := svc.ReviewQueue(r.Context(), f, page, perPage) + if err != nil { + writeReview(w, 0, nil, err) + return + } + out := make([]reviewRowShape, 0, len(rows)) + for _, row := range rows { + out = append(out, reviewRowJSON(row)) + } + wire.WriteJSON(w, http.StatusOK, lagoon.Paginate(out, page, perPage, total)) +} + +// ReviewShow answers GET /api/v1/quiz-reviews/{id}: one open answer with +// its question, model answer, rubric, grading and review. +func (h *Handlers) ReviewShow(w http.ResponseWriter, r *http.Request) { + id, ok := reviewID(w, r) + if !ok { + return + } + svc, ok := h.service(w) + if !ok { + return + } + d, err := svc.ReviewItem(r.Context(), id) + answerReview(w, d, err) +} + +// ReviewConfirm answers POST /api/v1/quiz-reviews/{id}/confirm: the +// grader's score becomes final; 409 no_grader_score for a pending or +// failed answer. +func (h *Handlers) ReviewConfirm(w http.ResponseWriter, r *http.Request) { + id, ok := reviewID(w, r) + if !ok { + return + } + by, ok := h.reviewer(w, r) + if !ok { + return + } + svc, ok := h.service(w) + if !ok { + return + } + d, err := svc.Confirm(r.Context(), id, by) + answerReview(w, d, err) +} + +// answerReview answers one reviewed answer, or maps err. +func answerReview(w http.ResponseWriter, d *classes.ReviewDetail, err error) { + if err != nil { + writeReview(w, 0, nil, err) + return + } + writeReview(w, http.StatusOK, reviewDetailJSON(*d), nil) +} diff --git a/controllers/api/reviews_test.go b/controllers/api/reviews_test.go new file mode 100644 index 0000000..cc9312a --- /dev/null +++ b/controllers/api/reviews_test.go @@ -0,0 +1,169 @@ +package api + +import ( + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + "unicode/utf8" + + "git.golem15.com/golem15/sm-quizzes-plugin/classes" + "git.golem15.com/golem15/summercms/modules/backpack" +) + +func reviewRowFixture(text string) classes.ReviewRow { + at := time.Date(2026, 11, 9, 12, 0, 0, 0, time.UTC) + gp := 1.5 + name := "Anna" + return classes.ReviewRow{ + AnswerID: 41, Status: "graded", + QuizID: 3, QuizSlug: "pojecia", QuizTitle: "Pojęcia", QuizMode: "", + QuestionID: 7, QuestionText: "Czym jest odstraszanie?", QuestionPoints: 2, + AttemptID: 9, AttemptCompletedAt: &at, + UserID: 5, Email: "anna@example.test", Name: &name, + Text: text, GraderPoints: &gp, Points: 1.5, GradedAt: &at, UpdatedAt: at, + } +} + +func marshalMap(t *testing.T, v any) (string, map[string]any) { + t.Helper() + raw, err := json.Marshal(v) + if err != nil { + t.Fatal(err) + } + var out map[string]any + if err := json.Unmarshal(raw, &out); err != nil { + t.Fatal(err) + } + return string(raw), out +} + +// TestReviewJSON: a queue row cuts the text to 300 runes and says so, +// keeps a short text whole, and names its keys in snake_case; the detail +// carries the whole text with the model answer, rubric and grading error. +func TestReviewJSON(t *testing.T) { + long := strings.Repeat("ż", 301) + _, out := marshalMap(t, reviewRowJSON(reviewRowFixture(long))) + if got := out["text"].(string); utf8.RuneCountInString(got) != 300 || out["text_truncated"] != true { + t.Errorf("long text = %d runes, truncated %v; want 300, true", utf8.RuneCountInString(got), out["text_truncated"]) + } + + short := strings.Repeat("a", 120) + raw, out := marshalMap(t, reviewRowJSON(reviewRowFixture(short))) + if out["text"] != short || out["text_truncated"] != false { + t.Errorf("short text = %v, truncated %v; want whole, false", out["text"], out["text_truncated"]) + } + for _, key := range []string{"answer_id", "status", "quiz", "question", "attempt", "student", "text", "text_truncated", "grader_points", "points", "graded_at", "reviewed_at", "updated_at"} { + if _, ok := out[key]; !ok { + t.Errorf("row lacks %q: %s", key, raw) + } + } + if q := out["quiz"].(map[string]any); q["slug"] != "pojecia" || q["mode"] != "learning" { + t.Errorf("quiz = %v, want slug pojecia and mode learning", q) + } + if a := out["attempt"].(map[string]any); a["id"] != float64(9) || a["completed"] != true { + t.Errorf("attempt = %v, want id 9 completed", a) + } + if s := out["student"].(map[string]any); s["user_id"] != float64(5) || s["email"] != "anna@example.test" || s["name"] != "Anna" || s["surname"] != nil { + t.Errorf("student = %v", s) + } + for _, leak := range []string{"rubric", "answer_text", "grading_error"} { + if strings.Contains(raw, `"`+leak+`"`) { + t.Errorf("row carries %q: %s", leak, raw) + } + } + + model, rubric, failure := "model", "R-RUBRIC", "grader timed out" + d := classes.ReviewDetail{ + ReviewRow: reviewRowFixture(long), AnswerText: &model, Rubric: &rubric, MaxLength: 2000, + GradingError: &failure, GradingAttempts: 3, + } + raw, out = marshalMap(t, reviewDetailJSON(d)) + if got := out["text"].(string); got != long { + t.Errorf("detail text has %d runes, want the whole 301", utf8.RuneCountInString(got)) + } + q := out["question"].(map[string]any) + if q["answer_text"] != "model" || q["rubric"] != "R-RUBRIC" || q["max_length"] != float64(2000) || q["points"] != float64(2) { + t.Errorf("detail question = %v", q) + } + for _, key := range []string{"grader", "grader_feedback", "grading_error", "grading_attempts", "is_correct", "review_comment", "reviewed_by", "reviewed_at"} { + if _, ok := out[key]; !ok { + t.Errorf("detail lacks %q: %s", key, raw) + } + } + if out["grading_error"] != failure || out["grading_attempts"] != float64(3) { + t.Errorf("detail grading = %v / %v", out["grading_error"], out["grading_attempts"]) + } +} + +// TestReviewErrorMapping: the review conflicts are 409s with a code, a +// missing answer a 404, invalid input a 422, other errors an opaque 500. +func TestReviewErrorMapping(t *testing.T) { + for name, tc := range map[string]struct { + err error + 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, ""}, + } { + rec := httptest.NewRecorder() + writeReview(rec, 0, nil, tc.err) + if rec.Code != tc.code { + t.Errorf("%s = %d, want %d: %s", name, rec.Code, tc.code, rec.Body.String()) + continue + } + if tc.key != "" { + var out map[string]any + _ = json.Unmarshal(rec.Body.Bytes(), &out) + if out["code"] != tc.key || out["error"] != true || out["message"] == "" { + t.Errorf("%s body = %v, want code %s", name, out, tc.key) + } + } + } + rec := httptest.NewRecorder() + writeReview(rec, http.StatusOK, map[string]any{"x": 1}, nil) + if rec.Code != http.StatusOK || !strings.Contains(rec.Body.String(), `"data"`) { + t.Errorf("success = %d %s", rec.Code, rec.Body.String()) + } +} + +// TestReviewHandlersBeforeDatabase: bad filters are a 422 and a bad id a +// 404 before the database; valid requests without one are a 503. +func TestReviewHandlersBeforeDatabase(t *testing.T) { + h := New(backpack.New(nil)) + for target, want := range map[string]string{ + "/?status=done": "status must be one of", + "/?user_id=abc": "user_id must be a positive integer", + "/?attempt_id=0": "attempt_id must be a positive integer", + "/?attempt_id=-3": "attempt_id must be a positive integer", + "/?user_id=99999999999": "user_id must be a positive integer", + } { + code, out := call(t, h.ReviewList, "GET", target, "", 7, nil) + if msg, _ := out["message"].(string); code != http.StatusUnprocessableEntity || !strings.Contains(msg, want) { + t.Errorf("list %s = %d %v, want 422 with %q", target, code, out, want) + } + } + for _, id := range []string{"", "0", "abc"} { + for name, fn := range map[string]http.HandlerFunc{"show": h.ReviewShow, "confirm": h.ReviewConfirm} { + code, _ := call(t, fn, "POST", "/", "", 7, map[string]string{"id": id}) + if code != http.StatusNotFound { + t.Errorf("%s id %q = %d, want 404", name, id, code) + } + } + } + for name, fn := range map[string]http.HandlerFunc{"list": h.ReviewList, "show": h.ReviewShow, "confirm": h.ReviewConfirm} { + code, _ := call(t, fn, "GET", "/?status=graded", "", 7, map[string]string{"id": "3"}) + if code != http.StatusServiceUnavailable { + t.Errorf("%s without a database = %d, want 503", name, code) + } + } + if code, _ := call(t, h.ReviewConfirm, "POST", "/", "", 0, map[string]string{"id": "3"}); code != http.StatusUnauthorized { + t.Errorf("confirm without a user = %d, want 401", code) + } +} diff --git a/mcp.go b/mcp.go index 57c6f7d..0a59a89 100644 --- a/mcp.go +++ b/mcp.go @@ -4,11 +4,12 @@ import "git.golem15.com/golem15/summercms/modules/pact" var _ pact.HasMCP = (*Plugin)(nil) -// MCP declares the owner's quiz tools (exposed as quizzes_*). Every tool +// MCP declares the owner's quiz tools (exposed as quizzes_*) and the +// review tools of the open-answer queue (quizzes_review_*). Every tool // lists the golem15.quizzes.access_quizzes permission, so a user whose // groups lack it never lists them, and every route the tools bind checks // it again (D-13). The plugin still requires only golem15.user and names // no OAuth server. func (p *Plugin) MCP() pact.MCP { - return pact.MCP{Tools: quizTools()} + return pact.MCP{Tools: append(quizTools(), reviewTools()...)} } diff --git a/mcp_reviews.go b/mcp_reviews.go new file mode 100644 index 0000000..ebbe5a1 --- /dev/null +++ b/mcp_reviews.go @@ -0,0 +1,58 @@ +package quizzes + +import ( + "net/http" + + "git.golem15.com/golem15/summercms/modules/pact" +) + +// reviewListInput is the input of quizzes_review_list. +type reviewListInput struct { + Quiz string `json:"quiz,omitempty" query:"quiz" jsonschema:"only answers to this quiz, by slug"` + Status string `json:"status,omitempty" query:"status" jsonschema:"open (the default: pending, graded and failed, everything not reviewed yet), pending, graded, failed, reviewed or all"` + UserID uint `json:"user_id,omitempty" query:"user_id" jsonschema:"only answers of this student (user id)"` + AttemptID uint `json:"attempt_id,omitempty" query:"attempt_id" jsonschema:"only answers of this attempt"` + Page int `json:"page,omitempty" query:"page" jsonschema:"page number, from 1"` + PerPage int `json:"per_page,omitempty" query:"per_page" jsonschema:"rows per page, 1-100; default 20"` +} + +// reviewAnswerInput is the input of the tools that act on one answer. +type reviewAnswerInput struct { + ID uint `json:"id" path:"id" jsonschema:"the answer_id, as listed by quizzes_review_list"` +} + +// 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). +func reviewTools() []pact.MCPTool { + return []pact.MCPTool{ + { + Name: "review_list", + Title: "List open answers to review", + Description: "Lists the answers students wrote to open questions, oldest change first, for you to review. status: open (default: pending, graded and failed, everything not reviewed yet), pending (waiting for the grader or for you), graded (the grader gave a score you have not confirmed), failed (the grader could not score it; set the score yourself), reviewed or all. Filters: quiz (slug), user_id, attempt_id; page and per_page (1-100, default 20). Each row: answer_id, status, quiz {id, slug, title, mode}, question {id, text, points}, attempt {id, completed, completed_at}, student {user_id, email, name, surname}, text (the first 300 characters, text_truncated when cut), grader_points, points, graded_at, reviewed_at, updated_at. Read one answer in full with quizzes_review_show. It shows student emails, names and answers." + needsRead, + Method: http.MethodGet, + Path: ReviewPrefix, + Input: reviewListInput{}, + Permissions: quizPermissions, + }, + { + Name: "review_show", + Title: "Show one open answer for review", + Description: "Shows one open answer for review, owner view with the solution: question {id, text, points, answer_text (the model answer), rubric (the grading rules), max_length}, the student's full text, quiz, attempt, student, status, grader (the grader code), grader_points and grader_feedback (the grader's score and note, final only once status is reviewed), grading_error (why the grader failed), grading_attempts, points (what counts in the attempt's score now), is_correct, review_comment (shown to the student), reviewed_at and reviewed_by {user_id, admin_id}. Never pass the rubric or the model answer to a student." + needsRead, + Method: http.MethodGet, + Path: ReviewPrefix + "/{id}", + Input: reviewAnswerInput{}, + Permissions: quizPermissions, + }, + { + Name: "review_confirm", + Title: "Confirm the grader's score", + Description: "Confirms the grader's score of one graded answer: status becomes reviewed and points become grader_points, final over any later grading, and a finished attempt's score is recalculated. A pending or failed answer has no score to confirm and is refused with 409 code no_grader_score (set the score with quizzes_review_set_score instead); confirming a reviewed answer changes nothing. Returns the answer as quizzes_review_show does." + needsWrite, + Method: http.MethodPost, + Path: ReviewPrefix + "/{id}/confirm", + Input: reviewAnswerInput{}, + Permissions: quizPermissions, + Idempotent: &yes, + }, + } +} diff --git a/mcp_test.go b/mcp_test.go index a27c116..31d249e 100644 --- a/mcp_test.go +++ b/mcp_test.go @@ -84,6 +84,9 @@ var pinnedTools = []string{ "quizzes_publish", "quizzes_quiz_stats", "quizzes_reorder_questions", + "quizzes_review_confirm", + "quizzes_review_list", + "quizzes_review_show", "quizzes_show_quiz", "quizzes_unpublish", "quizzes_update_question", diff --git a/plugin_test.go b/plugin_test.go index 4a3b9dd..61325d5 100644 --- a/plugin_test.go +++ b/plugin_test.go @@ -81,6 +81,9 @@ func TestRoutesRegistered(t *testing.T) { "PATCH /api/v1/quizzes/{slug}/questions/{id}", "DELETE /api/v1/quizzes/{slug}/questions/{id}", "PUT /api/v1/quizzes/{slug}/questions/order", + "GET /api/v1/quiz-reviews", + "GET /api/v1/quiz-reviews/{id}", + "POST /api/v1/quiz-reviews/{id}/confirm", } var got []string for _, r := range router.Routes() { @@ -96,7 +99,8 @@ func TestRoutesRegistered(t *testing.T) { } continue } - if r.Pattern == "/api/v1/quizzes" || strings.HasPrefix(r.Pattern, "/api/v1/quizzes/") { + if r.Pattern == "/api/v1/quizzes" || strings.HasPrefix(r.Pattern, "/api/v1/quizzes/") || + r.Pattern == "/api/v1/quiz-reviews" || strings.HasPrefix(r.Pattern, "/api/v1/quiz-reviews/") { for _, mw := range []string{"user.api_token", "throttle:golem15.quizzes.manage", "user.permission:golem15.quizzes.access_quizzes"} { if !slices.Contains(r.Middleware, mw) { t.Errorf("%s %s is not behind %s: %v", r.Method, r.Pattern, mw, r.Middleware) diff --git a/routes.go b/routes.go index 79702cb..f917bf0 100644 --- a/routes.go +++ b/routes.go @@ -11,6 +11,11 @@ var _ pact.HasRoutes = (*Plugin)(nil) // ManagePrefix is the path prefix of the management API. const ManagePrefix = "/api/v1/quizzes" +// ReviewPrefix is the path prefix of the owner review routes (the MCP +// review tools). It is a separate prefix on purpose: under ManagePrefix a +// literal segment such as /reviews would shadow the quiz whose slug it is. +const ReviewPrefix = "/api/v1/quiz-reviews" + // permQuizzes is the permission that lets a user manage quizzes, in the // admin and through the management API. const permQuizzes = "golem15.quizzes.access_quizzes" @@ -24,6 +29,8 @@ const bucketManage = "golem15.quizzes.manage" // its application-wide API token: the import route, and the owner manage // routes the MCP quiz tools bind, which also need the access_quizzes // permission and run under the per-user golem15.quizzes.manage throttle. +// The owner review routes under /api/v1/quiz-reviews (the MCP review +// tools) run behind the same middleware and permission (D-13). func (p *Plugin) Routes(r pact.Router) error { h := api.New(p.app) r.Group("/_quizzes/api/v1", surf.Use("jwt.auth"), func(g pact.Router) { @@ -75,5 +82,14 @@ func (p *Plugin) Routes(r pact.Router) error { g.Put("/{slug}/questions/order", h.ManageReorderQuestions, "user.scope:write", perm) g.Where("slug", `[a-z0-9-]+`) }) + // The owner review routes (MCP review tools): the queue of open + // answers and the owner's decisions on them. + r.Group(ReviewPrefix, surf.Use("user.api_token", "throttle:"+bucketManage), func(g pact.Router) { + g.Get("", h.ReviewList, "user.scope:read", perm) + g.Get("/{id}", h.ReviewShow, "user.scope:read", perm) + g.Where("id", "[0-9]+") + g.Post("/{id}/confirm", h.ReviewConfirm, "user.scope:write", perm) + g.Where("id", "[0-9]+") + }) return nil }