diff --git a/common/djangoapps/util/model_utils.py b/common/djangoapps/util/model_utils.py index 41ca3bfa2d..9ba07f04b1 100644 --- a/common/djangoapps/util/model_utils.py +++ b/common/djangoapps/util/model_utils.py @@ -165,30 +165,3 @@ def slugify(value): value = unicodedata.normalize('NFKD', value).encode('ascii', 'ignore').decode('ascii') value = re.sub(r'[^\w\s-]', '', value).strip().lower() return mark_safe(re.sub(r'[-\s]+', '-', value)) - - -def generate_unique_readable_id(name, queryset, lookup_field): - """Generates a unique readable id from name by appending a numeric suffix. - - Args: - name (string): Name to generate the id from. May include spaces. - queryset (QuerySet): QuerySet to check for uniqueness within. - lookup_field (string): Field name on the model that corresponds to the - unique identifier. - - Returns: - string: generated unique identifier - """ - candidate = slugify(name) - conflicts = queryset.filter(**{lookup_field + '__startswith': candidate}).values_list(lookup_field, flat=True) - - if conflicts and candidate in conflicts: - suffix = 2 - while True: - new_id = candidate + '-' + str(suffix) - if new_id not in conflicts: - candidate = new_id - break - suffix += 1 - - return candidate diff --git a/lms/djangoapps/teams/models.py b/lms/djangoapps/teams/models.py index a0d872e1ae..609ff88933 100644 --- a/lms/djangoapps/teams/models.py +++ b/lms/djangoapps/teams/models.py @@ -10,7 +10,7 @@ from django.utils.translation import ugettext_lazy from django_countries.fields import CountryField from xmodule_django.models import CourseKeyField -from util.model_utils import generate_unique_readable_id +from util.model_utils import slugify from student.models import LanguageField, CourseEnrollment from .errors import AlreadyOnTeamInCourse, NotEnrolledInCourseForTeam @@ -51,9 +51,9 @@ class CourseTeam(models.Model): team uses, as ISO 639-1 code. """ - - team_id = generate_unique_readable_id(name, cls.objects.all(), 'team_id') - discussion_topic_id = uuid4().hex + unique_id = uuid4().hex + team_id = slugify(name)[0:20] + '-' + unique_id + discussion_topic_id = unique_id course_team = cls( team_id=team_id, diff --git a/lms/djangoapps/teams/tests/test_views.py b/lms/djangoapps/teams/tests/test_views.py index d70a647b66..aff0241341 100644 --- a/lms/djangoapps/teams/tests/test_views.py +++ b/lms/djangoapps/teams/tests/test_views.py @@ -533,20 +533,28 @@ class TestCreateTeamAPI(TeamAPITestCase): def test_access(self, user, status): team = self.post_create_team(status, self.build_team_data(name="New Team"), user=user) if status == 200: - self.assertEqual(team['id'], 'new-team') - self.assertIn('discussion_topic_id', team) + self.verify_expected_team_id(team, 'new-team') teams = self.get_teams_list(user=user) self.assertIn("New Team", [team['name'] for team in teams['results']]) + def verify_expected_team_id(self, team, expected_prefix): + """ Verifies that the team id starts with the specified prefix and ends with the discussion_topic_id """ + self.assertIn('id', team) + self.assertIn('discussion_topic_id', team) + self.assertEqual(team['id'], expected_prefix + '-' + team['discussion_topic_id']) + def test_naming(self): new_teams = [ self.post_create_team(data=self.build_team_data(name=name), user=self.create_and_enroll_student()) - for name in ["The Best Team", "The Best Team", "The Best Team", "The Best Team 2"] + for name in ["The Best Team", "The Best Team", "A really long team name"] ] - self.assertEquals( - [team['id'] for team in new_teams], - ['the-best-team', 'the-best-team-2', 'the-best-team-3', 'the-best-team-2-2'] - ) + # Check that teams with the same name have unique IDs. + self.verify_expected_team_id(new_teams[0], 'the-best-team') + self.verify_expected_team_id(new_teams[1], 'the-best-team') + self.assertNotEqual(new_teams[0]['id'], new_teams[1]['id']) + + # Verify expected truncation behavior with names > 20 characters. + self.verify_expected_team_id(new_teams[2], 'a-really-long-team-n') @ddt.data((400, { 'name': 'Bad Course ID', @@ -616,6 +624,10 @@ class TestCreateTeamAPI(TeamAPITestCase): language='fr' ), user=creator) + # Verify the id (it ends with a unique hash, which is the same as the discussion_id). + self.verify_expected_team_id(team, 'fully-specified-team') + del team['id'] + # Remove date_created and discussion_topic_id because they change between test runs del team['date_created'] del team['discussion_topic_id'] @@ -643,7 +655,6 @@ class TestCreateTeamAPI(TeamAPITestCase): 'is_active': True, 'topic_id': 'great-topic', 'course_id': str(self.test_course_1.id), - 'id': 'fully-specified-team', 'description': 'Another fantastic team' })