diff --git a/backend/alembic/versions/c3d4e5f6a7b8_question_feedback.py b/backend/alembic/versions/c3d4e5f6a7b8_question_feedback.py new file mode 100644 index 0000000..c21d3fd --- /dev/null +++ b/backend/alembic/versions/c3d4e5f6a7b8_question_feedback.py @@ -0,0 +1,44 @@ +"""Feedback on a question, replacing the comment thread + +A comment thread under every question is a discussion that needs moderating. +What people used it for was telling an educator something was wrong, which is a +private report someone is meant to act on. This is that. + +Revision ID: c3d4e5f6a7b8 +Revises: b2c3d4e5f6a7 +""" +import sqlalchemy as sa +from alembic import op + +revision = "c3d4e5f6a7b8" +down_revision = "b2c3d4e5f6a7" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + # Guarded: create_all() may have built it already on a fresh deploy. + if "question_feedback" in sa.inspect(op.get_bind()).get_table_names(): + return + op.create_table( + "question_feedback", + sa.Column("id", sa.Integer(), primary_key=True, index=True), + sa.Column("question_id", sa.Integer(), + sa.ForeignKey("questions.id", ondelete="CASCADE"), nullable=False), + sa.Column("user_id", sa.Integer(), + sa.ForeignKey("users.id", ondelete="SET NULL"), nullable=True), + sa.Column("message", sa.Text(), nullable=False), + sa.Column("status", sa.String(length=20), nullable=False, server_default="open"), + sa.Column("reply", sa.Text(), nullable=True), + sa.Column("replied_by", sa.Integer(), + sa.ForeignKey("users.id", ondelete="SET NULL"), nullable=True), + sa.Column("replied_at", sa.DateTime(), nullable=True), + sa.Column("created_at", sa.DateTime(), server_default=sa.func.now()), + ) + op.create_index("ix_question_feedback_question_id", "question_feedback", ["question_id"]) + op.create_index("ix_question_feedback_status", "question_feedback", ["status"]) + + +def downgrade() -> None: + if "question_feedback" in sa.inspect(op.get_bind()).get_table_names(): + op.drop_table("question_feedback") diff --git a/backend/app/main.py b/backend/app/main.py index 01bc441..b5cd3c2 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -12,6 +12,7 @@ setup_logging(settings.LOG_LEVEL) from app.database import engine, Base, SessionLocal from app.routers import auth, documents, quizzes, attempts, admin, tts, nextcloud, categories, questions, question_categories, favorites, teach, contact, tags, flashcards, courses, mobile, mynote, exams from app.routers import access +from app.routers import feedback from app.routers import study_tools, uploads, articles, comments, share, collections, study_plans, media, search, ai_mode from app.utils.auth import get_password_hash @@ -614,6 +615,7 @@ app.include_router(uploads.router) app.include_router(auth.router, prefix="/api/auth", tags=["auth"]) app.include_router(articles.router, prefix="/api/articles", tags=["articles"]) app.include_router(access.router, prefix="/api/access", tags=["access"]) +app.include_router(feedback.router, prefix="/api/feedback", tags=["feedback"]) app.include_router(exams.router, prefix="/api/exams", tags=["exams"]) app.include_router(study_plans.router, prefix="/api/study-plans", tags=["study-plans"]) app.include_router(media.router, prefix="/api/media", tags=["media"]) diff --git a/backend/app/models/__init__.py b/backend/app/models/__init__.py index ce0a2b9..d65f8ca 100644 --- a/backend/app/models/__init__.py +++ b/backend/app/models/__init__.py @@ -40,3 +40,5 @@ __all__ = [ "UserCollection", "UserCollectionQuestion", ] + +from app.models.feedback import QuestionFeedback # noqa: F401 diff --git a/backend/app/models/feedback.py b/backend/app/models/feedback.py new file mode 100644 index 0000000..2e2bda3 --- /dev/null +++ b/backend/app/models/feedback.py @@ -0,0 +1,30 @@ +from datetime import datetime + +from sqlalchemy import Column, DateTime, ForeignKey, Integer, String, Text + +from app.database import Base + + +class QuestionFeedback(Base): + """A learner telling an educator something is wrong with a question. + + It replaces the comment thread that used to sit under every question. A + discussion is public and needs moderating; this is a private report that + someone is expected to act on, which is what people were using comments for + anyway. It carries the question id because that is what an educator needs + to find the thing being reported. + """ + + __tablename__ = "question_feedback" + + id = Column(Integer, primary_key=True, index=True) + question_id = Column(Integer, ForeignKey("questions.id", ondelete="CASCADE"), nullable=False, index=True) + user_id = Column(Integer, ForeignKey("users.id", ondelete="SET NULL"), nullable=True, index=True) + message = Column(Text, nullable=False) + #: open | resolved. Kept rather than deleted on resolve, so a question with + #: a history of the same complaint is visibly that. + status = Column(String(20), nullable=False, default="open", index=True) + reply = Column(Text, nullable=True) + replied_by = Column(Integer, ForeignKey("users.id", ondelete="SET NULL"), nullable=True) + replied_at = Column(DateTime, nullable=True) + created_at = Column(DateTime, default=datetime.utcnow, index=True) diff --git a/backend/app/routers/attempts.py b/backend/app/routers/attempts.py index 0387620..9f94333 100644 --- a/backend/app/routers/attempts.py +++ b/backend/app/routers/attempts.py @@ -9,7 +9,7 @@ from pydantic import BaseModel from sqlalchemy.orm import Session from sqlalchemy import case, func -from app.services.attempt_expiry import active_key, progress_key, settle_if_expired +from app.services.attempt_expiry import active_key, load_saved, progress_key, settle_if_expired from app.services.question_figures import figures_for_questions from app.services.study_plan_context import mark_block_complete, plan_context_for_quizzes, unmark_block_complete from app.database import get_db @@ -768,6 +768,50 @@ def quiz_analysis( } +class _LiveAnswer: + """An answer that has been given but not yet submitted. + + Shaped like an AttemptAnswer so the analysis does not have to care which of + the two it is reading. Not persisted: submitting is what writes answers, + and reading a page must not. + """ + + __slots__ = ("question_id", "user_answer", "is_correct", "seconds_spent") + + def __init__(self, question_id, user_answer, is_correct): + self.question_id = question_id + self.user_answer = user_answer + self.is_correct = is_correct + # Per-question timing is submitted with the attempt, so a live session + # has none yet. None, not zero — those are different claims. + self.seconds_spent = None + + +def _rows_from_progress(db: Session, user_id: int, attempt: QuizAttempt) -> list: + """Grade what is saved for a live attempt, so it can be analysed mid-session.""" + try: + import redis as redis_lib + + from app.config import settings + r = redis_lib.from_url(settings.REDIS_URL, decode_responses=True) + saved = load_saved(r, user_id, attempt.id) + except Exception: + logger.warning("Redis unavailable for live analysis of attempt %s", attempt.id, exc_info=True) + return [] + answers = (saved or {}).get("answers") or {} + if not answers: + return [] + graded = grade_quiz_answers( + get_quiz_questions(db, attempt.quiz_id), + [(int(qid), value) for qid, value in answers.items()], + attempt.selected_question_ids, + ) + # Every question, not only the answered ones: the unanswered are what make + # the figure read "1 of 34" and the table show the rest as skipped, exactly + # as a finished session does. + return [_LiveAnswer(question.id, answer, correct) for question, answer, correct in graded] + + @router.get("/{attempt_id}/analysis") def attempt_analysis( attempt_id: int, @@ -788,6 +832,12 @@ def attempt_analysis( quiz = db.get(Quiz, attempt.quiz_id) rows = db.query(AttemptAnswer).filter(AttemptAnswer.attempt_id == attempt_id).all() + # A session still in progress has no AttemptAnswer rows — they are written + # on submit — so the analysis of one read as entirely empty while the + # session list, which counts the saved progress, said a question had been + # answered. The two are now looking at the same thing. + if not rows and attempt.completed_at is None: + rows = _rows_from_progress(db, current_user.id, attempt) question_ids = [row.question_id for row in rows] questions = {q.id: q for q in db.query(Question).filter(Question.id.in_(question_ids)).all()} \ if question_ids else {} diff --git a/backend/app/routers/feedback.py b/backend/app/routers/feedback.py new file mode 100644 index 0000000..819670a --- /dev/null +++ b/backend/app/routers/feedback.py @@ -0,0 +1,155 @@ +"""Feedback on a question: reported by a learner, worked through by an educator. + +The comment thread that used to sit under every question was a discussion +nobody moderated. What it was actually used for was telling an educator +something was wrong — a private report someone is meant to act on — so that is +what this is. + +An educator sees what is outstanding, which question each report is about, and +can reply, resolve or delete it. Resolving keeps the report: a question with a +history of the same complaint should visibly have one. +""" +from datetime import datetime + +from fastapi import APIRouter, Depends, HTTPException, Query +from pydantic import BaseModel, Field +from sqlalchemy import func +from sqlalchemy.orm import Session + +from app.database import get_db +from app.models.feedback import QuestionFeedback +from app.models.question import Question +from app.models.user import User +from app.utils.auth import get_current_user +from app.utils.category_grants import is_question_manager, manageable_categories + +router = APIRouter() + +MAX_MESSAGE = 2000 + + +class FeedbackIn(BaseModel): + message: str = Field(min_length=3, max_length=MAX_MESSAGE) + + +class ReplyIn(BaseModel): + reply: str | None = Field(default=None, max_length=MAX_MESSAGE) + status: str | None = None + + +def _json(row: QuestionFeedback, senders: dict[int, User], questions: dict[int, Question]) -> dict: + sender = senders.get(row.user_id) + question = questions.get(row.question_id) + return { + "id": row.id, + "question_id": row.question_id, + # The stem, trimmed: an educator scanning a list needs to recognise the + # question, not read it. + "question_excerpt": (getattr(question, "question_text", "") or "")[:120], + "message": row.message, + "status": row.status, + "reply": row.reply, + "replied_at": row.replied_at, + "created_at": row.created_at, + "from_name": getattr(sender, "name", None) or "A learner", + "from_email": getattr(sender, "email", None), + } + + +def _decorate(db: Session, rows: list[QuestionFeedback]) -> list[dict]: + if not rows: + return [] + senders = {u.id: u for u in db.query(User).filter( + User.id.in_({r.user_id for r in rows if r.user_id}))} if any(r.user_id for r in rows) else {} + questions = {q.id: q for q in db.query(Question).filter( + Question.id.in_({r.question_id for r in rows}))} + return [_json(row, senders, questions) for row in rows] + + +def _require_manager(db: Session, user: User) -> None: + if not is_question_manager(db, user): + raise HTTPException(403, "Reviewing feedback requires moderator access or a grant") + + +def _visible(db: Session, user: User, query): + """Narrow to the questions this educator may manage. None means all.""" + scope = manageable_categories(db, user) + if scope is None: + return query + if not scope: + return query.filter(QuestionFeedback.id.is_(None)) + return query.join(Question, Question.id == QuestionFeedback.question_id).filter( + Question.question_category_id.in_(scope)) + + +@router.post("/questions/{question_id}", status_code=201) +def leave_feedback(question_id: int, data: FeedbackIn, db: Session = Depends(get_db), + current_user: User = Depends(get_current_user)): + """Report something about a question. Any signed-in learner may.""" + if not db.query(Question.id).filter(Question.id == question_id).first(): + raise HTTPException(404, "Question not found") + row = QuestionFeedback(question_id=question_id, user_id=current_user.id, + message=data.message.strip()) + db.add(row) + db.commit() + return {"sent": True, "id": row.id} + + +@router.get("/questions/{question_id}") +def feedback_for_question(question_id: int, db: Session = Depends(get_db), + current_user: User = Depends(get_current_user)): + """Every report about one question, for the educator editing it.""" + _require_manager(db, current_user) + rows = (db.query(QuestionFeedback) + .filter(QuestionFeedback.question_id == question_id) + .order_by(QuestionFeedback.status.desc(), QuestionFeedback.created_at.desc()) + .all()) + return _decorate(db, rows) + + +@router.get("/open") +def open_feedback(limit: int = Query(8, ge=1, le=50), db: Session = Depends(get_db), + current_user: User = Depends(get_current_user)): + """What is outstanding — the count for a badge, and the newest few. + + Answers quietly for someone with no editorial access rather than refusing, + so the header can ask for it without first asking who is asking. + """ + if not is_question_manager(db, current_user): + return {"open": 0, "items": []} + base = _visible(db, current_user, + db.query(QuestionFeedback).filter(QuestionFeedback.status == "open")) + total = base.with_entities(func.count(QuestionFeedback.id)).scalar() or 0 + rows = base.order_by(QuestionFeedback.created_at.desc()).limit(limit).all() + return {"open": int(total), "items": _decorate(db, rows)} + + +@router.patch("/{feedback_id}") +def answer_feedback(feedback_id: int, data: ReplyIn, db: Session = Depends(get_db), + current_user: User = Depends(get_current_user)): + """Reply to a report, resolve it, or reopen it.""" + _require_manager(db, current_user) + row = db.get(QuestionFeedback, feedback_id) + if not row: + raise HTTPException(404, "Feedback not found") + if data.status is not None: + if data.status not in ("open", "resolved"): + raise HTTPException(400, "status must be open or resolved") + row.status = data.status + if data.reply is not None: + row.reply = data.reply.strip() or None + row.replied_by = current_user.id + row.replied_at = datetime.utcnow() + db.commit() + return _decorate(db, [row])[0] + + +@router.delete("/{feedback_id}", status_code=204) +def delete_feedback(feedback_id: int, db: Session = Depends(get_db), + current_user: User = Depends(get_current_user)): + _require_manager(db, current_user) + row = db.get(QuestionFeedback, feedback_id) + if not row: + raise HTTPException(404, "Feedback not found") + db.delete(row) + db.commit() diff --git a/backend/tests/test_feedback.py b/backend/tests/test_feedback.py new file mode 100644 index 0000000..0b3523e --- /dev/null +++ b/backend/tests/test_feedback.py @@ -0,0 +1,114 @@ +"""Feedback on a question: reported by a learner, worked through by an educator. + +Disposable SQLite. The rules worth pinning: any learner may report, only +someone with editorial access may read or answer, resolving keeps the report, +and the badge answers quietly for someone with no access rather than refusing. +""" +import unittest + +import test_quiz_builder as fixtures +from app.models.category_grant import CategoryGrant +from app.models.feedback import QuestionFeedback +from app.routers import feedback + + +class FeedbackTests(unittest.TestCase): + def setUp(self): + self.bank = fixtures.BuilderTests() + self.bank.setUp() + self.client = self.bank.client + self.db = self.bank.db + self.client.app.include_router(feedback.router, prefix="/feedback") + self.bank.user = self.bank.owner + + def tearDown(self): + self.bank.tearDown() + + def send(self, question_id=1, message="The answer key looks wrong."): + return self.client.post(f"/feedback/questions/{question_id}", json={"message": message}) + + def test_any_learner_may_report_and_it_names_the_question(self): + self.bank.user = self.bank.peer + response = self.send() + self.assertEqual(response.status_code, 201, response.text) + row = self.db.query(QuestionFeedback).one() + self.assertEqual((row.question_id, row.user_id, row.status), (1, self.bank.peer.id, "open")) + + def test_a_report_about_nothing_is_refused(self): + self.assertEqual(self.send(question_id=9999).status_code, 404) + self.assertEqual(self.client.post("/feedback/questions/1", json={"message": "no"}).status_code, 422) + + def test_only_an_educator_may_read_or_answer(self): + self.send() + row = self.db.query(QuestionFeedback).one() + self.bank.user = self.bank.peer + self.assertEqual(self.client.get("/feedback/questions/1").status_code, 403) + self.assertEqual(self.client.patch(f"/feedback/{row.id}", json={"status": "resolved"}).status_code, 403) + self.assertEqual(self.client.delete(f"/feedback/{row.id}").status_code, 403) + + def test_the_badge_answers_quietly_for_someone_with_no_access(self): + self.send() + self.bank.user = self.bank.peer + body = self.client.get("/feedback/open").json() + # Not a 403: the header asks for this without first asking who is asking. + self.assertEqual(body, {"open": 0, "items": []}) + + def test_an_educator_sees_what_is_outstanding_with_the_question_it_is_about(self): + self.send() + self.bank.user = self.bank.mod + body = self.client.get("/feedback/open").json() + self.assertEqual(body["open"], 1) + self.assertEqual(body["items"][0]["question_id"], 1) + self.assertTrue(body["items"][0]["question_excerpt"]) + self.assertEqual(body["items"][0]["from_name"], self.bank.owner.name) + + def test_replying_resolves_and_keeps_the_report(self): + self.send() + row_id = self.db.query(QuestionFeedback).one().id + self.bank.user = self.bank.mod + answered = self.client.patch(f"/feedback/{row_id}", + json={"reply": "Fixed, thank you.", "status": "resolved"}) + self.assertEqual(answered.status_code, 200, answered.text) + self.assertEqual(answered.json()["reply"], "Fixed, thank you.") + # Kept, not deleted: a question with a history of the same complaint + # should visibly have one. + self.db.expire_all() + self.assertEqual(self.db.query(QuestionFeedback).count(), 1) + self.assertEqual(self.client.get("/feedback/open").json()["open"], 0) + + def test_a_resolved_report_can_be_reopened(self): + self.send() + row_id = self.db.query(QuestionFeedback).one().id + self.bank.user = self.bank.mod + self.client.patch(f"/feedback/{row_id}", json={"status": "resolved"}) + self.client.patch(f"/feedback/{row_id}", json={"status": "open"}) + self.assertEqual(self.client.get("/feedback/open").json()["open"], 1) + self.assertEqual(self.client.patch(f"/feedback/{row_id}", json={"status": "maybe"}).status_code, 400) + + def test_deleting_removes_it(self): + self.send() + row_id = self.db.query(QuestionFeedback).one().id + self.bank.user = self.bank.mod + self.assertEqual(self.client.delete(f"/feedback/{row_id}").status_code, 204) + self.assertEqual(self.db.query(QuestionFeedback).count(), 0) + + def test_a_granted_educator_sees_only_their_own_branch(self): + # Questions 1 and 2 sit under the Root branch; 6 is filed nowhere, so + # no grant over the tree can reach it. + self.send(question_id=1) + self.send(question_id=6) + self.db.add(CategoryGrant(category_id=1, user_id=self.bank.peer.id)) + self.db.commit() + + self.bank.user = self.bank.peer + body = self.client.get("/feedback/open").json() + self.assertEqual(body["open"], 1) + self.assertEqual(body["items"][0]["question_id"], 1) + + # A moderator sees both, including the one filed nowhere. + self.bank.user = self.bank.mod + self.assertEqual(self.client.get("/feedback/open").json()["open"], 2) + + +if __name__ == "__main__": + unittest.main() diff --git a/backend/tests/test_session_lifecycle.py b/backend/tests/test_session_lifecycle.py index 8ec08a1..461fdb7 100644 --- a/backend/tests/test_session_lifecycle.py +++ b/backend/tests/test_session_lifecycle.py @@ -233,3 +233,48 @@ class QuizAnalysisTests(unittest.TestCase): quiz_id = self.bank.generate(is_shared=False, category_ids=[1], count=2).json()["id"] self.bank.user = self.bank.peer self.assertEqual(self.client.get(f"/attempts/quiz/{quiz_id}/analysis").status_code, 403) + + +class LiveAnalysisTests(unittest.TestCase): + """A session can be analysed while it is still being sat.""" + + def setUp(self): + self.bank = fixtures.BuilderTests() + self.bank.setUp() + self.client = self.bank.client + self.db = self.bank.db + self.client.app.include_router(attempts.router, prefix="/attempts") + self.bank.user = self.bank.owner + self.store = {} + self.redis = fake_redis(self.store) + + def tearDown(self): + self.bank.tearDown() + + def test_answers_given_but_not_submitted_are_analysed(self): + quiz_id = self.bank.generate(is_shared=True, category_ids=[1], count=2).json()["id"] + with patch.dict(sys.modules, {"redis": self.redis}): + aid = self.client.post(f"/attempts/start?quiz_id={quiz_id}").json()["id"] + # One answered, saved to progress; nothing submitted. + self.store[f"quiz_progress:{self.bank.owner.id}:{aid}"] = json.dumps({"answers": {"1": "yes"}}) + + with patch.dict(sys.modules, {"redis": self.redis}): + body = self.client.get(f"/attempts/{aid}/analysis").json() + + # It used to report 0/0 with an empty table while the session list, + # which reads the same saved progress, said one had been answered. + self.assertEqual(body["answered"], 1) + self.assertGreater(body["total"], 1) + self.assertEqual(body["score"], 1) + statuses = {q["status"] for q in body["questions"]} + self.assertIn("correct", statuses) + self.assertIn("skipped", statuses) + # Nothing was written: submitting is what records answers. + self.assertEqual(self.db.query(AttemptAnswer).filter_by(attempt_id=aid).count(), 0) + + def test_a_live_attempt_with_nothing_answered_reads_as_nothing_answered(self): + quiz_id = self.bank.generate(is_shared=True, category_ids=[1], count=2).json()["id"] + with patch.dict(sys.modules, {"redis": self.redis}): + aid = self.client.post(f"/attempts/start?quiz_id={quiz_id}").json()["id"] + body = self.client.get(f"/attempts/{aid}/analysis").json() + self.assertEqual((body["answered"], body["score"]), (0, 0)) diff --git a/frontend/src/components/CommentSection.jsx b/frontend/src/components/CommentSection.jsx deleted file mode 100644 index 2143cb8..0000000 --- a/frontend/src/components/CommentSection.jsx +++ /dev/null @@ -1,102 +0,0 @@ -import { useCallback, useEffect, useState } from 'react' -import ReactMarkdown from 'react-markdown' -import remarkGfm from 'remark-gfm' -import api from '../api/client' - -const LIMIT = 20 - -export default function CommentSection({ articleId, questionId }) { - const [comments, setComments] = useState([]) - const [total, setTotal] = useState(0) - const [offset, setOffset] = useState(0) - const [draft, setDraft] = useState('') - const [error, setError] = useState('') - const [submitting, setSubmitting] = useState(false) - const [loaded, setLoaded] = useState(false) - - const params = articleId ? { article_id: articleId } : { question_id: questionId } - - const load = useCallback(async (off = 0) => { - try { - const res = await api.get('/comments/', { params: { ...params, limit: LIMIT, offset: off } }) - const list = res.data?.comments || [] - setComments(prev => off === 0 ? list : [...prev, ...list]) - setTotal(res.data?.total || 0) - setOffset(off) - } catch { setComments([]); setTotal(0) } - finally { setLoaded(true) } - }, [articleId, questionId]) - - useEffect(() => { setComments([]); setLoaded(false); load(0) }, [load]) - - const submit = async () => { - setError('') - setSubmitting(true) - try { - const res = await api.post('/comments/', { ...params, content: draft }) - setComments(prev => [res.data, ...prev.filter(c => c.id !== res.data.id)]) - setTotal(t => t + 1) - setDraft('') - } catch (err) { - setError(typeof err.response?.data?.detail === 'string' ? err.response.data.detail : 'Could not post comment') - } finally { setSubmitting(false) } - } - - const moderate = async (id, status) => { - setError('') - try { - const res = await api.patch(`/comments/${id}`, { status }) - setComments(prev => prev.map(c => c.id === id ? res.data : c)) - } catch { setError('Could not moderate comment') } - } - - if (!loaded) return null - - return ( -
-
-

Discussion{total > 0 ? ` · ${total}` : ''}

- Visible to everyone after educator approval. -
-
-