Skip to content

Analyze responses: avoid re-initialising questions for every row - #1943

Merged
sangwinc merged 6 commits into
devfrom
iss1942
Nov 10, 2026
Merged

sangwinc merged 6 commits into
devfrom
iss1942

Conversation

@adamant-pwn

@adamant-pwn adamant-pwn commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #1942.

Analyze responses loaded the student's whole quiz attempt for every row and initialised the question through the CAS each time, so large quizzes timed out (HTTP 504 after 120 s on our quiz).

This PR:

  1. initialises one question per question version and seed (the only state STACK's apply_attempt_state() uses) and reuses it for every matching response;
  2. reads the steps of all reported question attempts in one query, instead of building a question_attempt per row, which made and initialised its own copy of the question. The steps are chosen as get_steps_with_submitted_response_iterator() does, using the attempt's behaviour, and autosaved steps are handled as in question_attempt::load_from_records().

On a local replica (200 students, 14 STACK questions) the report output is unchanged apart from the report timestamp. A question with 321 attempts and 2 inputs takes 0.8 s instead of 133 s, and one with 444 attempts and 23 inputs takes 9.6 s instead of 274 s. (With only the first change, these were 10.1 s and 81.4 s.)

New tests compare the chosen steps with the question engine's iterator for each fixture, including an autosaved answer, and cover a reported question in a later slot.

Prepared with AI assistance.

adamant-pwn and others added 5 commits October 28, 2026 01:04
create_summary() loaded the whole question usage for every reported
question attempt, which initialises every question in the student's
quiz attempt. For STACK questions each initialisation is a CAS call,
so the report timed out on large quizzes. Load just the reported
question attempt with question_engine_data_mapper::load_question_attempt().

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The question state STACK needs for summarising depends only on the
question version and the seed, so initialise each such question once
and reuse it for every matching response instead of once per row.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Repeat the single-input case with another STACK question in slot 1, so
the reported question attempt is in slot 2 of a multi-question attempt.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Building a question_attempt per row still made and initialised a copy of
the question for every row. Read the steps of all reported question
attempts in one query instead, choose the steps with a submitted response
as the question engine's iterator does (using the attempt's behaviour),
and keep autosaved steps as question_attempt::load_from_records does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@EJMFarrow

Copy link
Copy Markdown
Collaborator

@sangwinc @adamant-pwn I've had a look over this and it seems great. Admittedly, being certain it absolutely replicates what was happening before would be a lot of work so after an initial sanity check, I got one AI to mark the other AI's homework. Running some tests both versions came up with the same output. The only possible issue I found was:

(/var/www/html/MDEV/public/question/type/stack/stack/questionreport.class.php:224) now loads the question directly with question_bank::load_question(). Unlike Moodle’s previous attempt loader, this has no fallback for a deleted, missing, or unloadable historical question version. Since the report intentionally includes all versions of a question, such data would now make the page fail rather than produce Moodle’s missing-question output.

This is an edge case but definitely possible.

@adamant-pwn

Copy link
Copy Markdown
Collaborator Author

@EJMFarrow Thanks, I reproduced the crash with a deleted historical question and with missing STACK options.

The fix keeps the saved final response summary once per affected attempt and continues analysing readable versions. Without the historical definition, earlier responses and detailed JSON are omitted. Regression tests cover both failures, mixed versions and repeated submissions, including checking that the final summary is not counted twice.

The pre-PR code also fails in the deleted-question case: Moodle supplies a missing-question object, but the report then calls summarise_response_json() on it. The fix handles that case too.

Prepared with AI assistance.

@EJMFarrow

Copy link
Copy Markdown
Collaborator

Fantastic, @adamant-pwn . Looks good. Thank you!

@sangwinc
sangwinc merged commit badb68d into dev Nov 10, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants