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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Jun 30, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91f9875
Add timeout and better error message to quiz submit request
ihalaij1 May 30, 2023
339be66
Move submit modal dialog error message from progress bar to alert box
ihalaij1 Jun 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions assets/js/aplus.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,7 @@ $(function() {
var defaults = {
loader_selector: ".modal-progress",
loader_text_selector: ".progress-bar",
alert_text_selector: ".modal-submit-error",
title_selector: ".modal-title",
content_selector: ".modal-body",
error_message_attribute: "data-msg-error",
Expand All @@ -382,6 +383,7 @@ $(function() {
init: function() {
this.loader = this.element.find(this.settings.loader_selector);
this.loaderText = this.loader.find(this.settings.loader_text_selector);
this.alertText = this.loader.find(this.settings.alert_text_selector);
this.title = this.element.find(this.settings.title_selector);
this.content = this.element.find(this.settings.content_selector);
this.messages = {
Expand All @@ -407,6 +409,7 @@ $(function() {
open: function(data) {
this.title.hide();
this.content.hide();
this.alertText.hide();
this.loaderText
.removeClass('progress-bar-danger').addClass('active')
.text(data || this.messages.loading);
Expand All @@ -418,9 +421,10 @@ $(function() {
},

showError: function(data) {
this.loaderText
.removeClass('active').addClass('progress-bar-danger')
.text(data || this.messages.error);
if (data) {
this.alertText.text(data).show();
}
this.loaderText.removeClass('active').addClass('progress-bar-danger').text(this.messages.error);
},

showContent: function(data) {
Expand Down
15 changes: 9 additions & 6 deletions edit_course/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@ def test_course_clone(self):
'key_month': 'Test',
}, follow=True)
self.assertEqual(response.status_code, 200)
self.assertNotContains(response, "alert alert-danger")
self.assertContains(response, "alert alert-success")
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)
Comment on lines +36 to +38
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


# Partial clone
response = self.client.post(url, {
Expand All @@ -48,8 +49,9 @@ def test_course_clone(self):
'key_month': 'Test',
}, follow=True)
self.assertEqual(response.status_code, 200)
self.assertNotContains(response, "alert alert-danger")
self.assertContains(response, "alert alert-success")
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)

instance = CourseInstance.objects.get(id=1)
self.assertEqual(instance.url, instance_url)
Expand Down Expand Up @@ -104,8 +106,9 @@ def test_clone_from_sis(self):
'sis': '123',
}, follow=True)
self.assertEqual(response.status_code, 200)
self.assertNotContains(response, "alert alert-danger")
self.assertContains(response, "alert alert-success")
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)

# The asserted values are coming from in sis_test.py
sis_instance = CourseInstance.objects.get(course=instance.course, url="sis-test")
Expand Down
3 changes: 2 additions & 1 deletion exercise/static/exercise/chapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,8 @@
data: formData,
contentType: false,
processData: false,
dataType: "html"
dataType: "html",
timeout: 10000,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}).fail(function(xhr, textStatus, errorThrown) {
//$(form_element).find(":input").prop("disabled", false);
//exercise.showLoader("error");
Expand Down
9 changes: 5 additions & 4 deletions locale/en/LC_MESSAGES/django.po
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3842,7 +3842,7 @@ msgid "TOTAL_NUMBER_OF_SUBMITTERS"
msgstr "Total number of submitters"

#: exercise/templates/exercise/_exercise_wait.html
#: exercise/templates/exercise/exercise.html
#: exercise/templates/exercise/exercise.html templates/base.html
msgid "EXERCISE_ERROR_COMMUNICATION"
msgstr ""
"There was a problem loading the assignment. Try loading the page again "
Expand Down Expand Up @@ -3979,9 +3979,10 @@ msgstr "Posting submission..."
#: exercise/templates/exercise/_submit_progress.html
msgid "SUBMISSION_ERROR_ALERT_COMMUNICATION_ERROR_W_SERVICE"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ihalaij1 ihalaij1 Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

msgstr ""
"There was an error sending the submission for grading, so no submission was "
"consumed. You can try again. Make sure you're connected to the internet. "
"Course staff have been informed if the problem is with the service."
"An error occurred while sending the submission for grading. Make sure you're "
ihalaij1 marked this conversation as resolved.
Show resolved Hide resolved
"connected to the internet. If a submission was consumed, the submission will "
"be automatically graded after the service is available again. Course staff "
"have been informed in case the problem is with the service."

#: exercise/templates/exercise/_template_files.html
msgid "EXERCISE_HAS_NO_TEMPLATE"
Expand Down
9 changes: 5 additions & 4 deletions locale/fi/LC_MESSAGES/django.po
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3849,7 +3849,7 @@ msgid "TOTAL_NUMBER_OF_SUBMITTERS"
msgstr "Palauttaneita opiskelijoita yhteensä"

#: exercise/templates/exercise/_exercise_wait.html
#: exercise/templates/exercise/exercise.html
#: exercise/templates/exercise/exercise.html templates/base.html
msgid "EXERCISE_ERROR_COMMUNICATION"
msgstr ""
"Tehtävän lataaminen epäonnistui. Yritä uudelleen myöhemmin. "
Expand Down Expand Up @@ -3987,9 +3987,10 @@ msgstr "Palautusta lähetetään..."
#: exercise/templates/exercise/_submit_progress.html
msgid "SUBMISSION_ERROR_ALERT_COMMUNICATION_ERROR_W_SERVICE"
msgstr ""
"Palautuksen lähettämisessä arvosteluun tapahtui virhe eikä palautuskertoja "
"kulunut. Voit yrittää vielä uudestaan. Tarkistathan internet-yhteytesi. "
"Henkilökunnalle on ilmoitettu mikäli onglema on palvelussa."
"Palautuksen lähettämisessä arvosteluun tapahtui virhe. Tarkistathan internet-"
"yhteytesi. Jos palautuskerta käytettiin, palautus arvostellaan "
"automaattisesti, kun palvelu on jälleen käytettävissä. Kurssihenkilökunnalle "
"on ilmoitettu mikäli ongelma on palvelussa."

#: exercise/templates/exercise/_template_files.html
msgid "EXERCISE_HAS_NO_TEMPLATE"
Expand Down
3 changes: 3 additions & 0 deletions templates/base.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,9 @@ <h4>{% translate "SITE" %}</h4>
<div class="modal-dialog modal-lg">
<div class="modal-content">
<div class="modal-progress">
<div class="modal-submit-error alert alert-danger" style="display:none">
{% translate "EXERCISE_ERROR_COMMUNICATION" %}
</div>
<div class="progress">
<div class="progress-bar progress-bar-striped active" role="progressbar" style="width:100%"
data-msg-error="{% translate 'LOADING_FAILED' %}">
Expand Down