Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: better error handling on worker error #133

Merged
merged 5 commits into from
Nov 7, 2024
Merged

Conversation

janbritz
Copy link
Contributor

@janbritz janbritz commented Oct 1, 2024

Falls es ein Worker-Error beim Starten, Anzeigen oder Bewerten einer Frage Auftritt, wird das nutzerfreundlicher angezeigt:
image

Und auch geloggt:
image

Sollten eine Frage (teilweise) beantwortet worden sein, werden beim Auftreten eines Fehlers alle Antworten gespeichert.
Falls ein Worker-Fehler beim Bewerten enstanden ist, wird die lehrende Person darauf aufmerksam gemacht, indem in der Scoring-Übersicht "Needs manual scoring" steht.

@janbritz janbritz marked this pull request as ready for review October 1, 2024 09:34
Copy link
Member

@MHajoha MHajoha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gefällt mir 👍

Dass das Rescoring eines Attempts, der Aufgrund eines Fehlers auf $needsgrading steht, nicht funktioniert, liegt an mir: #134

Ich frage mich gerade noch, ob wir beim Start eines Attempts wirklich den Fehler schlucken sollten. Ich tendiere zu nein, weil ich mir kein Szenario vorstellen kann, in dem man den Attempt dann verwenden kann. Es gibt ja dann keine Inputs, die wir für zukünftiges Regrading abspeichern müssen.

question.php Outdated Show resolved Hide resolved
Copy link

@larsbonczek larsbonczek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ich finde das request_failed event sollte aufgeteilt werden in einzelne events für die konkreten Situationen, in denen es auftreten kann. Sprich:

  • starting_attempt_failed
  • viewing_attempt_failed
  • grading_response_failed

Das event viewing_attempt_failed sollte zum Beispiel $this->data['crud'] = 'r' haben und das event grading_response_failed sollte $this->data['edulevel'] = self::LEVEL_TEACHING haben. $this->other['message'] kann dann wegfallen.

In der description der jeweiligen events sollte noch die ID des Quizzes, des Versuches und der Frage sowie die userid erwähnt werden.

Zum Beispiel:

The user with id '???' encountered an error in the question with id '???' when starting a new attempt with id '???' in the quiz with id '???':
Error while fetching from server. Error number: 7

classes/event/request_failed.php Outdated Show resolved Hide resolved
@MartinGauk
Copy link
Contributor

Da die Fehler nur bei den API-Aufrufen abgefangen werden, müssen wir auch an anderen Code-Stellen aufpassen, dass Exceptions nicht den ganzen Test kaputt machen. Aufgefallen ist mir hier in der grade_response Methode wenige Zeilen tiefer
throw new coding_exception("Unrecognized scoring code: $attemptscored->scoringcode");
Entweder muss das auch gleich wieder abgefangen werden oder besser: Das API Model unter classes/api/attempt_scored.php prüft, ob das Attribut einen laut der QPPE erlaubten Wert hat.

Ich frage mich gerade noch, ob wir beim Start eines Attempts wirklich den Fehler schlucken sollten. Ich tendiere zu nein, weil ich mir kein Szenario vorstellen kann, in dem man den Attempt dann verwenden kann. Es gibt ja dann keine Inputs, die wir für zukünftiges Regrading abspeichern müssen.

Das ist ein guter Punkt. start_attempt wird danach nicht wieder aufgerufen und aus Moodle-Sicht ist ja kein Fehler passiert. Die Frage kann in diesem Testversuch also nie korrekt angezeigt werden, auch wenn der Server wieder erreichbar ist. Ich würde daher auch sagen, dass in diesem Fall der Fehler nicht geschluckt werden sollte.

@MartinGauk
Copy link
Contributor

Ich denke, es ist auch noch sinnvoll, debugging() aufzurufen, damit Moodle den Fehler ggf. ins error.log des Moodle-Servers schreiben kann.

classes/array_converter/array_converter.php Show resolved Hide resolved
classes/event/grading_response_failed.php Outdated Show resolved Hide resolved
classes/event/starting_attempt_failed.php Outdated Show resolved Hide resolved
classes/event/grading_response_failed.php Outdated Show resolved Hide resolved
classes/event/starting_attempt_failed.php Outdated Show resolved Hide resolved
classes/event/viewing_attempt_failed.php Outdated Show resolved Hide resolved
question.php Outdated Show resolved Hide resolved
question.php Outdated Show resolved Hide resolved
question.php Outdated Show resolved Hide resolved
@janbritz janbritz requested a review from larsbonczek October 29, 2024 09:05
@janbritz janbritz requested a review from larsbonczek October 30, 2024 09:27
@janbritz janbritz force-pushed the feat/always-save-answers branch 3 times, most recently from 823c51f to b0762d1 Compare October 30, 2024 10:33
question.php Outdated Show resolved Hide resolved
question.php Outdated Show resolved Hide resolved
Comment on lines +325 to +342
// Trigger error event.
$params = [
'context' => $PAGE->context,
// TODO: It would be nice to set a 'relateduserid'.
'other' => [
'questionid' => $this->id,
'errormessage' => $t->getMessage(),
],
];
$event = \qtype_questionpy\event\grading_response_failed::create($params);
$event->trigger();
debugging($event->get_description());

// As the server was not able to score the response, we mark this question with manual scoring.
return [0, question_state::$needsgrading];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An die User-ID könntest du mit $this->get_behaviour()->get_pending_step()->get_user_id() kommen, wobei ich nicht weiß, ob das vielleicht ein bisschen unsauber ist.

@janbritz janbritz force-pushed the feat/always-save-answers branch from b0f4456 to 72e2c55 Compare November 7, 2024 10:40
@janbritz janbritz merged commit 513fedc into dev Nov 7, 2024
6 checks passed
@janbritz janbritz deleted the feat/always-save-answers branch November 7, 2024 10:45
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.

4 participants