From c6d8d1a9737711b2ecc297ad200ddf680c233cf8 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Mon, 9 Nov 2026 14:33:56 +0100 Subject: [PATCH] feat: edit quiz questions over MCP - quizzes_create_question, update_question, delete_question and reorder_questions on /api/v1/quizzes/{slug}/questions - update_question replaces answers by id (kept, created, deleted), so answer ids stored in past attempts stay valid; foreign ids are refused - classes.CheckQuestion holds the importer's per-question rules, shared with the manage routes; attempt scores are never touched (D-16) --- classes/importer.go | 78 ++++---- classes/importer_test.go | 33 ++++ classes/manage.go | 327 +++++++++++++++++++++++++++++++++ classes/manage_test.go | 71 +++++++ controllers/api/manage.go | 105 +++++++++++ controllers/api/manage_test.go | 38 +++- manage_api_postgres_test.go | 82 +++++++++ mcp_quizzes.go | 84 +++++++++ mcp_test.go | 4 + plugin_test.go | 4 + routes.go | 10 + 11 files changed, 792 insertions(+), 44 deletions(-) diff --git a/classes/importer.go b/classes/importer.go index c0c9a7d..396d9b9 100644 --- a/classes/importer.go +++ b/classes/importer.go @@ -97,40 +97,7 @@ func ParseImport(raw []byte) (*ImportQuiz, error) { problems = append(problems, "questions must not be empty") } for i, q := range doc.Questions { - at := fmt.Sprintf("questions[%d]", i) - if strings.TrimSpace(q.Text) == "" { - problems = append(problems, at+".text is required") - } - if !slices.Contains(models.QuestionTypes, q.Type) { - problems = append(problems, fmt.Sprintf("%s.type must be one of %s", at, strings.Join(models.QuestionTypes, ", "))) - continue - } - if q.Points != nil && *q.Points < 0 { - problems = append(problems, at+".points must not be negative") - } - correct := 0 - for j, a := range q.Answers { - if strings.TrimSpace(a.Text) == "" { - problems = append(problems, fmt.Sprintf("%s.answers[%d].text is required", at, j)) - } - if a.IsCorrect { - correct++ - } - } - switch q.Type { - case models.TypeSingle: - if len(q.Answers) < 2 || correct != 1 { - problems = append(problems, at+": a single question needs at least two answers and exactly one correct") - } - case models.TypeMultiple: - if len(q.Answers) < 2 || correct < 1 { - problems = append(problems, at+": a multiple question needs at least two answers and one or more correct") - } - case models.TypeSelfCheck: - if len(q.Answers) > 0 { - problems = append(problems, at+": a self_check question has no answers; use answer_text") - } - } + problems = append(problems, CheckQuestion(fmt.Sprintf("questions[%d]", i), q)...) } if len(problems) > 0 { return nil, fmt.Errorf("invalid quiz: %s", strings.Join(problems, "; ")) @@ -138,6 +105,49 @@ func ParseImport(raw []byte) (*ImportQuiz, error) { return &doc, nil } +// CheckQuestion returns the problems of one question, each prefixed with +// at (such as "questions[0]"): the text is required, the type is one of +// models.QuestionTypes, points are not negative, answer texts are +// required, a single question has at least two answers and exactly one +// correct, a multiple question at least two and one or more correct, and a +// self_check question none. The importer and the manage routes share it. +func CheckQuestion(at string, q ImportQuestion) []string { + var problems []string + if strings.TrimSpace(q.Text) == "" { + problems = append(problems, at+".text is required") + } + if !slices.Contains(models.QuestionTypes, q.Type) { + return append(problems, fmt.Sprintf("%s.type must be one of %s", at, strings.Join(models.QuestionTypes, ", "))) + } + if q.Points != nil && *q.Points < 0 { + problems = append(problems, at+".points must not be negative") + } + correct := 0 + for j, a := range q.Answers { + if strings.TrimSpace(a.Text) == "" { + problems = append(problems, fmt.Sprintf("%s.answers[%d].text is required", at, j)) + } + if a.IsCorrect { + correct++ + } + } + switch q.Type { + case models.TypeSingle: + if len(q.Answers) < 2 || correct != 1 { + problems = append(problems, at+": a single question needs at least two answers and exactly one correct") + } + case models.TypeMultiple: + if len(q.Answers) < 2 || correct < 1 { + problems = append(problems, at+": a multiple question needs at least two answers and one or more correct") + } + case models.TypeSelfCheck: + if len(q.Answers) > 0 { + problems = append(problems, at+": a self_check question has no answers; use answer_text") + } + } + return problems +} + // ImportResult reports what an import wrote. type ImportResult struct { Quiz models.Quiz diff --git a/classes/importer_test.go b/classes/importer_test.go index 5c32ac4..8753204 100644 --- a/classes/importer_test.go +++ b/classes/importer_test.go @@ -1,6 +1,7 @@ package classes_test import ( + "encoding/json" "os" "strings" "testing" @@ -82,3 +83,35 @@ func TestParseImportTestMode(t *testing.T) { t.Errorf("sample = %+v %v, want no test-mode keys", doc, err) } } + +// TestCheckQuestion: the per-question rules ParseImport applies, shared +// with the manage routes; ParseImport reports exactly these problems. +func TestCheckQuestion(t *testing.T) { + neg := -1 + for name, tc := range map[string]struct { + q classes.ImportQuestion + want []string + }{ + "no text": {classes.ImportQuestion{Type: "self_check"}, []string{"questions[0].text is required"}}, + "bad type": {classes.ImportQuestion{Text: "Q", Type: "essay"}, []string{"questions[0].type must be one of single, multiple, self_check"}}, + "negative points": {classes.ImportQuestion{Text: "Q", Type: "self_check", Points: &neg}, []string{"questions[0].points must not be negative"}}, + "single two right": {classes.ImportQuestion{Text: "Q", Type: "single", Answers: []classes.ImportAnswer{{Text: "a", IsCorrect: true}, {Text: "b", IsCorrect: true}}}, []string{"questions[0]: a single question needs at least two answers and exactly one correct"}}, + "multiple one item": {classes.ImportQuestion{Text: "Q", Type: "multiple", Answers: []classes.ImportAnswer{{Text: "a", IsCorrect: true}}}, []string{"questions[0]: a multiple question needs at least two answers and one or more correct"}}, + "self_check answer": {classes.ImportQuestion{Text: "Q", Type: "self_check", Answers: []classes.ImportAnswer{{Text: "a"}}}, []string{"questions[0]: a self_check question has no answers; use answer_text"}}, + "blank answer": {classes.ImportQuestion{Text: "Q", Type: "single", Answers: []classes.ImportAnswer{{Text: " ", IsCorrect: true}, {Text: "b"}}}, []string{"questions[0].answers[0].text is required"}}, + "valid single": {classes.ImportQuestion{Text: "Q", Type: "single", Answers: []classes.ImportAnswer{{Text: "a", IsCorrect: true}, {Text: "b"}}}, nil}, + } { + got := classes.CheckQuestion("questions[0]", tc.q) + if strings.Join(got, "; ") != strings.Join(tc.want, "; ") { + t.Errorf("%s: %q, want %q", name, got, tc.want) + } + doc, _ := json.Marshal(map[string]any{"title": "T", "questions": []classes.ImportQuestion{tc.q}}) + _, err := classes.ParseImport(doc) + switch { + case tc.want == nil && err != nil: + t.Errorf("%s: ParseImport: %v", name, err) + case tc.want != nil && (err == nil || err.Error() != "invalid quiz: "+strings.Join(tc.want, "; ")): + t.Errorf("%s: ParseImport = %v, want the same problems", name, err) + } + } +} diff --git a/classes/manage.go b/classes/manage.go index 4236d61..6767f0a 100644 --- a/classes/manage.go +++ b/classes/manage.go @@ -8,6 +8,7 @@ package classes import ( "context" "errors" + "fmt" "math" "slices" "strings" @@ -20,6 +21,10 @@ import ( "gorm.io/gorm/clause" ) +// ErrQuestionNotFound is a question id that is not a question of the quiz. +// It is an ErrNotFound. +var ErrQuestionNotFound = fmt.Errorf("%w: question", ErrNotFound) + // ValidationError is manage input that breaks a rule (a 422). Problems // read like the importer's: "title is required". type ValidationError struct { @@ -591,3 +596,325 @@ func BuildStats(quiz models.Quiz, questions []models.Question, students []Studen } return out } + +// ManagedAnswerInput is one answer of a question write. An ID keeps and +// updates that answer of the question; no ID creates one. Explanation nil +// keeps a kept answer's explanation, "" clears it. +type ManagedAnswerInput struct { + ID *uint `json:"id"` + Text string `json:"text"` + IsCorrect bool `json:"is_correct"` + Explanation *string `json:"explanation"` +} + +// QuestionInput is the fields of a question create or partial update; nil +// means not sent. Answers, when sent, is the question's full new answer +// list in order. An empty string clears category, answer_text and +// explanation. +type QuestionInput struct { + Text *string `json:"text"` + Type *string `json:"type"` + Category *string `json:"category"` + AnswerText *string `json:"answer_text"` + Explanation *string `json:"explanation"` + Points *int `json:"points"` + SortOrder *int `json:"sort_order"` + Answers *[]ManagedAnswerInput `json:"answers"` +} + +// MergeQuestion applies the sent fields of in, and its answers list when +// sent, to current (a zero Question for a create) and returns the result +// in the importer's shape, for CheckQuestion. An answer id that is not an +// answer of current, or is listed twice, is a problem. +func MergeQuestion(current models.Question, in QuestionInput) (ImportQuestion, []string) { + q := ImportQuestion{ + Text: current.Text, Type: current.Type, Category: current.Category, + AnswerText: current.AnswerText, Explanation: current.Explanation, + } + points := current.Points + if current.ID == 0 { + points = 1 + } + if in.Text != nil { + q.Text = strings.TrimSpace(*in.Text) + } + if in.Type != nil { + q.Type = *in.Type + } + if in.Category != nil { + q.Category = nilIfBlank(*in.Category, true) + } + if in.AnswerText != nil { + q.AnswerText = nilIfBlank(*in.AnswerText, false) + } + if in.Explanation != nil { + q.Explanation = nilIfBlank(*in.Explanation, false) + } + if in.Points != nil { + points = *in.Points + } + q.Points = &points + var problems []string + if in.Category != nil && utf8.RuneCountInString(strings.TrimSpace(*in.Category)) > 255 { + problems = append(problems, "category must be at most 255 characters") + } + if in.Answers == nil { + for _, a := range current.Answers { + q.Answers = append(q.Answers, ImportAnswer{Text: a.Text, IsCorrect: a.IsCorrect, Explanation: a.Explanation}) + } + return q, problems + } + byID := map[uint]models.Answer{} + for _, a := range current.Answers { + byID[a.ID] = a + } + seen := map[uint]bool{} + q.Answers = make([]ImportAnswer, 0, len(*in.Answers)) + for i, a := range *in.Answers { + out := ImportAnswer{Text: strings.TrimSpace(a.Text), IsCorrect: a.IsCorrect, Explanation: a.Explanation} + if a.ID != nil { + kept, ok := byID[*a.ID] + switch { + case !ok: + problems = append(problems, fmt.Sprintf("answers[%d].id does not belong to this question", i)) + case seen[*a.ID]: + problems = append(problems, fmt.Sprintf("answers[%d].id is listed twice", i)) + case a.Explanation == nil: + out.Explanation = kept.Explanation + } + seen[*a.ID] = true + } + if out.Explanation != nil { + out.Explanation = nilIfBlank(*out.Explanation, false) + } + q.Answers = append(q.Answers, out) + } + return q, problems +} + +// CheckNewQuestion is the rule check of a create, before any database +// access: text and type are required, answers carry no id, and the merged +// question passes CheckQuestion. +func CheckNewQuestion(in QuestionInput) []string { + var problems []string + if in.Text == nil { + problems = append(problems, "text is required") + } + if in.Type == nil { + problems = append(problems, "type is required") + } + if len(problems) > 0 { + return problems + } + merged, problems := MergeQuestion(models.Question{}, in) + if in.SortOrder != nil && *in.SortOrder < 1 { + problems = append(problems, "sort_order must be at least 1") + } + return append(problems, CheckQuestion("question", merged)...) +} + +// managedQuizID loads the id of the quiz with this slug, or ErrNotFound. +func managedQuizID(tx *gorm.DB, slug string) (uint, error) { + var quiz models.Quiz + if err := lockedQuiz(tx, slug, &quiz); err != nil { + return 0, err + } + return quiz.ID, nil +} + +// CreateQuestion adds a question with its answers to the quiz with this +// slug: at sort_order when given, else after the last question. Points +// default to 1; answers are ordered as sent. A missing slug is ErrNotFound. +func CreateQuestion(ctx context.Context, db *gorm.DB, slug string, in QuestionInput) (*models.Question, error) { + if err := invalid(CheckNewQuestion(in)); err != nil { + return nil, err + } + merged, _ := MergeQuestion(models.Question{}, in) + var q models.Question + err := lagoon.Transaction(ctx, db, func(ctx context.Context, tx *gorm.DB) error { + tx = tx.WithContext(ctx) + quizID, err := managedQuizID(tx, slug) + if err != nil { + return err + } + order := 0 + if in.SortOrder != nil { + order = *in.SortOrder + } else if err := tx.Model(&models.Question{}).Where("quiz_id = ?", quizID). + Select("COALESCE(MAX(sort_order), 0) + 1").Scan(&order).Error; err != nil { + return err + } + q = models.Question{ + QuizID: quizID, Text: merged.Text, Type: merged.Type, Category: merged.Category, + AnswerText: merged.AnswerText, Explanation: merged.Explanation, SortOrder: order, Points: *merged.Points, + } + if err := tx.Omit(clause.Associations).Create(&q).Error; err != nil { + return err + } + for i, a := range merged.Answers { + ans := models.Answer{QuestionID: q.ID, Text: a.Text, IsCorrect: a.IsCorrect, Explanation: a.Explanation, SortOrder: i + 1} + if err := tx.Omit(clause.Associations).Create(&ans).Error; err != nil { + return err + } + q.Answers = append(q.Answers, ans) + } + return nil + }) + if err != nil { + return nil, err + } + return &q, nil +} + +// UpdateQuestion changes only the sent fields of one question of the quiz +// with this slug. When answers is sent it is the full new list: answers +// with an id are updated in place (so the answer ids stored in past +// attempts stay valid), answers without one are created, answers missing +// from the list are deleted, and the order is the list order. The merged +// question must pass CheckQuestion. Stored attempt answers and scores are +// never touched (D-16). A missing quiz is ErrNotFound, a missing question +// ErrQuestionNotFound. +func UpdateQuestion(ctx context.Context, db *gorm.DB, slug string, id uint, in QuestionInput) (*models.Question, error) { + if in.SortOrder != nil && *in.SortOrder < 1 { + return nil, invalid([]string{"sort_order must be at least 1"}) + } + var q models.Question + err := lagoon.Transaction(ctx, db, func(ctx context.Context, tx *gorm.DB) error { + tx = tx.WithContext(ctx) + quizID, err := managedQuizID(tx, slug) + if err != nil { + return err + } + err = tx.Clauses(clause.Locking{Strength: "UPDATE", Table: clause.Table{Name: clause.CurrentTable}}). + Where("id = ? AND quiz_id = ?", id, quizID).Take(&q).Error + if errors.Is(err, gorm.ErrRecordNotFound) { + return ErrQuestionNotFound + } + if err != nil { + return err + } + if err := tx.Where("question_id = ?", q.ID).Order("sort_order ASC, id ASC").Find(&q.Answers).Error; err != nil { + return err + } + merged, problems := MergeQuestion(q, in) + if err := invalid(append(problems, CheckQuestion("question", merged)...)); err != nil { + return err + } + q.Text, q.Type, q.Category = merged.Text, merged.Type, merged.Category + q.AnswerText, q.Explanation, q.Points = merged.AnswerText, merged.Explanation, *merged.Points + if in.SortOrder != nil { + q.SortOrder = *in.SortOrder + } + if err := tx.Omit(clause.Associations).Save(&q).Error; err != nil { + return err + } + if in.Answers == nil { + return nil + } + current := map[uint]models.Answer{} + for _, a := range q.Answers { + current[a.ID] = a + } + answers := make([]models.Answer, 0, len(merged.Answers)) + keep := map[uint]bool{} + for i, a := range merged.Answers { + ans := models.Answer{QuestionID: q.ID} + if sent := (*in.Answers)[i]; sent.ID != nil { + ans = current[*sent.ID] + keep[ans.ID] = true + } + ans.Text, ans.IsCorrect, ans.Explanation, ans.SortOrder = a.Text, a.IsCorrect, a.Explanation, i+1 + if err := tx.Omit(clause.Associations).Save(&ans).Error; err != nil { + return err + } + answers = append(answers, ans) + } + for _, a := range q.Answers { + if !keep[a.ID] { + if err := tx.Where("id = ?", a.ID).Delete(&models.Answer{}).Error; err != nil { + return err + } + } + } + q.Answers = answers + return nil + }) + if err != nil { + return nil, err + } + return &q, nil +} + +// DeleteQuestion removes one question of the quiz with this slug. The +// foreign keys cascade to its answers and the attempt answers given to +// it; finished attempts keep their stored scores (D-16). +func DeleteQuestion(ctx context.Context, db *gorm.DB, slug string, id uint) error { + return lagoon.Transaction(ctx, db, func(ctx context.Context, tx *gorm.DB) error { + tx = tx.WithContext(ctx) + quizID, err := managedQuizID(tx, slug) + if err != nil { + return err + } + res := tx.Where("id = ? AND quiz_id = ?", id, quizID).Delete(&models.Question{}) + if res.Error != nil { + return res.Error + } + if res.RowsAffected == 0 { + return ErrQuestionNotFound + } + return nil + }) +} + +// CheckQuestionOrder is the check of a reorder list before any database +// access: not empty, no id twice. +func CheckQuestionOrder(ids []uint) []string { + seen := map[uint]bool{} + for _, id := range ids { + if seen[id] { + return []string{"question_ids must list every question of the quiz exactly once"} + } + seen[id] = true + } + if len(ids) == 0 { + return []string{"question_ids must list every question of the quiz exactly once"} + } + return nil +} + +// ReorderQuestions sets sort_order = index + 1 for ids, which must be +// exactly the quiz's question ids, and returns the managed quiz. +func ReorderQuestions(ctx context.Context, db *gorm.DB, slug string, ids []uint) (*models.Quiz, error) { + if err := invalid(CheckQuestionOrder(ids)); err != nil { + return nil, err + } + err := lagoon.Transaction(ctx, db, func(ctx context.Context, tx *gorm.DB) error { + tx = tx.WithContext(ctx) + quizID, err := managedQuizID(tx, slug) + if err != nil { + return err + } + var have []uint + if err := tx.Model(&models.Question{}).Where("quiz_id = ?", quizID).Pluck("id", &have).Error; err != nil { + return err + } + sorted := slices.Clone(ids) + slices.Sort(sorted) + slices.Sort(have) + if !slices.Equal(sorted, have) { + return invalid([]string{"question_ids must list every question of the quiz exactly once"}) + } + now := time.Now().UTC() + for i, id := range ids { + if err := tx.Model(&models.Question{}).Where("id = ?", id). + Updates(map[string]any{"sort_order": i + 1, "updated_at": now}).Error; err != nil { + return err + } + } + return nil + }) + if err != nil { + return nil, err + } + return ManagedQuiz(ctx, db, slug) +} diff --git a/classes/manage_test.go b/classes/manage_test.go index b556956..b1817e0 100644 --- a/classes/manage_test.go +++ b/classes/manage_test.go @@ -112,3 +112,74 @@ func TestBuildStats(t *testing.T) { t.Errorf("most_chosen_wrong by count = %+v, want 103", w) } } + +func uid(n uint) *uint { return &n } + +// TestQuestionInputRules: MergeQuestion applies a partial update and the +// answers list to the current question, and with CheckQuestion refuses +// what the importer would refuse. +func TestQuestionInputRules(t *testing.T) { + single := models.Question{ID: 1, Text: "Stolica?", Type: models.TypeSingle, Points: 2, Answers: []models.Answer{ + {ID: 10, QuestionID: 1, Text: "Warszawa", IsCorrect: true, Explanation: str("Od 1596.")}, + {ID: 11, QuestionID: 1, Text: "Kraków"}, + }} + card := models.Question{ID: 2, Text: "Odporność?", Type: models.TypeSelfCheck, AnswerText: str("Zdolność"), Points: 1} + check := func(current models.Question, in classes.QuestionInput) (classes.ImportQuestion, []string) { + merged, problems := classes.MergeQuestion(current, in) + return merged, append(problems, classes.CheckQuestion("question", merged)...) + } + + merged, problems := check(single, classes.QuestionInput{Text: str(" Stolica Polski? ")}) + if len(problems) != 0 || merged.Text != "Stolica Polski?" || *merged.Points != 2 || len(merged.Answers) != 2 || *merged.Answers[0].Explanation != "Od 1596." { + t.Errorf("text only: %+v %v", merged, problems) + } + answers := []classes.ManagedAnswerInput{{ID: uid(99), Text: "Gdańsk", IsCorrect: true}, {ID: uid(11), Text: "Kraków"}} + if _, problems := check(single, classes.QuestionInput{Answers: &answers}); !strings.Contains(strings.Join(problems, "; "), "answers[0].id does not belong to this question") { + t.Errorf("foreign answer id: %v", problems) + } + twice := []classes.ManagedAnswerInput{{ID: uid(10), Text: "Warszawa", IsCorrect: true}, {ID: uid(10), Text: "Kraków"}} + if _, problems := check(single, classes.QuestionInput{Answers: &twice}); !strings.Contains(strings.Join(problems, "; "), "listed twice") { + t.Errorf("answer id twice: %v", problems) + } + two := []classes.ManagedAnswerInput{{ID: uid(10), Text: "Warszawa", IsCorrect: true}, {ID: uid(11), Text: "Kraków", IsCorrect: true}} + if _, problems := check(single, classes.QuestionInput{Answers: &two}); !strings.Contains(strings.Join(problems, "; "), "exactly one correct") { + t.Errorf("single with two correct: %v", problems) + } + if _, problems := check(single, classes.QuestionInput{Type: str(models.TypeSelfCheck)}); !strings.Contains(strings.Join(problems, "; "), "self_check question has no answers") { + t.Errorf("type to self_check with answers left: %v", problems) + } + kept := []classes.ManagedAnswerInput{{ID: uid(10), Text: "Warszawa", IsCorrect: true}, {Text: "Gdańsk", Explanation: str("")}} + merged, problems = check(single, classes.QuestionInput{Answers: &kept, Explanation: str("")}) + if len(problems) != 0 || len(merged.Answers) != 2 || *merged.Answers[0].Explanation != "Od 1596." || merged.Answers[1].Explanation != nil || merged.Explanation != nil { + t.Errorf("kept and new answers: %+v %v", merged, problems) + } + toSingle := []classes.ManagedAnswerInput{{Text: "Tak", IsCorrect: true}, {Text: "Nie"}} + merged, problems = check(card, classes.QuestionInput{Type: str(models.TypeSingle), Answers: &toSingle, AnswerText: str("")}) + if len(problems) != 0 || merged.Type != models.TypeSingle || merged.AnswerText != nil { + t.Errorf("self_check to single: %+v %v", merged, problems) + } + if problems := classes.CheckNewQuestion(classes.QuestionInput{Text: str("Q")}); !strings.Contains(strings.Join(problems, "; "), "type is required") { + t.Errorf("create without type: %v", problems) + } + withID := []classes.ManagedAnswerInput{{ID: uid(10), Text: "a", IsCorrect: true}, {Text: "b"}} + if problems := classes.CheckNewQuestion(classes.QuestionInput{Text: str("Q"), Type: str("single"), Answers: &withID}); !strings.Contains(strings.Join(problems, "; "), "does not belong") { + t.Errorf("create with an answer id: %v", problems) + } + if problems := classes.CheckNewQuestion(classes.QuestionInput{Text: str("Q"), Type: str("self_check")}); len(problems) != 0 { + t.Errorf("minimal self_check create: %v", problems) + } +} + +// TestCheckQuestionOrder: an empty list and a repeated id are refused +// before the database; a missing or foreign id is refused by +// ReorderQuestions against the quiz's questions. +func TestCheckQuestionOrder(t *testing.T) { + for _, ids := range [][]uint{nil, {}, {1, 2, 1}} { + if problems := classes.CheckQuestionOrder(ids); len(problems) != 1 { + t.Errorf("%v: %v, want one problem", ids, problems) + } + } + if problems := classes.CheckQuestionOrder([]uint{3, 1, 2}); len(problems) != 0 { + t.Errorf("valid order: %v", problems) + } +} diff --git a/controllers/api/manage.go b/controllers/api/manage.go index dd2c89c..a7d002d 100644 --- a/controllers/api/manage.go +++ b/controllers/api/manage.go @@ -11,6 +11,7 @@ import ( "errors" "io" "net/http" + "strconv" "strings" "git.golem15.com/golem15/sm-quizzes-plugin/classes" @@ -43,6 +44,8 @@ func writeManaged(w http.ResponseWriter, status int, data any, err error) { wire.WriteJSON(w, status, map[string]any{"data": data}) case errors.As(err, &invalid): wire.WriteJSON(w, http.StatusUnprocessableEntity, map[string]any{"message": invalid.Error()}) + case errors.Is(err, classes.ErrQuestionNotFound): + wire.WriteJSON(w, http.StatusNotFound, map[string]any{"error": true, "message": "Question not found."}) case errors.Is(err, classes.ErrNotFound): wire.WriteJSON(w, http.StatusNotFound, map[string]any{"error": true, "message": "Quiz not found."}) case errors.Is(err, classes.ErrQuizExists): @@ -251,3 +254,105 @@ func (h *Handlers) ManageQuizStats(w http.ResponseWriter, r *http.Request) { } writeManaged(w, http.StatusOK, quizStats(stats), nil) } + +// questionID reads the {id} path value: anything but a positive integer +// is the 404 of a missing question. +func questionID(w http.ResponseWriter, r *http.Request) (uint, bool) { + id, err := strconv.ParseUint(r.PathValue("id"), 10, 32) + if err != nil || id == 0 { + writeManaged(w, 0, nil, classes.ErrQuestionNotFound) + return 0, false + } + return uint(id), true +} + +// ManageCreateQuestion answers POST /api/v1/quizzes/{slug}/questions: 201 +// with the new question and its answers. +func (h *Handlers) ManageCreateQuestion(w http.ResponseWriter, r *http.Request) { + var in classes.QuestionInput + if !readManaged(w, r, &in) || refuse(w, classes.CheckNewQuestion(in)) { + return + } + db, ok := h.manageDB(w) + if !ok { + return + } + q, err := classes.CreateQuestion(r.Context(), db, r.PathValue("slug"), in) + if err != nil { + writeManaged(w, 0, nil, err) + return + } + writeManaged(w, http.StatusCreated, managedQuestion(*q), nil) +} + +// ManageUpdateQuestion answers PATCH /api/v1/quizzes/{slug}/questions/{id}: +// only the sent fields change; a sent answers list replaces the answers by +// id. +func (h *Handlers) ManageUpdateQuestion(w http.ResponseWriter, r *http.Request) { + id, ok := questionID(w, r) + if !ok { + return + } + var in classes.QuestionInput + if !readManaged(w, r, &in) { + return + } + if in.SortOrder != nil && *in.SortOrder < 1 { + refuse(w, []string{"sort_order must be at least 1"}) + return + } + db, ok := h.manageDB(w) + if !ok { + return + } + q, err := classes.UpdateQuestion(r.Context(), db, r.PathValue("slug"), id, in) + if err != nil { + writeManaged(w, 0, nil, err) + return + } + writeManaged(w, http.StatusOK, managedQuestion(*q), nil) +} + +// ManageDeleteQuestion answers DELETE /api/v1/quizzes/{slug}/questions/{id} +// with {id, quiz_slug}. +func (h *Handlers) ManageDeleteQuestion(w http.ResponseWriter, r *http.Request) { + id, ok := questionID(w, r) + if !ok { + return + } + db, ok := h.manageDB(w) + if !ok { + return + } + slug := r.PathValue("slug") + if err := classes.DeleteQuestion(r.Context(), db, slug, id); err != nil { + writeManaged(w, 0, nil, err) + return + } + writeManaged(w, http.StatusOK, map[string]any{"id": id, "quiz_slug": slug}, nil) +} + +// reorderBody is the PUT /{slug}/questions/order body. +type reorderBody struct { + QuestionIDs []uint `json:"question_ids"` +} + +// ManageReorderQuestions answers PUT /api/v1/quizzes/{slug}/questions/order +// with {"question_ids": [...]}, every question of the quiz exactly once, +// with the quiz and its questions in the new order. +func (h *Handlers) ManageReorderQuestions(w http.ResponseWriter, r *http.Request) { + var body reorderBody + if !readManaged(w, r, &body) || refuse(w, classes.CheckQuestionOrder(body.QuestionIDs)) { + return + } + db, ok := h.manageDB(w) + if !ok { + return + } + slug := r.PathValue("slug") + if _, err := classes.ReorderQuestions(r.Context(), db, slug, body.QuestionIDs); err != nil { + writeManaged(w, 0, nil, err) + return + } + h.writeManagedQuiz(w, r, db, http.StatusOK, slug) +} diff --git a/controllers/api/manage_test.go b/controllers/api/manage_test.go index f005f1f..ea435a4 100644 --- a/controllers/api/manage_test.go +++ b/controllers/api/manage_test.go @@ -18,16 +18,20 @@ func TestManageHandlersWithoutDatabase(t *testing.T) { target string body string }{ - "list": {h.ManageListQuizzes, "GET", "/", ""}, - "show": {h.ManageShowQuiz, "GET", "/", ""}, - "create": {h.ManageCreateQuiz, "POST", "/", `{"title":"Pojęcia"}`}, - "update": {h.ManageUpdateQuiz, "PATCH", "/", `{"max_attempts":0}`}, - "delete": {h.ManageDeleteQuiz, "DELETE", "/?confirm=x", ""}, - "publish": {h.ManagePublish, "POST", "/", ""}, - "unpublish": {h.ManageUnpublish, "POST", "/", ""}, - "stats": {h.ManageQuizStats, "GET", "/", ""}, + "list": {h.ManageListQuizzes, "GET", "/", ""}, + "show": {h.ManageShowQuiz, "GET", "/", ""}, + "create": {h.ManageCreateQuiz, "POST", "/", `{"title":"Pojęcia"}`}, + "update": {h.ManageUpdateQuiz, "PATCH", "/", `{"max_attempts":0}`}, + "delete": {h.ManageDeleteQuiz, "DELETE", "/?confirm=x", ""}, + "publish": {h.ManagePublish, "POST", "/", ""}, + "unpublish": {h.ManageUnpublish, "POST", "/", ""}, + "stats": {h.ManageQuizStats, "GET", "/", ""}, + "create question": {h.ManageCreateQuestion, "POST", "/", `{"text":"Q","type":"self_check"}`}, + "update question": {h.ManageUpdateQuestion, "PATCH", "/", `{"text":"Q"}`}, + "delete question": {h.ManageDeleteQuestion, "DELETE", "/", ""}, + "reorder": {h.ManageReorderQuestions, "PUT", "/", `{"question_ids":[2,1]}`}, } { - code, _ := call(t, tc.fn, tc.method, tc.target, tc.body, 7, map[string]string{"slug": "x"}) + code, _ := call(t, tc.fn, tc.method, tc.target, tc.body, 7, map[string]string{"slug": "x", "id": "3"}) if code != http.StatusServiceUnavailable { t.Errorf("%s without a database = %d, want 503", name, code) } @@ -56,10 +60,24 @@ func TestManageWritesRefuseBadInputBeforeDatabase(t *testing.T) { "delete without confirm": {h.ManageDeleteQuiz, "DELETE", "/", "", "confirm must repeat the quiz slug"}, "delete wrong confirm": {h.ManageDeleteQuiz, "DELETE", "/?confirm=y", "", "confirm must repeat the quiz slug"}, } { - code, out := call(t, tc.fn, tc.method, tc.target, tc.body, 7, map[string]string{"slug": "x"}) + code, out := call(t, tc.fn, tc.method, tc.target, tc.body, 7, map[string]string{"slug": "x", "id": "3"}) msg, _ := out["message"].(string) if code != http.StatusUnprocessableEntity || !strings.Contains(msg, tc.want) { t.Errorf("%s = %d %v, want 422 with %q", name, code, out, tc.want) } } } + +// TestManageQuestionIDs: a question id that is not a positive integer is +// the 404 of a missing question, before the database. +func TestManageQuestionIDs(t *testing.T) { + h := New(backpack.New(nil)) + for _, id := range []string{"", "0", "abc", "-1"} { + for name, fn := range map[string]http.HandlerFunc{"update": h.ManageUpdateQuestion, "delete": h.ManageDeleteQuestion} { + code, out := call(t, fn, "PATCH", "/", `{}`, 7, map[string]string{"slug": "x", "id": id}) + if code != http.StatusNotFound || out["message"] != "Question not found." { + t.Errorf("%s id %q = %d %v, want 404 Question not found.", name, id, code, out) + } + } + } +} diff --git a/manage_api_postgres_test.go b/manage_api_postgres_test.go index 5cec24a..28a0123 100644 --- a/manage_api_postgres_test.go +++ b/manage_api_postgres_test.go @@ -191,6 +191,88 @@ VALUES ('Editors', 'editors', '{"golem15.quizzes.access_quizzes":1}', NOW(), NOW } }) + t.Run("questions", func(t *testing.T) { + const qp = prefix + "/pytania/questions" + c.ok(http.StatusCreated, "POST", prefix, token, `{"title":"Pytania"}`) + single := data(c.ok(http.StatusCreated, "POST", qp, token, + `{"text":"Stolica?","type":"single","points":2,"answers":[{"text":"Warszawa","is_correct":true},{"text":"Kraków"},{"text":"Gdańsk"}]}`)) + card := data(c.ok(http.StatusCreated, "POST", qp, token, `{"text":"Odporność?","type":"self_check","answer_text":"Zdolność"}`)) + if single["sort_order"] != float64(1) || card["sort_order"] != float64(2) || len(single["answers"].([]any)) != 3 { + t.Fatalf("created questions = %v %v", single, card) + } + if code, _ := c.do("POST", qp, token, `{"text":"Zła","type":"single","answers":[{"text":"a"},{"text":"b"}]}`); code != http.StatusUnprocessableEntity { + t.Errorf("single without a correct answer = %d, want 422", code) + } + singleID := uint(single["id"].(float64)) + cardID := uint(card["id"].(float64)) + ans := single["answers"].([]any) + warszawa := uint(ans[0].(map[string]any)["id"].(float64)) + krakow := uint(ans[1].(map[string]any)["id"].(float64)) + gdansk := uint(ans[2].(map[string]any)["id"].(float64)) + + // A student finishes an attempt with the right answer. + c.ok(http.StatusOK, "POST", prefix+"/pytania/publish", token, "") + _, jwt := newUser(t, gdb, "student@example.com") + att := data(c.ok(http.StatusCreated, "POST", apiPrefix+"/quizzes/pytania/attempts", jwt, "")) + attPath := fmt.Sprintf("%s/attempts/%d", apiPrefix, int(att["id"].(float64))) + c.ok(http.StatusOK, "POST", attPath+"/answers", jwt, fmt.Sprintf(`{"question_id":%d,"answer_ids":[%d]}`, singleID, warszawa)) + c.ok(http.StatusOK, "POST", attPath+"/answers", jwt, fmt.Sprintf(`{"question_id":%d,"known":true}`, cardID)) + c.ok(http.StatusOK, "POST", attPath+"/finish", jwt, "") + var before models.Attempt + must(t, gdb.Take(&before, int(att["id"].(float64))).Error) + + // Kraków becomes the correct answer: Warszawa is kept (same id), + // Gdańsk is deleted, Poznań is created. + body := fmt.Sprintf(`{"text":"Stolica Małopolski?","answers":[{"id":%d,"text":"Kraków","is_correct":true},{"id":%d,"text":"Warszawa"},{"text":"Poznań"}]}`, krakow, warszawa) + upd := data(c.ok(http.StatusOK, "PATCH", fmt.Sprintf("%s/%d", qp, singleID), token, body)) + got := upd["answers"].([]any) + if len(got) != 3 || got[0].(map[string]any)["id"] != float64(krakow) || got[1].(map[string]any)["id"] != float64(warszawa) || got[2].(map[string]any)["id"] == float64(gdansk) { + t.Errorf("updated answers = %v", got) + } + var n int64 + must(t, gdb.Model(&models.Answer{}).Where("id = ?", gdansk).Count(&n).Error) + if n != 0 { + t.Errorf("Gdańsk was not deleted") + } + var after models.Attempt + must(t, gdb.Take(&after, before.ID).Error) + if after.ScorePoints != before.ScorePoints || *after.ScorePercent != *before.ScorePercent { + t.Errorf("attempt score changed by an edit: %v -> %v", before.ScorePoints, after.ScorePoints) + } + foreign := fmt.Sprintf(`{"answers":[{"id":%d,"text":"x","is_correct":true},{"text":"y"}]}`, gdansk) + if code, _ := c.do("PATCH", fmt.Sprintf("%s/%d", qp, singleID), token, foreign); code != http.StatusUnprocessableEntity { + t.Errorf("foreign answer id = %d, want 422", code) + } + if code, _ := c.do("PATCH", fmt.Sprintf("%s/%d", qp, 999999), token, `{"text":"x"}`); code != http.StatusNotFound { + t.Errorf("missing question = %d, want 404", code) + } + + // Reorder refuses a partial list and sets the order. + if code, _ := c.do("PUT", qp+"/order", token, fmt.Sprintf(`{"question_ids":[%d]}`, cardID)); code != http.StatusUnprocessableEntity { + t.Errorf("partial reorder = %d, want 422", code) + } + out := data(c.ok(http.StatusOK, "PUT", qp+"/order", token, fmt.Sprintf(`{"question_ids":[%d,%d]}`, cardID, singleID))) + qs := out["questions"].([]any) + if qs[0].(map[string]any)["id"] != float64(cardID) || qs[0].(map[string]any)["sort_order"] != float64(1) { + t.Errorf("reordered = %v", qs) + } + + // Deleting a question removes the attempt answers given to it; the + // attempt keeps its score. + del := data(c.ok(http.StatusOK, "DELETE", fmt.Sprintf("%s/%d", qp, cardID), token, "")) + if del["quiz_slug"] != "pytania" { + t.Errorf("delete question = %v", del) + } + must(t, gdb.Model(&models.AttemptAnswer{}).Where("question_id = ?", cardID).Count(&n).Error) + if n != 0 { + t.Errorf("attempt answers of the deleted question = %d", n) + } + must(t, gdb.Take(&after, before.ID).Error) + if after.ScorePoints != before.ScorePoints { + t.Errorf("attempt score changed by a delete: %v -> %v", before.ScorePoints, after.ScorePoints) + } + }) + t.Run("import still works", func(t *testing.T) { raw, err := os.ReadFile("testdata/quiz.json") must(t, err) diff --git a/mcp_quizzes.go b/mcp_quizzes.go index 0dfaf66..47617d6 100644 --- a/mcp_quizzes.go +++ b/mcp_quizzes.go @@ -114,6 +114,53 @@ type importAnswerInput struct { Explanation *string `json:"explanation,omitempty" jsonschema:"shown with the solution"` } +// answerInput is one answer of quizzes_update_question's answers list. +type answerInput struct { + ID *uint `json:"id,omitempty" jsonschema:"the id of an existing answer to keep; leave out to create one"` + Text string `json:"text" jsonschema:"the answer text"` + IsCorrect bool `json:"is_correct,omitempty" jsonschema:"true for a correct answer"` + Explanation *string `json:"explanation,omitempty" jsonschema:"shown with the solution; left out keeps a kept answer's explanation, an empty string clears it"` +} + +// createQuestionInput is the input of quizzes_create_question. +type createQuestionInput struct { + Slug string `json:"slug" path:"slug" jsonschema:"the quiz slug"` + Text string `json:"text" jsonschema:"the question"` + Type string `json:"type" jsonschema:"single (exactly one correct answer), multiple (one or more correct) or self_check (a flashcard with answer_text and no answers)"` + Category *string `json:"category,omitempty" jsonschema:"a category for learning rounds"` + AnswerText *string `json:"answer_text,omitempty" jsonschema:"the answer a self_check card reveals"` + Explanation *string `json:"explanation,omitempty" jsonschema:"shown with the solution"` + Points *int `json:"points,omitempty" jsonschema:"points, zero or more; default 1"` + SortOrder *int `json:"sort_order,omitempty" jsonschema:"position, at least 1; default: after the last question"` + Answers []importAnswerInput `json:"answers,omitempty" jsonschema:"the choices of a single or multiple question in order, at least two"` +} + +// updateQuestionInput is the input of quizzes_update_question. +type updateQuestionInput struct { + Slug string `json:"slug" path:"slug" jsonschema:"the quiz slug"` + ID uint `json:"id" path:"id" jsonschema:"the question id, as listed by quizzes_show_quiz"` + Text *string `json:"text,omitempty" jsonschema:"the new question text"` + Type *string `json:"type,omitempty" jsonschema:"single, multiple or self_check"` + Category *string `json:"category,omitempty" jsonschema:"the new category; an empty string clears it"` + AnswerText *string `json:"answer_text,omitempty" jsonschema:"the new self_check answer; an empty string clears it"` + Explanation *string `json:"explanation,omitempty" jsonschema:"the new explanation; an empty string clears it"` + Points *int `json:"points,omitempty" jsonschema:"points, zero or more"` + SortOrder *int `json:"sort_order,omitempty" jsonschema:"position, at least 1"` + Answers *[]answerInput `json:"answers,omitempty" jsonschema:"the full new answer list in order: answers with an id are kept and updated, answers without an id are created, answers missing from the list are deleted; leave out to keep the answers"` +} + +// deleteQuestionInput is the input of quizzes_delete_question. +type deleteQuestionInput struct { + Slug string `json:"slug" path:"slug" jsonschema:"the quiz slug"` + ID uint `json:"id" path:"id" jsonschema:"the question id"` +} + +// reorderInput is the input of quizzes_reorder_questions. +type reorderInput struct { + Slug string `json:"slug" path:"slug" jsonschema:"the quiz slug"` + QuestionIDs []uint `json:"question_ids" jsonschema:"every question id of the quiz exactly once, in the new order"` +} + func quizTools() []pact.MCPTool { return []pact.MCPTool{ { @@ -201,5 +248,42 @@ func quizTools() []pact.MCPTool { Input: statsInput{}, Permissions: quizPermissions, }, + { + Name: "create_question", + Title: "Add a question", + Description: "Adds a question to a quiz: text and type (single, multiple or self_check) are required; category, answer_text (the self_check answer), explanation, points (default 1), sort_order (default: after the last question) and answers [{text, is_correct, explanation}] are optional. A single question needs at least two answers and exactly one correct, a multiple question at least two and one or more correct, a self_check question none. Returns the question with its answer ids." + needsWrite, + Method: http.MethodPost, + Path: ManagePrefix + "/{slug}/questions", + Input: createQuestionInput{}, + Permissions: quizPermissions, + }, + { + Name: "update_question", + Title: "Change one question", + Description: "Changes only the fields you send on one question: text, type, category, answer_text, explanation, points, sort_order; an empty string clears category, answer_text and explanation. answers, when sent, is the full new list: answers with an id are kept and updated (they keep their ids), answers without an id are created, answers missing from the list are deleted; an id of another question is refused. The changed question must pass the same rules as quizzes_create_question. Editing a question of a quiz with attempts is allowed: finished attempts keep their scores and their reviews show the current question." + needsWrite, + Method: http.MethodPatch, + Path: ManagePrefix + "/{slug}/questions/{id}", + Input: updateQuestionInput{}, + Permissions: quizPermissions, + }, + { + Name: "delete_question", + Title: "Delete a question", + Description: "Deletes one question for good, with its answers and the attempt answers students gave to it. Finished attempts keep their scores, but their reviews lose the question. Returns id and quiz_slug." + needsWrite, + Method: http.MethodDelete, + Path: ManagePrefix + "/{slug}/questions/{id}", + Input: deleteQuestionInput{}, + Permissions: quizPermissions, + Destructive: &yes, + }, + { + Name: "reorder_questions", + Title: "Reorder questions", + Description: "Sets the question order of a quiz: question_ids must list every question of the quiz exactly once, or the call is refused; the first id gets sort_order 1. Returns the quiz with its questions as quizzes_show_quiz does." + needsWrite, + Method: http.MethodPut, + Path: ManagePrefix + "/{slug}/questions/order", + Input: reorderInput{}, + Permissions: quizPermissions, + }, } } diff --git a/mcp_test.go b/mcp_test.go index 62606bc..a27c116 100644 --- a/mcp_test.go +++ b/mcp_test.go @@ -75,14 +75,18 @@ func mcpTestApp(t *testing.T) (*backpack.App, []party.Plugin) { // pinnedTools is the sorted tool list of golem15.user plus this plugin. var pinnedTools = []string{ + "quizzes_create_question", "quizzes_create_quiz", + "quizzes_delete_question", "quizzes_delete_quiz", "quizzes_import_quiz", "quizzes_list_quizzes", "quizzes_publish", "quizzes_quiz_stats", + "quizzes_reorder_questions", "quizzes_show_quiz", "quizzes_unpublish", + "quizzes_update_question", "quizzes_update_quiz", "user_me", } diff --git a/plugin_test.go b/plugin_test.go index 333d61e..7d2a50b 100644 --- a/plugin_test.go +++ b/plugin_test.go @@ -77,6 +77,10 @@ func TestRoutesRegistered(t *testing.T) { "POST /api/v1/quizzes/{slug}/publish", "POST /api/v1/quizzes/{slug}/unpublish", "GET /api/v1/quizzes/{slug}/stats", + "POST /api/v1/quizzes/{slug}/questions", + "PATCH /api/v1/quizzes/{slug}/questions/{id}", + "DELETE /api/v1/quizzes/{slug}/questions/{id}", + "PUT /api/v1/quizzes/{slug}/questions/order", } var got []string for _, r := range router.Routes() { diff --git a/routes.go b/routes.go index 853feaf..79702cb 100644 --- a/routes.go +++ b/routes.go @@ -64,6 +64,16 @@ func (p *Plugin) Routes(r pact.Router) error { g.Where("slug", `[a-z0-9-]+`) g.Get("/{slug}/stats", h.ManageQuizStats, "user.scope:read", perm) g.Where("slug", `[a-z0-9-]+`) + g.Post("/{slug}/questions", h.ManageCreateQuestion, "user.scope:write", perm) + g.Where("slug", `[a-z0-9-]+`) + g.Patch("/{slug}/questions/{id}", h.ManageUpdateQuestion, "user.scope:write", perm) + g.Where("slug", `[a-z0-9-]+`) + g.Where("id", "[0-9]+") + g.Delete("/{slug}/questions/{id}", h.ManageDeleteQuestion, "user.scope:write", perm) + g.Where("slug", `[a-z0-9-]+`) + g.Where("id", "[0-9]+") + g.Put("/{slug}/questions/order", h.ManageReorderQuestions, "user.scope:write", perm) + g.Where("slug", `[a-z0-9-]+`) }) return nil }