Add timeout and better error message to quiz submit request #1185

ihalaij1 · 2023-05-30T11:30:06Z

Description

What?

Previously when the student's internet connection or MOOC-Grader was down, submitting a questionnaire resulted in the error message Connecting to the assessment service failed! to be displayed. A more suitable error message had existed, but it was not shown because of a bug.

This bug is now fixed by adding a timeout to the submit request (the request's failure handler is now properly called). The existing error message SUBMISSION_ERROR_ALERT_COMMUNICATION_ERROR_W_SERVICE was also modified to be more descriptive.

If A+ can't be reached, a submission is obviously not consumed by making the submit request. However, a submission is consumed if A+ receives the submit request, even if MOOC-Grader can't be reached. In this case, the submission is put into the Initialized state and should be automatically sent to the grading service when it is available.

It is not easy to distinguish whether the user's internet connection is down or if the grading service is down, since they both result in a similar timeout in chapter.js. This is why we use the same error message for both scenarios.

Closes #852

Testing

What type of test did you run?

Tested scenarios where a questionnaire was submitted while A+ or MOOC-Grader wasn't reachable. The error message was displayed properly in both cases.

Did you test the changes in

Chrome
Firefox
This pull request cannot be tested in the browser.

Translation

Did you modify or add new strings in the user interface? (Read about how to create translation)

Programming style

Did you follow our style guides?
Did you use Python type hinting in all functions that you added or edited? (type hints for function parameters and return values)

Have you updated the README or other relevant documentation?

documents inside the doc directory.
README.md.
Aplus Manual.
Other documentation (mention below which documentation).

Is it Done?

Reviewer has finished the code review
After the review, the developer has made changes accordingly
Customer/Teacher has accepted the implementation of the feature

markkuriekkinen · 2023-06-01T16:46:43Z

I was testing this and I still got an error that is hard to read inside the error progress bar:

I made a submission. I had a breakpoint in the ExerciseView.post() method. A+ did not respond to the HTTP request in time since I stopped the Django code with the debugger.

In the browser developer tools, I could see the full error message:

There was a problem loading the assignment. Try loading the page again later. The course staff has been notified. If the problem persists for over a day and hasn't been addressed on the course news, please contact course staff.

django.po msgid "EXERCISE_ERROR_COMMUNICATION"

markkuriekkinen

Good catch with the jQuery.ajax timeout. I was surprised to see that there are still submission errors that are hard to read inside the alert progress bar. Could you still double-check this? I wrote a message in the normal conversation chain (outside this review).

locale/en/LC_MESSAGES/django.po

ihalaij1 · 2023-06-07T06:09:52Z

I moved the error message from the progress bar to a separate alert box now.

Closes apluslms#852

In the submit modal dialog in chapters, the error message was displayed inside a progress bar. That was not a good idea since it was hard to read and the latter half of the message disappeared since it did not fit into the progress bar. The error message is now moved to a separate alert box above the progress bar.

markkuriekkinen

Nice, I will rebase and merge this now!

There are multiple, a little different error messages for the grading connection errors in the code base. Some of them could be maybe removed and replaced with the almost identical ones. It is not immediately obvious why there are multiple similar strings (in django.po). Some of them are used in similar situations (based on a quick glance), some in a clearly different scenario. Since most of them are old, we should check if they are really up-to-date. Some of them promise that the course staff is emailed about the errors, but I am not sure if the email is really sent in all cases.

Similar strings include:

SUBMISSION_ERROR_ALERT_COMMUNICATION_ERROR_W_SERVICE
EXERCISE_ERROR_COMMUNICATION
EXERCISE_SERVICE_ERROR_CONNECTION_FAILED
ASSESSMENT_SERVICE_ERROR_CONNECTION_FAILED
ERROR_ALERT_EXERCISE_ASSESSMENT_SERVICE_MALFUNCTIONING

markkuriekkinen · 2023-06-30T13:11:14Z

edit_course/tests.py

+        stripped_content = response.content.decode("utf-8").replace("modal-submit-error alert alert-danger", "")
+        self.assertNotIn("alert alert-danger", stripped_content)
+        self.assertIn("alert alert-success", stripped_content)


This looks a bit strange. What is the idea here?

base.html now includes a hidden div with class="modal-submit-error alert alert-danger".
It has to be stripped away before asserting in the course cloning tests so that the tests don't fail, because they think course cloning failed and rendered a different alert.

markkuriekkinen · 2023-06-30T15:31:14Z

locale/en/LC_MESSAGES/django.po

@@ -3962,9 +3962,10 @@ msgstr "Posting submission..."
 #: exercise/templates/exercise/_submit_progress.html
 msgid "SUBMISSION_ERROR_ALERT_COMMUNICATION_ERROR_W_SERVICE"


After the latest changes, when is this string really used? I could see EXERCISE_ERROR_COMMUNICATION during short testing.

This is shown, for example, when submitting a questionnaire fails because grader can't be reached.

OK, I was testing with a programming assignment and I could only see EXERCISE_ERROR_COMMUNICATION then.

markkuriekkinen · 2023-06-30T15:46:27Z

exercise/static/exercise/chapter.js

@@ -460,7 +460,8 @@
        data: formData,
        contentType: false,
        processData: false,
-        dataType: "html"
+        dataType: "html",
+        timeout: 10000,


We could increase the timeout to 20 seconds. 10 seconds is not enough if there are issues with connecting to the grader. A+ retries the connection to the grader three times by default. 10 seconds is not enough for A+ to finish those three connection attempts to the grader.

A+ shows the error message EXERCISE_SERVICE_ERROR_CONNECTION_FAILED after it has failed to connect to the grader. The 10-second timeout in the JavaScript hides that error.

a-plus/exercise/protocol/aplus.py

Line 36 in c76e3fb

_('EXERCISE_SERVICE_ERROR_CONNECTION_FAILED'))

a-plus/lib/remote_page.py

Lines 60 to 62 in c76e3fb

last_retry = len(settings.EXERCISE_HTTP_RETRIES) - 1

n = 0

while n <= last_retry:

Currently, the error SUBMISSION_ERROR_ALERT_COMMUNICATION_ERROR_W_SERVICE is shown when the timeout occurs, e.g., when submitting a questionnaire fails due to grader being unavailable.

If the timeout is increased, which message will show? Do we want both to show?

So A+ now actually shows different errors for questionnaires and asynchronous assignments when the timeout triggers. With the 10-second timeout, I would only see EXERCISE_ERROR_COMMUNICATION in a programming assignment. With 20-second time, the A+ backend has time to trigger EXERCISE_SERVICE_ERROR_CONNECTION_FAILED.

I already made a new commit that changes the chapter.js timeout to 20 seconds. There is still room to improve the error messages and remove unnecessary overlaps.

markkuriekkinen · 2023-06-30T16:25:37Z

One issue related to the emails that maybe were not sent to the course staff even though the error message promised so:
#1051

Timeout was added to the AJAX submissions in chapter.js in the pull request apluslms#1185. However, 10 seconds was too short compared to the retry loop when A+ is not able to immediately connect to the grader. Increase the timeout to 20 seconds so that A+ can finish the connection attempts to the grader before chapter.js causes a timeout for the submission attempt. Pull request comment: apluslms#1185 (comment) The retry loop in the code: https://github.com/apluslms/a-plus/blob/88f2dc91667eed12a275427585643b6c787dfa1c/lib/remote_page.py#L60-L62

ihalaij1 added this to the Summer 2023 milestone May 30, 2023

ihalaij1 force-pushed the fix-submission-error-msg branch from 95d4e31 to c12e7fa Compare May 30, 2023 11:32

ihalaij1 removed this from the Summer 2023 milestone May 30, 2023

ihalaij1 requested a review from markkuriekkinen May 30, 2023 11:46

ihalaij1 assigned markkuriekkinen May 30, 2023

markkuriekkinen assigned ihalaij1 and unassigned markkuriekkinen Jun 1, 2023

markkuriekkinen requested changes Jun 1, 2023

View reviewed changes

locale/en/LC_MESSAGES/django.po Show resolved Hide resolved

ihalaij1 requested a review from markkuriekkinen June 7, 2023 06:10

ihalaij1 force-pushed the fix-submission-error-msg branch 3 times, most recently from fe697fe to a7f50ad Compare June 7, 2023 06:47

ihalaij1 assigned markkuriekkinen and unassigned ihalaij1 Jun 28, 2023

ihalaij1 added 2 commits June 30, 2023 15:49

Add timeout and better error message to quiz submit request

91f9875

Closes apluslms#852

markkuriekkinen approved these changes Jun 30, 2023

View reviewed changes

markkuriekkinen force-pushed the fix-submission-error-msg branch from a7f50ad to 339be66 Compare June 30, 2023 16:22

markkuriekkinen merged commit 88f2dc9 into apluslms:master Jun 30, 2023
8 checks passed

markkuriekkinen mentioned this pull request Jun 30, 2023

Same submission communication error message used despite where the error is #852

Closed

ihalaij1 mentioned this pull request Sep 1, 2023

Check and remove duplicate strings related to connection errors #1244

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add timeout and better error message to quiz submit request #1185

Add timeout and better error message to quiz submit request #1185

ihalaij1 commented May 30, 2023

markkuriekkinen commented Jun 1, 2023

markkuriekkinen left a comment

ihalaij1 commented Jun 7, 2023

markkuriekkinen left a comment

markkuriekkinen Jun 30, 2023

ihalaij1 Jun 30, 2023

markkuriekkinen Jun 30, 2023

ihalaij1 Jun 30, 2023 •

edited

Loading

markkuriekkinen Jun 30, 2023

markkuriekkinen Jun 30, 2023

ihalaij1 Jun 30, 2023

markkuriekkinen Jun 30, 2023

markkuriekkinen commented Jun 30, 2023

		@@ -3962,9 +3962,10 @@ msgstr "Posting submission..."
		#: exercise/templates/exercise/_submit_progress.html
		msgid "SUBMISSION_ERROR_ALERT_COMMUNICATION_ERROR_W_SERVICE"

	last_retry = len(settings.EXERCISE_HTTP_RETRIES) - 1
	n = 0
	while n <= last_retry:

Add timeout and better error message to quiz submit request #1185

Add timeout and better error message to quiz submit request #1185

Conversation

ihalaij1 commented May 30, 2023

Description

Testing

Translation

Programming style

Have you updated the README or other relevant documentation?

Is it Done?

markkuriekkinen commented Jun 1, 2023

markkuriekkinen left a comment

Choose a reason for hiding this comment

ihalaij1 commented Jun 7, 2023

markkuriekkinen left a comment

Choose a reason for hiding this comment

markkuriekkinen Jun 30, 2023

Choose a reason for hiding this comment

ihalaij1 Jun 30, 2023

Choose a reason for hiding this comment

markkuriekkinen Jun 30, 2023

Choose a reason for hiding this comment

ihalaij1 Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

markkuriekkinen Jun 30, 2023

Choose a reason for hiding this comment

markkuriekkinen Jun 30, 2023

Choose a reason for hiding this comment

ihalaij1 Jun 30, 2023

Choose a reason for hiding this comment

markkuriekkinen Jun 30, 2023

Choose a reason for hiding this comment

markkuriekkinen commented Jun 30, 2023

ihalaij1 Jun 30, 2023 •

edited

Loading