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

Fix LTI submission polling #1162

Closed
wants to merge 1 commit into from

Conversation

EerikSaksi
Copy link
Contributor

Fixes #1114

What?
Fix not polling the grader for results in the LTI exercise view

Why?
The user might think the grading is stuck, or would have to constantly refresh to get the results

How?
The _lti_exercise_wait.html template wasn't being loaded, but now loads in the super block.

Fixes #1114

Testing

Remember to add or update unit tests for new features and changes.

What type of test did you run?

  • Accessibility test using the WAVE extension.
  • Django unit tests.
  • Selenium tests.
  • Other test. (Add a description below)
  • Manual testing.

Added sleep to exercise to simulate grader delay in local environment, ensured that the grader is being polled, and that the UI is updated when the grading result is ready.

Did you test the changes in

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

Think of what is affected by these changes and could become broken

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

Clean up your git commit history before submitting the pull request!

@markkuriekkinen markkuriekkinen self-requested a review April 14, 2023 10:22
@markkuriekkinen markkuriekkinen self-assigned this Apr 14, 2023
Copy link
Contributor

@markkuriekkinen markkuriekkinen left a comment

Choose a reason for hiding this comment

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

We could reorganize this slightly better still.

{% block exerciseinfo %}
{{ block.super }}
{% include 'exercise/_submission_info.html' %}
{% include "lti_tool/_lti_exercise_wait.html" %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think _lti_exercise_wait.html belongs to the exerciseinfo block. The exerciseinfo block is intended for the column on the right side of the exercise and submission pages.

Block exercise_wait is confusing since it is not defined at all in exercise_base.html or exercise.html. The purpose of the block exercise_wait needs to be checked. Perhaps, we will remove it completely also from submission.html.

{% block exercise_wait %}
{% include "lti_tool/_lti_exercise_wait.html" %}
{% endblock %}

{% block exerciseinfo %}
{{ block.super }}
{% include 'exercise/_submission_info.html' %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{% include 'exercise/_submission_info.html' %}

Since this template inherits submission.html, {{ block.super }} already includes submission_info.html. Including it twice causes the same box to appear twice in the rendered html page.

@EerikSaksi
Copy link
Contributor Author

The loading bar seems to go into the correct place by moving it inside the exercisecontent block and calling super.
Screenshot from 2023-04-19 12-05-02

{% block exercisecontent %}
	{% include "lti_tool/_lti_exercise_wait.html" %}
	{{ block.super }}
{% endblock %}

Should I find a better solution or is this viable?

markkuriekkinen added a commit to markkuriekkinen/a-plus that referenced this pull request May 19, 2023
The A+ LTI Tool v1.3 (apluslms#1104) implementation broke the polling of
the submission feedback in full-view exercise pages.
The polling was also broken in the LTI exercise full-view pages.

Remove the URL `lti-submission-poll` because it used the same view
as the normal `submission-poll` URL. The template
`_lti_exercise_wait.html` is not needed since it was a copy of
`_exercise_wait.html` with a different poll URL.

The template block `exercise_wait` is now unnecessary.
It was added in apluslms#1104. Adding the block was a mistake originally
since the parent templates did not have such a block and
it basically did not render anything. The block `exercise_wait`
was not part of any existing block.

The template `lti_submission.html` should not override
the block `exerciseinfo`. It was copied from `submission.html` and
the submission info page was thus rendered twice in
the LTI submission page.

Fixes apluslms#1114
Closes apluslms#1162
@markkuriekkinen
Copy link
Contributor

I chose a different approach and finished it in the commit ad95bc9.

It is important to note that the submission feedback polling has been broken since #1104 in the normal full-view exercise pages, not only in the LTI Tool exercise pages. I fixed them all in my commit.

About what Eerik suggested:

{% block exercisecontent %}
	{% include "lti_tool/_lti_exercise_wait.html" %}
	{{ block.super }}
{% endblock %}

Should I find a better solution or is this viable?

The template block exercise_wait was broken. When I removed it, the include _exercise_wait.html part was again back inside the block exercisecontent. Then, lti_submission.html does not need to separately include _exercise_wait.html any longer.

Your suggestion also moved the waiting progress bar to the top, even though it has been located under the other content.

markkuriekkinen added a commit to markkuriekkinen/a-plus that referenced this pull request Jun 2, 2023
The A+ LTI Tool v1.3 (apluslms#1104) implementation broke the polling of
the submission feedback in full-view exercise pages.
The polling was also broken in the LTI exercise full-view pages.

Remove the URL `lti-submission-poll` because it used the same view
as the normal `submission-poll` URL. The template
`_lti_exercise_wait.html` is not needed since it was a copy of
`_exercise_wait.html` with a different poll URL.

Poll.js is changed so that the final target URL can be configured
through HTML data attributes. Previously, poll.js had a hardcoded
assumption that the final target URL could be derived from
the poll URL by dropping the word "poll" from the end of
the poll URL. That was not flexible enough for LTI Tool URLs that
start with the "/lti" prefix.

The template block `exercise_wait` is now unnecessary.
It was added in apluslms#1104. Adding the block was a mistake originally
since the parent templates did not have such a block and
it basically did not render anything. The block `exercise_wait`
was not part of any existing block.

The template `lti_submission.html` should not override
the block `exerciseinfo`. It was copied from `submission.html` and
the submission info page was thus rendered twice in
the LTI submission page.

Fixes apluslms#1114
Closes apluslms#1162
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

LTI Tool: Problems with submission received view
2 participants