Coverage for server / routes / teacher / teacher_question_import.py: 86%
130 statements
« prev ^ index » next coverage.py v7.13.4, created at 2026-10-04 09:33 +0000
1"""Teacher question-import routes.
3Thin proxy over the Question Ingest service. The frontend talks only to this API; the
4ingest service is not reachable from a browser.
6Nothing here writes to the question bank. Commit returns validated payloads which are
7then created through TeacherQuestionService, so imported questions go through exactly
8the same path — and the same _validate_question_data — as hand-authored ones.
10Developer: Allan Ninal
11"""
13import logging
15from fastapi import APIRouter, Depends, File, Form, HTTPException, Request, UploadFile
16from pydantic import BaseModel, Field
18from server.authentication.auth0_bearer import Auth0Bearer
19from server.rate_limit import limiter
20from server.services.growthbook import require_feature
21from server.services.teacher import question_dedup, question_import
22from server.services.teacher.teacher_question import TeacherQuestionService
23from server.utilities.user_id_helper import to_user_id
25logger = logging.getLogger(__name__)
26router = APIRouter()
27teacher_question_service = TeacherQuestionService()
29def _teacher_id(request: Request):
30 return to_user_id(request.state.user_details["uuid"])
33def _import_owner(request: Request) -> str:
34 """The id the ingest service scopes an import job to.
36 Stringified because a local-JWT teacher's id is an ObjectId while an Auth0 one is
37 already a string; the ingest service keys on the text either way.
38 """
39 return str(_teacher_id(request))
42_TEACHER_ONLY = [
43 Depends(Auth0Bearer(access_levels=["teacher"])),
44 Depends(require_feature("teacher.bulk_upload_questions")),
45]
48# Fields the ingest service adds for provenance, which are NOT part of any
49# teacher_questionbank template. `_validate_question_data` rejects every key a template
50# does not declare, so leaving them in the payload failed the whole commit with
51# "Unexpected fields found: source" — every import, for every file. They are split off
52# here and handed to `create` as arguments instead, so the provenance is still recorded.
53_PROVENANCE_FIELDS = ("source", "questionId")
56def _split_provenance(payload: dict) -> tuple[dict, dict]:
57 """Return (payload without provenance fields, the provenance fields it carried)."""
58 provenance = {key: payload[key] for key in _PROVENANCE_FIELDS if key in payload}
59 if not provenance:
60 return payload, {}
61 return {k: v for k, v in payload.items() if k not in provenance}, provenance
64# Texas grade levels, and the difficulty vocabulary both banks accept. Checked HERE, at
65# upload, because the alternative is a batch that parses for half a minute and then
66# fails whole at commit — every question rejected over one value the teacher typed on
67# the first screen.
68_GRADE_RANGE = range(3, 13)
69_DIFFICULTIES = {"easy", "average", "advance"}
72def _assert_batch_defaults(grade_level: int | None, difficulty: str | None) -> None:
73 if grade_level is not None and grade_level not in _GRADE_RANGE:
74 raise HTTPException(
75 status_code=422,
76 detail=f"grade_level must be between 3 and 12; got {grade_level}",
77 )
78 if difficulty is not None and difficulty.strip() and difficulty.strip().lower() not in _DIFFICULTIES:
79 raise HTTPException(
80 status_code=422,
81 detail=(
82 f"difficulty must be one of Easy, Average, Advance; got {difficulty!r}"
83 ),
84 )
87def _upload_filename(filename: str | None, question_type: str) -> str:
88 """The name the ingest service reads the format from.
90 Defaulting a missing name to "upload.csv" is safe for a spreadsheet and wrong for
91 anything else: the ingest service picks its reader by suffix, so a nameless PDF
92 would be read as a CSV, and "Auto-detect" — which it accepts for PDFs only — would
93 be refused outright.
94 """
95 if filename and filename.strip():
96 return filename
97 return "upload.pdf" if question_type == "Auto-detect" else "upload.csv"
100def _reraise(exc: question_import.IngestRejected) -> HTTPException:
101 """Pass the ingest service's rejection through with its own status and detail.
103 Never collapse these to 500 — the detail names the row and column that failed, and
104 that is the whole value of it to a teacher. Never turn them into 401 either: a
105 non-/auth/ 401 makes the SPA refresh and redirect to "/", losing the import.
106 """
107 return HTTPException(status_code=exc.status_code, detail=exc.detail)
110@router.post(
111 "/upload",
112 dependencies=_TEACHER_ONLY,
113 summary="Upload a question file and start an import job",
114)
115@limiter.limit("10/minute")
116async def upload_question_file(
117 request: Request,
118 file: UploadFile = File(...),
119 question_type: str = Form(...),
120 # Applied to every question in the file. A document states neither: a STAAR
121 # released-item PDF names the TEKS, the points and the answer for each item, but a
122 # grade appears nowhere in it and a difficulty is a judgement, not a fact. Without
123 # these two the import parses fine and then cannot be saved at all.
124 grade_level: int | None = Form(default=None),
125 difficulty: str | None = Form(default=None),
126) -> dict:
127 """Hand the file to the ingest service and return its job id.
129 Returns immediately; the client polls the status endpoint. Holding the connection
130 for the duration of the parse is what loses work when a connection drops.
131 """
132 _assert_batch_defaults(grade_level, difficulty)
133 content = await file.read()
134 try:
135 return await question_import.create_import(
136 filename=_upload_filename(file.filename, question_type),
137 content=content,
138 question_type=question_type,
139 owner=_import_owner(request),
140 grade_level=grade_level,
141 difficulty=difficulty,
142 )
143 except question_import.IngestRejected as exc:
144 raise _reraise(exc) from exc
145 except question_import.IngestUnavailable as exc:
146 logger.error("question ingest unavailable on upload: %s", exc)
147 raise HTTPException(
148 status_code=503,
149 detail="the question import service is unavailable; nothing was imported",
150 ) from exc
153class PastedQuestion(BaseModel):
154 """One question typed or pasted by a teacher, rather than uploaded as a file."""
156 text: str = Field(..., min_length=1, max_length=50_000)
157 question_type: str
160@router.post(
161 "/text",
162 dependencies=_TEACHER_ONLY,
163 summary="Add a single question by pasting it, and start an import job",
164)
165@limiter.limit("20/minute")
166async def upload_question_text(request: Request, body: PastedQuestion) -> dict:
167 """Hand pasted text to the ingest service and return its job id.
169 Returns the same shape as a file upload on purpose: the client polls, reviews and
170 commits through the endpoints it already uses. A teacher adding one question should
171 not meet a different flow from a teacher adding forty.
173 The rate limit is higher than the upload one (20 vs 10). Pasting single questions is
174 a natural burst -- someone adding five in a row is ordinary use, not abuse -- while
175 each request is far cheaper than a file.
176 """
177 try:
178 return await question_import.create_import_from_text(
179 text=body.text, question_type=body.question_type, owner=_import_owner(request)
180 )
181 except question_import.IngestRejected as exc:
182 raise _reraise(exc) from exc
183 except question_import.IngestUnavailable as exc:
184 logger.error("question ingest unavailable on paste: %s", exc)
185 raise HTTPException(
186 status_code=503,
187 detail="the question import service is unavailable; nothing was imported",
188 ) from exc
191@router.get(
192 "/{job_id}/status",
193 dependencies=_TEACHER_ONLY,
194 summary="Progress and per-row errors for an import job",
195)
196async def import_status(request: Request, job_id: str) -> dict:
197 try:
198 return await question_import.get_import(job_id, owner=_import_owner(request))
199 except question_import.IngestRejected as exc:
200 raise _reraise(exc) from exc
201 except question_import.IngestUnavailable as exc:
202 raise HTTPException(
203 status_code=503, detail="the question import service is unavailable"
204 ) from exc
207@router.get(
208 "/{job_id}/questions",
209 dependencies=_TEACHER_ONLY,
210 summary="Proposed questions for review — nothing has been saved",
211)
212async def import_questions(request: Request, job_id: str) -> dict:
213 """Return the parsed questions, each flagged if it duplicates one already owned.
215 Flagged HERE, not just skipped at commit, so the teacher sees what will be skipped
216 while they can still act on it — remove it, or edit it into something new.
217 """
218 try:
219 payload = await question_import.get_import_questions(
220 job_id, owner=_import_owner(request)
221 )
222 except question_import.IngestRejected as exc:
223 raise _reraise(exc) from exc
224 except question_import.IngestUnavailable as exc:
225 raise HTTPException(
226 status_code=503, detail="the question import service is unavailable"
227 ) from exc
229 questions = payload.get("questions") or []
230 if questions:
231 try:
232 duplicate_indices, _ = await question_dedup.mark_duplicates(
233 _teacher_id(request), questions
234 )
235 for index in duplicate_indices:
236 questions[index]["duplicate"] = True
237 payload["duplicates"] = len(duplicate_indices)
238 except Exception: # noqa: BLE001 - review must still work without the check
239 # Report it as UNKNOWN rather than zero. "0 duplicates" is a plausible
240 # answer and would be read as "nothing is a duplicate" — the one wrong
241 # answer that looks right.
242 logger.exception("duplicate check failed for import %s", job_id)
243 payload["duplicates"] = None
245 return payload
248@router.post(
249 "/{job_id}/commit",
250 dependencies=_TEACHER_ONLY,
251 summary="Approve a reviewed import and create the questions",
252)
253@limiter.limit("10/minute")
254async def commit_import(request: Request, job_id: str, body: dict) -> dict:
255 """Create the reviewed questions in the teacher's question bank.
257 Questions are created through TeacherQuestionService.create — the same path a
258 hand-authored question takes — so imports obey identical validation rather than a
259 parallel write path that could drift from it.
261 Every payload is validated BEFORE any is written. That way the common failure
262 (a payload the API's validator rejects) leaves the bank untouched, instead of
263 stopping halfway with some questions saved and no clear record of which.
264 """
265 try:
266 result = await question_import.commit_import(
267 job_id, body, owner=_import_owner(request)
268 )
269 except question_import.IngestRejected as exc:
270 raise _reraise(exc) from exc
271 except question_import.IngestUnavailable as exc:
272 logger.error("question ingest unavailable on commit: %s", exc)
273 raise HTTPException(
274 status_code=503,
275 detail="the question import service is unavailable; nothing was committed",
276 ) from exc
278 payloads = result.get("payloads") or []
279 if not payloads:
280 raise HTTPException(
281 status_code=422,
282 detail="the import produced no questions to create",
283 )
285 # Duplicates are skipped, not rejected: the rest of the file still imports.
286 # NOT wrapped in try/except, unlike the review step. If the check itself fails we
287 # would be choosing between blocking this import and silently creating the exact
288 # duplicates this exists to prevent — and only one of those is recoverable by the
289 # teacher. Matches the staff implementation.
290 teacher_id = _teacher_id(request)
291 duplicate_indices, _ = await question_dedup.mark_duplicates(teacher_id, payloads)
292 skipped = set(duplicate_indices)
294 # Provenance is split off before validation and before the payloads are reused in
295 # phase 2, so both phases see exactly the same dict. Splitting per phase would let
296 # phase 1 pass and phase 2 fail on a payload phase 1 never saw.
297 provenance = []
298 for index, payload in enumerate(payloads):
299 payloads[index], carried = _split_provenance(payload)
300 provenance.append(carried)
302 # Phase 1 — validate everything, write nothing.
303 problems = []
304 for index, payload in enumerate(payloads, start=1):
305 if index - 1 in skipped:
306 continue
307 try:
308 await teacher_question_service._validate_question_data(payload)
309 except ValueError as exc:
310 problems.append({"question": index, "message": str(exc)})
312 if problems:
313 # A failure here means the ingest service accepted something this API rejects —
314 # the two validators have diverged. Surface it plainly rather than writing a
315 # partial batch.
316 logger.warning(
317 "import %s: %d payload(s) rejected by teacher validation", job_id, len(problems)
318 )
319 raise HTTPException(
320 status_code=422,
321 detail={
322 "message": (
323 f"{len(problems)} of {len(payloads)} questions failed validation; "
324 "no questions were created"
325 ),
326 "problems": problems,
327 },
328 )
330 # Phase 2 — write. Validation already passed, so a failure here is infrastructural.
331 created, failed = [], []
332 for index, payload in enumerate(payloads, start=1):
333 if index - 1 in skipped:
334 continue
335 try:
336 # `questionId` is deliberately not forwarded: the teacher bank has no such
337 # field on any template and its serializer would never return it, so storing
338 # it would write something no read path can ever show.
339 question = await teacher_question_service.create(
340 request, payload, source=provenance[index - 1].get("source")
341 )
342 created.append(question)
343 except Exception as exc: # noqa: BLE001 - report, never lose the tally
344 logger.exception("import %s: question %d failed to create", job_id, index)
345 failed.append({"question": index, "message": f"{type(exc).__name__}: {exc}"})
347 return {
348 "job_id": job_id,
349 "state": result.get("state"),
350 "created": len(created),
351 "skipped_duplicates": len(skipped),
352 "failed": len(failed),
353 # Named explicitly so a partial write can never be mistaken for a clean run.
354 "partial": bool(failed and created),
355 "failures": failed,
356 "questions": created,
357 }