fix: the answer side of a question needs an attempt, or the job of writing it
Asked for: no tutor on a question outside a session. The tutor is handed the correct answer and told it may explain it, so it is answer-side content — and once that rule is written down, the same rule catches two bigger holes: - `GET /questions/bank` returned `correct_answer`, `explanation`, `option_explanations`, `key_points` and `attending_tip` for every question in the bank, to any signed-in learner. It is the question manager's listing, but nothing stopped anyone calling it: the whole answer key, one request away from the questions it answers. Stems are still listed to everyone; the answer side now goes only to whoever writes that question. - The explanation image behind a question was readable by the same rule, with no attempt behind it. "Whoever writes it" is one function now — `may_edit_question` — and it means moderation, authorship, or an editorial grant that reaches where the question is filed. Everyone else earns the answer by sitting the question, which is what an attempt is. The bank browse, the search, the session and the review are all unchanged; the frontend already sends `attempt_id` everywhere it shows an answer. The question manager was reachable by a learner with no grant, and would now load as a bank of stems with every answer field blanked — a broken page rather than a door that is not theirs. It says so instead. Also, while looking at where cards surface: the answer review showed neither the topic reading nor the cards written against a question, though the player has shown both under the answer for a while — and the review is the one place a learner goes through everything they got wrong. The list form of that component fetched its cards and then dropped them on the floor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN
This commit is contained in:
Daniel
parent
c1391c1554
commit
c22e7f9547
8 changed files with 149 additions and 15 deletions
|
|
@ -36,7 +36,8 @@ from app.services.quiz_builder import (bank_query, bank_question_predicate, cate
|
|||
create_saved_test, exam_scope_predicate, generate_test)
|
||||
from app.utils.auth import get_current_user, require_moderator
|
||||
from app.utils.category_grants import (assert_can_manage_category, assert_user_can_manage,
|
||||
is_question_manager, manageable_categories, question_in_scope, require_question_manager)
|
||||
is_question_manager, manageable_categories, question_in_scope, question_scope_predicate,
|
||||
require_question_manager)
|
||||
|
||||
router = APIRouter()
|
||||
|
||||
|
|
@ -434,6 +435,18 @@ def get_question_bank(
|
|||
|
||||
quiz_cache: dict[int, str] = {}
|
||||
cat_cache: dict[int, str] = {}
|
||||
# Which of these the caller writes rather than sits. This listing used to
|
||||
# hand every signed-in learner the correct option and the explanation for
|
||||
# the whole bank — the answer key, one HTTP request away from the questions
|
||||
# it answers. Editors still get it; everyone else gets the stem.
|
||||
scope = question_scope_predicate(db, current_user)
|
||||
if scope is None:
|
||||
editable = {qu.id for qu in questions}
|
||||
else:
|
||||
editable = {row[0] for row in db.query(Question.id).filter(
|
||||
Question.id.in_([qu.id for qu in questions]), scope).all()} if questions else set()
|
||||
editable |= {qu.id for qu in questions
|
||||
if qu.user_id is not None and qu.user_id == current_user.id}
|
||||
link_rows = db.query(QuestionCategoryLink.question_id, QuestionCategoryLink.category_id).filter(
|
||||
QuestionCategoryLink.question_id.in_([qu.id for qu in questions])).all() if questions else []
|
||||
extra_map: dict[int, list[int]] = {}
|
||||
|
|
@ -461,13 +474,14 @@ def get_question_bank(
|
|||
"question_text": qu.question_text,
|
||||
"question_type": qu.question_type,
|
||||
"options": qu.options,
|
||||
"correct_answer": qu.correct_answer,
|
||||
"explanation": qu.explanation,
|
||||
# The answer side, for the people who write it.
|
||||
"correct_answer": qu.correct_answer if qu.id in editable else None,
|
||||
"explanation": qu.explanation if qu.id in editable else None,
|
||||
"image_path": qu.image_path,
|
||||
"explanation_image_path": qu.explanation_image_path,
|
||||
"option_explanations": qu.option_explanations,
|
||||
"key_points": qu.key_points,
|
||||
"attending_tip": qu.attending_tip,
|
||||
"explanation_image_path": qu.explanation_image_path if qu.id in editable else None,
|
||||
"option_explanations": qu.option_explanations if qu.id in editable else None,
|
||||
"key_points": qu.key_points if qu.id in editable else None,
|
||||
"attending_tip": qu.attending_tip if qu.id in editable else None,
|
||||
"difficulty": qu.difficulty,
|
||||
"user_id": qu.user_id,
|
||||
"match_source": "semantic" if qu.id in semantic_ids else "keyword",
|
||||
|
|
|
|||
|
|
@ -51,8 +51,31 @@ def set_quiz_shared(db, quiz, user, shared):
|
|||
return {"id": quiz.id, "is_shared": quiz.is_shared, "is_published": quiz.is_published}
|
||||
|
||||
|
||||
def may_edit_question(db, question, user) -> bool:
|
||||
"""Whether this person writes this question, rather than sits it.
|
||||
|
||||
Three ways in: moderation, authorship, or an editorial grant that covers
|
||||
where the question is filed. It is what separates reading an answer because
|
||||
it is your job from reading it because you found the URL.
|
||||
"""
|
||||
if user.is_moderator:
|
||||
return True
|
||||
if question.user_id is not None and question.user_id == user.id:
|
||||
return True
|
||||
from app.utils.category_grants import question_scope_predicate
|
||||
predicate = question_scope_predicate(db, user)
|
||||
if predicate is None:
|
||||
return True
|
||||
return db.query(Question.id).filter(Question.id == question.id, predicate).first() is not None
|
||||
|
||||
|
||||
def require_question_access(db, question, user, attempt_id=None, review=False):
|
||||
"""Authorize tutor/answer content or a stem."""
|
||||
"""Authorize tutor/answer content or a stem.
|
||||
|
||||
`review=True` means answer-side content: the correct option, the
|
||||
explanation, the picture of the explanation, and the tutor — which is given
|
||||
the answer and told it may explain it, so it is answer-side too.
|
||||
"""
|
||||
if question is None:
|
||||
raise HTTPException(404, "Question not found")
|
||||
if attempt_id is not None:
|
||||
|
|
@ -67,5 +90,11 @@ def require_question_access(db, question, user, attempt_id=None, review=False):
|
|||
if user.is_moderator:
|
||||
return
|
||||
if db.query(Question.id).filter(Question.id == question.id, bank_question_predicate(user)).first():
|
||||
return
|
||||
# In the bank, so the stem is theirs to read. The answer beside it is
|
||||
# not: without an attempt, the only people who see it are the people
|
||||
# who write it. Being able to reach a question was never the same as
|
||||
# having earned its answer, and the tutor is an answer read aloud.
|
||||
if not review or may_edit_question(db, question, user):
|
||||
return
|
||||
raise HTTPException(403, "Sit this question to see its answer")
|
||||
raise HTTPException(403, "Question is private or requires an authorized study/review attempt")
|
||||
|
|
|
|||
|
|
@ -179,6 +179,38 @@ class CategoryGrantTests(unittest.TestCase):
|
|||
self.assertFalse(moderator_summary["scoped"])
|
||||
self.assertEqual(moderator_summary["total"], 4)
|
||||
|
||||
def test_the_bank_listing_hands_the_answer_only_to_whoever_writes_it(self):
|
||||
"""Any signed-in learner could read the whole answer key from /bank.
|
||||
|
||||
The listing is the question manager's, but nothing stopped a learner
|
||||
calling it, and it returned the correct option, the explanation, the
|
||||
option explanations and the attending tip for every question in the
|
||||
bank. The stem is bank content; the answer beside it is not.
|
||||
"""
|
||||
self.user = self.outsider
|
||||
rows = self.client.get("/questions/bank").json()["questions"]
|
||||
self.assertEqual(len(rows), 4) # every stem is still listed
|
||||
for row in rows:
|
||||
self.assertTrue(row["question_text"])
|
||||
self.assertIsNone(row["correct_answer"])
|
||||
self.assertIsNone(row["explanation"])
|
||||
self.assertIsNone(row["attending_tip"])
|
||||
self.assertIsNone(row["explanation_image_path"])
|
||||
|
||||
# An educator sees the answer where their grant reaches, and only there.
|
||||
self.grant(category_id=1, user_id=3)
|
||||
self.user = self.outsider
|
||||
answers = {row["id"]: row["correct_answer"]
|
||||
for row in self.client.get("/questions/bank").json()["questions"]}
|
||||
self.assertEqual(answers[1], "yes") # Cardiology
|
||||
self.assertEqual(answers[2], "yes") # a descendant of it
|
||||
self.assertIsNone(answers[3]) # Neurology
|
||||
self.assertIsNone(answers[4]) # filed nowhere
|
||||
|
||||
self.user = self.mod
|
||||
self.assertTrue(all(row["correct_answer"] == "yes"
|
||||
for row in self.client.get("/questions/bank").json()["questions"]))
|
||||
|
||||
def test_ungranted_user_is_locked_out_of_question_management(self):
|
||||
self.user = self.outsider
|
||||
self.assertEqual(self.client.get("/questions/manage/summary").status_code, 403)
|
||||
|
|
|
|||
|
|
@ -82,10 +82,20 @@ class PrivacyTests(unittest.TestCase):
|
|||
self.assertIn(self.chat(999).status_code, (403, 404))
|
||||
for boundary in (self.quota, self.model, self.similar, self.ai):
|
||||
boundary.assert_not_called()
|
||||
for qid in (1, 3, 4, 5):
|
||||
res = self.chat(qid)
|
||||
self.assertEqual(res.status_code, 200, res.text)
|
||||
self.assertEqual(self.similar.call_args.args[2].id, 1)
|
||||
# The tutor is handed the correct answer and told it may explain it, so
|
||||
# outside a session it is an answer key. Being able to reach a question
|
||||
# in the bank is not the same as having earned its answer: without an
|
||||
# attempt it opens for the people who write the question, and nobody
|
||||
# else — and it refuses before the quota, the model or the AI is asked.
|
||||
for qid in (1, 4, 5):
|
||||
self.assertEqual(self.chat(qid).status_code, 403)
|
||||
for boundary in (self.quota, self.model, self.similar, self.ai):
|
||||
boundary.assert_not_called()
|
||||
res = self.chat(3) # the owner's own question
|
||||
self.assertEqual(res.status_code, 200, res.text)
|
||||
# The context search is run as the caller, not as whoever wrote it.
|
||||
self.assertEqual(self.similar.call_args.args[2].id, self.owner.id)
|
||||
self.assertEqual(self.similar.call_args.args[1].id, 3)
|
||||
self.login(self.mod)
|
||||
self.assertEqual(self.chat(4).status_code, 200)
|
||||
|
||||
|
|
@ -149,11 +159,15 @@ class PrivacyTests(unittest.TestCase):
|
|||
self.client.headers.clear()
|
||||
self.assertEqual(self.client.get('/uploads/questions/stem-4.png').status_code, 401)
|
||||
self.login(self.owner, cookie=True)
|
||||
for path in ('stem-1', 'stem-3', 'stem-4', 'answer-1', 'answer-3'):
|
||||
# An answer image is answer-side: 'answer-3' opens because question 3 is
|
||||
# this learner's own, and 'answer-1' is checked below because it is not.
|
||||
for path in ('stem-1', 'stem-3', 'stem-4', 'answer-3'):
|
||||
res = self.client.get(f'/uploads/questions/{path}.png')
|
||||
self.assertEqual(res.status_code, 200, res.text)
|
||||
self.assertEqual(res.headers['cache-control'], 'private, no-store')
|
||||
self.assertEqual(res.headers['vary'], 'Cookie, Authorization')
|
||||
# Somebody else's answer, with no attempt behind it: refused.
|
||||
self.assertEqual(self.client.get('/uploads/questions/answer-1.png').status_code, 404)
|
||||
# A derivative may be kept by the browser that asked for it — never by
|
||||
# a shared cache, which in front of access-controlled images is how one
|
||||
# learner is served another's private figure. The original still says
|
||||
|
|
|
|||
|
|
@ -227,6 +227,10 @@ class TutorFigureTests(unittest.TestCase):
|
|||
# against it", which is the case being tested.
|
||||
patch.object(vision_service, "catalogue", return_value={}).start()
|
||||
vision_service._catalogue, vision_service._catalogue_at = {}, 0.0
|
||||
# The tutor is answer-side content, so outside a session it opens only
|
||||
# for whoever writes the question. What is under test here is what a
|
||||
# model is shown once it is open, so the caller is an educator.
|
||||
self.privacy.login(self.privacy.mod)
|
||||
|
||||
def tearDown(self):
|
||||
self.privacy.tearDown()
|
||||
|
|
|
|||
|
|
@ -55,17 +55,31 @@ export default function QuestionReadingLinks({ questionId, variant = 'list' }) {
|
|||
)
|
||||
}
|
||||
|
||||
// One deck, however many of its cards are tied here: three lines naming the
|
||||
// same deck are three doors to the same room.
|
||||
const decks = []
|
||||
for (const card of cards) {
|
||||
if (!decks.some(deck => deck.deck_id === card.deck_id)) decks.push(card)
|
||||
}
|
||||
return (
|
||||
<div className="question-reading" data-testid="question-reading">
|
||||
<strong>Topic reading</strong>
|
||||
<ul>
|
||||
{links.map(link => (
|
||||
{(links || []).map(link => (
|
||||
<li key={`${link.article_id}-${link.section_id || 'all'}`}>
|
||||
<Link to={link.section_id ? `/articles/${link.article_id}?section=${link.section_id}` : `/articles/${link.article_id}`}>
|
||||
{link.title}{link.section_title ? ` — ${link.section_title}` : ''}
|
||||
</Link>
|
||||
</li>
|
||||
))}
|
||||
{/* The cards an educator wrote against this question. They were
|
||||
fetched here and then dropped on the floor by this branch, so they
|
||||
showed under the answer in a session and nowhere else. */}
|
||||
{decks.map(card => (
|
||||
<li key={`deck-${card.deck_id}`}>
|
||||
<Link to={`/flashcards/${card.deck_id}/study`}>▦ {card.deck_title}</Link>
|
||||
</li>
|
||||
))}
|
||||
</ul>
|
||||
</div>
|
||||
)
|
||||
|
|
|
|||
|
|
@ -137,6 +137,26 @@ export default function QuestionManagerPage() {
|
|||
// Where the editor sends you back to, filters and page intact.
|
||||
const location = useLocation()
|
||||
|
||||
// Nothing to manage. The page is not moderator-only — an educator with a
|
||||
// grant belongs here — so it cannot be gated in the router; it is gated on
|
||||
// what the grant tree says. Without one it used to load the whole bank as a
|
||||
// list of stems with every answer field blanked, which reads as a broken
|
||||
// page rather than a door that is not yours.
|
||||
if (scope && !scope.is_moderator && !scope.can_manage_questions) {
|
||||
return (
|
||||
<div className="qm-page">
|
||||
<div className="qm-header"><div><h1>Questions</h1></div></div>
|
||||
<div className="card empty-state" role="note">
|
||||
<p>Editing the question bank needs an editorial grant.</p>
|
||||
<p style={{ color: 'var(--text-muted)', fontSize: '0.9rem' }}>
|
||||
Ask an administrator for one, naming the topics you write about.
|
||||
</p>
|
||||
<Link className="btn btn-secondary" to="/question-bank">Back to the bank</Link>
|
||||
</div>
|
||||
</div>
|
||||
)
|
||||
}
|
||||
|
||||
return (
|
||||
<div className="qm-page">
|
||||
{previewing && (
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ import { useClaimSessionChrome } from '../context/SessionChrome'
|
|||
import useMediaQuery from '../hooks/useMediaQuery'
|
||||
import QuizTools, { LabValues } from '../components/QuizTools'
|
||||
import FigureStrip from '../components/FigureStrip'
|
||||
import QuestionReadingLinks from '../components/QuestionReadingLinks'
|
||||
import { optionLetter } from '../utils/options'
|
||||
import { uploadUrl } from '../utils/uploads'
|
||||
import './QuizPlayer.css'
|
||||
|
|
@ -256,6 +257,12 @@ export default function ResultsPage() {
|
|||
)}
|
||||
</div>
|
||||
)}
|
||||
|
||||
{/* Where to read it up, and the cards written against it. The
|
||||
player has offered these under the answer for a while; the
|
||||
review did not, which is the one place a learner is going
|
||||
through everything they got wrong. */}
|
||||
<QuestionReadingLinks questionId={ans.question_id} variant="chips" />
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
|
|
|
|||
Loading…
Reference in a new issue