[POC] feat: flexible groups support proof of concept

[mariajgrimaldi]

Hi! I implemented a rough POC following up on the previous comment I left:

Technical rundown

First, we implemented this new group-type definition called CourseGroup or group for short as I will call it throughout this post. Don't mistake the new group-type with the current content groups.

This class is similar to the cohort's so it implements whatever we think would fit the class definition. For this POC, I added a reference to users I called professors.

class CourseGroup(models.Model):
    """
    This model represents the new group-type related info.

    .. no_pii:
    """
    course_user_group = models.OneToOneField(CourseUserGroup, unique=True, related_name='group',
                                             on_delete=models.CASCADE)

    professors = models.ManyToManyField(User, db_index=True, related_name='course_groups_professors', blank=True)

    @classmethod
    def create(cls, group_name=None, course_id=None, course_user_group=None, professor=None):
        if course_user_group is None:
            course_user_group, __ = CourseUserGroup.create(
                group_name,
                course_id,
                group_type=CourseUserGroup.GROUPS,
            )

        course_group, __ = cls.objects.get_or_create(
            course_user_group=course_user_group,
        )
        if professor:
            course_group.professors.add(professor)

        return course_group

The idea is to use this class as the cohort's, so we implemented a method to add groups to courses --ie. we simplified the cohort's one.

def is_group_exists(course_key, name):
    """
    Check if a group already exists.
    """
    return CourseUserGroup.objects.filter(course_id=course_key, group_type=CourseUserGroup.GROUPS, name=name).exists()


def add_group_to_course(name, course_key, professor=None):
    """
    Adds a group to a course.
    """
    if is_group_exists(course_key, name):
        raise ValueError("You cannot create two groups with the same name")

    try:
        course = get_course_by_id(course_key)
    except Http404:
        raise ValueError("Invalid course_key")  # lint-amnesty, pylint: disable=raise-missing-from

    return CourseGroup.create(
        group_name=name,
        course_id=course.id,
        professor=professor,
    ).course_user_group

So for this to work, we need to add support for a new group-type to CourseUserGroup. As mentioned, we expected a smooth integration since the class' docstrings explained that other types could be added. So CourseUserGroup works with our group group type, we're adding a new GROUP_TYPE choice to be model here:

    GROUPS = 'groups'
    GROUP_TYPE_CHOICES = ((COHORT, 'Cohort'), (GROUPS, 'Groups'),)

Now, we create new migrations for the LMS.

tutor dev exec lms ./manage.py lms makemigrations
tutor dev exec lms ./manage.py lms migrate

Done! Here's how I created a new group type:

####### Create a group in a course ########

# First we create the professors associated with our new group
professor, _ = User.objects.get_or_create(
    username="professor",
    email="professor@example.com",
)

# Now, we get the course key for the course we want to create a group in
course_key = CourseKey.from_string("course-v1:edX+DemoX+Demo_Course")

# Now we create a group in that course with the help of the add_group_to_course function
group_name = "Demo Group"
group = add_group_to_course(
    group_name,
    course_key,
    professor=professor,
)

# Now, we can get the groups associated with our course with the get_course_groups function
groups = get_course_groups(course_id=course_key)
> groups
> [<CourseUserGroup: Demo Group>]

Where get_course_groups:

def get_course_groups(course_id=None):
    query_set = CourseUserGroup.objects.filter(
        course_id=course_id,
        group_type=CourseUserGroup.GROUPS,
    )
    return list(query_set)

Now, we need a way of adding students to our new group. Again, drawing inspiration from the cohort's implementation, we created an intermediate model that maps students and our new group type:

class GroupMembership(models.Model):
    """
    Used internally to enforce particular conditions.

    .. no_pii:
    """
    course_user_group = models.ForeignKey(CourseUserGroup, on_delete=models.CASCADE)
    user = models.ForeignKey(User, on_delete=models.CASCADE)
    course_id = CourseKeyField(max_length=255)

    def clean_fields(self, *args, **kwargs):  # lint-amnesty, pylint: disable=signature-differs
        if self.course_id is None:
            self.course_id = self.course_user_group.course_id
        super().clean_fields(*args, **kwargs)

    @classmethod
    def assign(cls, group, user):
        with transaction.atomic():
            membership, created = cls.objects.select_for_update().get_or_create(
                user=user,
                course_id=group.course_id,
                course_user_group=group,
            )
            membership.course_user_group.users.add(user)
        return membership

This model implements the assign method, which is used to assign users to cohorts or new group-types -- like in this case. We also implemented a method to add students to groups as part of the API :

def add_user_to_group(group, username_or_email_or_user):
    try:
        if hasattr(username_or_email_or_user, 'email'):
            user = username_or_email_or_user
        else:
            user = get_user_by_username_or_email(username_or_email_or_user)

        return GroupMembership.assign(group, user)
    except User.DoesNotExist as ex:  # Note to self: TOO COHORT SPECIFIC!
        # If username_or_email is an email address, store in database.
        try:
            return (None, None, True)
        except ValidationError as invalid:
            if "@" in username_or_email_or_user:  # lint-amnesty, pylint: disable=no-else-raise
                raise invalid
            else:
                raise ex  # lint-amnesty, pylint: disable=raise-missing-from

Here's how I used it:

# Since we only have one group, we can get it by index
group = groups[0]

# We can also get the group by its ID
group = get_group_by_id(course_key, groups[0].id)

####### Add student to group ########

# Now we can add a student to our group, in this case student already exists and it's enrolled in the course

# Our student's username
username = "student"

# Add student to the group we created before with the add_user_to_group function
add_user_to_group(group, "student")

# Now, we can get group members
members = group.users.all()

This is what we currently have:

• A definition for Class: CourseGroup
• Support for our new group type
• Mapping between students and groups

Now, to test out this approach, we implemented one of the use cases specified: content groups. To map users into content groups, we implemented the GroupPartitionScheme to define our new group type: groups -- there are currently two, cohorts and random. This was implemented as a class in a Django plugin which was recognized as a new UserPartition by using the openedx.user_partition_scheme entry point:

class GroupPartitionScheme:
    """
    This scheme uses lms cohorts (CourseUserGroups) and group-partition
    mappings (CourseUserGroupPartitionGroup) to map lms users into Partition
    Groups.
    """

    @classmethod
    def get_grouped_user_partition(cls, course):
        for user_partition in course.user_partitions:
            if user_partition.scheme == cls:
                return user_partition

        return None

    @classmethod
    def get_group_for_user(cls, course_key, user, user_partition):
        """
        Returns the (Content) Group from the specified user partition to which the user
        is assigned, via their group-type membership and any mappings from groups
        to partitions / (content) groups that might exist.

        If the user has no group-type mapping, or there is no (valid) group ->
        partition group mapping found, the function returns None.
        """
        group = get_group(user, course_key)
        if group is None:
            return None

        group_id, partition_id = get_group_info_for_group(group)
        if partition_id is None:
            return None

        if partition_id != user_partition.id:
            return None

        try:
            return user_partition.get_group(group_id)
        except NoSuchUserPartitionGroupError:
            return None

This is not a model, but instead the scheme that uses CourseUserGroup/GroupMembership and CourseUserGroupPartitionGroup to map learners into content groups. It implements two methods:

The most important method is get_group_for_user which:

• Gets the group assignment for the user/course key combination (GroupMembership).
• Gets the UserPartition/(content_group)group_id combination associated with that class (CourseUserGroupPartitionGroup).
• Returns the (content_group) group associated with the UserPartition/group_id combination.

This method is broadly used to get the user's access to course content.

Here's the cohort's scheme methods where we heavily draw inspiration from. Basically, they're a more simplified version.

After installing the plugin that implements our new scheme into edx-platform (this plugin), the services now recognize our new scheme:

In [1]: from xmodule.partitions.partitions import UserPartition
In [22]: UserPartition.get_scheme("cohort")
Out[22]: openedx.core.djangoapps.course_groups.partition_scheme.CohortPartitionScheme

In [23]: UserPartition.get_scheme("group")
Out[23]: platform_plugin_sample.extensions.groups.GroupPartitionScheme

After some tweaking here and there in edx-platform so this new partition scheme was recognized, we managed to create new content groups associated with the new partition scheme:

Screencast.from.20-07-23.17.53.48.mp4

Notice how there are two content groups sections, the first associated with cohorts and the second one with groups! Also, we added course content to those content groups:

And tested with two different students, one belonging to group 1 and the other one to group 2, so content groups are working with this new group-type.

Now, what happens when I try adding a third student to both groups? This operation is allowed since our GroupMembership doesn't implement the uniqueness conditions. The main issue is that .get is broadly used throughout the implementation so we'll get errors like:

    raise self.model.MultipleObjectsReturned(
openedx.core.djangoapps.course_groups.models.GroupMembership.MultipleObjectsReturned: get() returned more than one GroupMembership -- it returned 2!

WARNING!

Throughout this implementation, I used the word group as our new grouping mechanism but after testing it all out I realized how confusing it was so we should change the naming 😓

[openedx-webhooks]

Thanks for the pull request, @mariajgrimaldi! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate.

[mariajgrimaldi]

Here's a script with the commands I used while testing this out: https://gist.github.com/mariajgrimaldi/96d79d3d7006d9c1116d3848454f60a0

It's documented with in-line docs so it's easier to understand.

[ormsbee]

FYI @feanil, @bmtcril, @kdmccormick, @jmakowski1123

[mariajgrimaldi]

I'm closing this PR in favor of this more specific implementation, which connects teams to content groups: #33105

[openedx-webhooks]

@mariajgrimaldi Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

[openedx-webhooks]

@mariajgrimaldi Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.