Use A+ as an LTI 1.3 tool #1104

murhum1 · 2022-12-02T09:34:20Z

Description

This PR adds functionality to A+ that enables it to be used as an LTI 1.3 tool in Moodle and possibly other LTI platforms.

Why?
Historically, the A+ Astra moodle plugin that enables A+ exercises to be used in Moodle, required updates whenever the deployed Moodle version changed. By using the LTI standard to add the functionality instead, we ensure compatibility through version updates.

How?
The PR adds LTI protocol specific urls, views, and templates that allow embedding A+ material in an LTI context, and allow users to submit exercises to be graded in A+, with grading results being sent back to the Moodle gradebook. LTI deep linking is also supported, allowing teachers to select content via a UI.

Testing

What type of test did you run?

Tested in Moodle:

Adding A+ as an LTI tool via Moodle's admin user interface
Selecting content to embed in an activity as a teacher
Browsing embedded material as student and teacher
Submitting exercises as student and teacher
Verifying that grades are recorded in Moodle's gradebook
- Teacher grades are not sent to Moodle, though

Did you test the changes in

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

Mostly Firefox; Chrome only occasionally during development.

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

Most of the functionality is standalone and should not affect main A+ functionality. Some templates have been modified, mainly adding overwritable blocks, which shouldn't break things. Some code was added to submitting exercise solutions and receiving grades, but the additions are only used in LTI contexts.

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).

Documentation regarding the local development setup should be added later.

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

Known issues

ACOS exercises do not have their grading results sent to Moodle - this still requires extra thought.

PasiSa

Made some minor stylistic comments. There are quite a lot of commits, I think many of them could still be combined together (just based on a quick glance on commit messages).

exercise/views.py

lti_tool/templates/lti_tool/exercise_list.html

lti_tool/urls.py

lti_tool/utils.py

lti_tool/urls.py

lti_tool/utils.py

PasiSa · 2022-12-06T12:13:26Z

lti_tool/utils.py

+from userprofile.models import UserProfile
+from course.models import Enrollment
+
+def get_launch_url(request):


Recently we have applied type hints at method definitions. Could do so also in these new lti_tool modules, e.g.
def get_launch_url(request: HttpRequest) -> str:

Now that we are running out time, I think we have to skip the static type hinting.

lti_tool/views.py

ihalaij1

I requested some minor changes.

ihalaij1 · 2022-12-08T09:10:07Z

exercise/templates/exercise/submission.html

+		<div id="exercise">
+			{{ submission.feedback|safe }}
+		</div>


I don't think the HTML inside Jinja2 statements should be indented more. If you imagine the HTML with the if-elif-else-statements removed, then it is incorrectly indented. This file should be checked and the other indentations also reverted.

Note our coding style guide: https://apluslms.github.io/contribute/styleguides/html/

ihalaij1 · 2022-12-08T09:13:59Z

lti_tool/templates/lti_tool/lti_submission.html

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


The include shouldn't be indented.

Why not? It is easier to read when the code inside a block or tag is indented more than the surrounding block/tag.

lti_tool/views.py

markkuriekkinen

Nice work! I have plenty of comments, but most of them are just small changes/improvements.

You asked about the Acos exercises. I think their points are received in the following API function and the submission is also created in the A+ database only at that time:

a-plus/exercise/api/views.py

Lines 125 to 132 in 9b530cb

    
           status, errors, students = self.exercise.check_submission_allowed(student) 
        
           if status != self.exercise.SUBMIT_STATUS.ALLOWED: 
        
               return Response({'success': False, 'errors': errors}) 
        
           submission = Submission.objects.create(exercise=self.exercise) 
        
           submission.submitters.set(students) 
        
           # grade and update submission with data 
        
           return Response(_post_async_submission(request, self.exercise, submission, errors))

requirements.txt

markkuriekkinen · 2022-12-08T10:50:30Z

exercise/templates/exercise/submission.html

+		<div id="exercise">
+			{{ submission.feedback|safe }}
+		</div>


Note our coding style guide: https://apluslms.github.io/contribute/styleguides/html/

aplus/settings.py

lib/models.py

lti_tool/views.py

exercise/protocol/aplus.py

markkuriekkinen · 2022-12-16T17:23:03Z

@murhum1 after your git force push, the commit history got broken in the same way what happened once before. Your branch contains a few unwanted copies of commits that already exist in the master branch. Though, you have rebased them so that their commit checksums have changed.

murhum1 · 2022-12-19T08:45:18Z

@murhum1 after your git force push, the commit history got broken in the same way what happened once before. Your branch contains a few unwanted copies of commits that already exist in the master branch. Though, you have rebased them so that their commit checksums have changed.

Sorry, seems like I accidentally included extra commits in the rebase. This should be fixed now.

markkuriekkinen

Great! I have a couple of more comments that are quick to address. Are you able to finish them before the holidays?

Question about the git commit history: it looks like you have squashed all the git commits into one. Do you prefer to keep it that way? I think you previously said that one large commit is not optimal, but I can accept the squashed history too. Separating git commits is very time-consuming and we don't have much time left.

lti_tool/utils.py

lti_tool/views.py

markkuriekkinen · 2022-12-20T17:48:40Z

lti_tool/views.py

+
+            if "exercise_path" in launch_params:
+                url_params = {
+                    k: launch_params[k] for k in ["exercise_path", "module_slug", "course_slug", "instance_slug"]


I suppose it is possible that the teacher modifies the custom parameters in the Moodle external activity settings. Then, if they remove, e.g., the "module_slug" parameter, this line crashes to KeyError since the launch_params dictionary does not contain the key. At least, the students can't do that to cause crashes on purpose.

lti_tool/views.py

markkuriekkinen · 2022-12-20T18:21:08Z

lti_tool/views.py

+            for e in children:
+                if e.max_points > 0:
+                    li = LineItem()
+                    li.set_tag(str(e.id)).set_score_maximum(e.max_points).set_label(str(e))


Did we yet talk about multilingual courses (Finnish and English materials)? What kind of name is displayed in the Moodle gradebook for the exercise grade item? Anyway, if it contains both languages now, we can return to that later and clean it up.

It does contain both languages - let's check up on that later, then.

markkuriekkinen · 2022-12-20T18:43:14Z

lti_tool/views.py

+        # Create gradebook items
+        ags = self.message_launch.get_ags()
+        children = self.exercise.children.all()
+        if len(children) > 0:


Does the existence of children imply that this is a chapter with embedded exercises in it? Then, the gradebook items are created for the child exercises, right?

This is not important now, but in theory, any learning object may have several layers of children. I think exercise.children includes only the direct children, not the "grandchildren" etc. We don't really have any courses that use such extra layers of children.

Yea, right now it only creates gradebook items for children and not grandchildren - was not aware there could be more layers.

murhum1 · 2022-12-21T09:48:57Z

Thanks for the comments! I should've fixed the last issues now.

I figured squashing the commits to one (+ PR change requests separately) was the least painful way of doing this for now.

markkuriekkinen

Thanks! I will squash the git commits and merge this pull request now. I will soon also write more comments that we could discuss after the holidays. Further fixing and tweaking may continue in new pull requests.

markkuriekkinen · 2022-12-21T13:51:46Z

lti_tool/views.py

+
+class LtiSessionMixin(BaseMixin):
+
+    @csrf_exempt


I got surprised that you added this csrf_exempt in the last changes. Why is this needed? Through this mixin class, I assume the exempt affects almost all LTI views.

markkuriekkinen · 2022-12-21T14:14:29Z

exercise/async_views.py

+        try:
+            if submission.lti_launch_id:
+                send_lti_points(request, submission)
+        except AttributeError:


I don't think this try-except is needed now that the property submission.lti_launch_id contains the same try-except.

A+ may be used as an external tool from LTI platforms such as Moodle. There are some unfinished parts left in this feature, for example, the LTI grading service is missing for Acos server assignments and other assignment services that create new, graded submissions by sending the submission data via the assignment service in the end of the student's workflow. The discussion in the pull request contains more items to be fixed later. apluslms#1104 Mikko and Eerik implemented small parts of the feature. Markus did most of the work in this commit. Co-authored-by: Murhu Mikko <mikko.murhu@aalto.fi> Co-authored-by: Saksi Eerik <saksi.eerik@gmail.com>

markkuriekkinen · 2023-01-04T19:24:30Z

Remaining issues that I know thus far.

Bigger design question:

Is it necessary to make new view classes for LTI: LtiExerciseView etc.? What if they used the same normal views like ExerciseView and the same URLs accordingly? New /lti/ URLs cause extra work in changing the URLs in the views. The new LTI view classes manipulate the URLs saved in the cache on the fly and it looks hacky.

The LTI classes mainly just set different HTML templates and otherwise, they are the same as the normal views. A view class could also select HTML templates dynamically in the get_template_names() method:
https://docs.djangoproject.com/en/3.2/ref/class-based-views/mixins-simple/#django.views.generic.base.TemplateResponseMixin.get_template_names

Pseudo-code about the idea: normal views could use different LTI templates if the request includes LTI parameters. Otherwise, the normal template is rendered.

def get_template_names(self, ...):
    if request includes LTI parameters:
        # "lti" should maybe be inserted in the middle so that the directory paths would make more sense, but this is just a sketch.
        return "lti/" + self.template_name
    else:
        return self.template_name

Is this related at all? Iframe links and error http status: #1115

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

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

PasiSa requested changes Dec 6, 2022

View reviewed changes

ihalaij1 requested changes Dec 8, 2022

View reviewed changes

markkuriekkinen self-requested a review December 8, 2022 11:38

markkuriekkinen self-assigned this Dec 8, 2022

markkuriekkinen suggested changes Dec 9, 2022

View reviewed changes

markkuriekkinen assigned murhum1 and unassigned markkuriekkinen Dec 9, 2022

murhum1 force-pushed the lti branch from 870858d to d2e29b3 Compare December 16, 2022 09:18

murhum1 force-pushed the lti branch from d2e29b3 to 350ff0f Compare December 19, 2022 08:40

markkuriekkinen suggested changes Dec 20, 2022

View reviewed changes

markkuriekkinen assigned markkuriekkinen and unassigned murhum1 Dec 21, 2022

markkuriekkinen approved these changes Dec 21, 2022

View reviewed changes

markkuriekkinen force-pushed the lti branch from 53c6ab5 to 1a125cb Compare December 21, 2022 18:24

markkuriekkinen merged commit 9bf29a8 into apluslms:master Dec 21, 2022

markkuriekkinen added this to the v1.18 milestone Dec 21, 2022

markkuriekkinen added area: LTI LTI protocol (Learning Tools Interoperability, worldwide standard) effort: months Solving this issue could take months. Starting with the creation of a new branch to the merging labels Dec 21, 2022

markkuriekkinen mentioned this pull request Dec 21, 2022

A+ as LTI Tool v1.3: properly set up CSRF tokens and do not exempt unnecessarily #1099

Closed

markkuriekkinen mentioned this pull request Jan 20, 2023

LTI Tool v1.3: add LTI grading support for Acos exercises (AJAX submission) #1154

Closed

This was referenced Feb 3, 2023

LTI Tool v1.3: Launch view crashes if expected LTI parameters are missing #1155

Open

LTI Tool: Problems with submission received view #1114

Closed

markkuriekkinen mentioned this pull request May 19, 2023

Fix LTI submission polling #1162

Closed

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use A+ as an LTI 1.3 tool #1104

Use A+ as an LTI 1.3 tool #1104

murhum1 commented Dec 2, 2022

PasiSa left a comment

PasiSa Dec 6, 2022

markkuriekkinen Dec 20, 2022

ihalaij1 left a comment

ihalaij1 Dec 8, 2022

markkuriekkinen Dec 8, 2022

ihalaij1 Dec 8, 2022

markkuriekkinen Dec 9, 2022

markkuriekkinen left a comment

markkuriekkinen Dec 8, 2022

markkuriekkinen commented Dec 16, 2022

murhum1 commented Dec 19, 2022

markkuriekkinen left a comment

markkuriekkinen Dec 20, 2022

markkuriekkinen Feb 3, 2023

markkuriekkinen Dec 20, 2022

murhum1 Dec 21, 2022

markkuriekkinen Dec 20, 2022

murhum1 Dec 21, 2022

murhum1 commented Dec 21, 2022

markkuriekkinen left a comment

markkuriekkinen Dec 21, 2022

markkuriekkinen Dec 21, 2022

markkuriekkinen commented Jan 4, 2023 •

edited

Loading

	status, errors, students = self.exercise.check_submission_allowed(student)
	if status != self.exercise.SUBMIT_STATUS.ALLOWED:
	return Response({'success': False, 'errors': errors})
	submission = Submission.objects.create(exercise=self.exercise)
	submission.submitters.set(students)

	# grade and update submission with data
	return Response(_post_async_submission(request, self.exercise, submission, errors))

Use A+ as an LTI 1.3 tool #1104

Use A+ as an LTI 1.3 tool #1104

Conversation

murhum1 commented Dec 2, 2022

Description

Testing

Programming style

Have you updated the README or other relevant documentation?

Is it Done?

PasiSa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihalaij1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markkuriekkinen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markkuriekkinen commented Dec 16, 2022

murhum1 commented Dec 19, 2022

markkuriekkinen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murhum1 commented Dec 21, 2022

markkuriekkinen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markkuriekkinen commented Jan 4, 2023 • edited Loading

markkuriekkinen commented Jan 4, 2023 •

edited

Loading