feat: Improve LTI profile user

openedx_lti_tool_plugin/models.py

@@ -3,10 +3,12 @@
 import uuid
 from typing import TypeVar
 
+import shortuuid
 from django.contrib.auth import get_user_model
 from django.contrib.auth.models import AbstractBaseUser
 from django.core.exceptions import ValidationError
 from django.db import models, transaction
+from django.db.models import Q
 from django.utils.translation import gettext_lazy as _
 from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import CourseKey
@@ -82,33 +84,84 @@ def __init__(self, *args: tuple, **kwargs: dict):
         self._initial_pii = self.pii
 
     @property
-    def username(self) -> str:
-        """str: Username."""
-        return f'{app_config.name}.{self.uuid}'
+    def short_uuid(self) -> str:
+        """str: Short UUID."""
+        return shortuuid.encode(self.uuid)[:8]
 
     @property
-    def email(self) -> str:
-        """str: Email address."""
-        return f'{self.uuid}@{app_config.name}'
+    def given_name(self) -> str:
+        """str: Given name."""
+        return self.pii.get('given_name', '')
+
+    @property
+    def middle_name(self) -> str:
+        """str: Middle name."""
+        return self.pii.get('middle_name', '')
+
+    @property
+    def family_name(self) -> str:
+        """str: Family name."""
+        return self.pii.get('family_name', '')
 
     @property
     def name(self) -> str:
         """str: Name."""
-        name = self.pii.get('name', '')
-        given_name = self.pii.get('given_name', '')
-        middle_name = self.pii.get('middle_name', '')
-        family_name = self.pii.get('family_name', '')
+        if self.given_name and self.middle_name and self.family_name:
+            return f'{self.given_name} {self.middle_name} {self.family_name}'
+
+        if self.given_name and self.family_name:
+            return f'{self.given_name} {self.family_name}'
+
+        return self.given_name
+
+    @property
+    def username(self) -> str:
+        """str: Username."""
+        if not self.given_name:
+            return f'{self.short_uuid}'
 
-        if name:
-            return name
+        return (
+            f'{self.given_name.lower()[:30]}'
+            f'{self.family_name.lower()[:1]}'
+            f'.{self.short_uuid}'
+        )
 
-        if given_name and middle_name and family_name:
-            return f'{given_name} {middle_name} {family_name}'
+    @property
+    def email(self) -> str:
+        """str: Email address."""
+        return f'{self.uuid}@{app_config.name}'
 
-        if given_name and family_name:
-            return f'{given_name} {family_name}'
+    def user_collision(self) -> bool:
+        """Check for user collision.
 
-        return given_name
+        Returns:
+            True if a user collides with another user.
+            False if no user collision is found.
+
+        """
+        return get_user_model().objects.filter(
+            Q(email=self.email)
+            | Q(username=self.username)
+        ).exclude(
+            email=self.email,
+            username=self.username,
+        ).exists()
+
+    def configure_user(self):
+        """Configure user."""
+        # Configure if no user is set.
+        if getattr(self, 'user', None):
+            return
+        # Regenerate uuid on user collision.
+        while self.user_collision():
+            self.uuid = uuid.uuid4()
+        # Get or create user.
+        self.user, _created = get_user_model().objects.get_or_create(
+            email=self.email,
+            username=self.username,
+        )
+        # Set unusable user password.
+        self.user.set_unusable_password()
 
     @transaction.atomic
     def save(self, *args: tuple, **kwargs: dict):
@@ -121,18 +174,12 @@ def save(self, *args: tuple, **kwargs: dict):
         """
         # Merge initial instance PII data with new PII data.
         self.pii = {**self._initial_pii, **self.pii}
-        # Get or create user.
-        if not getattr(self, 'user', None):
-            self.user, _created = get_user_model().objects.get_or_create(
-                username=self.username,
-                email=self.email,
-            )
-            # Set unusable user password.
-            self.user.set_unusable_password()
+        # Configure user.
+        self.configure_user()
         # Update or create user profile.
         user_profile().objects.update_or_create(
             user=self.user,
-            defaults={'name': self.name},
+            defaults={'name': self.name[:255]},
         )
 
         return super().save(*args, **kwargs)

openedx_lti_tool_plugin/tests/test_models.py

@@ -1,4 +1,7 @@
 """Test models module."""
+import random
+import string
+import uuid
 from unittest.mock import MagicMock, call, patch
 
 import ddt
@@ -19,6 +22,7 @@
 GIVEN_NAME = 'random-given-name'
 MIDDLE_NAME = 'random-middle-name'
 FAMILY_NAME = 'random-family-name'
+GIVEN_NAME_LARGER = ''.join(random.choices(string.ascii_lowercase, k=300))
 
 
 @ddt.ddt
@@ -29,7 +33,7 @@ def setUp(self):
         """Set up test fixtures."""
         super().setUp()
         self.pii = {'x': 'x'}
-        self.new_pii = {'y': 'y'}
+        self.new_pii = {'y': 'y', 'name': GIVEN_NAME_LARGER}
         self.lti_profile = LtiProfile.objects.create(
             platform_id=ISS,
             client_id=AUD,
@@ -42,83 +46,152 @@ def test_class_instance_attributes(self):
         """Test class instance attributes."""
         self.assertEqual(self.lti_profile._initial_pii, self.pii)
 
-    @patch(f'{MODULE_PATH}.super')
-    @patch(f'{MODULE_PATH}.user_profile')
+    @patch.object(get_user_model().objects, 'filter')
+    @patch(f'{MODULE_PATH}.Q')
+    def test_user_collision(
+        self,
+        q_mock: MagicMock,
+        user_filter_mock: MagicMock,
+    ):
+        """Test user_collision method."""
+        result = self.lti_profile.user_collision()
+
+        q_mock.assert_has_calls([
+            call(email=self.lti_profile.email),
+            call(username=self.lti_profile.username),
+        ])
+        user_filter_mock.assert_called_once_with(q_mock() | q_mock())
+        user_filter_mock().exclude.assert_called_once_with(
+            email=self.lti_profile.email,
+            username=self.lti_profile.username,
+        )
+        user_filter_mock().exclude().exists.assert_called_once_with()
+        self.assertEqual(result, user_filter_mock().exclude().exists())
+
     @patch.object(get_user_model(), 'set_unusable_password')
     @patch.object(get_user_model().objects, 'get_or_create')
-    @patch(f'{MODULE_PATH}.getattr', return_value=False)
-    def test_save_without_user(
+    @patch(f'{MODULE_PATH}.uuid.uuid4', return_value=uuid.uuid4())
+    @patch.object(LtiProfile, 'user_collision')
+    @patch(f'{MODULE_PATH}.getattr', return_value=None)
+    def test_configure_user_without_user_with_user_collision(
         self,
         getattr_mock: MagicMock,
+        user_collision_mock: MagicMock,
+        uuid4_mock: MagicMock,
         user_get_or_create_mock: MagicMock,
         user_set_unusable_password_mock: MagicMock,
-        user_profile_mock: MagicMock,
-        super_mock: MagicMock,
     ):
-        """Test save method without LtiProfile.user attribute value."""
+        """Test configure_user method without user with user collision."""
+        user_collision_mock.side_effect = [True, False]
         user_get_or_create_mock.return_value = self.lti_profile.user, None
-        self.lti_profile.pii = self.new_pii
 
-        self.lti_profile.save([], {})
+        self.lti_profile.configure_user()
 
-        self.assertEqual(self.lti_profile.pii, {**self.pii, **self.new_pii})
         getattr_mock.assert_called_once_with(self.lti_profile, 'user', None)
+        user_collision_mock.assert_has_calls([call(), call()])
+        uuid4_mock.assert_called_once_with()
         user_get_or_create_mock.assert_called_once_with(
-            username=self.lti_profile.username,
             email=self.lti_profile.email,
+            username=self.lti_profile.username,
         )
         user_set_unusable_password_mock.assert_called_once_with()
-        user_profile_mock().objects.update_or_create.assert_called_once_with(
-            user=self.lti_profile.user,
-            defaults={'name': self.lti_profile.name},
+
+    @patch.object(get_user_model(), 'set_unusable_password')
+    @patch.object(get_user_model().objects, 'get_or_create')
+    @patch(f'{MODULE_PATH}.uuid.uuid4')
+    @patch.object(LtiProfile, 'user_collision', return_value=False)
+    @patch(f'{MODULE_PATH}.getattr', return_value=None)
+    def test_configure_user_without_user_witout_user_collision(
+        self,
+        getattr_mock: MagicMock,
+        user_collision_mock: MagicMock,
+        uuid4_mock: MagicMock,
+        user_get_or_create_mock: MagicMock,
+        user_set_unusable_password_mock: MagicMock,
+    ):
+        """Test configure_user method without user without user collision."""
+        user_get_or_create_mock.return_value = self.lti_profile.user, None
+
+        self.lti_profile.configure_user()
+
+        getattr_mock.assert_called_once_with(self.lti_profile, 'user', None)
+        user_collision_mock.assert_called_once_with()
+        uuid4_mock.assert_not_called()
+        user_get_or_create_mock.assert_called_once_with(
+            email=self.lti_profile.email,
+            username=self.lti_profile.username,
         )
-        super_mock().save.assert_called_once_with([], {})
+        user_set_unusable_password_mock.assert_called_once_with()
 
-    @patch(f'{MODULE_PATH}.super')
-    @patch(f'{MODULE_PATH}.user_profile')
     @patch.object(get_user_model(), 'set_unusable_password')
     @patch.object(get_user_model().objects, 'get_or_create')
+    @patch.object(get_user_model().objects, 'filter')
     @patch(f'{MODULE_PATH}.getattr')
-    def test_save_with_user(
+    def test_configure_user_with_user(
         self,
         getattr_mock: MagicMock,
+        user_filter_mock: MagicMock,
         user_get_or_create_mock: MagicMock,
         user_set_unusable_password_mock: MagicMock,
+    ):
+        """Test configure_user method with user."""
+        user_get_or_create_mock.return_value = self.lti_profile.user, None
+
+        self.lti_profile.configure_user()
+
+        getattr_mock.assert_called_once_with(self.lti_profile, 'user', None)
+        user_filter_mock.assert_not_called()
+        user_filter_mock().exclude.assert_not_called()
+        user_get_or_create_mock.assert_not_called()
+        user_set_unusable_password_mock.assert_not_called()
+
+    @patch(f'{MODULE_PATH}.super')
+    @patch(f'{MODULE_PATH}.user_profile')
+    @patch.object(LtiProfile, 'configure_user')
+    def test_save(
+        self,
+        configure_user_mock: MagicMock,
         user_profile_mock: MagicMock,
         super_mock: MagicMock,
     ):
-        """Test save method with LtiProfile.user attribute value."""
-        user_get_or_create_mock.return_value = self.lti_profile.user, None
+        """Test save method."""
         self.lti_profile.pii = self.new_pii
 
         self.lti_profile.save([], {})
 
         self.assertEqual(self.lti_profile.pii, {**self.pii, **self.new_pii})
-        getattr_mock.assert_called_once_with(self.lti_profile, 'user', None)
-        user_get_or_create_mock.assert_not_called()
-        user_set_unusable_password_mock.assert_not_called()
+        configure_user_mock.assert_called_once_with()
         user_profile_mock().objects.update_or_create.assert_called_once_with(
             user=self.lti_profile.user,
-            defaults={'name': self.lti_profile.name},
+            defaults={'name': self.lti_profile.name[:255]},
         )
         super_mock().save.assert_called_once_with([], {})
 
-    def test_username_property(self):
-        """Test username property."""
-        self.assertEqual(
-            self.lti_profile.username,
-            f'{app_config.name}.{self.lti_profile.uuid}',
-        )
+    @patch(f'{MODULE_PATH}.shortuuid.encode')
+    def test_short_uuid_property(self, encode_mock: MagicMock):
+        """Test short_uuid property."""
+        self.assertEqual(self.lti_profile.short_uuid, encode_mock.return_value[:8])
+        encode_mock.assert_called_once_with(self.lti_profile.uuid)
 
-    def test_email_property(self):
-        """Test email property."""
-        self.assertEqual(
-            self.lti_profile.email,
-            f'{self.lti_profile.uuid}@{app_config.name}',
-        )
+    def test_given_name_property(self):
+        """Test given_name property."""
+        self.lti_profile.pii = {'given_name': GIVEN_NAME}
+
+        self.assertEqual(self.lti_profile.given_name, GIVEN_NAME)
+
+    def test_middle_name_property(self):
+        """Test middle_name property."""
+        self.lti_profile.pii = {'middle_name': MIDDLE_NAME}
+
+        self.assertEqual(self.lti_profile.middle_name, MIDDLE_NAME)
+
+    def test_family_name_property(self):
+        """Test family_name property."""
+        self.lti_profile.pii = {'family_name': FAMILY_NAME}
+
+        self.assertEqual(self.lti_profile.family_name, FAMILY_NAME)
 
     @ddt.data(
-        ({'name': NAME}, NAME),
         (
             {
                 'given_name': GIVEN_NAME,
@@ -144,6 +217,39 @@ def test_name_property(self, name_data: dict, name_return: str):
 
         self.assertEqual(self.lti_profile.name, name_return)
 
+    @ddt.data(
+        (
+            {
+                'given_name': GIVEN_NAME_LARGER,
+            },
+            f'{GIVEN_NAME_LARGER.lower()[:30]}.',
+        ),
+        (
+            {
+                'given_name': GIVEN_NAME_LARGER,
+                'family_name': FAMILY_NAME,
+            },
+            f'{GIVEN_NAME_LARGER.lower()[:30]}{FAMILY_NAME.lower()[:1]}.',
+        ),
+        ({}, ''),
+    )
+    @ddt.unpack
+    def test_username_property(self, name_data: dict, name_return: str):
+        """Test username property."""
+        self.lti_profile.pii = name_data
+
+        self.assertEqual(
+            self.lti_profile.username,
+            f'{name_return}{self.lti_profile.short_uuid}',
+        )
+
+    def test_email_property(self):
+        """Test email property."""
+        self.assertEqual(
+            self.lti_profile.email,
+            f'{self.lti_profile.uuid}@{app_config.name}',
+        )
+
     def test_str_method(self):
         """Test __str__ method."""
         self.assertEqual(

requirements/base.in

@@ -7,3 +7,4 @@ pylti1p3                  # LTI 1.3 Advantage Tool implementation
 edx-opaque-keys           # Open edX course and XBlock identities API
 edx-toggles               # Library and utilities for implementing and reporting on feature toggles.
 celery                    # Asynchronous task execution library
+shortuuid                 # Library that generates concise, unambiguous, URL-safe UUIDs