Merge pull request #10295 from edx/efischer/cohort_membership_model

TNL-3478 Cohort Membership race condition fix

lms/djangoapps/instructor_task/tasks_helper.py

@@ -1486,8 +1486,7 @@ def cohort_students_and_upload(_xmodule_instance_args, _entry_id, course_id, tas
                 continue
 
             try:
-                with transaction.commit_on_success():
-                    add_user_to_cohort(cohorts_status[cohort_name]['cohort'], username_or_email)
+                add_user_to_cohort(cohorts_status[cohort_name]['cohort'], username_or_email)
                 cohorts_status[cohort_name]['Students Added'] += 1
                 task_progress.succeeded += 1
             except User.DoesNotExist:

lms/djangoapps/instructor_task/tests/test_tasks_helper.py

@@ -20,7 +20,7 @@ from certificates.tests.factories import GeneratedCertificateFactory, Certificat
 from course_modes.models import CourseMode
 from courseware.tests.factories import InstructorFactory
 from instructor_task.tests.test_base import InstructorTaskCourseTestCase, TestReportMixin, InstructorTaskModuleTestCase
-from openedx.core.djangoapps.course_groups.models import CourseUserGroupPartitionGroup
+from openedx.core.djangoapps.course_groups.models import CourseUserGroupPartitionGroup, CohortMembership
 from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory
 import openedx.core.djangoapps.user_api.course_tag.api as course_tag_api
 from openedx.core.djangoapps.user_api.partition_schemes import RandomUserPartitionScheme
@@ -144,8 +144,10 @@ class TestInstructorGradeReport(InstructorGradeReportTestCase):
         magneto = u'MàgnëtÖ'
         cohort1 = CohortFactory(course_id=course.id, name=professor_x)
         cohort2 = CohortFactory(course_id=course.id, name=magneto)
-        cohort1.users.add(user1)
-        cohort2.users.add(user2)
+        membership1 = CohortMembership(course_user_group=cohort1, user=user1)
+        membership1.save()
+        membership2 = CohortMembership(course_user_group=cohort2, user=user2)
+        membership2.save()
 
         self._verify_cell_data_for_user(user1.username, course.id, 'Cohort Name', professor_x)
         self._verify_cell_data_for_user(user2.username, course.id, 'Cohort Name', magneto)
@@ -1399,8 +1401,10 @@ class TestCohortStudents(TestReportMixin, InstructorTaskCourseTestCase):
         )
 
     def test_move_users_to_new_cohort(self):
-        self.cohort_1.users.add(self.student_1)
-        self.cohort_2.users.add(self.student_2)
+        membership1 = CohortMembership(course_user_group=self.cohort_1, user=self.student_1)
+        membership1.save()
+        membership2 = CohortMembership(course_user_group=self.cohort_2, user=self.student_2)
+        membership2.save()
 
         result = self._cohort_students_and_upload(
             u'username,email,cohort\n'
@@ -1417,8 +1421,10 @@ class TestCohortStudents(TestReportMixin, InstructorTaskCourseTestCase):
         )
 
     def test_move_users_to_same_cohort(self):
-        self.cohort_1.users.add(self.student_1)
-        self.cohort_2.users.add(self.student_2)
+        membership1 = CohortMembership(course_user_group=self.cohort_1, user=self.student_1)
+        membership1.save()
+        membership2 = CohortMembership(course_user_group=self.cohort_2, user=self.student_2)
+        membership2.save()
 
         result = self._cohort_students_and_upload(
             u'username,email,cohort\n'

openedx/core/djangoapps/course_groups/cohorts.py

@@ -6,7 +6,6 @@ forums, and to the cohort admin views.
 import logging
 import random
 
-from django.db import transaction
 from django.db.models.signals import post_save, m2m_changed
 from django.dispatch import receiver
 from django.http import Http404
@@ -17,7 +16,13 @@ from eventtracking import tracker
 from request_cache.middleware import RequestCache
 from student.models import get_user_by_username_or_email
 
-from .models import CourseUserGroup, CourseCohort, CourseCohortsSettings, CourseUserGroupPartitionGroup
+from .models import (
+    CourseUserGroup,
+    CourseCohort,
+    CourseCohortsSettings,
+    CourseUserGroupPartitionGroup,
+    CohortMembership
+)
 
 
 log = logging.getLogger(__name__)
@@ -140,7 +145,6 @@ def get_cohorted_commentables(course_key):
     return ans
 
 
-@transaction.commit_on_success
 def get_cohort(user, course_key, assign=True, use_cached=False):
     """Returns the user's cohort for the specified course.
 
@@ -202,7 +206,8 @@ def get_cohort(user, course_key, assign=True, use_cached=False):
             assignment_type=CourseCohort.RANDOM
         ).course_user_group
 
-    user.course_groups.add(cohort)
+    membership = CohortMembership(course_user_group=cohort, user=user)
+    membership.save()
 
     return request_cache.data.setdefault(cache_key, cohort)
 
@@ -343,25 +348,9 @@ def add_user_to_cohort(cohort, username_or_email):
         ValueError if user already present in this cohort.
     """
     user = get_user_by_username_or_email(username_or_email)
-    previous_cohort_name = None
-    previous_cohort_id = None
 
-    course_cohorts = CourseUserGroup.objects.filter(
-        course_id=cohort.course_id,
-        users__id=user.id,
-        group_type=CourseUserGroup.COHORT
-    )
-    if course_cohorts.exists():
-        if course_cohorts[0] == cohort:
-            raise ValueError("User {user_name} already present in cohort {cohort_name}".format(
-                user_name=user.username,
-                cohort_name=cohort.name
-            ))
-        else:
-            previous_cohort = course_cohorts[0]
-            previous_cohort.users.remove(user)
-            previous_cohort_name = previous_cohort.name
-            previous_cohort_id = previous_cohort.id
+    membership = CohortMembership(course_user_group=cohort, user=user)
+    membership.save()
 
     tracker.emit(
         "edx.cohort.user_add_requested",
@@ -369,12 +358,11 @@ def add_user_to_cohort(cohort, username_or_email):
             "user_id": user.id,
             "cohort_id": cohort.id,
             "cohort_name": cohort.name,
-            "previous_cohort_id": previous_cohort_id,
-            "previous_cohort_name": previous_cohort_name,
+            "previous_cohort_id": membership.previous_cohort_id,
+            "previous_cohort_name": membership.previous_cohort_name,
         }
     )
-    cohort.users.add(user)
-    return (user, previous_cohort_name)
+    return (user, membership.previous_cohort_name)
 
 
 def get_group_info_for_cohort(cohort, use_cached=False):

openedx/core/djangoapps/course_groups/migrations/0005_cohort_membership.py

+# -*- coding: utf-8 -*-
+from south.utils import datetime_utils as datetime
+from south.db import db
+from south.v2 import SchemaMigration
+from django.db import models
+
+
+class Migration(SchemaMigration):
+
+    def forwards(self, orm):
+        # Adding model 'CohortMembership'
+        db.create_table('course_groups_cohortmembership', (
+            ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)),
+            ('course_user_group', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['course_groups.CourseUserGroup'])),
+            ('user', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['auth.User'])),
+            ('course_id', self.gf('xmodule_django.models.CourseKeyField')(max_length=255)),
+        ))
+        db.send_create_signal('course_groups', ['CohortMembership'])
+
+        # Adding unique constraint on 'CohortMembership', fields ['user', 'course_id']
+        db.create_unique('course_groups_cohortmembership', ['user_id', 'course_id'])
+
+
+    def backwards(self, orm):
+        # Removing unique constraint on 'CohortMembership', fields ['user', 'course_id']
+        db.delete_unique('course_groups_cohortmembership', ['user_id', 'course_id'])
+
+        # Deleting model 'CohortMembership'
+        db.delete_table('course_groups_cohortmembership')
+
+
+    models = {
+        'auth.group': {
+            'Meta': {'object_name': 'Group'},
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}),
+            'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'})
+        },
+        'auth.permission': {
+            'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'},
+            'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+            'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'name': ('django.db.models.fields.CharField', [], {'max_length': '50'})
+        },
+        'auth.user': {
+            'Meta': {'object_name': 'User'},
+            'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}),
+            'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}),
+            'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
+            'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
+            'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}),
+            'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
+            'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}),
+            'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}),
+            'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'})
+        },
+        'contenttypes.contenttype': {
+            'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"},
+            'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+            'name': ('django.db.models.fields.CharField', [], {'max_length': '100'})
+        },
+        'course_groups.cohortmembership': {
+            'Meta': {'unique_together': "(('user', 'course_id'),)", 'object_name': 'CohortMembership'},
+            'course_id': ('xmodule_django.models.CourseKeyField', [], {'max_length': '255'}),
+            'course_user_group': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['course_groups.CourseUserGroup']"}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"})
+        },
+        'course_groups.coursecohort': {
+            'Meta': {'object_name': 'CourseCohort'},
+            'assignment_type': ('django.db.models.fields.CharField', [], {'default': "'manual'", 'max_length': '20'}),
+            'course_user_group': ('django.db.models.fields.related.OneToOneField', [], {'related_name': "'cohort'", 'unique': 'True', 'to': "orm['course_groups.CourseUserGroup']"}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'})
+        },
+        'course_groups.coursecohortssettings': {
+            'Meta': {'object_name': 'CourseCohortsSettings'},
+            '_cohorted_discussions': ('django.db.models.fields.TextField', [], {'null': 'True', 'db_column': "'cohorted_discussions'", 'blank': 'True'}),
+            'always_cohort_inline_discussions': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
+            'course_id': ('xmodule_django.models.CourseKeyField', [], {'unique': 'True', 'max_length': '255', 'db_index': 'True'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'is_cohorted': ('django.db.models.fields.BooleanField', [], {'default': 'False'})
+        },
+        'course_groups.courseusergroup': {
+            'Meta': {'unique_together': "(('name', 'course_id'),)", 'object_name': 'CourseUserGroup'},
+            'course_id': ('xmodule_django.models.CourseKeyField', [], {'max_length': '255', 'db_index': 'True'}),
+            'group_type': ('django.db.models.fields.CharField', [], {'max_length': '20'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'name': ('django.db.models.fields.CharField', [], {'max_length': '255'}),
+            'users': ('django.db.models.fields.related.ManyToManyField', [], {'db_index': 'True', 'related_name': "'course_groups'", 'symmetrical': 'False', 'to': "orm['auth.User']"})
+        },
+        'course_groups.courseusergrouppartitiongroup': {
+            'Meta': {'object_name': 'CourseUserGroupPartitionGroup'},
+            'course_user_group': ('django.db.models.fields.related.OneToOneField', [], {'to': "orm['course_groups.CourseUserGroup']", 'unique': 'True'}),
+            'created_at': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}),
+            'group_id': ('django.db.models.fields.IntegerField', [], {}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'partition_id': ('django.db.models.fields.IntegerField', [], {}),
+            'updated_at': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'})
+        }
+    }
+
+    complete_apps = ['course_groups']

openedx/core/djangoapps/course_groups/migrations/0006_cohort_membership_data_migrate.py

+# -*- coding: utf-8 -*-
+from south.utils import datetime_utils as datetime
+from south.db import db
+from south.v2 import DataMigration
+from django.db import models, IntegrityError, transaction
+
+
+class Migration(DataMigration):
+
+    def forwards(self, orm):
+        # Matches CourseUserGroup.COHORT
+        cohort_type = 'cohort'
+
+        for cohort_group in orm.CourseUserGroup.objects.all():
+            for user in cohort_group.users.all():
+                current_course_groups = orm.CourseUserGroup.objects.filter(
+                    course_id=cohort_group.course_id,
+                    users__id=user.id,
+                    group_type=cohort_type
+                )
+                current_user_groups = user.course_groups.filter(
+                    course_id=cohort_group.course_id,
+                    group_type=cohort_type
+                )
+
+                unioned_set = set(current_course_groups).union(set(current_user_groups))
+
+                # Per product guidance, fix problem users by arbitrarily choosing a single membership to retain
+                arbitrary_cohort_to_keep = unioned_set.pop()
+
+                try:
+                    membership = orm.CohortMembership(
+                        course_user_group=arbitrary_cohort_to_keep,
+                        user=user,
+                        course_id=arbitrary_cohort_to_keep.course_id
+                    )
+                    membership.save()
+                except IntegrityError:
+                    # It's possible a user already has a conflicting entry in the db. Treat that as correct.
+                    unioned_set.add(arbitrary_cohort_to_keep)
+                    try:
+                        valid_membership = orm.CohortMembership.objects.get(
+                            course_id = cohort_group.course_id,
+                            user__id=user.id
+                        )
+                        actual_cohort_to_keep = orm.CourseUserGroup.objects.get(
+                            id=valid_membership.course_user_group.id
+                        )
+                        unioned_set.remove(actual_cohort_to_keep)
+                    except KeyError:
+                        actual_cohort_to_keep.users.add(user)
+
+                for cohort_itr in unioned_set:
+                    cohort_itr.users.remove(user)
+                    user.course_groups.remove(cohort_itr)
+
+    def backwards(self, orm):
+        # A backwards migration just means dropping the table, which 0005 handles in its backwards() method
+        pass
+
+    models = {
+        'auth.group': {
+            'Meta': {'object_name': 'Group'},
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}),
+            'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'})
+        },
+        'auth.permission': {
+            'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'},
+            'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+            'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'name': ('django.db.models.fields.CharField', [], {'max_length': '50'})
+        },
+        'auth.user': {
+            'Meta': {'object_name': 'User'},
+            'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}),
+            'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}),
+            'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
+            'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
+            'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}),
+            'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
+            'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}),
+            'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}),
+            'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'})
+        },
+        'contenttypes.contenttype': {
+            'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"},
+            'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+            'name': ('django.db.models.fields.CharField', [], {'max_length': '100'})
+        },
+        'course_groups.cohortmembership': {
+            'Meta': {'unique_together': "(('user', 'course_id'),)", 'object_name': 'CohortMembership'},
+            'course_id': ('xmodule_django.models.CourseKeyField', [], {'max_length': '255'}),
+            'course_user_group': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['course_groups.CourseUserGroup']"}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"})
+        },
+        'course_groups.coursecohort': {
+            'Meta': {'object_name': 'CourseCohort'},
+            'assignment_type': ('django.db.models.fields.CharField', [], {'default': "'manual'", 'max_length': '20'}),
+            'course_user_group': ('django.db.models.fields.related.OneToOneField', [], {'related_name': "'cohort'", 'unique': 'True', 'to': "orm['course_groups.CourseUserGroup']"}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'})
+        },
+        'course_groups.coursecohortssettings': {
+            'Meta': {'object_name': 'CourseCohortsSettings'},
+            '_cohorted_discussions': ('django.db.models.fields.TextField', [], {'null': 'True', 'db_column': "'cohorted_discussions'", 'blank': 'True'}),
+            'always_cohort_inline_discussions': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
+            'course_id': ('xmodule_django.models.CourseKeyField', [], {'unique': 'True', 'max_length': '255', 'db_index': 'True'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'is_cohorted': ('django.db.models.fields.BooleanField', [], {'default': 'False'})
+        },
+        'course_groups.courseusergroup': {
+            'Meta': {'unique_together': "(('name', 'course_id'),)", 'object_name': 'CourseUserGroup'},
+            'course_id': ('xmodule_django.models.CourseKeyField', [], {'max_length': '255', 'db_index': 'True'}),
+            'group_type': ('django.db.models.fields.CharField', [], {'max_length': '20'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'name': ('django.db.models.fields.CharField', [], {'max_length': '255'}),
+            'users': ('django.db.models.fields.related.ManyToManyField', [], {'db_index': 'True', 'related_name': "'course_groups'", 'symmetrical': 'False', 'to': "orm['auth.User']"})
+        },
+        'course_groups.courseusergrouppartitiongroup': {
+            'Meta': {'object_name': 'CourseUserGroupPartitionGroup'},
+            'course_user_group': ('django.db.models.fields.related.OneToOneField', [], {'to': "orm['course_groups.CourseUserGroup']", 'unique': 'True'}),
+            'created_at': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}),
+            'group_id': ('django.db.models.fields.IntegerField', [], {}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'partition_id': ('django.db.models.fields.IntegerField', [], {}),
+            'updated_at': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'})
+        }
+    }
+
+    complete_apps = ['course_groups']

openedx/core/djangoapps/course_groups/migrations/rerun_0006.sh

+#!/bin/bash
+if [ $# -eq 0 ]; then
+    echo "$0: usage: rerun_0006.sh <arguments>. At minimum, '--settings=<environment>' is expected."
+    exit 1
+fi
+
+./manage.py lms migrate course_groups 0005 --fake "$@"
+./manage.py lms migrate course_groups 0006 "$@"

openedx/core/djangoapps/course_groups/models.py

@@ -6,7 +6,8 @@ import json
 import logging
 
 from django.contrib.auth.models import User
-from django.db import models
+from django.db import models, transaction, IntegrityError
+from django.core.exceptions import ValidationError
 from xmodule_django.models import CourseKeyField
 
 log = logging.getLogger(__name__)
@@ -37,7 +38,7 @@ class CourseUserGroup(models.Model):
 
     # For now, only have group type 'cohort', but adding a type field to support
     # things like 'question_discussion', 'friends', 'off-line-class', etc
-    COHORT = 'cohort'
+    COHORT = 'cohort'  # If changing this string, update it in migration 0006.forwards() as well
     GROUP_TYPE_CHOICES = ((COHORT, 'Cohort'),)
     group_type = models.CharField(max_length=20, choices=GROUP_TYPE_CHOICES)
 
@@ -58,6 +59,97 @@ class CourseUserGroup(models.Model):
         )
 
 
+class CohortMembership(models.Model):
+    """Used internally to enforce our particular definition of uniqueness"""
+
+    course_user_group = models.ForeignKey(CourseUserGroup)
+    user = models.ForeignKey(User)
+    course_id = CourseKeyField(max_length=255)
+
+    previous_cohort = None
+    previous_cohort_name = None
+    previous_cohort_id = None
+
+    class Meta(object):
+        unique_together = (('user', 'course_id'), )
+
+    # The sole purpose of overriding this method is to get the django 1.6 behavior of allowing 'validate_unique'
+    # For django 1.8 upgrade, just remove this method and allow the base method to be called instead.
+    # Reference: https://docs.djangoproject.com/en/1.6/ref/models/instances/, under "Validating Objects"
+    def full_clean(self, **kwargs):
+        self.clean_fields()
+        self.clean()
+        if 'validate_unique' not in kwargs or kwargs['validate_unique'] is True:
+            self.validate_unique()
+
+    def clean_fields(self, *args, **kwargs):
+        if self.course_id is None:
+            self.course_id = self.course_user_group.course_id
+        super(CohortMembership, self).clean_fields(*args, **kwargs)
+
+    def clean(self):
+        if self.course_user_group.group_type != CourseUserGroup.COHORT:  # pylint: disable=E1101
+            raise ValidationError("CohortMembership cannot be used with CourseGroup types other than COHORT")
+        if self.course_user_group.course_id != self.course_id:
+            raise ValidationError("Non-matching course_ids provided")
+
+    def save(self, *args, **kwargs):
+        # Avoid infinite recursion if creating from get_or_create() call below.
+        if 'force_insert' in kwargs and kwargs['force_insert'] is True:
+            super(CohortMembership, self).save(*args, **kwargs)
+            return
+
+        self.full_clean(validate_unique=False)
+
+        # This loop has been created to allow for optimistic locking, and retrial in case of losing a race condition.
+        # The limit is 2, since select_for_update ensures atomic updates. Creation is the only possible race condition.
+        max_retries = 2
+        success = False
+        for __ in range(max_retries):
+            # The following 2 "transaction" lines force a fresh read, they can be removed once we're on django 1.8
+            # http://stackoverflow.com/questions/3346124/how-do-i-force-django-to-ignore-any-caches-and-reload-data
+            with transaction.commit_manually():
+                transaction.commit()
+
+            with transaction.commit_on_success():
+
+                try:
+                    saved_membership, created = CohortMembership.objects.select_for_update().get_or_create(
+                        user__id=self.user.id,  # pylint: disable=E1101
+                        course_id=self.course_id,
+                        defaults={
+                            'course_user_group': self.course_user_group,
+                            'user': self.user
+                        }
+                    )
+                except IntegrityError:  # This can happen if simultaneous requests try to create a membership
+                    transaction.rollback()
+                    continue
+
+                if not created:
+                    if saved_membership.course_user_group == self.course_user_group:
+                        raise ValueError("User {user_name} already present in cohort {cohort_name}".format(
+                            user_name=self.user.username,  # pylint: disable=E1101
+                            cohort_name=self.course_user_group.name
+                        ))
+                    self.previous_cohort = saved_membership.course_user_group
+                    self.previous_cohort_name = saved_membership.course_user_group.name
+                    self.previous_cohort_id = saved_membership.course_user_group.id
+                    self.previous_cohort.users.remove(self.user)
+
+                saved_membership.course_user_group = self.course_user_group
+                self.course_user_group.users.add(self.user)  # pylint: disable=E1101
+
+                #note: in django 1.8, we can call save with updated_fields=['course_user_group']
+                super(CohortMembership, saved_membership).save()
+
+            success = True
+            break
+
+        if not success:
+            raise IntegrityError("Unable to save membership after {} tries, aborting.".format(max_retries))
+
+
 class CourseUserGroupPartitionGroup(models.Model):
     """
     Create User Partition Info.

openedx/core/djangoapps/course_groups/tests/helpers.py

@@ -11,7 +11,7 @@ from xmodule.modulestore.django import modulestore
 from xmodule.modulestore import ModuleStoreEnum
 
 from ..cohorts import set_course_cohort_settings
-from ..models import CourseUserGroup, CourseCohort, CourseCohortsSettings
+from ..models import CourseUserGroup, CourseCohort, CourseCohortsSettings, CohortMembership
 
 
 class CohortFactory(DjangoModelFactory):
@@ -41,6 +41,15 @@ class CourseCohortFactory(DjangoModelFactory):
     class Meta(object):
         model = CourseCohort
 
+    @post_generation
+    def memberships(self, create, extracted, **kwargs):  # pylint: disable=unused-argument
+        """
+        Returns the memberships linking users to this cohort.
+        """
+        for user in self.course_user_group.users.all():  # pylint: disable=E1101
+            membership = CohortMembership(user=user, course_user_group=self.course_user_group)
+            membership.save()
+
     course_user_group = factory.SubFactory(CohortFactory)
     assignment_type = 'manual'
 

openedx/core/djangoapps/course_groups/tests/test_cohorts.py

@@ -4,6 +4,7 @@ Tests for cohorts
 # pylint: disable=no-member
 import ddt
 from mock import call, patch
+import before_after
 
 from django.contrib.auth.models import User
 from django.db import IntegrityError
@@ -635,6 +636,45 @@ class TestCohorts(ModuleStoreTestCase):
             lambda: cohorts.add_user_to_cohort(first_cohort, "non_existent_username")
         )
 
+    @patch("openedx.core.djangoapps.course_groups.cohorts.tracker")
+    def add_user_to_cohorts_race_condition(self, mock_tracker):
+        """
+        Makes use of before_after to force a race condition, in order to
+        confirm handling of such conditions is done correctly.
+        """
+        course_user = UserFactory(username="Username", email="a@b.com")
+        course = modulestore().get_course(self.toy_course_key)
+        CourseEnrollment.enroll(course_user, self.toy_course_key)
+        first_cohort = CohortFactory(course_id=course.id, name="FirstCohort")
+        second_cohort = CohortFactory(course_id=course.id, name="SecondCohort")
+
+        # This before_after contextmanager allows for reliable reproduction of a race condition.
+        # It will break before the first save() call creates an entry, and then run add_user_to_cohort again.
+        # Because this second call will write before control is returned, the first call will be writing stale data.
+        # This test confirms that the first add_user_to_cohort call can handle this stale read condition properly.
+        # Proper handling is defined as treating calls as sequential, with write time deciding the order.
+        with before_after.before_after(
+            'django.db.models.Model.save',
+            after_ftn=cohorts.add_user_to_cohort(second_cohort, course_user.username),
+            autospec=True
+        ):
+            # This method will read, then break, then try to write stale data.
+            # It should fail at that, then retry with refreshed data
+            cohorts.add_user_to_cohort(first_cohort, course_user.username)
+
+        mock_tracker.emit.assert_any_call(
+            "edx.cohort.user_add_requested",
+            {
+                "user_id": course_user.id,
+                "cohort_id": first_cohort.id,
+                "cohort_name": first_cohort.name,
+                "previous_cohort_id": second_cohort.id,
+                "previous_cohort_name": second_cohort.name,
+            }
+        )
+        # Note that the following get() will fail with MultipleObjectsReturned if race condition is not handled.
+        self.assertEqual(first_cohort.users.get(), course_user)
+
     def test_get_course_cohort_settings(self):
         """
         Test that cohorts.get_course_cohort_settings is working as expected.

requirements/edx/base.txt

@@ -120,6 +120,7 @@ django_debug_toolbar==1.3.2
 
 # Used for testing
 astroid==1.3.8
+before_after==0.1.3
 bok-choy==0.4.7
 chrono==1.0.2
 coverage==4.0