From 0e7e54b4aaad304563990a76fd69e0fa7d67a726 Mon Sep 17 00:00:00 2001
From: Arslan Ashraf <34372316+arslanashraf7@users.noreply.github.com>
Date: Mon, 18 Nov 2024 15:37:36 +0500
Subject: [PATCH 1/3] feat: add canvas integration support (#319)
* feat: add canvas integration support
(cherry picked from commit fdb818a4652e4dfdc8158ca5f15db95f8dd126c3)
---
lms/djangoapps/instructor/views/api.py | 3 +++
lms/djangoapps/instructor_task/views.py | 6 ++++++
.../instructor_dashboard/instructor_dashboard.js | 3 +++
lms/static/js/instructor_dashboard/util.js | 6 ++++--
.../html-datatable.underscore | 16 ++++++++++++++++
.../instructor_dashboard_2.html | 2 +-
6 files changed, 33 insertions(+), 3 deletions(-)
create mode 100644 lms/templates/instructor/instructor_dashboard_2/html-datatable.underscore
diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py
index 84058dfeae49..c486ec2ea447 100644
--- a/lms/djangoapps/instructor/views/api.py
+++ b/lms/djangoapps/instructor/views/api.py
@@ -2516,6 +2516,9 @@ def _list_instructor_tasks(request, course_id, serialize_data=None):
else:
# Specifying for single problem's history
tasks = task_api.get_instructor_task_history(course_id, module_state_key)
+ elif request.GET.get('include_canvas') is not None:
+ from ol_openedx_canvas_integration.task_helpers import get_filtered_instructor_tasks # pylint: disable=import-error
+ tasks = get_filtered_instructor_tasks(course_id, request.user)
else:
# If no problem or student, just get currently running tasks
tasks = task_api.get_running_instructor_tasks(course_id)
diff --git a/lms/djangoapps/instructor_task/views.py b/lms/djangoapps/instructor_task/views.py
index 51e2812577fe..c1a31e089901 100644
--- a/lms/djangoapps/instructor_task/views.py
+++ b/lms/djangoapps/instructor_task/views.py
@@ -138,7 +138,9 @@ def get_task_completion_info(instructor_task): # lint-amnesty, pylint: disable=
student = None
problem_url = None
+ entrance_exam_url = None
email_id = None
+ task_input = None
try:
task_input = json.loads(instructor_task.task_input)
except ValueError:
@@ -192,6 +194,10 @@ def get_task_completion_info(instructor_task): # lint-amnesty, pylint: disable=
else: # num_succeeded < num_attempted
# Translators: {action} is a past-tense verb that is localized separately. {succeeded} and {attempted} are counts. # lint-amnesty, pylint: disable=line-too-long
msg_format = _("Problem {action} for {succeeded} of {attempted} students")
+ elif task_input and task_input.get('course_key'):
+ from ol_openedx_canvas_integration.utils import get_task_output_formatted_message # pylint: disable=import-error
+ msg_format = get_task_output_formatted_message(task_output)
+ succeeded = True
elif email_id is not None:
# this reports on actions on bulk emails
if num_attempted == 0:
diff --git a/lms/static/js/instructor_dashboard/instructor_dashboard.js b/lms/static/js/instructor_dashboard/instructor_dashboard.js
index f87e9db8e814..14f47fce6f36 100644
--- a/lms/static/js/instructor_dashboard/instructor_dashboard.js
+++ b/lms/static/js/instructor_dashboard/instructor_dashboard.js
@@ -176,6 +176,9 @@ such that the value can be defined later than this assignment (file load order).
}, {
constructor: window.InstructorDashboard.sections.ECommerce,
$element: idashContent.find('.' + CSS_IDASH_SECTION + '#e-commerce')
+ }, {
+ constructor: window.InstructorDashboard.sections.CanvasIntegration,
+ $element: idashContent.find('.' + CSS_IDASH_SECTION + '#canvas_integration')
}, {
constructor: window.InstructorDashboard.sections.Membership,
$element: idashContent.find('.' + CSS_IDASH_SECTION + '#membership')
diff --git a/lms/static/js/instructor_dashboard/util.js b/lms/static/js/instructor_dashboard/util.js
index 4e24c44c77ec..3818ba189c76 100644
--- a/lms/static/js/instructor_dashboard/util.js
+++ b/lms/static/js/instructor_dashboard/util.js
@@ -51,7 +51,8 @@
enableColumnReorder: false,
autoHeight: true,
rowHeight: 100,
- forceFitColumns: true
+ forceFitColumns: true,
+ enableTextSelectionOnCells: true
};
columns = [
{
@@ -492,7 +493,8 @@
enableCellNavigation: true,
enableColumnReorder: false,
rowHeight: 30,
- forceFitColumns: true
+ forceFitColumns: true,
+ enableTextSelectionOnCells: true
};
columns = [
{
diff --git a/lms/templates/instructor/instructor_dashboard_2/html-datatable.underscore b/lms/templates/instructor/instructor_dashboard_2/html-datatable.underscore
new file mode 100644
index 000000000000..89da91b17881
--- /dev/null
+++ b/lms/templates/instructor/instructor_dashboard_2/html-datatable.underscore
@@ -0,0 +1,16 @@
+
+
<%- title %>
+
+ <%_.forEach(header, function (h) {%>
+ | <%- h %> |
+ <%})%>
+
+ <%_.forEach(data, function (row) {%>
+
+ <%_.forEach(row, function (value) {%>
+ | <%- value %> |
+ <%})%>
+
+ <%})%>
+
+
diff --git a/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html b/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html
index 27ed2d3dadcd..1479677245a1 100644
--- a/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html
+++ b/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html
@@ -91,7 +91,7 @@
## Include Underscore templates
<%block name="header_extras">
-% for template_name in ["cohorts", "discussions", "enrollment-code-lookup-links", "cohort-editor", "cohort-group-header", "cohort-selector", "cohort-form", "notification", "cohort-state", "divided-discussions-inline", "divided-discussions-course-wide", "cohort-discussions-category", "cohort-discussions-subcategory", "certificate-allowlist", "certificate-allowlist-editor", "certificate-bulk-allowlist", "certificate-invalidation", "membership-list-widget"]:
+% for template_name in ["cohorts", "discussions", "enrollment-code-lookup-links", "cohort-editor", "cohort-group-header", "cohort-selector", "cohort-form", "notification", "cohort-state", "divided-discussions-inline", "divided-discussions-course-wide", "cohort-discussions-category", "cohort-discussions-subcategory", "certificate-allowlist", "certificate-allowlist-editor", "certificate-bulk-allowlist", "certificate-invalidation", "membership-list-widget", "html-datatable"]:
From 4b6df42be375ecd75c41f6e345f074f2b67df3e1 Mon Sep 17 00:00:00 2001
From: "kshitij.sobti"
Date: Mon, 19 May 2025 15:42:25 +0530
Subject: [PATCH 2/3] feat: Use dropdown for units when more then 15
When dealing with subsections that have a lot of units, show a dropdown for
unit selection to simplify navigation.
(cherry picked from commit aaca11fd47aea5176137dd9970eed4ef850bae79)
---
lms/templates/seq_block.html | 22 +++++-
xmodule/js/src/sequence/display.js | 76 +++++++++++++++++++
.../SequenceBlockDisplay.css | 31 +++++++-
3 files changed, 126 insertions(+), 3 deletions(-)
diff --git a/lms/templates/seq_block.html b/lms/templates/seq_block.html
index 3c8699bc8f18..9c7c900e5084 100644
--- a/lms/templates/seq_block.html
+++ b/lms/templates/seq_block.html
@@ -4,6 +4,23 @@
from django.conf import settings
%>
+
+
% else:
% for idx, item in enumerate(items):
-
+
% endif
+ % if show_dropdown:
+ ## <%include file='seq_dropdown.html' args="items=items[15:], start_index=15"/>
+ % endif
diff --git a/xmodule/js/src/sequence/display.js b/xmodule/js/src/sequence/display.js
index 0c136a5b580f..c8a692fbbf95 100644
--- a/xmodule/js/src/sequence/display.js
+++ b/xmodule/js/src/sequence/display.js
@@ -4,6 +4,8 @@
(function() {
'use strict';
+ const { HtmlUtils } = window.edx;
+
this.Sequence = (function() {
function Sequence(element, runtime) {
var self = this;
@@ -26,6 +28,9 @@
this.goto = function(event) {
return Sequence.prototype.goto.apply(self, [event]);
};
+ this.toggleDropdown = function(event) {
+ return Sequence.prototype.toggleDropdown.apply(self, [event]);
+ };
this.toggleArrows = function() {
return Sequence.prototype.toggleArrows.apply(self);
};
@@ -38,6 +43,12 @@
this.displayTabTooltip = function(event) {
return Sequence.prototype.displayTabTooltip.apply(self, [event]);
};
+ this.renderDropdown = function() {
+ return Sequence.prototype.renderDropdown.apply(self);
+ }
+ this.handleClickOutsideDropdown = function(event) {
+ return Sequence.prototype.handleClickOutsideDropdown.apply(self, [event]);
+ }
this.arrowKeys = {
LEFT: 37,
UP: 38,
@@ -62,23 +73,87 @@
this.showCompletion = this.el.data('show-completion');
this.keydownHandler($(element).find('#sequence-list .tab'));
this.base_page_title = ($('title').data('base-title') || '').trim();
+ this.dropdownButtonTpl = _.template($('#dropdown-button-tpl').text())({});
+ this.renderDropdown();
this.bind();
this.render(parseInt(this.el.data('position'), 10));
}
+ Sequence.prototype.renderDropdown = function() {
+ // Renders the dropdown when there isn't enough space for all units in the bar
+ // Hide the dropdown by default and only show if needed.
+ this.$('#sequence-list > #dropdown-container').hide();
+ this.$(`#sequence-list > li.sequence-list-item`).show();
+ // Calculate the number of tabs that can fit comfortably and if the
+ // number of units is greater we show the dropdown.
+ const tabListWidth = this.$('#sequence-list').width();
+ const singleTabWidth = this.$('#sequence-list > li:first').width();
+ const tabCount = this.$('#sequence-list > li.sequence-list-item').length;
+ const overFlowCount = Math.floor(tabListWidth / singleTabWidth);
+ // Reduce 1 to offsets index and another one to accommodate the button
+ const overFlowIdx = overFlowCount - 2;
+ const showDropdown = overFlowCount < tabCount;
+ if (!showDropdown) {
+ return;
+ }
+ // If the dropdown button doesn't exist add it, otherwise move the
+ // existing button to the correct place.
+ if (this.$('#sequence-list > #dropdown-container').length === 0) {
+ // xss-lint: disable=javascript-jquery-insertion
+ this.$('#sequence-list > li.sequence-list-item').eq(overFlowIdx).after(this.dropdownButtonTpl);
+ } else {
+ this.$('#sequence-list > li.sequence-list-item').eq(overFlowIdx)
+ // xss-lint: disable=javascript-jquery-insertion
+ .after(this.$('#sequence-list > #dropdown-container'));
+ }
+ // Show the dropdown UX and hide all the overflowing unit buttons.
+ this.$('#sequence-list > #dropdown-container').show();
+ this.$(`#sequence-list > li.sequence-list-item:lt(${overFlowIdx + 1})`).show();
+ this.$(`#sequence-list > li.sequence-list-item:gt(${overFlowIdx})`).hide();
+ const dropdownList = this.$('#dropdown-sequence-list > ol');
+ // The dropdown buttons are modified copies of the unit nav buttons.
+ dropdownList.empty();
+ this.$(`#sequence-list > li.sequence-list-item:gt(${overFlowIdx})`).each((idx, el) => {
+ const cloneEl = $(el).clone();
+ const navButton = cloneEl.find("button");
+ const unitTitle = navButton.data('page-title');
+ navButton.click(self.goto);
+ navButton.find(".sequence-tooltip").remove();
+ navButton.find("span.icon").after(
+ HtmlUtils.joinHtml(HtmlUtils.HTML(''), unitTitle, HtmlUtils.HTML('')).toString()
+ );
+ //xss-lint: disable=javascript-jquery-insert-into-target
+ cloneEl.show().appendTo(dropdownList);
+ });
+ }
+
Sequence.prototype.$ = function(selector) {
return $(selector, this.el);
};
Sequence.prototype.bind = function() {
this.$('#sequence-list .nav-item').click(this.goto);
+ $(document).click(this.handleClickOutsideDropdown);
+ this.$('#dropdown-sequence-list .dropdown-item').click(this.goto);
+ this.$('#dropdown-sequence-list-button').click(this.toggleDropdown);
this.$('#sequence-list .nav-item').keypress(this.keyDownHandler);
this.el.on('bookmark:add', this.addBookmarkIconToActiveNavItem);
this.el.on('bookmark:remove', this.removeBookmarkIconFromActiveNavItem);
this.$('#sequence-list .nav-item').on('focus mouseenter', this.displayTabTooltip);
this.$('#sequence-list .nav-item').on('blur mouseleave', this.hideTabTooltip);
+ $(window).on('resize', _.debounce(this.renderDropdown.bind(this), 200));
};
+ Sequence.prototype.handleClickOutsideDropdown = function(event) {
+ if(!this.$('#dropdown-container')?.[0]?.contains(event.target)) {
+ this.$('#dropdown-sequence-list').hide();
+ }
+ }
+
+ Sequence.prototype.toggleDropdown = function() {
+ $('#dropdown-sequence-list').toggle();
+ }
+
Sequence.prototype.previousNav = function(focused, index) {
var $navItemList,
$sequenceList = $(focused).parent().parent();
@@ -289,6 +364,7 @@
Sequence.prototype.goto = function(event) {
var alertTemplate, alertText, isBottomNav, newPosition, widgetPlacement;
event.preventDefault();
+ this.$('#dropdown-sequence-list').hide();
// Links from courseware ..., was .target_tab
if ($(event.currentTarget).hasClass('seqnav')) {
diff --git a/xmodule/static/css-builtin-blocks/SequenceBlockDisplay.css b/xmodule/static/css-builtin-blocks/SequenceBlockDisplay.css
index 53671879f6ec..cbc5b19f7994 100644
--- a/xmodule/static/css-builtin-blocks/SequenceBlockDisplay.css
+++ b/xmodule/static/css-builtin-blocks/SequenceBlockDisplay.css
@@ -78,6 +78,13 @@
position: relative;
height: 100%;
flex-grow: 1;
+ max-width: calc(100% - 80px);
+}
+
+@media (min-width: 768px) {
+ .xmodule_display.xmodule_SequenceBlock .sequence-nav .sequence-list-wrapper {
+ max-width: calc(100% - 160px);
+ }
}
@media (max-width: 575.98px) {
@@ -91,7 +98,7 @@
display: flex;
}
-.xmodule_display.xmodule_SequenceBlock .sequence-nav ol li {
+.xmodule_display.xmodule_SequenceBlock .sequence-nav ol li, .dropdown-toggle {
box-sizing: border-box;
min-width: 40px;
flex-grow: 1;
@@ -104,7 +111,11 @@
border-right-style: solid;
}
-.xmodule_display.xmodule_SequenceBlock .sequence-nav ol li button {
+.xmodule_display.xmodule_SequenceBlock .sequence-nav ol li .dropdown-toggle {
+ height: 49px !important;
+}
+
+.xmodule_display.xmodule_SequenceBlock .sequence-nav ol li button, .dropdown-toggle {
width: 100%;
height: 49px;
position: relative;
@@ -119,6 +130,22 @@
overflow: visible;
}
+.xmodule_display.xmodule_SequenceBlock .sequence-nav #dropdown-container ol li button {
+ display: flex;
+ align-items: center;
+ padding-left: 0.5rem;
+ padding-right: 0.5rem;
+}
+
+.xmodule_display.xmodule_SequenceBlock .sequence-nav #dropdown-container ol li button .unit-title {
+ display: flex;
+ flex-grow: 1;
+ text-overflow: ellipsis;
+ overflow: hidden;
+ white-space: nowrap;
+ margin: 0 0.5rem;
+}
+
.xmodule_display.xmodule_SequenceBlock .sequence-nav ol li button .icon {
display: inline-block;
line-height: 100%;
From dc60e55d7c78ab32ccbabf401e0cbff46b66acf8 Mon Sep 17 00:00:00 2001
From: "kshitij.sobti"
Date: Fri, 29 Aug 2025 18:21:03 +0530
Subject: [PATCH 3/3] feat: Add support for using LTI data to populate user
profile
Currently the LTI provider implementation auto-creates a random user when
logging in, however, the LTI launch can include relevant user details such as
their email, full name and even a username. This change makes the LTI code
use the provided details if the "Use lti pii" setting is set in the Django
admin.
(cherry picked from commit 0bed7d7127408a20bc67746f5213a29c7dc03264)
---
.../0005_lticonsumer_use_lti_pii.py | 18 +++
lms/djangoapps/lti_provider/models.py | 4 +
.../lti_provider/tests/test_users.py | 107 ++++++++++++++++--
lms/djangoapps/lti_provider/users.py | 36 ++++--
lms/djangoapps/lti_provider/views.py | 4 +-
5 files changed, 151 insertions(+), 18 deletions(-)
create mode 100644 lms/djangoapps/lti_provider/migrations/0005_lticonsumer_use_lti_pii.py
diff --git a/lms/djangoapps/lti_provider/migrations/0005_lticonsumer_use_lti_pii.py b/lms/djangoapps/lti_provider/migrations/0005_lticonsumer_use_lti_pii.py
new file mode 100644
index 000000000000..9728dd8d60dd
--- /dev/null
+++ b/lms/djangoapps/lti_provider/migrations/0005_lticonsumer_use_lti_pii.py
@@ -0,0 +1,18 @@
+# Generated by Django 4.2.23 on 2025-08-29 12:43
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+ dependencies = [
+ ('lti_provider', '0004_require_user_account'),
+ ]
+
+ operations = [
+ migrations.AddField(
+ model_name='lticonsumer',
+ name='use_lti_pii',
+ field=models.BooleanField(blank=True, default=False, help_text='When checked, the platform will automatically use any personal information provided via LTI to create an account. Otherwise an anonymous account will be created.'),
+ ),
+ ]
diff --git a/lms/djangoapps/lti_provider/models.py b/lms/djangoapps/lti_provider/models.py
index 94582d4bf0b4..f763321748a9 100644
--- a/lms/djangoapps/lti_provider/models.py
+++ b/lms/djangoapps/lti_provider/models.py
@@ -40,6 +40,10 @@ class LtiConsumer(models.Model):
"in this instance. This is required only for linking learner accounts with "
"the LTI consumer. See the Open edX LTI Provider documentation for more details."
))
+ use_lti_pii = models.BooleanField(blank=True, default=False, help_text=_(
+ "When checked, the platform will automatically use any personal information provided "
+ "via LTI to create an account. Otherwise an anonymous account will be created."
+ ))
@staticmethod
def get_or_supplement(instance_guid, consumer_key):
diff --git a/lms/djangoapps/lti_provider/tests/test_users.py b/lms/djangoapps/lti_provider/tests/test_users.py
index fa8274eef30e..e94caae5a688 100644
--- a/lms/djangoapps/lti_provider/tests/test_users.py
+++ b/lms/djangoapps/lti_provider/tests/test_users.py
@@ -2,10 +2,11 @@
Tests for the LTI user management functionality
"""
-
+import itertools
import string
from unittest.mock import MagicMock, PropertyMock, patch
+import ddt
import pytest
from django.contrib.auth.models import AnonymousUser, User # lint-amnesty, pylint: disable=imported-auth-user
from django.core.exceptions import PermissionDenied
@@ -73,6 +74,7 @@ def test_random_username_generator(self):
f"Username has forbidden character '{username[char]}'"
+@ddt.ddt
@patch('lms.djangoapps.lti_provider.users.switch_user', autospec=True)
@patch('lms.djangoapps.lti_provider.users.create_lti_user', autospec=True)
class AuthenticateLtiUserTest(TestCase):
@@ -156,7 +158,10 @@ def test_auto_linking_of_users_using_lis_person_contact_email_primary(self, crea
create_user.assert_called_with(self.lti_user_id, self.lti_consumer)
users.authenticate_lti_user(request, self.lti_user_id, self.auto_linking_consumer)
- create_user.assert_called_with(self.lti_user_id, self.auto_linking_consumer, self.old_user.email)
+ create_user.assert_called_with(self.lti_user_id, self.auto_linking_consumer, {
+ "email": self.old_user.email,
+ "full_name": "",
+ })
def test_auto_linking_of_users_using_lis_person_contact_email_primary_case_insensitive(self, create_user, switch_user): # pylint: disable=line-too-long
request = RequestFactory().post("/", {"lis_person_contact_email_primary": self.old_user.email.upper()})
@@ -166,7 +171,10 @@ def test_auto_linking_of_users_using_lis_person_contact_email_primary_case_insen
create_user.assert_called_with(self.lti_user_id, self.lti_consumer)
users.authenticate_lti_user(request, self.lti_user_id, self.auto_linking_consumer)
- create_user.assert_called_with(self.lti_user_id, self.auto_linking_consumer, request.user.email)
+ create_user.assert_called_with(self.lti_user_id, self.auto_linking_consumer, {
+ "email": self.old_user.email,
+ "full_name": "",
+ })
def test_raise_exception_trying_to_auto_link_unauthenticate_user(self, create_user, switch_user):
request = RequestFactory().post("/")
@@ -190,7 +198,57 @@ def test_authenticate_unauthenticated_user_after_auto_linking_of_user_account(se
assert not create_user.called
switch_user.assert_called_with(self.request, lti_user, self.auto_linking_consumer)
+ @ddt.data(
+ *itertools.product(
+ (
+ (
+ {
+ "lis_person_contact_email_primary": "some_email@example.com",
+ "lis_person_name_given": "John",
+ "lis_person_name_family": "Doe",
+ },
+ "some_email@example.com",
+ "John Doe",
+ ),
+ (
+ {
+ "lis_person_contact_email_primary": "some_email@example.com",
+ "lis_person_name_full": "John Doe",
+ "lis_person_name_given": "Jacob",
+ },
+ "some_email@example.com",
+ "John Doe",
+ ),
+ (
+ {"lis_person_contact_email_primary": "some_email@example.com", "lis_person_name_full": "John Doe"},
+ "some_email@example.com",
+ "John Doe",
+ ),
+ ({"lis_person_contact_email_primary": "some_email@example.com"}, "some_email@example.com", ""),
+ ({"lis_person_contact_email_primary": ""}, "", ""),
+ ({"lis_person_contact_email_primary": ""}, "", ""),
+ ({}, "", ""),
+ ),
+ [True, False],
+ )
+ )
+ @ddt.unpack
+ def test_create_user_when_user_account_not_required(self, params, enable_lti_pii, create_user, switch_user):
+ post_params, email, name = params
+ self.auto_linking_consumer.require_user_account = False
+ self.auto_linking_consumer.use_lti_pii = enable_lti_pii
+ self.auto_linking_consumer.save()
+ request = RequestFactory().post("/", post_params)
+ request.user = AnonymousUser()
+ users.authenticate_lti_user(request, self.lti_user_id, self.auto_linking_consumer)
+ if enable_lti_pii:
+ profile = {"email": email, "full_name": name, "username": self.lti_user_id}
+ create_user.assert_called_with(self.lti_user_id, self.auto_linking_consumer, profile)
+ else:
+ create_user.assert_called_with(self.lti_user_id, self.auto_linking_consumer)
+
+@ddt.ddt
class CreateLtiUserTest(TestCase):
"""
Tests for the create_lti_user function in users.py
@@ -222,22 +280,22 @@ def test_create_lti_user_creates_correct_user(self, uuid_mock, _username_mock):
@patch('lms.djangoapps.lti_provider.users.generate_random_edx_username', side_effect=['edx_id', 'new_edx_id'])
def test_unique_username_created(self, username_mock):
User(username='edx_id').save()
- users.create_lti_user('lti_user_id', self.lti_consumer)
+ users.create_lti_user('lti_user_id', self.lti_consumer, None)
assert username_mock.call_count == 2
assert User.objects.count() == 3
user = User.objects.get(username='new_edx_id')
assert user.email == 'new_edx_id@lti.example.com'
def test_existing_user_is_linked(self):
- lti_user = users.create_lti_user('lti_user_id', self.lti_consumer, self.existing_user.email)
+ lti_user = users.create_lti_user('lti_user_id', self.lti_consumer, {"email": self.existing_user.email})
assert lti_user.lti_consumer == self.lti_consumer
assert lti_user.edx_user == self.existing_user
def test_only_one_lti_user_edx_user_for_each_lti_consumer(self):
- users.create_lti_user('lti_user_id', self.lti_consumer, self.existing_user.email)
+ users.create_lti_user('lti_user_id', self.lti_consumer, {"email": self.existing_user.email})
with pytest.raises(IntegrityError):
- users.create_lti_user('lti_user_id', self.lti_consumer, self.existing_user.email)
+ users.create_lti_user('lti_user_id', self.lti_consumer, {"email": self.existing_user.email})
def test_create_multiple_lti_users_for_edx_user_if_lti_consumer_varies(self):
lti_consumer_2 = LtiConsumer(
@@ -247,11 +305,42 @@ def test_create_multiple_lti_users_for_edx_user_if_lti_consumer_varies(self):
)
lti_consumer_2.save()
- lti_user_1 = users.create_lti_user('lti_user_id', self.lti_consumer, self.existing_user.email)
- lti_user_2 = users.create_lti_user('lti_user_id', lti_consumer_2, self.existing_user.email)
+ lti_user_1 = users.create_lti_user('lti_user_id', self.lti_consumer, {"email": self.existing_user.email})
+ lti_user_2 = users.create_lti_user('lti_user_id', lti_consumer_2, {"email": self.existing_user.email})
assert lti_user_1.edx_user == lti_user_2.edx_user
+ def test_create_lti_user_with_full_profile(self):
+ lti_user = users.create_lti_user('lti_user_id', self.lti_consumer, {
+ "email": "some.user@example.com",
+ "full_name": "John Doe",
+ "username": "john_doe",
+ })
+ assert lti_user.edx_user.email == "some.user@example.com"
+ assert lti_user.edx_user.username == "john_doe"
+ assert lti_user.edx_user.profile.name == "John Doe"
+
+ @patch('lms.djangoapps.lti_provider.users.generate_random_edx_username', side_effect=['edx_id'])
+ def test_create_lti_user_with_missing_username_in_profile(self, mock):
+ lti_user = users.create_lti_user('lti_user_id', self.lti_consumer, {
+ "email": "some.user@example.com",
+ "full_name": "John Doe",
+ })
+ assert lti_user.edx_user.email == "some.user@example.com"
+ assert lti_user.edx_user.username == "edx_id"
+ assert lti_user.edx_user.profile.name == "John Doe"
+
+ @patch('lms.djangoapps.lti_provider.users.generate_random_edx_username', side_effect=['edx_id', 'edx_id123'])
+ def test_create_lti_user_with_duplicate_username_in_profile(self, mock):
+ lti_user = users.create_lti_user('lti_user_id', self.lti_consumer, {
+ "email": "some.user@example.com",
+ "full_name": "John Doe",
+ "username": self.existing_user.username,
+ })
+ assert lti_user.edx_user.email == "some.user@example.com"
+ assert lti_user.edx_user.username == "edx_id"
+ assert lti_user.edx_user.profile.name == "John Doe"
+
class LtiBackendTest(TestCase):
"""
diff --git a/lms/djangoapps/lti_provider/users.py b/lms/djangoapps/lti_provider/users.py
index d1ade6ead325..168d6c4c6dc1 100644
--- a/lms/djangoapps/lti_provider/users.py
+++ b/lms/djangoapps/lti_provider/users.py
@@ -19,6 +19,20 @@
from openedx.core.djangoapps.safe_sessions.middleware import mark_user_change_as_expected
+def get_lti_user_details(request):
+ """
+ Returns key LTI user details from the LTI launch request.
+ """
+ post_data = request.POST
+ email = post_data.get("lis_person_contact_email_primary", "").lower()
+ full_name = post_data.get("lis_person_name_full", "")
+ given_name = post_data.get("lis_person_name_given", "")
+ family_name = post_data.get("lis_person_name_family", "")
+ if not full_name and given_name:
+ full_name = f"{given_name} {family_name}"
+ return dict(email=email, full_name=full_name)
+
+
def authenticate_lti_user(request, lti_user_id, lti_consumer):
"""
Determine whether the user specified by the LTI launch has an existing
@@ -28,7 +42,7 @@ def authenticate_lti_user(request, lti_user_id, lti_consumer):
If the currently logged-in user does not match the user specified by the LTI
launch, log out the old user and log in the LTI identity.
"""
- lis_email = request.POST.get("lis_person_contact_email_primary")
+ profile = get_lti_user_details(request)
try:
lti_user = LtiUser.objects.get(
@@ -40,11 +54,14 @@ def authenticate_lti_user(request, lti_user_id, lti_consumer):
if lti_consumer.require_user_account:
# Verify that the email from the LTI Launch and the logged-in user are the same
# before linking the LtiUser with the edx_user.
- if request.user.is_authenticated and request.user.email.lower() == lis_email.lower():
- lti_user = create_lti_user(lti_user_id, lti_consumer, request.user.email)
+ if request.user.is_authenticated and request.user.email.lower() == profile["email"]:
+ lti_user = create_lti_user(lti_user_id, lti_consumer, profile)
else:
# Ask the user to login before linking.
raise PermissionDenied() from exc
+ elif lti_consumer.use_lti_pii:
+ profile["username"] = lti_user_id
+ lti_user = create_lti_user(lti_user_id, lti_consumer, profile)
else:
lti_user = create_lti_user(lti_user_id, lti_consumer)
@@ -55,11 +72,15 @@ def authenticate_lti_user(request, lti_user_id, lti_consumer):
switch_user(request, lti_user, lti_consumer)
-def create_lti_user(lti_user_id, lti_consumer, email=None):
+def create_lti_user(lti_user_id, lti_consumer, profile=None):
"""
Generate a new user on the edX platform with a random username and password,
and associates that account with the LTI identity.
"""
+ if profile is None:
+ profile = {}
+ email = profile.get("email")
+ edx_user_id = profile.get("username") or generate_random_edx_username()
edx_user = User.objects.filter(email=email).first() if email else None
if not edx_user:
@@ -67,8 +88,7 @@ def create_lti_user(lti_user_id, lti_consumer, email=None):
edx_password = str(uuid.uuid4())
while not created:
try:
- edx_user_id = generate_random_edx_username()
- edx_email = f"{edx_user_id}@{settings.LTI_USER_EMAIL_DOMAIN}"
+ edx_email = email if email else f"{edx_user_id}@{settings.LTI_USER_EMAIL_DOMAIN}"
with transaction.atomic():
edx_user = User.objects.create_user(
username=edx_user_id,
@@ -78,13 +98,13 @@ def create_lti_user(lti_user_id, lti_consumer, email=None):
# A profile is required if PREVENT_CONCURRENT_LOGINS flag is set.
# TODO: We could populate user information from the LTI launch here,
# but it's not necessary for our current uses.
- edx_user_profile = UserProfile(user=edx_user)
+ edx_user_profile = UserProfile(user=edx_user, name=profile.get("full_name", ""))
edx_user_profile.save()
created = True
except IntegrityError:
+ edx_user_id = generate_random_edx_username()
# The random edx_user_id wasn't unique. Since 'created' is still
# False, we will retry with a different random ID.
- pass
lti_user = LtiUser(
lti_consumer=lti_consumer,
diff --git a/lms/djangoapps/lti_provider/views.py b/lms/djangoapps/lti_provider/views.py
index 1106908112ed..b4da6547ea1f 100644
--- a/lms/djangoapps/lti_provider/views.py
+++ b/lms/djangoapps/lti_provider/views.py
@@ -32,7 +32,9 @@
OPTIONAL_PARAMETERS = [
'context_title', 'context_label', 'lis_result_sourcedid',
- 'lis_outcome_service_url', 'tool_consumer_instance_guid'
+ 'lis_outcome_service_url', 'tool_consumer_instance_guid',
+ "lis_person_name_full", "lis_person_name_given", "lis_person_name_family",
+ "lis_person_contact_email_primary",
]