From f4f741e509e42de4b254835c1a0d190f4e498b57 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 3 Jun 2025 17:15:07 +0200 Subject: [PATCH 01/12] docs: add arch decision record for the user groups model foundations --- docs/decisions/0001-purpose-of-this-repo.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/decisions/0001-purpose-of-this-repo.rst b/docs/decisions/0001-purpose-of-this-repo.rst index 9074278..1ab8c71 100644 --- a/docs/decisions/0001-purpose-of-this-repo.rst +++ b/docs/decisions/0001-purpose-of-this-repo.rst @@ -1,5 +1,5 @@ -0001 Purpose of This Repo -######################### +0001: Purpose of This Repo +########################## Status ****** From 57f558e46a5b9889b8b767764dc51f75bc59acec Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 4 Jun 2025 13:25:31 +0200 Subject: [PATCH 02/12] feat: add module structure for runtime components --- openedx_user_groups/api.py | 0 openedx_user_groups/backends.py | 0 openedx_user_groups/base.py | 0 openedx_user_groups/registry.py | 0 openedx_user_groups/service.py | 0 5 files changed, 0 insertions(+), 0 deletions(-) create mode 100644 openedx_user_groups/api.py create mode 100644 openedx_user_groups/backends.py create mode 100644 openedx_user_groups/base.py create mode 100644 openedx_user_groups/registry.py create mode 100644 openedx_user_groups/service.py diff --git a/openedx_user_groups/api.py b/openedx_user_groups/api.py new file mode 100644 index 0000000..e69de29 diff --git a/openedx_user_groups/backends.py b/openedx_user_groups/backends.py new file mode 100644 index 0000000..e69de29 diff --git a/openedx_user_groups/base.py b/openedx_user_groups/base.py new file mode 100644 index 0000000..e69de29 diff --git a/openedx_user_groups/registry.py b/openedx_user_groups/registry.py new file mode 100644 index 0000000..e69de29 diff --git a/openedx_user_groups/service.py b/openedx_user_groups/service.py new file mode 100644 index 0000000..e69de29 From 3c8ea1536d6ede7424fe722133e8a2529126e523 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 4 Jun 2025 20:11:20 +0200 Subject: [PATCH 03/12] feat: add models to manage user groups, criterion, and scopes --- openedx_user_groups/models.py | 153 +++++++++++++++++++++++++++++++++- 1 file changed, 152 insertions(+), 1 deletion(-) diff --git a/openedx_user_groups/models.py b/openedx_user_groups/models.py index 5d68073..59ddef0 100644 --- a/openedx_user_groups/models.py +++ b/openedx_user_groups/models.py @@ -1,3 +1,154 @@ """ -Database models for openedx_user_groups. +Core models for Open edX User Groups. + +In this module, we define the core models that represent user groups within the Open edX platform. Currently, +it includes: + +Models: +- UserGroup: Represents a group of users within the Open edX platform, allowing for the management and organization of +users into distinct groups. +- UserGroupMembership: Represents the memberships of users within user groups, linking users to their respective +groups. +- Criterion: Represents a criterion that can be used to filter or categorize user groups based on specific attributes or +behaviors. +- Scope: Represents the scope of a user group, defining the context in which the group operates, such as course or +site-wide. TODO: Add this model later on. + +With the following relationships: +- UserGroup has many UserGroupMembership, linking users to their respective groups. +- UserGroupMembership belongs to a UserGroup and a User, establishing the relationship between users and their +groups. This includes a many-to-many relationship between users and groups, allowing associating metadata to +the relationship when created. +- UserGroup can have many Criteria, allowing for the categorization of user groups based on specific attributes or +behaviors. +- A criterion is associated with a single UserGroup, allowing for filtering of user groups based on specific +attributes only for that group. +- Scope can be associated with a UserGroup, defining the context in which the group operates. A user group can +be associated only with one scope at a time. """ +from django.db import models + +from django.contrib.auth import get_user_model + +User = get_user_model() + + +class UserGroup(models.Model): + """Represents a group of users within the Open edX platform. + + This model allows for the management and organization of users into distinct groups. + + Attributes: + name (str): The name of the user group. + description (str): A brief description of the user group. + created_at (datetime): The timestamp when the user group was created. + updated_at (datetime): The timestamp when the user group was last updated. + last_membership_change (datetime): The timestamp of the last change to the user group membership + related to the group. + enabled (bool): Whether the user group is enabled. + scope (str): The scope of the user group, defining the context in which it operates. + users (ManyToManyField): The users that are members of the group. + """ + + id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False) + name = models.CharField(max_length=255, unique=True) + description = models.TextField(blank=True, null=True) + created_at = models.DateTimeField(auto_now_add=True) + updated_at = models.DateTimeField(auto_now=True) + last_membership_change = models.DateTimeField(auto_now=True) + enabled = models.BooleanField(default=True) + scope = models.ForeignKey(Scope, on_delete=models.CASCADE, blank=True, null=True) + users = models.ManyToManyField(User, through='UserGroupMembership') + + class Meta: + ordering = ['name'] + unique_together = ['id', 'scope'] + + + def __str__(self): + return self.name + + def get_users(self): + return self.users.all() + + def get_criteria(self): + return self.criteria.all() + + +class UserGroupMembership(models.Model): + """Represents the membership of a user in a user group. + + This model allows for the management and organization of users into distinct groups. + + Attributes: + user (User): The user who is a member of the group. + group (UserGroup): The group to which the user belongs. + joined_at (datetime): The timestamp when the user joined the group. + left_at (datetime): The timestamp when the user left the group. + is_active (bool): Whether the user is still a member of the group. + """ + + user = models.ForeignKey(User, on_delete=models.CASCADE) + group = models.ForeignKey(UserGroup, on_delete=models.CASCADE) + joined_at = models.DateTimeField(auto_now_add=True) + left_at = models.DateTimeField(blank=True, null=True) + is_active = models.BooleanField(default=True) + + def __str__(self): + return f"{self.user.username} - {self.group.name}" + + +class Criterion(models.Model): + """Represents a criterion that can be used to filter or categorize user groups based on specific attributes or + behaviors. + + Attributes: + name (str): The name of the criterion. + description (str): A brief description of the criterion. + criterion_type (str): The type of the criterion. + criterion_operator (str): The operator of the criterion. + criterion_config (dict): The configuration of the criterion. + group (UserGroup): The group to which the criterion belongs. + """ + + name = models.CharField(max_length=255) + description = models.TextField(blank=True, null=True) + criterion_type = models.CharField(max_length=255) # TODO: Although right now there is no restriction on the type, we should only allow for registered criteria types + criterion_operator = models.CharField(max_length=255) # TODO: Replace this with Enum later on + criterion_config = models.JSONField(default=dict) # No restrictions needed on the config, each criterion type will have its own config and validate it accordingly + group = models.ForeignKey(UserGroup, on_delete=models.CASCADE, related_name='criteria') + + def __str__(self): + return f"{self.name} - {self.group.name}" + + def get_criterion_type(self): + return self.criterion_type # TODO: Replace this with a method that returns the criterion type class as an object + + def get_criterion_operator(self): + return self.criterion_operator # TODO: Replace this so it returns the enum supported type? Is it needed? + + def get_criterion_config(self): + return self.criterion_config # Would I need to return the config as a dict? + + +class Scope(models.Model): + """Represents the scope of a user group. + + Attributes: + name (str): The name of the scope. + description (str): A brief description of the scope. + created_at (datetime): The timestamp when the scope was created. + updated_at (datetime): The timestamp when the scope was last updated. + """ + + name = models.CharField(max_length=255) + description = models.TextField(blank=True, null=True) + resource_type = models.CharField(max_length=255) # TODO: Replace this with an Enum, it should only be allowed to be one of the following: course, org or site. Maybe more? + resource_id = models.CharField(max_length=255) # TODO: Replace this with an actual opaque key or corresponding ID? + + def __str__(self): + return f"{self.name} - {self.resource_type} - {self.resource_id}" + + def get_resource_type(self): + return self.resource_type + \ No newline at end of file From 1b25daa60e88741563ec94cc8e5c50e104c8d018 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 10 Jun 2025 18:34:19 +0200 Subject: [PATCH 04/12] feat: add core runtime components for the user groups system - Add a plugin manager to support new criteria types - Add API to interact with the current models & runtime criteria - Add a few types of criteria for testing purposes - Implement test suite for models & API to test assumptions --- openedx_user_groups/api.py | 356 ++++++++++++++++++ openedx_user_groups/apps.py | 1 + openedx_user_groups/backends.py | 85 +++++ openedx_user_groups/criteria.py | 186 +++++++++ openedx_user_groups/criteria_types.py | 219 +++++++++++ openedx_user_groups/manager.py | 76 ++++ .../migrations/0001_initial.py | 150 ++++++++ openedx_user_groups/migrations/__init__.py | 1 + openedx_user_groups/models.py | 166 +++++--- openedx_user_groups/registry.py | 0 openedx_user_groups/service.py | 0 openedx_user_groups/{base.py => views.py} | 0 requirements/base.in | 2 +- requirements/base.txt | 26 ++ requirements/dev.txt | 63 ++++ requirements/doc.txt | 64 +++- requirements/quality.in | 1 + requirements/quality.txt | 65 ++++ requirements/test.in | 3 + requirements/test.txt | 66 +++- test_settings.py | 1 + tests/test_api.py | 347 +++++++++++++++++ tests/test_models.py | 233 +++++++++++- 23 files changed, 2035 insertions(+), 76 deletions(-) create mode 100644 openedx_user_groups/criteria.py create mode 100644 openedx_user_groups/criteria_types.py create mode 100644 openedx_user_groups/manager.py create mode 100644 openedx_user_groups/migrations/0001_initial.py create mode 100644 openedx_user_groups/migrations/__init__.py delete mode 100644 openedx_user_groups/registry.py delete mode 100644 openedx_user_groups/service.py rename openedx_user_groups/{base.py => views.py} (100%) create mode 100644 tests/test_api.py diff --git a/openedx_user_groups/api.py b/openedx_user_groups/api.py index e69de29..695b2c1 100644 --- a/openedx_user_groups/api.py +++ b/openedx_user_groups/api.py @@ -0,0 +1,356 @@ +"""Module for the API of the user groups app. + +Here we'll implement the API, evaluators and combinators. These components can be later moved to a service layer: + +- The API (basic interfaces) will be used by other services to create and manage user groups. +- The evaluators will be used to evaluate the membership of a user group. +- The combinators will be used to combine the criteria of a user group to get the final membership. +""" + +from django.contrib.auth import get_user_model +from django.db import transaction +from django.utils import timezone + +from openedx_user_groups.backends import DjangoORMBackendClient +from openedx_user_groups.criteria import BaseCriterionType +from openedx_user_groups.manager import load_criterion_class +from openedx_user_groups.models import Criterion, Scope, UserGroup, UserGroupMembership + +User = get_user_model() + + +# Public API for User Group operations +__all__ = [ + "get_or_create_group_and_scope", + "create_group_with_criteria", + "create_group_with_criteria_and_evaluate_membership", + "evaluate_and_update_membership_for_multiple_groups", + "get_groups_for_scope", + "get_group_by_id", + "get_group_by_name", + "get_group_by_name_and_scope", + "get_user_group_members", + "update_group_name_or_description", + "soft_delete_group", + "hard_delete_group", +] + + +def get_scope_type_from_content_type(content_type): + """ + Map Django ContentType to scope type names used by criteria. + + Args: + content_type: Django ContentType instance + + Returns: + str: Scope type name (e.g., "course", "organization", "instance") + """ + # Mapping from Django model names to scope types + model_to_scope_mapping = { + "course": "course", # When we have actual course models + "courseoverview": "course", # edx-platform course overview model + "organization": "organization", # Organization models + } + + model_name = content_type.model + return model_to_scope_mapping.get(model_name, "instance") # Default to instance + + +def get_or_create_group_and_scope( + name: str, description: str, scope_context: dict +) -> tuple[UserGroup, Scope]: + """Create a new user group with the given name, description, and scope. No criteria is associated with the group. + + Args: + name (str): The name of the user group. + description (str): A brief description of the user group. + scope (Scope): The scope of the user group. + + Returns: + UserGroup: The created user group. + """ + with transaction.atomic(): + scope, created = Scope.objects.get_or_create( + name=scope_context["name"], + content_type=scope_context["content_object"]["content_type"], + object_id=scope_context["content_object"]["object_id"], + ) + user_group, _ = UserGroup.objects.get_or_create( + name=name, + description=description, + scope=scope, + ) + return user_group, scope + + +def load_criterion_class_and_create_instance( + criterion_type: str, criterion_operator: str, criterion_config: dict +): + """Create a new criterion class. + + Args: + criterion_type (str): The type of the criterion. + criterion_operator (str): The operator of the criterion. + criterion_config (dict): The configuration of the criterion. + + Returns: + BaseCriterionType: The created criterion class. + """ + criterion_class = load_criterion_class(criterion_type) + criterion_instance = criterion_class(criterion_operator, criterion_config) + return criterion_instance + + +def create_group_with_criteria_from_data( + name: str, description: str, scope_context: dict, criterion_data: [dict] +): + """Create a new user group with the given name, description, scope, and criteria. + This criteria hasn't been instantiated and validated yet. + + Args: + name (str): The name of the user group. + description (str): A brief description of the user group. + scope_context (dict): The context of the scope. + criterion_data (list): A list of criterion data. + + Returns: + UserGroup: The created user group. + """ + with transaction.atomic(): + user_group, scope = get_or_create_group_and_scope( + name, description, scope_context + ) + for data in criterion_data: + criterion_instance = load_criterion_class_and_create_instance( + data["criterion_type"], + data["criterion_operator"], + data["criterion_config"], + ) + # TODO:Check if the criterion supports the scope type based on content type + # scope_type = get_scope_type_from_content_type(scope.content_type) + # assert scope_type in criterion_instance.scopes, f"Criterion does not support scope type '{scope_type}'. Supported scopes: {criterion_instance.scopes}" + Criterion.objects.create( + criterion_type=criterion_instance.criterion_type, + criterion_operator=criterion_instance.criterion_operator.value, + criterion_config=criterion_instance.criterion_config.model_dump(), # TODO: should we do this somewhere else? + user_group=user_group, + ) + return user_group + + +def evaluate_and_update_membership_for_group(group_id: int): + """Evaluate the membership of a user group based on the criteria and update the membership records. + + Args: + group_id (str): The ID of the user group. + """ + # TODO: This should be done asynchronously. + with transaction.atomic(): + user_group = get_group_by_id(group_id) + criteria = Criterion.objects.filter(user_group=user_group) + + # Evaluatate criteria and build list of Q objects - Done by what we called "combinator" + criteria_results = [] + for criterion in criteria: + criterion_instance = load_criterion_class_and_create_instance( + criterion.criterion_type, + criterion.criterion_operator, + criterion.criterion_config, + ) + result = criterion_instance.evaluate( + current_scope=user_group.scope, + backend_client=DjangoORMBackendClient, # TODO: for now we'd only support DjangoORMBackendClient. But I think we could pass a list of registered backend clients here. + ) + + criteria_results.append(result) + + # This is the reducer / accumulator part. - Done by what we called "evaluator engine" + # Combine the results using intersection (AND logic) for multiple criteria for single criteria we could just use the first result. + # TODO: For simplicity we're only considering AND logic for now. When considering OR logic we would need a logic tree for combining the results correctly. + if criteria_results: + # Start with the first QuerySet and intersect with subsequent ones + users = criteria_results[0] + for result in criteria_results[1:]: + users = users.intersection( + result + ) # TODO: is it better to use Q objects instead of QuerySets? + else: + # No criteria, return empty queryset + users = User.objects.none() + + # Update membership records - This should be done by the User Group service + # Simple membership update: clear existing and create new ones with basic metadata + user_group.usergroupmembership_set.all().delete() + + # Create new memberships + new_memberships = [ + UserGroupMembership(user=user, group=user_group, joined_at=timezone.now()) + for user in users + ] + UserGroupMembership.objects.bulk_create(new_memberships) + + # Update last membership change timestamp + user_group.last_membership_change = timezone.now() + user_group.save(update_fields=["last_membership_change"]) + + +def evaluate_and_update_membership_for_multiple_groups(group_ids: [int]): + """Evaluate the membership of a list of user groups based on the criteria and update the membership records. + + Args: + group_ids (list): The IDs of the user groups. + """ + with transaction.atomic(): + for group_id in group_ids: + evaluate_and_update_membership_for_group(group_id) + + +def create_group_with_criteria_and_evaluate_membership( + name: str, description: str, scope_context: dict, criterion_data: dict +): + """Create a new user group with the given name, description, scope, and criterion. + This criterion has been instantiated and validated. + + Args: + name (str): The name of the user group. + description (str): A brief description of the user group. + scope_context (dict): The context of the scope. + criterion_data (dict): The data of the criterion following the format of: + { + "criterion_type": str, + "criterion_operator": str, + "criterion_config": dict, + } + + Returns: + UserGroup: The created user group. + """ + with transaction.atomic(): + user_group = create_group_with_criteria_from_data( + name, description, scope_context, criterion_data + ) + evaluate_and_update_membership_for_group(user_group.id) + return user_group + + +def create_group_with_criteria( + name: str, description: str, scope_context: dict, criterion_data: [dict] +): + """Create a new user group with the given name, description, scope, and criteria. + This criteria hasn't been instantiated and validated yet. + + Args: + name (str): The name of the user group. + description (str): A brief description of the user group. + scope_context (dict): The context of the scope. + criterion_data (list): A list of criterion data following the format of: + { + "criterion_type": str, + "criterion_operator": str, + "criterion_config": dict, + } + + Returns: + UserGroup: The created user group. + """ + with transaction.atomic(): + user_group = create_group_with_criteria_from_data( + name, description, scope_context, criterion_data + ) + return user_group + + +def get_groups_for_scope(content_object_id: int): + """Get all user groups for a given scope. + + Args: + content_object_id (int): The ID of the content object. The idea would be to pass the ID of the course, + organization, or instance. + + Returns: + list: A list of user groups with minimum information. + """ + return Scope.objects.get(content_object_id=content_object_id).user_groups.all() + + +# TODO: THESE METHODS I HAVEN'T TESTED YET + +def get_group_by_id(group_id: int): + """Get a user group by its ID. + + Args: + group_id (int): The ID of the user group. + + Returns: + UserGroup: The user group. + """ + return UserGroup.objects.get(id=group_id) + + +def get_group_by_name(name: str): + """Get a user group by its name. + + Args: + name (str): The name of the user group. + + Returns: + UserGroup: The user group. + """ + return UserGroup.objects.get(name=name) + + +def get_group_by_name_and_scope(name: str, scope: str): + """Get a user group by its name and scope. TODO: should we allow multiple groups with the same name but different scopes? + + Args: + name (str): The name of the user group. + scope (str): The scope of the user group. + + Returns: + UserGroup: The user group. + """ + return UserGroup.objects.get(name=name, scope=scope) + + +def get_user_group_members(group_id: int): + """Get the members of a user group. + + Args: + group_id (int): The ID of the user group. + + Returns: + list: A list of users that are members of the user group. + """ + return UserGroup.objects.get(id=group_id).users.all() + + +def update_group_name_or_description(group_id: int, name: str, description: str): + """Update the name or description of a user group. + + Args: + group_id (str): The ID of the user group. + name (str): The name of the user group. + description (str): A brief description of the user group. + """ + UserGroup.objects.filter(id=group_id).update(name=name, description=description) + + +def hard_delete_group(group_id: int): + """Hard delete a user group. This will delete the group and all its criteria.""" + UserGroup.objects.filter(id=group_id).delete() + + +def soft_delete_group(group_id: int): + """Soft delete a user group. This will not delete the group, but it will prevent it from being used by disabling it.""" + UserGroup.objects.filter(id=group_id).update(is_active=False) + + +def enable_group(group_id: int): + """Enable a user group. This will allow it to be used again.""" + UserGroup.objects.filter(id=group_id).update(is_active=True) + + +def disable_group(group_id: int): + """Disable a user group. This will prevent it from being used.""" + UserGroup.objects.filter(id=group_id).update(is_active=False) diff --git a/openedx_user_groups/apps.py b/openedx_user_groups/apps.py index 7506810..6e79465 100644 --- a/openedx_user_groups/apps.py +++ b/openedx_user_groups/apps.py @@ -11,3 +11,4 @@ class OpenedxUserGroupsConfig(AppConfig): """ name = "openedx_user_groups" + default_auto_field = "django.db.models.BigAutoField" diff --git a/openedx_user_groups/backends.py b/openedx_user_groups/backends.py index e69de29..cbcf280 100644 --- a/openedx_user_groups/backends.py +++ b/openedx_user_groups/backends.py @@ -0,0 +1,85 @@ +"""Module for backend clients that the criteria can use to evaluate their conditions and +return users for the group. +""" + +from django.contrib.auth import get_user_model +from django.db.models import QuerySet + +from openedx_user_groups.models import Scope + +User = get_user_model() + + +class BackendClient: + """Base class for backend clients.""" + + +class DjangoORMBackendClient(BackendClient): + """Backend client that uses Django ORM get data for criteria evaluation. + + All these methods return querysets of users for the given scope, augmented with the + relevant data for the criterion. + + TODO: how can I always return a queryset of objects alongside users? I don't know if this is possible. + + Course vs Organization + - Course: + - get_enrollments (all students in the course) + - get_users (all users in the course) + - get_grade (all grades for the course) + - Organization: + - get_enrollments (all students in the organization) + - get_users (all users in the organization) + """ + + @staticmethod + def get_enrollments(scope: Scope) -> QuerySet: + """Provide an interface to get all user enrollments for a given scope. + + Args: + scope (Scope): The scope to get the enrollments for. + + Returns: + QuerySet: A queryset of user enrollments for the given scope. + """ + # TODO: need an API to get enrollment objects for a given course. Currently, there is no way + # of implementing unittests for this without edx-platform. Can be executed as part of the + # edx-platform tests though. + pass + + @staticmethod + def get_users(scope: Scope) -> QuerySet: + """Provide an interface to get all users for a given scope. + + For simplicity reasons, we'll consider all users in the current instance. The idea would be + to filter users depending on whether they're enrolled in a course in the org or in a course + itself, but since we don't have an API to access this data, we'll just return all users in the instance. + """ + return User.objects.all().exclude(is_staff=True, is_superuser=True) + + @staticmethod + def get_grade(scope: Scope) -> QuerySet: + """Provide an interface to get all grades for a given scope. + + This method should be implemented by the backend client. Use existent API methods to get the data for the scope. + """ + pass + + @staticmethod + def get_course_progress(scope: Scope) -> QuerySet: + """Provide an interface to get all course progress for a given scope. + + This method should be implemented by the backend client. Use existent API methods to get the data for the scope. + """ + pass + + +class SupersetBackendClient(BackendClient): + """Backend client that uses Superset to get data for criteria evaluation. + + This backend client is used to get data for the criteria evaluation from Superset. + """ + + # TODO: find a good example for this backend client. I don't know if there is an easy way of + # Implementing unittests for this since we'd need an Aspects instance for it to work? + # Maybe mocking the communication with Superset? diff --git a/openedx_user_groups/criteria.py b/openedx_user_groups/criteria.py new file mode 100644 index 0000000..9d3dafc --- /dev/null +++ b/openedx_user_groups/criteria.py @@ -0,0 +1,186 @@ +""" +This module is responsible for the base criterion type and the comparison operators. + +Here's a high level overview of the module: + +- The base criterion type is a class that defines the interface for all criterion types. +- It defines the supported operators, the configuration model, and the evaluation method. +- The comparison operators are used to compare the conditions with the configuration. +- The evaluation method is used to evaluate the criterion. +""" + +import logging +from abc import ABC, abstractmethod +from enum import Enum +from typing import Any, Dict, List, Type + +from django.db.models import Q, QuerySet +from pydantic import BaseModel, ValidationError + +logger = logging.getLogger(__name__) + + +class ComparisonOperator(str, Enum): + """Supported comparison operators for criterion evaluation.""" + + # Equality operators + EQUAL = "=" + NOT_EQUAL = "!=" + + # Comparison operators + GREATER_THAN = ">" + GREATER_THAN_OR_EQUAL = ">=" + LESS_THAN = "<" + LESS_THAN_OR_EQUAL = "<=" + + # String operators + CONTAINS = "contains" + NOT_CONTAINS = "not_contains" + + # List/Set operators + IN = "in" + NOT_IN = "not_in" + + # Existence operators + EXISTS = "exists" + NOT_EXISTS = "not_exists" + + +class BaseCriterionType(ABC): + """ + Base class for all criterion types. + + Each criterion type must implement this interface to provide validation + and evaluation logic for specific user group conditions. + """ + + # Must be overridden by subclasses + criterion_type: str = ( + None # This matches the criterion_type in the Criterion model, and is used to load the criterion class for evaluation purposes. + ) + description: str = None + + # Pydantic model for validating configuration. TODO: Should we use attrs instead? + ConfigModel: Type[BaseModel] = None + + # Supported operators for this criterion type + # This should be overridden by subclasses + supported_operators: List[ComparisonOperator] = None + + # As default, all criteria support all scopes. + scopes: List[str] = ["course", "organization", "instance"] + + # TODO: include these suggestions in the 0002 ADR? + + # TODO: This could be an option to handle estimated selectivity between criteria (0.0 = very restrictive, 1.0 = not restrictive) + # Lower values will be applied last for better performance. The evaluation engine could handle this by applying the criteria in order of estimated selectivity. + # estimated_selectivity: float = 0.5 + + # TODO: this might not be necessary, we're currently using it for validation purposes when creating a criterion. + # But we could just validate the config when saving the criterion by using the class methods directly. + def __init__( + self, + criterion_operator: str, + criterion_config: dict, + ): + self.criterion_operator = self.validate_operator(criterion_operator) + self.criterion_config = self.validate_config(criterion_config) + + def __init_subclass__(cls, **kwargs): + """Override to validate the subclass attributes.""" + super().__init_subclass__(**kwargs) + if cls.criterion_type is None: + raise ValueError( + f"Criterion class {cls.__name__} must define a 'criterion_type' attribute" + ) + if cls.description is None: + raise ValueError( + f"Criterion class {cls.__name__} must define a 'description' attribute" + ) + if cls.ConfigModel is None: + raise ValueError( + f"Criterion class {cls.__name__} must define a 'ConfigModel' attribute" + ) + if cls.supported_operators is None: + raise ValueError( + f"Criterion class {cls.__name__} must define a 'supported_operators' attribute" + ) + + def validate_config(self, config: Dict[str, Any]) -> BaseModel: + """ + Validate the configuration using the criterion's Pydantic (or attrs?) model. + + Args: + config: Raw configuration dictionary + + Returns: + Validated configuration as Pydantic model instance + + Raises: + ValidationError: If configuration is invalid + """ + try: + return self.ConfigModel( + **config + ) # TODO: this is the schematic approach for validating the config. + except ValidationError as e: + logger.error(f"Invalid configuration for {self.criterion_type}: {e}") + raise + + def validate_operator(self, operator: str) -> ComparisonOperator: + """ + Validate that the operator is supported by this criterion type. + + Args: + operator: String representation of the operator + + Returns: + Validated ComparisonOperator enum value + + Raises: + ValueError: If operator is not supported + """ + try: + op = ComparisonOperator(operator) + except ValueError: + raise ValueError(f"Unknown operator: {operator}") + + if op not in self.supported_operators: + raise ValueError( + f"Operator {operator} not supported by {self.criterion_type}. " + f"Supported operators: {[op.value for op in self.supported_operators]}" + ) + + return op + + @property + def supported_operators(self): + """Return the supported operators for this criterion type.""" + return self.supported_operators + + @property + def config_model( + self, + ): # TODO: this could be used to generate the schema for the configuration. Which can later be used for UI forms? + """Return the configuration model for this criterion type.""" + return self.ConfigModel + + @abstractmethod + def evaluate( + self, + config: BaseModel, + operator: ComparisonOperator, + scope_context: Dict[str, Any] = None, + ) -> QuerySet: # TODO: for simplicity return a queryset. + """ + Evaluate the criterion and return a Q object for filtering users. + + Args: + config: Validated configuration (Pydantic model instance) + operator: Comparison operator to use + scope_context: Optional context about the scope (e.g., course_id) + + Returns: + QuerySet: A queryset of users that match the criterion. + """ + pass diff --git a/openedx_user_groups/criteria_types.py b/openedx_user_groups/criteria_types.py new file mode 100644 index 0000000..9b90286 --- /dev/null +++ b/openedx_user_groups/criteria_types.py @@ -0,0 +1,219 @@ +"""Module for the criteria types that implement different logic for evaluating the membership of a user group. + +Here's a high level overview of the module: +- The criteria types are classes that inherit from the BaseCriterionType class. +- They implement the evaluate method, which is used to evaluate the criterion to determine the lists of users that match the criterion. +- Each criterion implements the evaluate method differently, based on the logic of the criterion. For example, the LastLoginCriterion evaluates the last login of the user, +while the CourseEnrollmentCriterion evaluates the course enrollment of the user. +- These criteria must be registered in the CriterionManager class so they can be loaded dynamically and be used by user groups. +""" + +from datetime import timedelta +from typing import Any, Dict, List, Type + +from django.db.models import Q +from django.utils import timezone +from pydantic import BaseModel + +from openedx_user_groups.backends import BackendClient +from openedx_user_groups.criteria import BaseCriterionType, ComparisonOperator +from openedx_user_groups.models import Scope + + +class ManualCriterion(BaseCriterionType): + """ + A criterion that is used to push a given list of users to a group. + """ + + criterion_type: str = "manual" + description: str = ( + "A criterion that is used to push a given list of users to a group." + ) + + class ConfigModel(BaseModel): + user_ids: List[str] # Usernames or emails + + # Supported operators for this criterion type + supported_operators: List[ComparisonOperator] = [ + ComparisonOperator.IN, + ComparisonOperator.NOT_IN, + ] + + def evaluate( + self, + current_scope: Scope, + backend_client: BackendClient = None, + ) -> Q: # TODO: Should this be Scope type instead of dict? + """ + Evaluate the criterion. + """ + return backend_client.get_users(current_scope).filter( + id__in=self.criterion_config.user_ids + ) + + +class CourseEnrollmentCriterion(BaseCriterionType): + """ + A criterion that is used to evaluate the membership of a user group based on the course enrollment mode of the user. + """ + + criterion_type: str = "course_enrollment" + description: str = ( + "A criterion that is used to evaluate the membership of a user group based on the course enrollment mode of the user." + ) + + class ConfigModel(BaseModel): + course_id: str # TODO: maybe we could use a single criterion with multiple attributes to filter by: mode, enrollment date, etc. + + supported_operators: List[ComparisonOperator] = [ + ComparisonOperator.IN, + ComparisonOperator.NOT_IN, + ComparisonOperator.EQUAL, + ComparisonOperator.NOT_EQUAL, + ComparisonOperator.GREATER_THAN, + ComparisonOperator.GREATER_THAN_OR_EQUAL, + ComparisonOperator.LESS_THAN, + ComparisonOperator.LESS_THAN_OR_EQUAL, + ] + scopes: List[str] = ["course"] + + def evaluate( + self, + config: ConfigModel, + operator: ComparisonOperator, + scope_context: Dict[str, Any] = None, + ) -> Q: + """ + Evaluate the criterion. + """ + # Placeholder implementation for POC + return Q(id__in=[]) + + +class LastLoginCriterion(BaseCriterionType): + """ + A criterion that is used to evaluate the membership of a user group based on the last login of the user. + """ + + criterion_type: str = "last_login" + description: str = ( + "A criterion that is used to evaluate the membership of a user group based on the last login of the user." + ) + + class ConfigModel(BaseModel): + days: int # TODO: can we use a single criterion with multiple attributes to filter by: days, country, etc.? + + supported_operators: List[ComparisonOperator] = [ + ComparisonOperator.EQUAL, + ComparisonOperator.NOT_EQUAL, + ComparisonOperator.GREATER_THAN, + ComparisonOperator.GREATER_THAN_OR_EQUAL, + ComparisonOperator.LESS_THAN, + ComparisonOperator.LESS_THAN_OR_EQUAL, + ] + + def evaluate( + self, + current_scope: Scope, + backend_client: BackendClient = None, # Dependency injection for the backend client + ) -> Q: + """ + Evaluate the criterion. + + The config.days represents "days since last login": + - GREATER_THAN 1 day = users who logged in more than 1 day ago (older login) + - LESS_THAN 1 day = users who logged in less than 1 day ago (more recent login) + """ + # Map operators to Django lookup operations + # For "days since last login" logic: + # - GREATER_THAN X days = last_login < (now - X days) [older than X days] + # - LESS_THAN X days = last_login > (now - X days) [more recent than X days] + queryset_operator_mapping = { + ComparisonOperator.EQUAL: "exact", # exactly X days ago (rarely used for datetime) + ComparisonOperator.NOT_EQUAL: "exact", # not exactly X days ago + ComparisonOperator.GREATER_THAN: "lt", # more than X days ago (older) + ComparisonOperator.GREATER_THAN_OR_EQUAL: "lte", # X days ago or older + ComparisonOperator.LESS_THAN: "gt", # less than X days ago (more recent) + ComparisonOperator.LESS_THAN_OR_EQUAL: "gte", # X days ago or more recent + } + + threshold_date = timezone.now() - timedelta(days=self.criterion_config.days) + query = { + "last_login__" + + queryset_operator_mapping[self.criterion_operator]: threshold_date + } + return backend_client.get_users(current_scope).filter( + **query + ) # TODO: is it better to use Q objects instead? + + +class EnrollmentModeCriterion(BaseCriterionType): + """ + A criterion that is used to evaluate the membership of a user group based on the enrollment mode of the user. + """ + + criterion_type: str = "enrollment_mode" + description: str = ( + "A criterion that is used to evaluate the membership of a user group based on the enrollment mode of the user." + ) + + class ConfigModel(BaseModel): + mode: str # TODO: should we use a single criterion with multiple attributes to filter by: mode, enrollment date, etc.? + + supported_operators: List[ComparisonOperator] = [ + ComparisonOperator.EQUAL, + ComparisonOperator.NOT_EQUAL, + ] + scopes: List[str] = ["course"] + + def evaluate( + self, + current_scope: Scope, + backend_client: BackendClient = None, + ) -> Q: + """ + Evaluate the criterion. + """ + # TODO: we should run the tests in the edx-platform environment so enrollment models or APIs are available. + + +class UserStaffStatusCriterion(BaseCriterionType): + """ + A criterion that filters users based on their staff status. + """ + + criterion_type: str = "user_staff_status" + description: str = ( + "A criterion that filters users based on whether they are staff members or not." + ) + + class ConfigModel(BaseModel): + is_staff: bool # True to filter for staff users, False for non-staff users + + supported_operators: List[ComparisonOperator] = ( + [ # TODO: I don't think we need to support any operator. Maybe a simple is true? + # ComparisonOperator.EQUAL, + # ComparisonOperator.NOT_EQUAL, + ] + ) + + def evaluate( + self, + current_scope: Scope, + backend_client: BackendClient = None, + ) -> Q: + """ + Evaluate the criterion based on user staff status. + + Args: + config: Configuration specifying whether to look for staff (True) or non-staff (False) users + operator: Comparison operator (EQUAL or NOT_EQUAL) + current_scope: The scope to filter users within + backend_client: Backend client to get users + + Returns: + Q object for filtering users + """ + return backend_client.get_users(current_scope).filter( + is_staff=self.criterion_config.is_staff + ) diff --git a/openedx_user_groups/manager.py b/openedx_user_groups/manager.py new file mode 100644 index 0000000..78b5b88 --- /dev/null +++ b/openedx_user_groups/manager.py @@ -0,0 +1,76 @@ +"""This module is responsible for loading the criterion classes as plugins. + +Here's a high level overview of the module: +- The CriterionManager subclassed the PluginManager class from edx-django-utils to implement the plugin discovery +for the criterion types. +- The criterion types should be registered in the _criterion_registry dictionary (FOR NOW). +- Ideally, we would use the PluginManager to discover the criterion types installed by plugins following this +format: + "openedx_user_groups.criteria": [ + "last_login = openedx_user_groups.criteria.examples:LastLoginCriterion", + "enrollment_mode = openedx_user_groups.criteria.examples:EnrollmentModeCriterion", + ] +""" + +from collections import OrderedDict + +from edx_django_utils.plugins import PluginManager +from stevedore.extension import ExtensionManager + +from openedx_user_groups.criteria import BaseCriterionType + + +class CriterionManager(PluginManager): + """Manager for criterion types.""" + + NAMESPACE = "openedx_user_groups.criteria" + + # Simple registry for POC - in production this would use plugin discovery + # Format matches entry points: "name = module.path:ClassName" + # TODO: what if I install a new one with the same name and override the old one? Need versioning and validation for this. ADR! + # TODO: Maybe default criterion shouldn't be registered as plugins? + _criterion_registry = { + "last_login": "openedx_user_groups.criteria_types:LastLoginCriterion", + "course_enrollment": "openedx_user_groups.criteria_types:CourseEnrollmentCriterion", + "manual": "openedx_user_groups.criteria_types:ManualCriterion", + "enrollment_mode": "openedx_user_groups.criteria_types:EnrollmentModeCriterion", + "enrolled_with_specific_mode": "openedx_user_groups.criteria_types:EnrollmentModeCriterion", + "user_staff_status": "openedx_user_groups.criteria_types:UserStaffStatusCriterion", + } + + @classmethod + def get_criterion_types(cls): + """Return list of available criterion type names.""" + # TODO: should be get_available_plugins(), but for now this is the closest we're going to implement. + return OrderedDict(cls._criterion_registry) + + @classmethod + def get_criterion_type_by_type(cls, criterion_type): + """Return the criterion type module path for a given name.""" + # TODO: use simplest approach for POC + return cls._criterion_registry.get(criterion_type, f"Unknown_{criterion_type}") + + @classmethod + def get_criterion_class_by_type(cls, criterion_type): + """Load and return the actual criterion class for a given name.""" + module_path = cls.get_criterion_type_by_type(criterion_type) + if module_path.startswith( + "Unknown_" + ): # TODO: should we raise an error instead? + return None + + module_name, class_name = module_path.split(":") + module = __import__(module_name, fromlist=[class_name]) + return getattr(module, class_name) + + +def load_criterion_class(criterion_type: str) -> BaseCriterionType: + """Load a criterion class by type. + + Args: + criterion_type (str): The type of the criterion to load. + + Returns: + BaseCriterionType: The criterion class. + """ + return CriterionManager.get_criterion_class_by_type(criterion_type) diff --git a/openedx_user_groups/migrations/0001_initial.py b/openedx_user_groups/migrations/0001_initial.py new file mode 100644 index 0000000..6e3da62 --- /dev/null +++ b/openedx_user_groups/migrations/0001_initial.py @@ -0,0 +1,150 @@ +# Generated by Django 4.2.21 on 2025-06-10 11:31 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import openedx_user_groups.models + + +class Migration(migrations.Migration): + + initial = True + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ("contenttypes", "0002_remove_content_type_name"), + ] + + operations = [ + migrations.CreateModel( + name="Scope", + fields=[ + ( + "id", + models.BigAutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("name", models.CharField(max_length=255)), + ("description", models.TextField(blank=True, null=True)), + ("object_id", models.PositiveIntegerField()), + ( + "content_type", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to="contenttypes.contenttype", + ), + ), + ], + ), + migrations.CreateModel( + name="UserGroup", + fields=[ + ( + "id", + models.BigAutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("name", models.CharField(max_length=255)), + ("description", models.TextField(blank=True, null=True)), + ("last_membership_change", models.DateTimeField(auto_now=True)), + ("enabled", models.BooleanField(default=True)), + ( + "scope", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="user_groups", + to="openedx_user_groups.scope", + ), + ), + ], + options={ + "ordering": ["name"], + }, + ), + migrations.CreateModel( + name="UserGroupMembership", + fields=[ + ( + "id", + models.BigAutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("joined_at", models.DateTimeField(auto_now_add=True)), + ("left_at", models.DateTimeField(blank=True, null=True)), + ("is_active", models.BooleanField(default=True)), + ( + "group", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to="openedx_user_groups.usergroup", + ), + ), + ( + "user", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to=settings.AUTH_USER_MODEL, + ), + ), + ], + ), + migrations.AddField( + model_name="usergroup", + name="users", + field=models.ManyToManyField( + through="openedx_user_groups.UserGroupMembership", + to=settings.AUTH_USER_MODEL, + ), + ), + migrations.CreateModel( + name="Criterion", + fields=[ + ( + "id", + models.BigAutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ( + "criterion_type", + models.CharField( + help_text="Must be one of the available criterion types from CriterionManager", + max_length=255, + validators=[openedx_user_groups.models.validate_criterion_type], + ), + ), + ("criterion_operator", models.CharField(max_length=255)), + ("criterion_config", models.JSONField(default=dict)), + ( + "user_group", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="criteria", + to="openedx_user_groups.usergroup", + ), + ), + ], + options={ + "ordering": ["criterion_type"], + }, + ), + migrations.AlterUniqueTogether( + name="usergroup", + unique_together={("name", "scope")}, + ), + ] diff --git a/openedx_user_groups/migrations/__init__.py b/openedx_user_groups/migrations/__init__.py new file mode 100644 index 0000000..f55a51f --- /dev/null +++ b/openedx_user_groups/migrations/__init__.py @@ -0,0 +1 @@ +# Migration package for openedx_user_groups diff --git a/openedx_user_groups/models.py b/openedx_user_groups/models.py index 59ddef0..38449bc 100644 --- a/openedx_user_groups/models.py +++ b/openedx_user_groups/models.py @@ -1,8 +1,8 @@ """ Core models for Open edX User Groups. -In this module, we define the core models that represent user groups within the Open edX platform. Currently, -it includes: +In this module, we define the core models that represent user groups within the Open edX platform. Here's a high level +overview of the module: Models: - UserGroup: Represents a group of users within the Open edX platform, allowing for the management and organization of @@ -12,7 +12,7 @@ - Criterion: Represents a criterion that can be used to filter or categorize user groups based on specific attributes or behaviors. - Scope: Represents the scope of a user group, defining the context in which the group operates, such as course or -site-wide. TODO: Add this model later on. +site-wide. With the following relationships: - UserGroup has many UserGroupMembership, linking users to their respective groups. @@ -25,14 +25,64 @@ attributes only for that group. - Scope can be associated with a UserGroup, defining the context in which the group operates. A user group can be associated only with one scope at a time. + +This module is not meant for production, it's only for POC purposes. """ -from django.db import models from django.contrib.auth import get_user_model +from django.contrib.contenttypes.fields import GenericForeignKey +from django.contrib.contenttypes.models import ContentType +from django.core.exceptions import ValidationError +from django.db import models + +from openedx_user_groups.manager import CriterionManager User = get_user_model() +def validate_criterion_type(value): + """Validate that the criterion type is one of the available types. + + Args: + value (str): The criterion type to validate. + + Raises: + ValidationError: If the criterion type is not one of the available types. + """ + try: + available_types = Criterion.available_criterion_types() + if value not in available_types: + raise ValidationError( + f"'{value}' is not a valid criterion type. " + f"Available types: {', '.join(available_types)}" + ) + except AttributeError: + # If CriterionManager is not implemented yet, skip validation + pass + + +class Scope(models.Model): + """Represents the scope of a user group. + + Attributes: + name (str): The name of the scope. + description (str): A brief description of the scope. Could be used for annotation purposes. + content_type (ForeignKey): The content type of the object that defines the scope. + object_id (PositiveIntegerField): The ID of the object that defines the scope. + content_object (GenericForeignKey): The object that defines the scope (e.g., course, organization). + """ + + name = models.CharField( + max_length=255 + ) # TODO: should we use something like: display_name? + description = models.TextField(blank=True, null=True) + content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE) + object_id = models.PositiveIntegerField() + content_object = GenericForeignKey( + "content_type", "object_id" + ) # TODO: how can we display this in a nice way? + + class UserGroup(models.Model): """Represents a group of users within the Open edX platform. @@ -41,8 +91,6 @@ class UserGroup(models.Model): Attributes: name (str): The name of the user group. description (str): A brief description of the user group. - created_at (datetime): The timestamp when the user group was created. - updated_at (datetime): The timestamp when the user group was last updated. last_membership_change (datetime): The timestamp of the last change to the user group membership related to the group. enabled (bool): Whether the user group is enabled. @@ -50,29 +98,48 @@ class UserGroup(models.Model): users (ManyToManyField): The users that are members of the group. """ - id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False) - name = models.CharField(max_length=255, unique=True) + name = models.CharField(max_length=255) description = models.TextField(blank=True, null=True) - created_at = models.DateTimeField(auto_now_add=True) - updated_at = models.DateTimeField(auto_now=True) last_membership_change = models.DateTimeField(auto_now=True) enabled = models.BooleanField(default=True) - scope = models.ForeignKey(Scope, on_delete=models.CASCADE, blank=True, null=True) - users = models.ManyToManyField(User, through='UserGroupMembership') + scope = models.ForeignKey( + Scope, + on_delete=models.CASCADE, + related_name="user_groups", + ) + users = models.ManyToManyField(User, through="UserGroupMembership") class Meta: - ordering = ['name'] - unique_together = ['id', 'scope'] - + ordering = ["name"] + unique_together = [ + "name", + "scope", + ] # A group name should be unique within a scope + + # TODO: should we enforce here the group's scope is the same as the criteria's scope here before saving or in the API? + + def save(self, *args, **kwargs): + """Save the user group. + + This method is overriden to: + - Prevent the scope of an existing user group from being changed. + """ + if self.pk is not None: + original = UserGroup.objects.get(pk=self.pk) + if original.scope != self.scope: + raise ValueError("Cannot change the scope of an existing user group") + super().save(*args, **kwargs) def __str__(self): return self.name - def get_users(self): - return self.users.all() + def criteria_templates(self): + """Return the criterion templates (classes) for the user group. - def get_criteria(self): - return self.criteria.all() + Returns: + list: A list of criterion templates (classes). + """ + return [criterion.criterion_type_template for criterion in self.criteria.all()] class UserGroupMembership(models.Model): @@ -103,52 +170,33 @@ class Criterion(models.Model): behaviors. Attributes: - name (str): The name of the criterion. - description (str): A brief description of the criterion. - criterion_type (str): The type of the criterion. + criterion_type (str): The type of the criterion. This is the name of the criterion type class used as a key to + load the class from the CriterionManager. criterion_operator (str): The operator of the criterion. criterion_config (dict): The configuration of the criterion. group (UserGroup): The group to which the criterion belongs. """ + criterion_type = models.CharField( + max_length=255, # When creating a new criterion, this should be one of the available criterion types. + validators=[validate_criterion_type], + help_text="Must be one of the available criterion types from CriterionManager", + ) + criterion_operator = models.CharField(max_length=255) + criterion_config = models.JSONField(default=dict) + user_group = models.ForeignKey( + UserGroup, on_delete=models.CASCADE, related_name="criteria" + ) - name = models.CharField(max_length=255) - description = models.TextField(blank=True, null=True) - criterion_type = models.CharField(max_length=255) # TODO: Although right now there is no restriction on the type, we should only allow for registered criteria types - criterion_operator = models.CharField(max_length=255) # TODO: Replace this with Enum later on - criterion_config = models.JSONField(default=dict) # No restrictions needed on the config, each criterion type will have its own config and validate it accordingly - group = models.ForeignKey(UserGroup, on_delete=models.CASCADE, related_name='criteria') + class Meta: + ordering = ["criterion_type"] def __str__(self): - return f"{self.name} - {self.group.name}" - - def get_criterion_type(self): - return self.criterion_type # TODO: Replace this with a method that returns the criterion type class as an object + return f"{self.criterion_type} - {self.user_group.name}" - def get_criterion_operator(self): - return self.criterion_operator # TODO: Replace this so it returns the enum supported type? Is it needed? - - def get_criterion_config(self): - return self.criterion_config # Would I need to return the config as a dict? - - -class Scope(models.Model): - """Represents the scope of a user group. - - Attributes: - name (str): The name of the scope. - description (str): A brief description of the scope. - created_at (datetime): The timestamp when the scope was created. - updated_at (datetime): The timestamp when the scope was last updated. - """ - - name = models.CharField(max_length=255) - description = models.TextField(blank=True, null=True) - resource_type = models.CharField(max_length=255) # TODO: Replace this with an Enum, it should only be allowed to be one of the following: course, org or site. Maybe more? - resource_id = models.CharField(max_length=255) # TODO: Replace this with an actual opaque key or corresponding ID? - - def __str__(self): - return f"{self.name} - {self.resource_type} - {self.resource_id}" + @staticmethod + def available_criterion_types(): + return CriterionManager.get_criterion_types() - def get_resource_type(self): - return self.resource_type - \ No newline at end of file + @property + def criterion_type_template(self): + return CriterionManager.get_criterion_class_by_type(self.criterion_type) diff --git a/openedx_user_groups/registry.py b/openedx_user_groups/registry.py deleted file mode 100644 index e69de29..0000000 diff --git a/openedx_user_groups/service.py b/openedx_user_groups/service.py deleted file mode 100644 index e69de29..0000000 diff --git a/openedx_user_groups/base.py b/openedx_user_groups/views.py similarity index 100% rename from openedx_user_groups/base.py rename to openedx_user_groups/views.py diff --git a/requirements/base.in b/requirements/base.in index 9f4002e..3652d4d 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -2,6 +2,6 @@ -c constraints.txt Django # Web application framework - +edx-django-utils # edX utilities for Django openedx-atlas diff --git a/requirements/base.txt b/requirements/base.txt index f835592..44bedad 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -6,11 +6,37 @@ # asgiref==3.8.1 # via django +cffi==1.17.1 + # via pynacl +click==8.2.1 + # via edx-django-utils django==4.2.21 # via # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt # -r requirements/base.in + # django-crum + # django-waffle + # edx-django-utils +django-crum==0.7.9 + # via edx-django-utils +django-waffle==4.2.0 + # via edx-django-utils +edx-django-utils==8.0.0 + # via -r requirements/base.in openedx-atlas==0.7.0 # via -r requirements/base.in +pbr==6.1.1 + # via stevedore +psutil==7.0.0 + # via edx-django-utils +pycparser==2.22 + # via cffi +pynacl==1.5.0 + # via edx-django-utils sqlparse==0.5.3 # via django +stevedore==5.4.1 + # via edx-django-utils + +# The following packages are considered to be unsafe in a requirements file: +# setuptools diff --git a/requirements/dev.txt b/requirements/dev.txt index 589a8a1..0539fbf 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -4,6 +4,10 @@ # # pip-compile --output-file=requirements/dev.txt requirements/dev.in # +annotated-types==0.7.0 + # via + # -r requirements/quality.txt + # pydantic asgiref==3.8.1 # via # -r requirements/quality.txt @@ -21,6 +25,10 @@ cachetools==6.0.0 # via # -r requirements/ci.txt # tox +cffi==1.17.1 + # via + # -r requirements/quality.txt + # pynacl chardet==5.2.0 # via # -r requirements/ci.txt @@ -32,6 +40,7 @@ click==8.2.1 # -r requirements/quality.txt # click-log # code-annotations + # edx-django-utils # edx-lint # pip-tools click-log==0.4.0 @@ -50,6 +59,8 @@ coverage[toml]==7.8.2 # via # -r requirements/quality.txt # pytest-cov +ddt==1.7.2 + # via -r requirements/quality.txt diff-cover==9.3.1 # via -r requirements/dev.in dill==0.4.0 @@ -64,11 +75,30 @@ django==4.2.21 # via # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt # -r requirements/quality.txt + # django-crum + # django-waffle + # edx-django-utils # edx-i18n-tools +django-crum==0.7.9 + # via + # -r requirements/quality.txt + # edx-django-utils +django-waffle==4.2.0 + # via + # -r requirements/quality.txt + # edx-django-utils +edx-django-utils==8.0.0 + # via -r requirements/quality.txt edx-i18n-tools==1.9.0 # via -r requirements/dev.in edx-lint==5.6.0 # via -r requirements/quality.txt +factory-boy==3.3.3 + # via -r requirements/quality.txt +faker==37.3.0 + # via + # -r requirements/quality.txt + # factory-boy filelock==3.18.0 # via # -r requirements/ci.txt @@ -136,8 +166,22 @@ pluggy==1.6.0 # tox polib==1.2.0 # via edx-i18n-tools +psutil==7.0.0 + # via + # -r requirements/quality.txt + # edx-django-utils pycodestyle==2.13.0 # via -r requirements/quality.txt +pycparser==2.22 + # via + # -r requirements/quality.txt + # cffi +pydantic==2.11.5 + # via -r requirements/quality.txt +pydantic-core==2.33.2 + # via + # -r requirements/quality.txt + # pydantic pydocstyle==6.3.0 # via -r requirements/quality.txt pygments==2.19.1 @@ -162,6 +206,10 @@ pylint-plugin-utils==0.8.2 # -r requirements/quality.txt # pylint-celery # pylint-django +pynacl==1.5.0 + # via + # -r requirements/quality.txt + # edx-django-utils pyproject-api==1.9.1 # via # -r requirements/ci.txt @@ -205,6 +253,7 @@ stevedore==5.4.1 # via # -r requirements/quality.txt # code-annotations + # edx-django-utils text-unidecode==1.3 # via # -r requirements/quality.txt @@ -215,6 +264,20 @@ tomlkit==0.13.2 # pylint tox==4.26.0 # via -r requirements/ci.txt +typing-extensions==4.14.0 + # via + # -r requirements/quality.txt + # pydantic + # pydantic-core + # typing-inspection +typing-inspection==0.4.1 + # via + # -r requirements/quality.txt + # pydantic +tzdata==2025.2 + # via + # -r requirements/quality.txt + # faker virtualenv==20.31.2 # via # -r requirements/ci.txt diff --git a/requirements/doc.txt b/requirements/doc.txt index f2737b3..949a906 100644 --- a/requirements/doc.txt +++ b/requirements/doc.txt @@ -8,6 +8,10 @@ accessible-pygments==0.0.5 # via pydata-sphinx-theme alabaster==1.0.0 # via sphinx +annotated-types==0.7.0 + # via + # -r requirements/test.txt + # pydantic asgiref==3.8.1 # via # -r requirements/test.txt @@ -23,13 +27,17 @@ build==1.2.2.post1 certifi==2025.4.26 # via requests cffi==1.17.1 - # via cryptography + # via + # -r requirements/test.txt + # cryptography + # pynacl charset-normalizer==3.4.2 # via requests click==8.2.1 # via # -r requirements/test.txt # code-annotations + # edx-django-utils code-annotations==2.3.0 # via -r requirements/test.txt coverage[toml]==7.8.2 @@ -38,10 +46,23 @@ coverage[toml]==7.8.2 # pytest-cov cryptography==45.0.3 # via secretstorage +ddt==1.7.2 + # via -r requirements/test.txt django==4.2.21 # via # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt # -r requirements/test.txt + # django-crum + # django-waffle + # edx-django-utils +django-crum==0.7.9 + # via + # -r requirements/test.txt + # edx-django-utils +django-waffle==4.2.0 + # via + # -r requirements/test.txt + # edx-django-utils doc8==1.1.2 # via -r requirements/doc.in docutils==0.21.2 @@ -51,6 +72,14 @@ docutils==0.21.2 # readme-renderer # restructuredtext-lint # sphinx +edx-django-utils==8.0.0 + # via -r requirements/test.txt +factory-boy==3.3.3 + # via -r requirements/test.txt +faker==37.3.0 + # via + # -r requirements/test.txt + # factory-boy id==1.5.0 # via twine idna==3.10 @@ -110,8 +139,20 @@ pluggy==1.6.0 # via # -r requirements/test.txt # pytest +psutil==7.0.0 + # via + # -r requirements/test.txt + # edx-django-utils pycparser==2.22 - # via cffi + # via + # -r requirements/test.txt + # cffi +pydantic==2.11.5 + # via -r requirements/test.txt +pydantic-core==2.33.2 + # via + # -r requirements/test.txt + # pydantic pydata-sphinx-theme==0.15.4 # via sphinx-book-theme pygments==2.19.1 @@ -122,6 +163,10 @@ pygments==2.19.1 # readme-renderer # rich # sphinx +pynacl==1.5.0 + # via + # -r requirements/test.txt + # edx-django-utils pyproject-hooks==1.2.0 # via build pytest==8.3.5 @@ -193,16 +238,29 @@ stevedore==5.4.1 # -r requirements/test.txt # code-annotations # doc8 + # edx-django-utils text-unidecode==1.3 # via # -r requirements/test.txt # python-slugify twine==6.1.0 # via -r requirements/doc.in -typing-extensions==4.13.2 +typing-extensions==4.14.0 # via + # -r requirements/test.txt # beautifulsoup4 + # pydantic + # pydantic-core # pydata-sphinx-theme + # typing-inspection +typing-inspection==0.4.1 + # via + # -r requirements/test.txt + # pydantic +tzdata==2025.2 + # via + # -r requirements/test.txt + # faker urllib3==2.2.3 # via # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt diff --git a/requirements/quality.in b/requirements/quality.in index 93661d9..1a56d7f 100644 --- a/requirements/quality.in +++ b/requirements/quality.in @@ -8,3 +8,4 @@ edx-lint # edX pylint rules and plugins isort # to standardize order of imports pycodestyle # PEP 8 compliance validation pydocstyle # PEP 257 compliance validation +pydantic # static type checking diff --git a/requirements/quality.txt b/requirements/quality.txt index 693dd0f..c5f2e82 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -4,6 +4,10 @@ # # pip-compile --output-file=requirements/quality.txt requirements/quality.in # +annotated-types==0.7.0 + # via + # -r requirements/test.txt + # pydantic asgiref==3.8.1 # via # -r requirements/test.txt @@ -12,11 +16,16 @@ astroid==3.3.10 # via # pylint # pylint-celery +cffi==1.17.1 + # via + # -r requirements/test.txt + # pynacl click==8.2.1 # via # -r requirements/test.txt # click-log # code-annotations + # edx-django-utils # edx-lint click-log==0.4.0 # via edx-lint @@ -28,14 +37,35 @@ coverage[toml]==7.8.2 # via # -r requirements/test.txt # pytest-cov +ddt==1.7.2 + # via -r requirements/test.txt dill==0.4.0 # via pylint django==4.2.21 # via # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt # -r requirements/test.txt + # django-crum + # django-waffle + # edx-django-utils +django-crum==0.7.9 + # via + # -r requirements/test.txt + # edx-django-utils +django-waffle==4.2.0 + # via + # -r requirements/test.txt + # edx-django-utils +edx-django-utils==8.0.0 + # via -r requirements/test.txt edx-lint==5.6.0 # via -r requirements/quality.in +factory-boy==3.3.3 + # via -r requirements/test.txt +faker==37.3.0 + # via + # -r requirements/test.txt + # factory-boy iniconfig==2.1.0 # via # -r requirements/test.txt @@ -70,8 +100,24 @@ pluggy==1.6.0 # via # -r requirements/test.txt # pytest +psutil==7.0.0 + # via + # -r requirements/test.txt + # edx-django-utils pycodestyle==2.13.0 # via -r requirements/quality.in +pycparser==2.22 + # via + # -r requirements/test.txt + # cffi +pydantic==2.11.5 + # via + # -r requirements/quality.in + # -r requirements/test.txt +pydantic-core==2.33.2 + # via + # -r requirements/test.txt + # pydantic pydocstyle==6.3.0 # via -r requirements/quality.in pylint==3.3.7 @@ -88,6 +134,10 @@ pylint-plugin-utils==0.8.2 # via # pylint-celery # pylint-django +pynacl==1.5.0 + # via + # -r requirements/test.txt + # edx-django-utils pytest==8.3.5 # via # -r requirements/test.txt @@ -117,12 +167,27 @@ stevedore==5.4.1 # via # -r requirements/test.txt # code-annotations + # edx-django-utils text-unidecode==1.3 # via # -r requirements/test.txt # python-slugify tomlkit==0.13.2 # via pylint +typing-extensions==4.14.0 + # via + # -r requirements/test.txt + # pydantic + # pydantic-core + # typing-inspection +typing-inspection==0.4.1 + # via + # -r requirements/test.txt + # pydantic +tzdata==2025.2 + # via + # -r requirements/test.txt + # faker # The following packages are considered to be unsafe in a requirements file: # setuptools diff --git a/requirements/test.in b/requirements/test.in index 6797160..ae9756f 100644 --- a/requirements/test.in +++ b/requirements/test.in @@ -6,3 +6,6 @@ pytest-cov # pytest extension for code coverage statistics pytest-django # pytest extension for better Django support code-annotations # provides commands used by the pii_check make target. +ddt # data-driven tests +factory-boy # for creating test data +faker # for creating test data diff --git a/requirements/test.txt b/requirements/test.txt index 855c88d..138f5c8 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -4,19 +4,49 @@ # # pip-compile --output-file=requirements/test.txt requirements/test.in # +annotated-types==0.7.0 + # via pydantic asgiref==3.8.1 # via # -r requirements/base.txt # django +cffi==1.17.1 + # via + # -r requirements/base.txt + # pynacl click==8.2.1 - # via code-annotations + # via + # -r requirements/base.txt + # code-annotations + # edx-django-utils code-annotations==2.3.0 # via -r requirements/test.in coverage[toml]==7.8.2 # via pytest-cov +ddt==1.7.2 + # via -r requirements/test.in # via # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt # -r requirements/base.txt + # django-crum + # django-waffle + # edx-django-utils +django-crum==0.7.9 + # via + # -r requirements/base.txt + # edx-django-utils +django-waffle==4.2.0 + # via + # -r requirements/base.txt + # edx-django-utils +edx-django-utils==8.0.0 + # via -r requirements/base.txt +factory-boy==3.3.3 + # via -r requirements/test.in +faker==37.3.0 + # via + # -r requirements/test.in + # factory-boy iniconfig==2.1.0 # via pytest jinja2==3.1.6 @@ -28,9 +58,27 @@ openedx-atlas==0.7.0 packaging==25.0 # via pytest pbr==6.1.1 - # via stevedore + # via + # -r requirements/base.txt + # stevedore pluggy==1.6.0 # via pytest +psutil==7.0.0 + # via + # -r requirements/base.txt + # edx-django-utils +pycparser==2.22 + # via + # -r requirements/base.txt + # cffi +pydantic==2.11.5 + # via -r requirements/test.in +pydantic-core==2.33.2 + # via pydantic +pynacl==1.5.0 + # via + # -r requirements/base.txt + # edx-django-utils pytest==8.3.5 # via # pytest-cov @@ -48,9 +96,21 @@ sqlparse==0.5.3 # -r requirements/base.txt # django stevedore==5.4.1 - # via code-annotations + # via + # -r requirements/base.txt + # code-annotations + # edx-django-utils text-unidecode==1.3 # via python-slugify +typing-extensions==4.14.0 + # via + # pydantic + # pydantic-core + # typing-inspection +typing-inspection==0.4.1 + # via pydantic +tzdata==2025.2 + # via faker # The following packages are considered to be unsafe in a requirements file: # setuptools diff --git a/test_settings.py b/test_settings.py index e78336f..036995b 100644 --- a/test_settings.py +++ b/test_settings.py @@ -55,6 +55,7 @@ def root(*args): "APP_DIRS": False, "OPTIONS": { "context_processors": [ + "django.template.context_processors.request", # required for admin navigation sidebar "django.contrib.auth.context_processors.auth", # this is required for admin "django.contrib.messages.context_processors.messages", # this is required for admin ], diff --git a/tests/test_api.py b/tests/test_api.py new file mode 100644 index 0000000..90128fd --- /dev/null +++ b/tests/test_api.py @@ -0,0 +1,347 @@ +"""Test Suite for the User Group interface (api.py) that could be used by other modules. + +This test suite is only for POC purposes, so it won't follow the best practices for testing, +this module could be refactored later on. + +This test suite will be used to test the public / private API of the User Group module. +""" + +import factory +from django.test import TestCase +from django.contrib.auth import get_user_model +from django.contrib.contenttypes.models import ContentType +from django.utils import timezone +from datetime import timedelta + +from openedx_user_groups.api import * +from openedx_user_groups.models import UserGroup, Scope, Criterion + + +User = get_user_model() + + +class CourseFactory(factory.Factory): + """Factory for creating Course-like objects for testing. + + Since we don't want to create a real Course model, this factory + generates dict objects that simulate course data. + """ + + class Meta: + model = dict # Use a dict to simulate a course object + + course_id = factory.Sequence(lambda n: f"course-v1:edX+Demo{n}+Course") + name = factory.Faker("sentence", nb_words=3) + description = factory.Faker("text", max_nb_chars=200) + id = factory.Sequence(lambda n: n) + + +class UserFactory(factory.django.DjangoModelFactory): + """Factory for creating User instances.""" + + class Meta: + model = User + + username = factory.Sequence(lambda n: f"user_{n}") + email = factory.LazyAttribute(lambda obj: f"{obj.username}@example.com") + first_name = factory.Faker("first_name") + last_name = factory.Faker("last_name") + + +class ScopeFactory(factory.django.DjangoModelFactory): + """Factory for creating Scope instances.""" + + class Meta: + model = Scope + + name = factory.Faker("sentence", nb_words=3) + description = factory.Faker("text", max_nb_chars=200) + # Use User model's ContentType as a default since it exists in test DB + content_type = factory.LazyFunction(lambda: ContentType.objects.get_for_model(User)) + object_id = factory.Sequence(lambda n: n) + + +class UserGroupFactory(factory.django.DjangoModelFactory): + """Factory for creating UserGroup instances.""" + + class Meta: + model = UserGroup + + name = factory.Faker("sentence", nb_words=2) + description = factory.Faker("text", max_nb_chars=200) + enabled = True + scope = factory.SubFactory(ScopeFactory) + + +class CriterionFactory(factory.django.DjangoModelFactory): + """Factory for creating Criterion instances.""" + + class Meta: + model = Criterion + + +class LastLoginCriterionFactory(CriterionFactory): + """Factory for creating LastLoginCriterion instances.""" + + criterion_type = "last_login" + criterion_operator = ">" # Login date is greater than 1 day ago + criterion_config = factory.Dict({"days": 1}) + + +class EnrollmentModeCriterionFactory(CriterionFactory): + """Factory for creating EnrollmentModeCriterion instances.""" + + criterion_type = "enrollment_mode" + criterion_operator = "=" + criterion_config = factory.Dict({"mode": "honor"}) + + +class UserStaffStatusCriterionFactory(CriterionFactory): + """Factory for creating UserStaffStatusCriterion instances.""" + + criterion_type = "user_staff_status" + criterion_operator = "=" + criterion_config = factory.Dict({"is_staff": False}) # Filter for non-staff users + + +class UserGroupAPITestCase(TestCase): + + @classmethod + def setUpTestData(cls): + """Set up test data that will be reused across all test methods.""" + cls.test_course = CourseFactory() + cls.course_content_type = ContentType.objects.get_for_model(User) + cls.test_scope = ScopeFactory( + name=cls.test_course["name"], + content_type=cls.course_content_type, + object_id=cls.test_course["id"], + ) + cls.test_user_group_data = UserGroupFactory.build(name="At Risk Students") + cls.last_login_criterion = LastLoginCriterionFactory.build() + cls.enrollment_mode_criterion = EnrollmentModeCriterionFactory.build() + cls.user_staff_status_criterion = UserStaffStatusCriterionFactory.build() + cls.scope_context = { + "name": cls.test_course["name"], + "content_object": { + "content_type": cls.course_content_type, + "object_id": cls.test_course["id"], + }, + } + + def test_create_group_with_no_criteria(self): + """Test that a group can be created with no criteria associated. + + Expected Results: + - The group is created successfully. + - The group has no criteria associated. + - The group has the correct name, description, and scope. + - The group has no members. + - The group is enabled. + """ + user_group, scope = get_or_create_group_and_scope( + name=self.test_user_group_data.name, + description=self.test_user_group_data.description, + scope_context=self.scope_context, + ) + + assert user_group is not None + assert user_group.name == self.test_user_group_data.name + assert user_group.description == self.test_user_group_data.description + assert scope.name == self.test_scope.name + assert user_group.criteria.count() == 0 + + def test_associate_multiple_groups_with_same_scope(self): + """Test that multiple groups can be associated with the same scope. + + Expected Results: + - The groups are created successfully. + - The groups have the correct name, description, and scope. + - The groups are associated with the same scope. + """ + user_group_1, scope_1 = get_or_create_group_and_scope( + name=f"{self.test_user_group_data.name}_1", + description=self.test_user_group_data.description, + scope_context=self.scope_context, + ) + user_group_2, scope_2 = get_or_create_group_and_scope( + name=f"{self.test_user_group_data.name}_2", + description=self.test_user_group_data.description, + scope_context=self.scope_context, + ) + + assert scope_1.name == self.test_scope.name + assert scope_2.name == self.test_scope.name + assert scope_1.name == scope_2.name + + def test_create_group_with_single_criterion(self): + """Test that a group can be created with a single criterion. + + Expected Results: + - The group is created successfully. + - The group has the correct name, description, and scope. + - The group has the correct criterion. + """ + user_group = create_group_with_criteria( + name=self.test_user_group_data.name, + description=self.test_user_group_data.description, + scope_context=self.scope_context, + criterion_data=[ + { + "criterion_type": self.last_login_criterion.criterion_type, + "criterion_operator": self.last_login_criterion.criterion_operator, + "criterion_config": self.last_login_criterion.criterion_config, + } + ], + ) + + assert user_group is not None + assert user_group.criteria.count() == 1 + + def test_create_group_with_multiple_criteria(self): + """Test that a group can be created with multiple criteria. + + Expected Results: + - The group is created successfully. + - The group has the correct name, description, and scope. + - The group has the correct criteria. + + In this case the criteria would be: + 1. Last login in the last 1 day + 2. Enrolled with honor mode + """ + user_group = create_group_with_criteria( + name=self.test_user_group_data.name, + description=self.test_user_group_data.description, + scope_context=self.scope_context, + criterion_data=[ + { + "criterion_type": self.last_login_criterion.criterion_type, + "criterion_operator": self.last_login_criterion.criterion_operator, + "criterion_config": self.last_login_criterion.criterion_config, + }, + { + "criterion_type": self.enrollment_mode_criterion.criterion_type, + "criterion_operator": self.enrollment_mode_criterion.criterion_operator, + "criterion_config": self.enrollment_mode_criterion.criterion_config, + }, + ], + ) + assert user_group is not None + assert user_group.criteria.count() == 2 + assert user_group.criteria.filter( + criterion_type=self.last_login_criterion.criterion_type + ).exists() + assert user_group.criteria.filter( + criterion_type=self.enrollment_mode_criterion.criterion_type + ).exists() + + def test_create_group_with_mismatched_criteria_scope(self): + """Test that a group can't be created with criteria that don't match the group's scope. + + Expected Results: + - The group is not created. + - An exception is raised. + """ + pass + + def test_create_group_with_criteria_and_evaluate_membership(self): + """Test that a group can be created with criteria and immediatly evaluated for membership. + + Expected Results: + - The group is created successfully. + - The group has the correct name, description, and scope. + - The group has the correct criteria. + - The group has the correct members. + + Criteria: + 1. Last login GREATER_THAN 1 day ago (meaning older than 1 day) + 2. User is non-staff (is_staff = False) + + Expected match: user_old_login_non_staff (2 days ago, non-staff) + """ + # Create users with different characteristics for testing + # Clean up any existing users + User.objects.all().delete() + + user_old_login_non_staff = UserFactory( + username="user_old_login_non_staff", + last_login=timezone.now() - timedelta(days=2), # 2 days ago (> 1 day ago) + is_staff=False, # non-staff + ) + user_recent_login_staff = UserFactory( + username="user_recent_login_staff", + last_login=timezone.now() - timedelta(hours=1), # 1 hour ago (< 1 day ago) + is_staff=True, # staff + ) + user_old_login_staff = UserFactory( + username="user_old_login_staff", + last_login=timezone.now() - timedelta(days=3), # 3 days ago (> 1 day ago) + is_staff=True, # staff (fails is_staff=False criterion) + ) + + # Create a group with criteria (last_login and staff_status) + user_group = create_group_with_criteria_and_evaluate_membership( + name=self.test_user_group_data.name, + description=self.test_user_group_data.description, + scope_context=self.scope_context, + criterion_data=[ + { + "criterion_type": self.last_login_criterion.criterion_type, + "criterion_operator": self.last_login_criterion.criterion_operator, + "criterion_config": self.last_login_criterion.criterion_config, + }, + { + "criterion_type": self.user_staff_status_criterion.criterion_type, + "criterion_operator": self.user_staff_status_criterion.criterion_operator, + "criterion_config": self.user_staff_status_criterion.criterion_config, + }, + ], + ) + assert user_group is not None + assert user_group.criteria.count() == 2 + assert user_group.users.count() == 1 + # Should match user_old_login_non_staff (old login AND non-staff) + assert user_group.users.first() == user_old_login_non_staff + + def test_evaluate_membership_for_multiple_groups(self): + """Test that the membership of multiple groups can be evaluated and updated. + + Expected Results: + - The groups are evaluated successfully. + - The groups have the correct members. + """ + # Clean up any existing users + User.objects.all().delete() + + # Create users with different characteristics for testing + user_old_login_non_staff = UserFactory( + username="user_old_login_non_staff", + last_login=timezone.now() - timedelta(days=2), # 2 days ago (> 1 day ago) + is_staff=False, # non-staff + ) + + # Create a groups with criteria + groups = [ + create_group_with_criteria( + name=f"{self.test_user_group_data.name}_{i}", + description=self.test_user_group_data.description, + scope_context=self.scope_context, + criterion_data=[ + { + "criterion_type": self.last_login_criterion.criterion_type, + "criterion_operator": self.last_login_criterion.criterion_operator, + "criterion_config": self.last_login_criterion.criterion_config, + } + ], + ) + for i in range(2) + ] + assert len(groups) == 2 + + # Evaluate the membership of the groups + evaluate_and_update_membership_for_multiple_groups( + [group.id for group in groups] + ) + + assert groups[0].users.count() == 1 + assert groups[1].users.count() == 1 diff --git a/tests/test_models.py b/tests/test_models.py index 2563c8e..c149192 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -1,15 +1,228 @@ -#!/usr/bin/env python -""" -Tests for the `openedx-user-groups` models module. +"""Test Suite for the User Group models. + +This test suite covers all model methods, properties, and behaviors defined in models.py. """ -import pytest +import factory +from django.db import IntegrityError +from django.test import TestCase +from django.contrib.auth import get_user_model +from django.contrib.contenttypes.models import ContentType +from openedx_user_groups.manager import CriterionManager +from openedx_user_groups.models import UserGroup, Criterion, Scope +User = get_user_model() -@pytest.mark.skip( - reason="Placeholder to allow pytest to succeed before real tests are in place." -) -def test_placeholder(): - """ - TODO: Delete this test once there are real tests. + +class CourseFactory(factory.Factory): + """Factory for creating simple course data objects for testing.""" + + class Meta: + model = dict + + course_id = factory.Sequence(lambda n: f"course-v1:edX+Demo{n}+Course") + name = factory.Faker("sentence", nb_words=3) + description = factory.Faker("text", max_nb_chars=200) + id = factory.Sequence(lambda n: n) + + +class TestUserGroupMethods(TestCase): + """Test UserGroup model methods and properties.""" + + @classmethod + def setUpTestData(cls): + """Set up test data for UserGroup tests.""" + # Create course data using the factory + cls.test_course = CourseFactory() + + # Use User model for ContentType (it has an existing table) + cls.course_content_type = ContentType.objects.get_for_model(User) + + cls.scope = Scope.objects.create( + name="Demo Course Scope", + description="Scope for the demo course", + content_type=cls.course_content_type, + object_id=cls.test_course["id"], + ) + cls.user_group = UserGroup.objects.create( + name="Test Group", description="A test group", scope=cls.scope + ) + + def test_user_group_str_method(self): + """Test UserGroup __str__ method returns the name. + + Expected Results: + - The __str__ method returns the name of the group. + """ + assert str(self.user_group) == "Test Group" + + def test_user_group_save_prevents_scope_change(self): + """Test that UserGroup.save() prevents changing scope of existing group. + + Expected Results: + - The group is not saved. + - An exception is raised. + """ + # Create another course data for the new scope + another_course = CourseFactory() + new_scope = Scope.objects.create( + name="New Scope", + description="Another scope", + content_type=self.course_content_type, + object_id=another_course["id"], + ) + + self.user_group.scope = new_scope + + with self.assertRaises(ValueError) as context: + self.user_group.save() + + assert "Cannot change the scope of an existing user group" in str( + context.exception + ) + + def test_user_group_criteria_classes_method(self): + """Test UserGroup criteria_classes method returns criterion types. + + Expected Results: + - The method returns a list of criterion types classes associated with the user group. + """ + user_group_with_criteria = UserGroup.objects.create( + name="Test Group with Criteria", + scope=self.scope, + ) + Criterion.objects.create( + user_group=user_group_with_criteria, + criterion_type="last_login", + criterion_operator=">=", + criterion_config={"days": 5}, + ) + + criterion_templates = user_group_with_criteria.criteria_templates() + assert len(criterion_templates) == 1 + assert criterion_templates[0] is not None + assert criterion_templates[0].criterion_type == "last_login" + + +class TestCriterionMethods(TestCase): + """Test Criterion model methods and properties.""" + + @classmethod + def setUpTestData(cls): + """Set up test data for Criterion tests.""" + # Create course data using the factory + cls.test_course = CourseFactory() + cls.course_content_type = ContentType.objects.get_for_model(User) + + cls.scope = Scope.objects.create( + name="Demo Course Scope", + content_type=cls.course_content_type, + object_id=cls.test_course["id"], + ) + cls.user_group = UserGroup.objects.create(name="Test Group", scope=cls.scope) + cls.criterion = Criterion.objects.create( + user_group=cls.user_group, + criterion_type="last_login", + criterion_operator=">=", + criterion_config={"days": 5}, + ) + + def test_criterion_str_method(self): + """Test Criterion __str__ method.""" + expected = "last_login - Test Group" + assert str(self.criterion) == expected + + def test_criterion_type_property(self): + """Test Criterion criterion_type property.""" + criterion_type = self.criterion.criterion_type_template + assert criterion_type is not None + assert criterion_type.criterion_type == "last_login" + + def test_get_available_criterion_types(self): + """Test that the get_available_criterion_types method returns the correct criterion types. + + Expected Results: + - The method returns a list of criterion types classes available for the entire system. + """ + available_types = Criterion.available_criterion_types() + assert CriterionManager.get_criterion_types() == available_types + + def test_criterion_type_validation(self): + """Test that invalid criterion types are rejected.""" + from django.core.exceptions import ValidationError + + # Test that invalid criterion type raises ValidationError + invalid_criterion = Criterion( + user_group=self.user_group, + criterion_type="invalid_type_that_does_not_exist", + criterion_operator="=", + ) + + # This should raise a ValidationError when full_clean() is called + with self.assertRaises(ValidationError) as context: + invalid_criterion.full_clean() + + # Check that the error is about criterion_type + assert "criterion_type" in str( + context.exception + ) or "is not a valid criterion type" in str(context.exception) + + +class TestModelConstraints(TestCase): + """Test model constraints and unique together constraints. + + We're not testing that Django works, but that the design of the models is correct. """ + + @classmethod + def setUpTestData(cls): + """Set up test data for constraint tests.""" + # Create course data using the factory + cls.test_course = CourseFactory() + cls.course_content_type = ContentType.objects.get_for_model(User) + + cls.scope = Scope.objects.create( + name="Demo Course Scope", + content_type=cls.course_content_type, + object_id=cls.test_course["id"], + ) + cls.user_group = UserGroup.objects.create(name="Test Group", scope=cls.scope) + + def test_user_group_unique_name_per_scope(self): + """Test that UserGroup name must be unique within a scope.""" + # This should work fine + UserGroup.objects.create(name="Unique Name", scope=self.scope) + + # This should raise an IntegrityError due to unique_together constraint + with self.assertRaises(IntegrityError): + UserGroup.objects.create(name="Unique Name", scope=self.scope) + + def test_user_group_same_name_different_scope(self): + """Test that UserGroup can have same name in different scopes.""" + # Create another course data and scope + another_course = CourseFactory() + another_scope = Scope.objects.create( + name="Another Scope", + content_type=self.course_content_type, + object_id=another_course["id"], + ) + + # This should work fine - same name but different scope + another_group = UserGroup.objects.create( + name="Test Group", # Same name as the one in setUpTestData + scope=another_scope, + ) + + assert another_group.name == self.user_group.name + assert another_group.scope != self.user_group.scope + + def test_criterion_unique_name_per_group(self): + """Test that Criterion name must be unique within a user group.""" + # This should work fine + Criterion.objects.create(criterion_type="last_login", user_group=self.user_group) + + # This should raise an IntegrityError due to unique_together constraint + with self.assertRaises(Exception): # Could be IntegrityError or ValidationError + Criterion.objects.create( + criterion_type="last_login", user_group=self.user_group + ) From 46620ce30d717f7b386eef504d2bef355d8e0757 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 23 Jun 2025 20:11:13 +0200 Subject: [PATCH 05/12] feat: add event-base refreshment mechanism for groups based on criteria * Add event-base refreshment mechanism for user groups based on criteria types, which when triggered would automatically add/remove users to/from the user group affected by it --- default.db | 0 openedx_user_groups/api.py | 56 ++++--- openedx_user_groups/apps.py | 12 ++ openedx_user_groups/criteria.py | 75 +++++++-- openedx_user_groups/criteria_types.py | 91 ++++++----- openedx_user_groups/events.py | 19 +++ openedx_user_groups/handlers.py | 29 ++++ openedx_user_groups/manager.py | 3 +- openedx_user_groups/models.py | 15 +- openedx_user_groups/tasks.py | 120 ++++++++++++++ requirements/base.in | 2 + requirements/base.txt | 67 +++++++- requirements/dev.txt | 86 +++++++++- requirements/doc.txt | 102 ++++++++++-- requirements/quality.txt | 104 ++++++++++-- requirements/test.txt | 109 +++++++++++-- tests/__init__.py | 0 tests/factories.py | 92 +++++++++++ tests/test_api.py | 160 ++++++------------- tests/test_models.py | 31 +++- tests/test_tasks.py | 217 ++++++++++++++++++++++++++ 21 files changed, 1151 insertions(+), 239 deletions(-) create mode 100644 default.db create mode 100644 openedx_user_groups/events.py create mode 100644 openedx_user_groups/handlers.py create mode 100644 openedx_user_groups/tasks.py create mode 100644 tests/__init__.py create mode 100644 tests/factories.py create mode 100644 tests/test_tasks.py diff --git a/default.db b/default.db new file mode 100644 index 0000000..e69de29 diff --git a/openedx_user_groups/api.py b/openedx_user_groups/api.py index 695b2c1..b28a918 100644 --- a/openedx_user_groups/api.py +++ b/openedx_user_groups/api.py @@ -33,6 +33,7 @@ "update_group_name_or_description", "soft_delete_group", "hard_delete_group", + "get_groups_for_user", ] @@ -72,7 +73,9 @@ def get_or_create_group_and_scope( """ with transaction.atomic(): scope, created = Scope.objects.get_or_create( - name=scope_context["name"], + name=scope_context[ + "name" + ], # TODO: what is this going to be? The course_key (CourseKey) as string? content_type=scope_context["content_object"]["content_type"], object_id=scope_context["content_object"]["object_id"], ) @@ -103,7 +106,7 @@ def load_criterion_class_and_create_instance( def create_group_with_criteria_from_data( - name: str, description: str, scope_context: dict, criterion_data: [dict] + name: str, description: str, scope_context: dict, criterion_data: [dict] # TODO: should we use pydantic models instead of dicts? ): """Create a new user group with the given name, description, scope, and criteria. This criteria hasn't been instantiated and validated yet. @@ -127,15 +130,9 @@ def create_group_with_criteria_from_data( data["criterion_operator"], data["criterion_config"], ) - # TODO:Check if the criterion supports the scope type based on content type - # scope_type = get_scope_type_from_content_type(scope.content_type) - # assert scope_type in criterion_instance.scopes, f"Criterion does not support scope type '{scope_type}'. Supported scopes: {criterion_instance.scopes}" - Criterion.objects.create( - criterion_type=criterion_instance.criterion_type, - criterion_operator=criterion_instance.criterion_operator.value, - criterion_config=criterion_instance.criterion_config.model_dump(), # TODO: should we do this somewhere else? - user_group=user_group, - ) + scope_type = get_scope_type_from_content_type(scope.content_type) + assert scope_type in criterion_instance.scopes, f"Criterion '{criterion_instance.criterion_type}' does not support scope type '{scope_type}'. Supported scopes: {criterion_instance.scopes}" + Criterion.objects.create(user_group=user_group, **criterion_instance.serialize()) return user_group @@ -145,20 +142,13 @@ def evaluate_and_update_membership_for_group(group_id: int): Args: group_id (str): The ID of the user group. """ - # TODO: This should be done asynchronously. + # TODO: We should enforce that this is done asynchronously. with transaction.atomic(): user_group = get_group_by_id(group_id) - criteria = Criterion.objects.filter(user_group=user_group) - # Evaluatate criteria and build list of Q objects - Done by what we called "combinator" criteria_results = [] - for criterion in criteria: - criterion_instance = load_criterion_class_and_create_instance( - criterion.criterion_type, - criterion.criterion_operator, - criterion.criterion_config, - ) - result = criterion_instance.evaluate( + for criterion in user_group.criteria.all(): + result = criterion.criterion_instance.evaluate( current_scope=user_group.scope, backend_client=DjangoORMBackendClient, # TODO: for now we'd only support DjangoORMBackendClient. But I think we could pass a list of registered backend clients here. ) @@ -185,7 +175,11 @@ def evaluate_and_update_membership_for_group(group_id: int): # Create new memberships new_memberships = [ - UserGroupMembership(user=user, group=user_group, joined_at=timezone.now()) + UserGroupMembership( + user=user, + group=user_group, + joined_at=timezone.now(), + ) for user in users ] UserGroupMembership.objects.bulk_create(new_memberships) @@ -274,8 +268,26 @@ def get_groups_for_scope(content_object_id: int): return Scope.objects.get(content_object_id=content_object_id).user_groups.all() +def get_groups_for_user(user_id: int): + """Get all user groups for a given user. + + This method is used to get all user groups for a given user. + It is used to check if the user is a member of any group. + + Args: + user_id (int): The ID of the user. + + Returns: + list: A list of user groups with minimum information. + """ + return UserGroupMembership.objects.filter( + user_id=user_id, is_active=True + ).select_related("group") + + # TODO: THESE METHODS I HAVEN'T TESTED YET + def get_group_by_id(group_id: int): """Get a user group by its ID. diff --git a/openedx_user_groups/apps.py b/openedx_user_groups/apps.py index 6e79465..178dce6 100644 --- a/openedx_user_groups/apps.py +++ b/openedx_user_groups/apps.py @@ -12,3 +12,15 @@ class OpenedxUserGroupsConfig(AppConfig): name = "openedx_user_groups" default_auto_field = "django.db.models.BigAutoField" + + def ready(self): + """ + Perform application initialization. + + This method connects the handler to all events that trigger updates to the user groups. + """ + from openedx_user_groups.criteria import BaseCriterionType + from openedx_user_groups.handlers import handle_user_group_update + + for event in BaseCriterionType.get_all_updated_by_events(): + event.connect(handle_user_group_update) diff --git a/openedx_user_groups/criteria.py b/openedx_user_groups/criteria.py index 9d3dafc..d730a4f 100644 --- a/openedx_user_groups/criteria.py +++ b/openedx_user_groups/criteria.py @@ -11,10 +11,12 @@ import logging from abc import ABC, abstractmethod +from collections import defaultdict from enum import Enum from typing import Any, Dict, List, Type from django.db.models import Q, QuerySet +from openedx_events.tooling import OpenEdxPublicSignal from pydantic import BaseModel, ValidationError logger = logging.getLogger(__name__) @@ -54,6 +56,13 @@ class BaseCriterionType(ABC): and evaluation logic for specific user group conditions. """ + # This is used to map events types to criterion types. For example: + # { + # "org.openedx.learning.user.staff_status.changed.v1": [UserStaffStatusCriterion], + # "org.openedx.learning.user.enrollment.changed.v1": [CourseEnrollmentCriterion], + # } + _event_to_class_map: Dict[str, List[str]] = defaultdict(list) + # Must be overridden by subclasses criterion_type: str = ( None # This matches the criterion_type in the Criterion model, and is used to load the criterion class for evaluation purposes. @@ -81,10 +90,13 @@ class BaseCriterionType(ABC): def __init__( self, criterion_operator: str, - criterion_config: dict, + criterion_config: dict | BaseModel, ): + if isinstance(criterion_config, BaseModel): + self.criterion_config = criterion_config # DO not validate if we're passing a pydantic model + else: + self.criterion_config = self.validate_config(criterion_config) self.criterion_operator = self.validate_operator(criterion_operator) - self.criterion_config = self.validate_config(criterion_config) def __init_subclass__(cls, **kwargs): """Override to validate the subclass attributes.""" @@ -101,10 +113,6 @@ def __init_subclass__(cls, **kwargs): raise ValueError( f"Criterion class {cls.__name__} must define a 'ConfigModel' attribute" ) - if cls.supported_operators is None: - raise ValueError( - f"Criterion class {cls.__name__} must define a 'supported_operators' attribute" - ) def validate_config(self, config: Dict[str, Any]) -> BaseModel: """ @@ -145,7 +153,11 @@ def validate_operator(self, operator: str) -> ComparisonOperator: except ValueError: raise ValueError(f"Unknown operator: {operator}") - if op not in self.supported_operators: + if ( + hasattr(self, "supported_operators") + and self.supported_operators + and op not in self.supported_operators + ): raise ValueError( f"Operator {operator} not supported by {self.criterion_type}. " f"Supported operators: {[op.value for op in self.supported_operators]}" @@ -153,11 +165,6 @@ def validate_operator(self, operator: str) -> ComparisonOperator: return op - @property - def supported_operators(self): - """Return the supported operators for this criterion type.""" - return self.supported_operators - @property def config_model( self, @@ -184,3 +191,47 @@ def evaluate( QuerySet: A queryset of users that match the criterion. """ pass + + def get_updated_by_events(self) -> List[str]: + """Return the events that trigger an update based on the criterion type. + + Returns: + List[str]: A list of events that trigger an update to the user groups. + """ + return self.updated_by_events + + @classmethod + def get_all_updated_by_events(cls) -> List[OpenEdxPublicSignal]: + """Return all events that trigger updates across all criterion types. + + This method also populates the _event_to_class_map class attribute that is used to map events to criterion + types. This is used to determine which criterion types are affected by an event. + + Returns: + List[OpenEdxPublicSignal]: A list of events that trigger an update to the user groups. + """ + events = set() + for subclass in cls.__subclasses__(): + if hasattr(subclass, "updated_by_events"): + events.update(subclass.updated_by_events) + for event in subclass.updated_by_events: + cls._event_to_class_map[event.event_type].append( + subclass.criterion_type + ) + return list(events) + + def serialize(self, *args, **kwargs): + """Return the criterion type, operator and config as a dictionary ready to be saved to the database. + + Args: + *args: Additional arguments to pass to the model_dump method. + **kwargs: Additional keyword arguments to pass to the model_dump method. + + Returns: + dict: A dictionary containing the criterion type, operator and config. + """ + return { + "criterion_type": self.criterion_type, + "criterion_operator": self.criterion_operator, + "criterion_config": self.criterion_config.model_dump(*args, **kwargs), + } diff --git a/openedx_user_groups/criteria_types.py b/openedx_user_groups/criteria_types.py index 9b90286..38f1691 100644 --- a/openedx_user_groups/criteria_types.py +++ b/openedx_user_groups/criteria_types.py @@ -8,11 +8,19 @@ - These criteria must be registered in the CriterionManager class so they can be loaded dynamically and be used by user groups. """ -from datetime import timedelta -from typing import Any, Dict, List, Type +from datetime import datetime, timedelta +from typing import Any, Dict, List, Optional, Type -from django.db.models import Q +import attr +from django.db.models import Q, QuerySet from django.utils import timezone +from openedx_events.learning.data import UserData +from openedx_events.learning.signals import ( + COURSE_ENROLLMENT_CHANGED, + COURSE_ENROLLMENT_CREATED, + SESSION_LOGIN_COMPLETED, +) +from openedx_events.tooling import OpenEdxPublicSignal from pydantic import BaseModel from openedx_user_groups.backends import BackendClient @@ -20,10 +28,21 @@ from openedx_user_groups.models import Scope +@attr.s(frozen=True) +class UserDataExtended(UserData): + is_staff = attr.ib(type=bool) + + +USER_STAFF_STATUS_CHANGED = OpenEdxPublicSignal( + event_type="org.openedx.learning.user.staff_status.changed.v1", + data={ + "user": UserDataExtended, + }, +) + + class ManualCriterion(BaseCriterionType): - """ - A criterion that is used to push a given list of users to a group. - """ + """A criterion that is used to push a given list of users to a group.""" criterion_type: str = "manual" description: str = ( @@ -43,27 +62,28 @@ def evaluate( self, current_scope: Scope, backend_client: BackendClient = None, - ) -> Q: # TODO: Should this be Scope type instead of dict? + ) -> QuerySet: """ Evaluate the criterion. """ - return backend_client.get_users(current_scope).filter( + return backend_client.get_users(current_scope).filter( # Currently side-wide, but should be filtered by scope id__in=self.criterion_config.user_ids ) class CourseEnrollmentCriterion(BaseCriterionType): - """ - A criterion that is used to evaluate the membership of a user group based on the course enrollment mode of the user. - """ + """A criterion that is used to evaluate the membership of a user group based on the course enrollment mode of the user.""" + updated_by_events = [COURSE_ENROLLMENT_CREATED, COURSE_ENROLLMENT_CHANGED] criterion_type: str = "course_enrollment" description: str = ( "A criterion that is used to evaluate the membership of a user group based on the course enrollment mode of the user." ) + # TODO: should we use a single criterion with multiple attributes to filter by: mode, enrollment date, etc.? This would be an example of how we could do it, instead of having multiple criteria with specific attributes? class ConfigModel(BaseModel): - course_id: str # TODO: maybe we could use a single criterion with multiple attributes to filter by: mode, enrollment date, etc. + mode: Optional[str] = None + enrollment_date: Optional[datetime] = None supported_operators: List[ComparisonOperator] = [ ComparisonOperator.IN, @@ -76,25 +96,28 @@ class ConfigModel(BaseModel): ComparisonOperator.LESS_THAN_OR_EQUAL, ] scopes: List[str] = ["course"] + updated_by_events = [COURSE_ENROLLMENT_CREATED, COURSE_ENROLLMENT_CHANGED] def evaluate( self, - config: ConfigModel, - operator: ComparisonOperator, - scope_context: Dict[str, Any] = None, - ) -> Q: + current_scope: Scope, + backend_client: BackendClient = None, + ) -> QuerySet: """ Evaluate the criterion. """ - # Placeholder implementation for POC - return Q(id__in=[]) + filters = {} + if self.criterion_config.mode: + filters["mode"] = self.criterion_config.mode + if self.criterion_config.enrollment_date: + filters["created__gte"] = self.criterion_config.enrollment_date + return backend_client.get_enrollments(current_scope).filter(**filters) class LastLoginCriterion(BaseCriterionType): - """ - A criterion that is used to evaluate the membership of a user group based on the last login of the user. - """ + """A criterion that is used to evaluate the membership of a user group based on the last login of the user.""" + updated_by_events = [SESSION_LOGIN_COMPLETED] criterion_type: str = "last_login" description: str = ( "A criterion that is used to evaluate the membership of a user group based on the last login of the user." @@ -116,7 +139,7 @@ def evaluate( self, current_scope: Scope, backend_client: BackendClient = None, # Dependency injection for the backend client - ) -> Q: + ) -> QuerySet: """ Evaluate the criterion. @@ -148,10 +171,9 @@ def evaluate( class EnrollmentModeCriterion(BaseCriterionType): - """ - A criterion that is used to evaluate the membership of a user group based on the enrollment mode of the user. - """ + """A criterion that is used to evaluate the membership of a user group based on the enrollment mode of the user.""" + updated_by_events = [COURSE_ENROLLMENT_CREATED, COURSE_ENROLLMENT_CHANGED] criterion_type: str = "enrollment_mode" description: str = ( "A criterion that is used to evaluate the membership of a user group based on the enrollment mode of the user." @@ -170,7 +192,7 @@ def evaluate( self, current_scope: Scope, backend_client: BackendClient = None, - ) -> Q: + ) -> QuerySet: """ Evaluate the criterion. """ @@ -178,10 +200,9 @@ def evaluate( class UserStaffStatusCriterion(BaseCriterionType): - """ - A criterion that filters users based on their staff status. - """ + """A criterion that filters users based on their staff status.""" + updated_by_events = [USER_STAFF_STATUS_CHANGED] criterion_type: str = "user_staff_status" description: str = ( "A criterion that filters users based on whether they are staff members or not." @@ -190,20 +211,12 @@ class UserStaffStatusCriterion(BaseCriterionType): class ConfigModel(BaseModel): is_staff: bool # True to filter for staff users, False for non-staff users - supported_operators: List[ComparisonOperator] = ( - [ # TODO: I don't think we need to support any operator. Maybe a simple is true? - # ComparisonOperator.EQUAL, - # ComparisonOperator.NOT_EQUAL, - ] - ) - def evaluate( self, current_scope: Scope, backend_client: BackendClient = None, - ) -> Q: - """ - Evaluate the criterion based on user staff status. + ) -> QuerySet: + """Evaluate the criterion based on user staff status. Args: config: Configuration specifying whether to look for staff (True) or non-staff (False) users diff --git a/openedx_user_groups/events.py b/openedx_user_groups/events.py new file mode 100644 index 0000000..9386561 --- /dev/null +++ b/openedx_user_groups/events.py @@ -0,0 +1,19 @@ +"""Interim module to define the events that trigger updates. + +This module is used to define the events that trigger updates to the user groups. + +This is a temporary module that will be replaced by the events defined in the openedx-events repository. +""" + +from openedx_events.tooling import OpenEdxPublicSignal + + +# .. event_type: org.openedx.learning.user.staff_status.changed.v1 +# .. event_name: USER_STAFF_STATUS_CHANGED +# .. event_key_field: user.id +# .. event_description: Emitted when the user staff status changes. +# .. event_data: UserStaffStatusData +# .. event_trigger_repository: openedx/edx-platform +USER_STAFF_STATUS_CHANGED = OpenEdxPublicSignal( + event_type="org.openedx.learning.user.staff_status.changed.v1", +) diff --git a/openedx_user_groups/handlers.py b/openedx_user_groups/handlers.py new file mode 100644 index 0000000..455aa6a --- /dev/null +++ b/openedx_user_groups/handlers.py @@ -0,0 +1,29 @@ +"""This module is responsible for handling event-based updates to user groups. + +It is responsible for: +- Adding users to user groups when they meet the criteria +- Removing users from user groups when they no longer meet the criteria +- Updating user groups when the criteria changes +""" + +import attr + +from openedx_user_groups.criteria import BaseCriterionType +from openedx_user_groups.tasks import orchestrate_user_groups_updates_based_on_events + + +def handle_user_group_update(sender, signal, **kwargs): + """Handler for all events related to user-groups criteria. + + This handler listens to all events configured within each criterion type and orchestrates the necessary updates to the user groups. + + Args: + sender: The sender of the signal. + signal: The signal that was sent. + **kwargs: Additional keyword arguments. + """ + orchestrate_user_groups_updates_based_on_events( + signal.event_type, + attr.asdict(kwargs.get("user")), + BaseCriterionType._event_to_class_map, # TODO: this is very specific for the new USER_STAFF_STATUS_CHANGED event and user-related events + ) diff --git a/openedx_user_groups/manager.py b/openedx_user_groups/manager.py index 78b5b88..812999e 100644 --- a/openedx_user_groups/manager.py +++ b/openedx_user_groups/manager.py @@ -27,7 +27,8 @@ class CriterionManager(PluginManager): # Simple registry for POC - in production this would use plugin discovery # Format matches entry points: "name = module.path:ClassName" - # TODO: what if I install a new one with the same name and override the old one? Need versioning and validation for this. ADR! + # TODO: what if I install a new one with the same name and override the old one? Log the override for the time being. + # TODO: maybe we can consider using a mirror to INSTALLED_APPS to check if the criterion is already registered? AND manage duplicates like this? # TODO: Maybe default criterion shouldn't be registered as plugins? _criterion_registry = { "last_login": "openedx_user_groups.criteria_types:LastLoginCriterion", diff --git a/openedx_user_groups/models.py b/openedx_user_groups/models.py index 38449bc..80aa460 100644 --- a/openedx_user_groups/models.py +++ b/openedx_user_groups/models.py @@ -28,12 +28,14 @@ This module is not meant for production, it's only for POC purposes. """ - from django.contrib.auth import get_user_model from django.contrib.contenttypes.fields import GenericForeignKey from django.contrib.contenttypes.models import ContentType from django.core.exceptions import ValidationError from django.db import models +import json + +from pydantic_core import from_json from openedx_user_groups.manager import CriterionManager @@ -176,6 +178,7 @@ class Criterion(models.Model): criterion_config (dict): The configuration of the criterion. group (UserGroup): The group to which the criterion belongs. """ + criterion_type = models.CharField( max_length=255, # When creating a new criterion, this should be one of the available criterion types. validators=[validate_criterion_type], @@ -200,3 +203,13 @@ def available_criterion_types(): @property def criterion_type_template(self): return CriterionManager.get_criterion_class_by_type(self.criterion_type) + + @property + def criterion_instance(self): + return self.criterion_type_template( + self.criterion_operator, self.criterion_config + ) + + @property + def config(self): + return from_json(self.criterion_config) diff --git a/openedx_user_groups/tasks.py b/openedx_user_groups/tasks.py new file mode 100644 index 0000000..a65743f --- /dev/null +++ b/openedx_user_groups/tasks.py @@ -0,0 +1,120 @@ +"""This module is responsible for handling background tasks related to user groups opetations. + +It is responsible for: +- Evaluate membership for a user group based on criteria +- Updating user groups when the criteria changes +- All operations that might be high impact and should be run in a background task +""" + +from celery import shared_task + +from openedx_user_groups.api import ( + evaluate_and_update_membership_for_group, + evaluate_and_update_membership_for_multiple_groups, +) +from openedx_user_groups.criteria import BaseCriterionType +from openedx_user_groups.models import Criterion, UserGroup, UserGroupMembership + + +def orchestrate_user_groups_updates_based_on_events( + event_type: str, + event_data: dict, + event_to_class_map: dict, +): + """ + Orchestrate user groups updates for all groups that are affected by the event. + + This operation will be triggered by an event from the Open edX Events library which is associated + with a sigle or multiple criteria types. + + This task will: + 1. Get all criteria types that are affected by the event + 2. Get all enabled groups that are configured with those criteria types + 3. Re-evaluate the membership for those groups if and only if: + - The event usually represents the state of a single membership. + - The membership state holds what was true at the time of the membership creation. + - If the event data doesn't match the membership state, then the membership will be updated. Also groups of the + same criteria type will be updated in case the user now belongs to another group. + 4. If there is no membership associated with the event, then all groups that are configured with the criteria + types will be updated. + """ + # Get the user from the event data + user_id = event_data.get("id") # TODO: is this always present? + if not user_id: + return + + # Get all criteria types affected by this event + affected_criteria_types = event_to_class_map.get(event_type, []) + if not affected_criteria_types: + return + + # Get all memberships for this user in groups with affected criteria types + memberships = ( + UserGroupMembership.objects.filter( + user_id=user_id, + is_active=True, + group__enabled=True, + group__criteria__criterion_type__in=affected_criteria_types, + ) + .select_related("group") + .prefetch_related("group__criteria") + .distinct() # Avoid duplicates if group has multiple affected criteria + ) + + # If there are no memberships for this user, then we should update all groups that are configured with the criteria types + if not memberships: + groups_to_update = UserGroup.objects.filter( + enabled=True, criteria__criterion_type__in=affected_criteria_types + ).values_list("id", flat=True) + evaluate_and_update_membership_for_multiple_groups(list(groups_to_update)) + return + + groups_to_update = set() + # Check existing memberships for state changes + for membership in memberships: + # Check if any of the group's criteria are affected and have state changes + for criterion in membership.group.criteria.all(): + if criterion.criterion_type in affected_criteria_types: + if check_if_membership_state_changed( + event_data, criterion.criterion_config + ): + groups_to_update.add(membership.group.id) + + # Also check groups where the user is NOT a member but might now qualify + # Get all groups with affected criteria types that the user is not currently in + current_group_ids = [m.group.id for m in memberships] + potential_groups = ( + UserGroup.objects.filter( + enabled=True, criteria__criterion_type__in=affected_criteria_types + ) + .exclude(id__in=current_group_ids) + .distinct() + ) + + # Add these groups for evaluation as the user might now qualify + groups_to_update.update(potential_groups.values_list("id", flat=True)) + + # Update all affected groups + if groups_to_update: + evaluate_and_update_membership_for_multiple_groups(list(groups_to_update)) + + +def check_if_membership_state_changed(event_data: dict, criterion_config: dict): + """Check if the membership state has changed based on the event data. + + This function will check if the event data matches the criterion config. + If the event data doesn't match the criterion config, then the membership state has changed. + + Args: + event_data: The data from the event + criterion_config: The configuration of the criterion + + Returns: + bool: True if the membership state has changed, False otherwise + """ + for key, value in criterion_config.items(): + if key not in event_data: + return False + if event_data[key] != value: + return True + return False diff --git a/requirements/base.in b/requirements/base.in index 3652d4d..76f0033 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -5,3 +5,5 @@ Django # Web application framework edx-django-utils # edX utilities for Django openedx-atlas +openedx-events # Open edX Events library for updating user groups +celery # Celery for background tasks diff --git a/requirements/base.txt b/requirements/base.txt index 44bedad..9cb33fe 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -4,12 +4,31 @@ # # pip-compile --output-file=requirements/base.txt requirements/base.in # +amqp==5.3.1 + # via kombu asgiref==3.8.1 # via django +attrs==25.3.0 + # via openedx-events +billiard==4.2.1 + # via celery +celery==5.5.3 + # via -r requirements/base.in cffi==1.17.1 # via pynacl click==8.2.1 - # via edx-django-utils + # via + # celery + # click-didyoumean + # click-plugins + # click-repl + # edx-django-utils +click-didyoumean==0.3.1 + # via celery +click-plugins==1.1.1 + # via celery +click-repl==0.3.0 + # via celery django==4.2.21 # via # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt @@ -17,26 +36,68 @@ django==4.2.21 # django-crum # django-waffle # edx-django-utils + # openedx-events django-crum==0.7.9 # via edx-django-utils django-waffle==4.2.0 # via edx-django-utils +dnspython==2.7.0 + # via pymongo +edx-ccx-keys==2.0.2 + # via openedx-events edx-django-utils==8.0.0 - # via -r requirements/base.in + # via + # -r requirements/base.in + # openedx-events +edx-opaque-keys[django]==3.0.0 + # via + # edx-ccx-keys + # openedx-events +fastavro==1.11.1 + # via openedx-events +kombu==5.5.4 + # via celery openedx-atlas==0.7.0 # via -r requirements/base.in +openedx-events==10.2.1 + # via -r requirements/base.in +packaging==25.0 + # via kombu pbr==6.1.1 # via stevedore +prompt-toolkit==3.0.51 + # via click-repl psutil==7.0.0 # via edx-django-utils pycparser==2.22 # via cffi +pymongo==4.13.1 + # via edx-opaque-keys pynacl==1.5.0 # via edx-django-utils +python-dateutil==2.9.0.post0 + # via celery +six==1.17.0 + # via + # edx-ccx-keys + # python-dateutil sqlparse==0.5.3 # via django stevedore==5.4.1 - # via edx-django-utils + # via + # edx-django-utils + # edx-opaque-keys +typing-extensions==4.14.0 + # via edx-opaque-keys +tzdata==2025.2 + # via kombu +vine==5.1.0 + # via + # amqp + # celery + # kombu +wcwidth==0.2.13 + # via prompt-toolkit # The following packages are considered to be unsafe in a requirements file: # setuptools diff --git a/requirements/dev.txt b/requirements/dev.txt index 0539fbf..c6869df 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -4,6 +4,10 @@ # # pip-compile --output-file=requirements/dev.txt requirements/dev.in # +amqp==5.3.1 + # via + # -r requirements/quality.txt + # kombu annotated-types==0.7.0 # via # -r requirements/quality.txt @@ -17,6 +21,14 @@ astroid==3.3.10 # -r requirements/quality.txt # pylint # pylint-celery +attrs==25.3.0 + # via + # -r requirements/quality.txt + # openedx-events +billiard==4.2.1 + # via + # -r requirements/quality.txt + # celery build==1.2.2.post1 # via # -r requirements/pip-tools.txt @@ -25,6 +37,8 @@ cachetools==6.0.0 # via # -r requirements/ci.txt # tox +celery==5.5.3 + # via -r requirements/quality.txt cffi==1.17.1 # via # -r requirements/quality.txt @@ -38,15 +52,31 @@ click==8.2.1 # via # -r requirements/pip-tools.txt # -r requirements/quality.txt + # celery + # click-didyoumean # click-log + # click-plugins + # click-repl # code-annotations # edx-django-utils # edx-lint # pip-tools +click-didyoumean==0.3.1 + # via + # -r requirements/quality.txt + # celery click-log==0.4.0 # via # -r requirements/quality.txt # edx-lint +click-plugins==1.1.1 + # via + # -r requirements/quality.txt + # celery +click-repl==0.3.0 + # via + # -r requirements/quality.txt + # celery code-annotations==2.3.0 # via # -r requirements/quality.txt @@ -79,6 +109,7 @@ django==4.2.21 # django-waffle # edx-django-utils # edx-i18n-tools + # openedx-events django-crum==0.7.9 # via # -r requirements/quality.txt @@ -87,18 +118,37 @@ django-waffle==4.2.0 # via # -r requirements/quality.txt # edx-django-utils +dnspython==2.7.0 + # via + # -r requirements/quality.txt + # pymongo +edx-ccx-keys==2.0.2 + # via + # -r requirements/quality.txt + # openedx-events edx-django-utils==8.0.0 - # via -r requirements/quality.txt + # via + # -r requirements/quality.txt + # openedx-events edx-i18n-tools==1.9.0 # via -r requirements/dev.in edx-lint==5.6.0 # via -r requirements/quality.txt +edx-opaque-keys[django]==3.0.0 + # via + # -r requirements/quality.txt + # edx-ccx-keys + # openedx-events factory-boy==3.3.3 # via -r requirements/quality.txt faker==37.3.0 # via # -r requirements/quality.txt # factory-boy +fastavro==1.11.1 + # via + # -r requirements/quality.txt + # openedx-events filelock==3.18.0 # via # -r requirements/ci.txt @@ -117,6 +167,10 @@ jinja2==3.1.6 # -r requirements/quality.txt # code-annotations # diff-cover +kombu==5.5.4 + # via + # -r requirements/quality.txt + # celery lxml[html-clean,html_clean]==5.4.0 # via # edx-i18n-tools @@ -133,12 +187,15 @@ mccabe==0.7.0 # pylint openedx-atlas==0.7.0 # via -r requirements/quality.txt +openedx-events==10.2.1 + # via -r requirements/quality.txt packaging==25.0 # via # -r requirements/ci.txt # -r requirements/pip-tools.txt # -r requirements/quality.txt # build + # kombu # pyproject-api # pytest # tox @@ -166,6 +223,10 @@ pluggy==1.6.0 # tox polib==1.2.0 # via edx-i18n-tools +prompt-toolkit==3.0.51 + # via + # -r requirements/quality.txt + # click-repl psutil==7.0.0 # via # -r requirements/quality.txt @@ -206,6 +267,10 @@ pylint-plugin-utils==0.8.2 # -r requirements/quality.txt # pylint-celery # pylint-django +pymongo==4.13.1 + # via + # -r requirements/quality.txt + # edx-opaque-keys pynacl==1.5.0 # via # -r requirements/quality.txt @@ -228,6 +293,10 @@ pytest-cov==6.1.1 # via -r requirements/quality.txt pytest-django==4.11.1 # via -r requirements/quality.txt +python-dateutil==2.9.0.post0 + # via + # -r requirements/quality.txt + # celery python-slugify==8.0.4 # via # -r requirements/quality.txt @@ -240,7 +309,9 @@ pyyaml==6.0.2 six==1.17.0 # via # -r requirements/quality.txt + # edx-ccx-keys # edx-lint + # python-dateutil snowballstemmer==3.0.1 # via # -r requirements/quality.txt @@ -254,6 +325,7 @@ stevedore==5.4.1 # -r requirements/quality.txt # code-annotations # edx-django-utils + # edx-opaque-keys text-unidecode==1.3 # via # -r requirements/quality.txt @@ -267,6 +339,7 @@ tox==4.26.0 typing-extensions==4.14.0 # via # -r requirements/quality.txt + # edx-opaque-keys # pydantic # pydantic-core # typing-inspection @@ -278,10 +351,21 @@ tzdata==2025.2 # via # -r requirements/quality.txt # faker + # kombu +vine==5.1.0 + # via + # -r requirements/quality.txt + # amqp + # celery + # kombu virtualenv==20.31.2 # via # -r requirements/ci.txt # tox +wcwidth==0.2.13 + # via + # -r requirements/quality.txt + # prompt-toolkit wheel==0.45.1 # via # -r requirements/pip-tools.txt diff --git a/requirements/doc.txt b/requirements/doc.txt index 949a906..328c654 100644 --- a/requirements/doc.txt +++ b/requirements/doc.txt @@ -8,22 +8,32 @@ accessible-pygments==0.0.5 # via pydata-sphinx-theme alabaster==1.0.0 # via sphinx -annotated-types==0.7.0 +amqp==5.3.1 # via # -r requirements/test.txt - # pydantic + # kombu asgiref==3.8.1 # via # -r requirements/test.txt # django +attrs==25.3.0 + # via + # -r requirements/test.txt + # openedx-events babel==2.17.0 # via # pydata-sphinx-theme # sphinx beautifulsoup4==4.13.4 # via pydata-sphinx-theme +billiard==4.2.1 + # via + # -r requirements/test.txt + # celery build==1.2.2.post1 # via -r requirements/doc.in +celery==5.5.3 + # via -r requirements/test.txt certifi==2025.4.26 # via requests cffi==1.17.1 @@ -36,8 +46,24 @@ charset-normalizer==3.4.2 click==8.2.1 # via # -r requirements/test.txt + # celery + # click-didyoumean + # click-plugins + # click-repl # code-annotations # edx-django-utils +click-didyoumean==0.3.1 + # via + # -r requirements/test.txt + # celery +click-plugins==1.1.1 + # via + # -r requirements/test.txt + # celery +click-repl==0.3.0 + # via + # -r requirements/test.txt + # celery code-annotations==2.3.0 # via -r requirements/test.txt coverage[toml]==7.8.2 @@ -55,6 +81,7 @@ django==4.2.21 # django-crum # django-waffle # edx-django-utils + # openedx-events django-crum==0.7.9 # via # -r requirements/test.txt @@ -63,6 +90,10 @@ django-waffle==4.2.0 # via # -r requirements/test.txt # edx-django-utils +dnspython==2.7.0 + # via + # -r requirements/test.txt + # pymongo doc8==1.1.2 # via -r requirements/doc.in docutils==0.21.2 @@ -72,14 +103,29 @@ docutils==0.21.2 # readme-renderer # restructuredtext-lint # sphinx +edx-ccx-keys==2.0.2 + # via + # -r requirements/test.txt + # openedx-events edx-django-utils==8.0.0 - # via -r requirements/test.txt + # via + # -r requirements/test.txt + # openedx-events +edx-opaque-keys[django]==3.0.0 + # via + # -r requirements/test.txt + # edx-ccx-keys + # openedx-events factory-boy==3.3.3 # via -r requirements/test.txt faker==37.3.0 # via # -r requirements/test.txt # factory-boy +fastavro==1.11.1 + # via + # -r requirements/test.txt + # openedx-events id==1.5.0 # via twine idna==3.10 @@ -107,6 +153,10 @@ jinja2==3.1.6 # sphinx keyring==25.6.0 # via twine +kombu==5.5.4 + # via + # -r requirements/test.txt + # celery markdown-it-py==3.0.0 # via rich markupsafe==3.0.2 @@ -123,10 +173,13 @@ nh3==0.2.21 # via readme-renderer openedx-atlas==0.7.0 # via -r requirements/test.txt +openedx-events==10.2.1 + # via -r requirements/test.txt packaging==25.0 # via # -r requirements/test.txt # build + # kombu # pydata-sphinx-theme # pytest # sphinx @@ -139,6 +192,10 @@ pluggy==1.6.0 # via # -r requirements/test.txt # pytest +prompt-toolkit==3.0.51 + # via + # -r requirements/test.txt + # click-repl psutil==7.0.0 # via # -r requirements/test.txt @@ -147,12 +204,6 @@ pycparser==2.22 # via # -r requirements/test.txt # cffi -pydantic==2.11.5 - # via -r requirements/test.txt -pydantic-core==2.33.2 - # via - # -r requirements/test.txt - # pydantic pydata-sphinx-theme==0.15.4 # via sphinx-book-theme pygments==2.19.1 @@ -163,6 +214,10 @@ pygments==2.19.1 # readme-renderer # rich # sphinx +pymongo==4.13.1 + # via + # -r requirements/test.txt + # edx-opaque-keys pynacl==1.5.0 # via # -r requirements/test.txt @@ -178,6 +233,10 @@ pytest-cov==6.1.1 # via -r requirements/test.txt pytest-django==4.11.1 # via -r requirements/test.txt +python-dateutil==2.9.0.post0 + # via + # -r requirements/test.txt + # celery python-slugify==8.0.4 # via # -r requirements/test.txt @@ -206,6 +265,11 @@ roman-numerals-py==3.1.0 # via sphinx secretstorage==3.3.3 # via keyring +six==1.17.0 + # via + # -r requirements/test.txt + # edx-ccx-keys + # python-dateutil snowballstemmer==3.0.1 # via sphinx soupsieve==2.7 @@ -239,6 +303,7 @@ stevedore==5.4.1 # code-annotations # doc8 # edx-django-utils + # edx-opaque-keys text-unidecode==1.3 # via # -r requirements/test.txt @@ -249,23 +314,28 @@ typing-extensions==4.14.0 # via # -r requirements/test.txt # beautifulsoup4 - # pydantic - # pydantic-core + # edx-opaque-keys # pydata-sphinx-theme - # typing-inspection -typing-inspection==0.4.1 - # via - # -r requirements/test.txt - # pydantic tzdata==2025.2 # via # -r requirements/test.txt # faker + # kombu urllib3==2.2.3 # via # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt # requests # twine +vine==5.1.0 + # via + # -r requirements/test.txt + # amqp + # celery + # kombu +wcwidth==0.2.13 + # via + # -r requirements/test.txt + # prompt-toolkit # The following packages are considered to be unsafe in a requirements file: # setuptools diff --git a/requirements/quality.txt b/requirements/quality.txt index c5f2e82..2d6f499 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -4,10 +4,12 @@ # # pip-compile --output-file=requirements/quality.txt requirements/quality.in # -annotated-types==0.7.0 +amqp==5.3.1 # via # -r requirements/test.txt - # pydantic + # kombu +annotated-types==0.7.0 + # via pydantic asgiref==3.8.1 # via # -r requirements/test.txt @@ -16,6 +18,16 @@ astroid==3.3.10 # via # pylint # pylint-celery +attrs==25.3.0 + # via + # -r requirements/test.txt + # openedx-events +billiard==4.2.1 + # via + # -r requirements/test.txt + # celery +celery==5.5.3 + # via -r requirements/test.txt cffi==1.17.1 # via # -r requirements/test.txt @@ -23,12 +35,28 @@ cffi==1.17.1 click==8.2.1 # via # -r requirements/test.txt + # celery + # click-didyoumean # click-log + # click-plugins + # click-repl # code-annotations # edx-django-utils # edx-lint +click-didyoumean==0.3.1 + # via + # -r requirements/test.txt + # celery click-log==0.4.0 # via edx-lint +click-plugins==1.1.1 + # via + # -r requirements/test.txt + # celery +click-repl==0.3.0 + # via + # -r requirements/test.txt + # celery code-annotations==2.3.0 # via # -r requirements/test.txt @@ -48,6 +76,7 @@ django==4.2.21 # django-crum # django-waffle # edx-django-utils + # openedx-events django-crum==0.7.9 # via # -r requirements/test.txt @@ -56,16 +85,35 @@ django-waffle==4.2.0 # via # -r requirements/test.txt # edx-django-utils +dnspython==2.7.0 + # via + # -r requirements/test.txt + # pymongo +edx-ccx-keys==2.0.2 + # via + # -r requirements/test.txt + # openedx-events edx-django-utils==8.0.0 - # via -r requirements/test.txt + # via + # -r requirements/test.txt + # openedx-events edx-lint==5.6.0 # via -r requirements/quality.in +edx-opaque-keys[django]==3.0.0 + # via + # -r requirements/test.txt + # edx-ccx-keys + # openedx-events factory-boy==3.3.3 # via -r requirements/test.txt faker==37.3.0 # via # -r requirements/test.txt # factory-boy +fastavro==1.11.1 + # via + # -r requirements/test.txt + # openedx-events iniconfig==2.1.0 # via # -r requirements/test.txt @@ -78,6 +126,10 @@ jinja2==3.1.6 # via # -r requirements/test.txt # code-annotations +kombu==5.5.4 + # via + # -r requirements/test.txt + # celery markupsafe==3.0.2 # via # -r requirements/test.txt @@ -86,9 +138,12 @@ mccabe==0.7.0 # via pylint openedx-atlas==0.7.0 # via -r requirements/test.txt +openedx-events==10.2.1 + # via -r requirements/test.txt packaging==25.0 # via # -r requirements/test.txt + # kombu # pytest pbr==6.1.1 # via @@ -100,6 +155,10 @@ pluggy==1.6.0 # via # -r requirements/test.txt # pytest +prompt-toolkit==3.0.51 + # via + # -r requirements/test.txt + # click-repl psutil==7.0.0 # via # -r requirements/test.txt @@ -111,13 +170,9 @@ pycparser==2.22 # -r requirements/test.txt # cffi pydantic==2.11.5 - # via - # -r requirements/quality.in - # -r requirements/test.txt + # via -r requirements/quality.in pydantic-core==2.33.2 - # via - # -r requirements/test.txt - # pydantic + # via pydantic pydocstyle==6.3.0 # via -r requirements/quality.in pylint==3.3.7 @@ -134,6 +189,10 @@ pylint-plugin-utils==0.8.2 # via # pylint-celery # pylint-django +pymongo==4.13.1 + # via + # -r requirements/test.txt + # edx-opaque-keys pynacl==1.5.0 # via # -r requirements/test.txt @@ -147,6 +206,10 @@ pytest-cov==6.1.1 # via -r requirements/test.txt pytest-django==4.11.1 # via -r requirements/test.txt +python-dateutil==2.9.0.post0 + # via + # -r requirements/test.txt + # celery python-slugify==8.0.4 # via # -r requirements/test.txt @@ -156,7 +219,11 @@ pyyaml==6.0.2 # -r requirements/test.txt # code-annotations six==1.17.0 - # via edx-lint + # via + # -r requirements/test.txt + # edx-ccx-keys + # edx-lint + # python-dateutil snowballstemmer==3.0.1 # via pydocstyle sqlparse==0.5.3 @@ -168,6 +235,7 @@ stevedore==5.4.1 # -r requirements/test.txt # code-annotations # edx-django-utils + # edx-opaque-keys text-unidecode==1.3 # via # -r requirements/test.txt @@ -177,17 +245,27 @@ tomlkit==0.13.2 typing-extensions==4.14.0 # via # -r requirements/test.txt + # edx-opaque-keys # pydantic # pydantic-core # typing-inspection typing-inspection==0.4.1 - # via - # -r requirements/test.txt - # pydantic + # via pydantic tzdata==2025.2 # via # -r requirements/test.txt # faker + # kombu +vine==5.1.0 + # via + # -r requirements/test.txt + # amqp + # celery + # kombu +wcwidth==0.2.13 + # via + # -r requirements/test.txt + # prompt-toolkit # The following packages are considered to be unsafe in a requirements file: # setuptools diff --git a/requirements/test.txt b/requirements/test.txt index 138f5c8..0ee3e7d 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -4,12 +4,24 @@ # # pip-compile --output-file=requirements/test.txt requirements/test.in # -annotated-types==0.7.0 - # via pydantic +amqp==5.3.1 + # via + # -r requirements/base.txt + # kombu asgiref==3.8.1 # via # -r requirements/base.txt # django +attrs==25.3.0 + # via + # -r requirements/base.txt + # openedx-events +billiard==4.2.1 + # via + # -r requirements/base.txt + # celery +celery==5.5.3 + # via -r requirements/base.txt cffi==1.17.1 # via # -r requirements/base.txt @@ -17,8 +29,24 @@ cffi==1.17.1 click==8.2.1 # via # -r requirements/base.txt + # celery + # click-didyoumean + # click-plugins + # click-repl # code-annotations # edx-django-utils +click-didyoumean==0.3.1 + # via + # -r requirements/base.txt + # celery +click-plugins==1.1.1 + # via + # -r requirements/base.txt + # celery +click-repl==0.3.0 + # via + # -r requirements/base.txt + # celery code-annotations==2.3.0 # via -r requirements/test.in coverage[toml]==7.8.2 @@ -31,6 +59,7 @@ ddt==1.7.2 # django-crum # django-waffle # edx-django-utils + # openedx-events django-crum==0.7.9 # via # -r requirements/base.txt @@ -39,30 +68,62 @@ django-waffle==4.2.0 # via # -r requirements/base.txt # edx-django-utils +dnspython==2.7.0 + # via + # -r requirements/base.txt + # pymongo +edx-ccx-keys==2.0.2 + # via + # -r requirements/base.txt + # openedx-events edx-django-utils==8.0.0 - # via -r requirements/base.txt + # via + # -r requirements/base.txt + # openedx-events +edx-opaque-keys[django]==3.0.0 + # via + # -r requirements/base.txt + # edx-ccx-keys + # openedx-events factory-boy==3.3.3 # via -r requirements/test.in faker==37.3.0 # via # -r requirements/test.in # factory-boy +fastavro==1.11.1 + # via + # -r requirements/base.txt + # openedx-events iniconfig==2.1.0 # via pytest jinja2==3.1.6 # via code-annotations +kombu==5.5.4 + # via + # -r requirements/base.txt + # celery markupsafe==3.0.2 # via jinja2 openedx-atlas==0.7.0 # via -r requirements/base.txt +openedx-events==10.2.1 + # via -r requirements/base.txt packaging==25.0 - # via pytest + # via + # -r requirements/base.txt + # kombu + # pytest pbr==6.1.1 # via # -r requirements/base.txt # stevedore pluggy==1.6.0 # via pytest +prompt-toolkit==3.0.51 + # via + # -r requirements/base.txt + # click-repl psutil==7.0.0 # via # -r requirements/base.txt @@ -71,10 +132,10 @@ pycparser==2.22 # via # -r requirements/base.txt # cffi -pydantic==2.11.5 - # via -r requirements/test.in -pydantic-core==2.33.2 - # via pydantic +pymongo==4.13.1 + # via + # -r requirements/base.txt + # edx-opaque-keys pynacl==1.5.0 # via # -r requirements/base.txt @@ -87,10 +148,19 @@ pytest-cov==6.1.1 # via -r requirements/test.in pytest-django==4.11.1 # via -r requirements/test.in +python-dateutil==2.9.0.post0 + # via + # -r requirements/base.txt + # celery python-slugify==8.0.4 # via code-annotations pyyaml==6.0.2 # via code-annotations +six==1.17.0 + # via + # -r requirements/base.txt + # edx-ccx-keys + # python-dateutil sqlparse==0.5.3 # via # -r requirements/base.txt @@ -100,17 +170,28 @@ stevedore==5.4.1 # -r requirements/base.txt # code-annotations # edx-django-utils + # edx-opaque-keys text-unidecode==1.3 # via python-slugify typing-extensions==4.14.0 # via - # pydantic - # pydantic-core - # typing-inspection -typing-inspection==0.4.1 - # via pydantic + # -r requirements/base.txt + # edx-opaque-keys tzdata==2025.2 - # via faker + # via + # -r requirements/base.txt + # faker + # kombu +vine==5.1.0 + # via + # -r requirements/base.txt + # amqp + # celery + # kombu +wcwidth==0.2.13 + # via + # -r requirements/base.txt + # prompt-toolkit # The following packages are considered to be unsafe in a requirements file: # setuptools diff --git a/tests/__init__.py b/tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/factories.py b/tests/factories.py new file mode 100644 index 0000000..7a59511 --- /dev/null +++ b/tests/factories.py @@ -0,0 +1,92 @@ +"""Factories for creating test data.""" + +import factory +from django.contrib.auth import get_user_model +from django.contrib.contenttypes.models import ContentType +from openedx_user_groups.models import UserGroup, Scope, Criterion + +User = get_user_model() + + +class CourseFactory(factory.Factory): + """Factory for creating Course-like objects for testing. + + Since we don't want to create a real Course model, this factory + generates dict objects that simulate course data. + """ + + class Meta: + model = dict # Use a dict to simulate a course object + + course_id = factory.Sequence(lambda n: f"course-v1:edX+Demo{n}+Course") + name = factory.Faker("sentence", nb_words=3) + description = factory.Faker("text", max_nb_chars=200) + id = factory.Sequence(lambda n: n) + + +class UserFactory(factory.django.DjangoModelFactory): + """Factory for creating User instances.""" + + class Meta: + model = User + + username = factory.Sequence(lambda n: f"user_{n}") + email = factory.LazyAttribute(lambda obj: f"{obj.username}@example.com") + first_name = factory.Faker("first_name") + last_name = factory.Faker("last_name") + + +class ScopeFactory(factory.django.DjangoModelFactory): + """Factory for creating Scope instances.""" + + class Meta: + model = Scope + + name = factory.Faker("sentence", nb_words=3) + description = factory.Faker("text", max_nb_chars=200) + # Use User model's ContentType as a default since it exists in test DB + content_type = factory.LazyFunction(lambda: ContentType.objects.get_for_model(User)) + object_id = factory.Sequence(lambda n: n) + + +class UserGroupFactory(factory.django.DjangoModelFactory): + """Factory for creating UserGroup instances.""" + + class Meta: + model = UserGroup + + name = factory.Faker("sentence", nb_words=2) + description = factory.Faker("text", max_nb_chars=200) + enabled = True + scope = factory.SubFactory(ScopeFactory) + + +class CriterionFactory(factory.django.DjangoModelFactory): + """Factory for creating Criterion instances.""" + + class Meta: + model = Criterion + + +class LastLoginCriterionFactory(CriterionFactory): + """Factory for creating LastLoginCriterion instances.""" + + criterion_type = "last_login" + criterion_operator = ">" # Login date is greater than 1 day ago + criterion_config = factory.Dict({"days": 1}) + + +class EnrollmentModeCriterionFactory(CriterionFactory): + """Factory for creating EnrollmentModeCriterion instances.""" + + criterion_type = "enrollment_mode" + criterion_operator = "=" + criterion_config = factory.Dict({"mode": "honor"}) + + +class UserStaffStatusCriterionFactory(CriterionFactory): + """Factory for creating UserStaffStatusCriterion instances.""" + + criterion_type = "user_staff_status" + criterion_operator = "=" + criterion_config = factory.Dict({"is_staff": False}) # Filter for non-staff users diff --git a/tests/test_api.py b/tests/test_api.py index 90128fd..b3a1239 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -12,98 +12,11 @@ from django.contrib.contenttypes.models import ContentType from django.utils import timezone from datetime import timedelta - +from tests.factories import * from openedx_user_groups.api import * from openedx_user_groups.models import UserGroup, Scope, Criterion -User = get_user_model() - - -class CourseFactory(factory.Factory): - """Factory for creating Course-like objects for testing. - - Since we don't want to create a real Course model, this factory - generates dict objects that simulate course data. - """ - - class Meta: - model = dict # Use a dict to simulate a course object - - course_id = factory.Sequence(lambda n: f"course-v1:edX+Demo{n}+Course") - name = factory.Faker("sentence", nb_words=3) - description = factory.Faker("text", max_nb_chars=200) - id = factory.Sequence(lambda n: n) - - -class UserFactory(factory.django.DjangoModelFactory): - """Factory for creating User instances.""" - - class Meta: - model = User - - username = factory.Sequence(lambda n: f"user_{n}") - email = factory.LazyAttribute(lambda obj: f"{obj.username}@example.com") - first_name = factory.Faker("first_name") - last_name = factory.Faker("last_name") - - -class ScopeFactory(factory.django.DjangoModelFactory): - """Factory for creating Scope instances.""" - - class Meta: - model = Scope - - name = factory.Faker("sentence", nb_words=3) - description = factory.Faker("text", max_nb_chars=200) - # Use User model's ContentType as a default since it exists in test DB - content_type = factory.LazyFunction(lambda: ContentType.objects.get_for_model(User)) - object_id = factory.Sequence(lambda n: n) - - -class UserGroupFactory(factory.django.DjangoModelFactory): - """Factory for creating UserGroup instances.""" - - class Meta: - model = UserGroup - - name = factory.Faker("sentence", nb_words=2) - description = factory.Faker("text", max_nb_chars=200) - enabled = True - scope = factory.SubFactory(ScopeFactory) - - -class CriterionFactory(factory.django.DjangoModelFactory): - """Factory for creating Criterion instances.""" - - class Meta: - model = Criterion - - -class LastLoginCriterionFactory(CriterionFactory): - """Factory for creating LastLoginCriterion instances.""" - - criterion_type = "last_login" - criterion_operator = ">" # Login date is greater than 1 day ago - criterion_config = factory.Dict({"days": 1}) - - -class EnrollmentModeCriterionFactory(CriterionFactory): - """Factory for creating EnrollmentModeCriterion instances.""" - - criterion_type = "enrollment_mode" - criterion_operator = "=" - criterion_config = factory.Dict({"mode": "honor"}) - - -class UserStaffStatusCriterionFactory(CriterionFactory): - """Factory for creating UserStaffStatusCriterion instances.""" - - criterion_type = "user_staff_status" - criterion_operator = "=" - criterion_config = factory.Dict({"is_staff": False}) # Filter for non-staff users - - class UserGroupAPITestCase(TestCase): @classmethod @@ -197,22 +110,60 @@ def test_create_group_with_single_criterion(self): assert user_group is not None assert user_group.criteria.count() == 1 - def test_create_group_with_multiple_criteria(self): - """Test that a group can be created with multiple criteria. + def test_create_group_with_multiple_criteria_invalid_scope(self): + """Test that a group can't be created with multiple criteria that don't match the group's scope. + + Expected Results: + - The group is not created. + - An exception is raised. + + In this case the criteria would be: + 1. Last login in the last 1 day - valid for instance/course scope + 2. Enrolled with honor mode - valid for course scope + Group scope: instance + """ + with self.assertRaises(AssertionError): + create_group_with_criteria( + name=self.test_user_group_data.name, + description=self.test_user_group_data.description, + scope_context=self.scope_context, + criterion_data=[ + { + "criterion_type": self.last_login_criterion.criterion_type, + "criterion_operator": self.last_login_criterion.criterion_operator, + "criterion_config": self.last_login_criterion.criterion_config, + }, + { + "criterion_type": self.enrollment_mode_criterion.criterion_type, + "criterion_operator": self.enrollment_mode_criterion.criterion_operator, + "criterion_config": self.enrollment_mode_criterion.criterion_config, + }, + ], + ) + + def test_create_group_with_multiple_criteria_valid_scope(self): + """Test that a group can be created with multiple criteria that match the group's scope. Expected Results: - The group is created successfully. - - The group has the correct name, description, and scope. - The group has the correct criteria. + - The group has the correct members. - In this case the criteria would be: - 1. Last login in the last 1 day - 2. Enrolled with honor mode + Criteria: + 1. Last login in the last 1 day - valid for instance/course scope + 2. Staff status - valid for instance/course scope + Group scope: instance """ user_group = create_group_with_criteria( name=self.test_user_group_data.name, description=self.test_user_group_data.description, - scope_context=self.scope_context, + scope_context={ + "name": self.test_course["name"], + "content_object": { + "content_type": self.course_content_type, + "object_id": self.test_course["id"], + }, + }, criterion_data=[ { "criterion_type": self.last_login_criterion.criterion_type, @@ -220,9 +171,9 @@ def test_create_group_with_multiple_criteria(self): "criterion_config": self.last_login_criterion.criterion_config, }, { - "criterion_type": self.enrollment_mode_criterion.criterion_type, - "criterion_operator": self.enrollment_mode_criterion.criterion_operator, - "criterion_config": self.enrollment_mode_criterion.criterion_config, + "criterion_type": self.user_staff_status_criterion.criterion_type, + "criterion_operator": self.user_staff_status_criterion.criterion_operator, + "criterion_config": self.user_staff_status_criterion.criterion_config, }, ], ) @@ -232,18 +183,9 @@ def test_create_group_with_multiple_criteria(self): criterion_type=self.last_login_criterion.criterion_type ).exists() assert user_group.criteria.filter( - criterion_type=self.enrollment_mode_criterion.criterion_type + criterion_type=self.user_staff_status_criterion.criterion_type ).exists() - def test_create_group_with_mismatched_criteria_scope(self): - """Test that a group can't be created with criteria that don't match the group's scope. - - Expected Results: - - The group is not created. - - An exception is raised. - """ - pass - def test_create_group_with_criteria_and_evaluate_membership(self): """Test that a group can be created with criteria and immediatly evaluated for membership. @@ -284,7 +226,7 @@ def test_create_group_with_criteria_and_evaluate_membership(self): name=self.test_user_group_data.name, description=self.test_user_group_data.description, scope_context=self.scope_context, - criterion_data=[ + criterion_data=[ # TODO: I'm worried about usability of this API. { "criterion_type": self.last_login_criterion.criterion_type, "criterion_operator": self.last_login_criterion.criterion_operator, diff --git a/tests/test_models.py b/tests/test_models.py index c149192..0a7f928 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -216,13 +216,28 @@ def test_user_group_same_name_different_scope(self): assert another_group.name == self.user_group.name assert another_group.scope != self.user_group.scope - def test_criterion_unique_name_per_group(self): - """Test that Criterion name must be unique within a user group.""" + def test_criterion_multiple_same_type_per_group(self): + """Test that multiple criteria of the same type can exist in a user group.""" # This should work fine - Criterion.objects.create(criterion_type="last_login", user_group=self.user_group) + criterion1 = Criterion.objects.create( + criterion_type="last_login", + criterion_operator=">=", + criterion_config={"days": 5}, + user_group=self.user_group, + ) - # This should raise an IntegrityError due to unique_together constraint - with self.assertRaises(Exception): # Could be IntegrityError or ValidationError - Criterion.objects.create( - criterion_type="last_login", user_group=self.user_group - ) + # This should also work fine - multiple criteria of same type are allowed + criterion2 = Criterion.objects.create( + criterion_type="last_login", + criterion_operator="<=", + criterion_config={"days": 10}, + user_group=self.user_group, + ) + + # Both criteria should exist + assert ( + Criterion.objects.filter( + user_group=self.user_group, criterion_type="last_login" + ).count() + == 2 + ) diff --git a/tests/test_tasks.py b/tests/test_tasks.py new file mode 100644 index 0000000..a74c21e --- /dev/null +++ b/tests/test_tasks.py @@ -0,0 +1,217 @@ +"""Test Suite for the User Group tasks. + +This test suite covers all tasks defined in tasks.py. +""" + +from django.test import TestCase +from django.utils import timezone +from datetime import timedelta +from django.contrib.contenttypes.models import ContentType +from django.contrib.auth import get_user_model + +from openedx_user_groups.tasks import orchestrate_user_groups_updates_based_on_events +from openedx_user_groups.handlers import handle_user_group_update +from openedx_user_groups.criteria import BaseCriterionType +from openedx_user_groups.api import * +from tests.factories import * + +from openedx_events.learning.data import UserPersonalData +from openedx_user_groups.criteria_types import ( + USER_STAFF_STATUS_CHANGED, + UserDataExtended, +) + +User = get_user_model() + + +class TestOrchestrateUserGroupsUpdatesBasedOnEvents(TestCase): + """Test the orchestrate_user_groups_updates_based_on_events task logic.""" + + @classmethod + def setUpTestData(cls): + """Set up the test environment.""" + for event in BaseCriterionType.get_all_updated_by_events(): + event.connect(handle_user_group_update) + cls.test_course = CourseFactory() + cls.course_content_type = ContentType.objects.get_for_model(User) + cls.test_scope = ScopeFactory( + name=cls.test_course["name"], + content_type=cls.course_content_type, + object_id=cls.test_course["id"], + ) + cls.test_user_group_data = UserGroupFactory.build(name="At Risk Students") + cls.last_login_criterion = LastLoginCriterionFactory.build() + cls.enrollment_mode_criterion = EnrollmentModeCriterionFactory.build() + cls.user_staff_status_criterion = UserStaffStatusCriterionFactory.build() + cls.scope_context = { + "name": cls.test_course["name"], + "content_object": { + "content_type": cls.course_content_type, + "object_id": cls.test_course["id"], + }, + } + cls.user_old_login_non_staff = UserFactory( + username="user_old_login_non_staff", + last_login=timezone.now() - timedelta(days=2), # 2 days ago (> 1 day ago) + is_staff=False, # non-staff + ) + cls.user_old_login_non_staff_2 = UserFactory( + username="user_old_login_non_staff_2", + last_login=timezone.now() - timedelta(days=2), # 2 days ago (> 1 day ago) + is_staff=False, # non-staff + ) + cls.user_recent_login_staff = UserFactory( + username="user_recent_login_staff", + last_login=timezone.now() - timedelta(hours=1), # 1 hour ago (< 1 day ago) + is_staff=True, # staff + ) + cls.user_old_login_staff = UserFactory( + username="user_old_login_staff", + last_login=timezone.now() - timedelta(days=3), # 3 days ago (> 1 day ago) + is_staff=True, # staff (fails is_staff=False criterion) + ) + cls.user_old_login_non_staff_group = create_group_with_criteria( # Returns user_old_login_non_staff and user_old_login_non_staff_2 + name="Old Login Non Staff Group", + description="Old Login Non Staff Group", + scope_context=cls.scope_context, + criterion_data=[ + { + "criterion_type": cls.last_login_criterion.criterion_type, + "criterion_operator": cls.last_login_criterion.criterion_operator, + "criterion_config": cls.last_login_criterion.criterion_config, + }, + { + "criterion_type": cls.user_staff_status_criterion.criterion_type, + "criterion_operator": cls.user_staff_status_criterion.criterion_operator, + "criterion_config": cls.user_staff_status_criterion.criterion_config, + }, + ], + ) + # TODO: during tests I found that I could create duplicated groups (same name, same scope) need to check it + # And only the last one is being created no error or warning is raised + cls.user_non_staff_status_group = create_group_with_criteria( # Returns user_old_login_staff, user_old_login_non_staff, user_old_login_non_staff_2 + name="Non Staff Status Group", + description="Non Staff Status Group", + scope_context=cls.scope_context, + criterion_data=[ + { + "criterion_type": cls.user_staff_status_criterion.criterion_type, + "criterion_operator": cls.user_staff_status_criterion.criterion_operator, + "criterion_config": cls.user_staff_status_criterion.criterion_config, + }, + ], + ) + evaluate_and_update_membership_for_multiple_groups( + [cls.user_old_login_non_staff_group.id, cls.user_non_staff_status_group.id] + ) + cls.new_user_non_staff = UserFactory( # Create user after evaluation + username="new_user_non_staff", + last_login=timezone.now() - timedelta(hours=1), # 1 hour ago (< 1 day ago) + is_staff=False, # staff + ) + + def test_orchestrate_updates_with_user_not_in_any_group(self): + """Test the event-based update for a user that is not in any group. + + Expected Results: + - Since the user doesn't belong to any group, then all criteria type affected by the event should be + updated. + """ + USER_STAFF_STATUS_CHANGED.send_event( + user=UserDataExtended( + is_staff=self.new_user_non_staff.is_staff, + pii=UserPersonalData( + username=self.new_user_non_staff.username, + email=self.new_user_non_staff.email, + name=f"{self.new_user_non_staff.first_name} {self.new_user_non_staff.last_name}", + ), + id=self.new_user_non_staff.id, + is_active=self.new_user_non_staff.is_active, + ), + ) + self.assertEqual(get_groups_for_user(self.new_user_non_staff.id).count(), 1) + + def test_orchestrate_updates_with_user_in_multiple_groups(self): + """Test the event-based update for a user that is in multiple groups. + + Expected Results: + - Since the user belongs to a single group, then the group that is configured with the criteria types should be updated. + - Also the other groups with the same criteria type should be updated. + """ + staff_user_group = create_group_with_criteria( + name="Staff User Group", + description="Staff User Group", + scope_context=self.scope_context, + criterion_data=[ + { + "criterion_type": self.user_staff_status_criterion.criterion_type, + "criterion_operator": self.user_staff_status_criterion.criterion_operator, + "criterion_config": {"is_staff": True}, + }, + ], + ) + evaluate_and_update_membership_for_multiple_groups([staff_user_group.id]) + assert self.user_old_login_non_staff not in staff_user_group.users.all() + assert ( + self.user_old_login_non_staff + in self.user_old_login_non_staff_group.users.all() + ) + + # Update the user to be staff before sending the event + self.user_old_login_non_staff.is_staff = True + self.user_old_login_non_staff.save() + + USER_STAFF_STATUS_CHANGED.send_event( + user=UserDataExtended( + is_staff=True, + pii=UserPersonalData( + username=self.user_old_login_non_staff.username, + email=self.user_old_login_non_staff.email, + name=f"{self.user_old_login_non_staff.first_name} {self.user_old_login_non_staff.last_name}", + ), + id=self.user_old_login_non_staff.id, + is_active=self.user_old_login_non_staff.is_active, + ) + ) + assert ( + self.user_old_login_non_staff + not in self.user_old_login_non_staff_group.users.all() + ) + assert self.user_old_login_non_staff in staff_user_group.users.all() + self.user_old_login_non_staff.is_staff = False + self.user_old_login_non_staff.save() + + def test_orchestrate_updates_when_there_is_no_change_in_membership_state(self): + """Test when the event-based update doesn't change the membership state. + + Expected Results: + - Since the update doesn't affect the membership state, then no groups should be updated. + """ + assert ( + self.user_old_login_non_staff + in self.user_old_login_non_staff_group.users.all() + ) + assert ( + self.user_old_login_non_staff + in self.user_non_staff_status_group.users.all() + ) + USER_STAFF_STATUS_CHANGED.send_event( + user=UserDataExtended( + is_staff=self.user_old_login_non_staff.is_staff, # No change in membership state + pii=UserPersonalData( + username=f"{self.user_old_login_non_staff.username}_2", # Changed username, but it doesn't affect the membership state + email=f"{self.user_old_login_non_staff.email}_2", + name=f"{self.user_old_login_non_staff.first_name}_2 {self.user_old_login_non_staff.last_name}_2", + ), + id=self.user_old_login_non_staff.id, + is_active=self.user_old_login_non_staff.is_active, + ) + ) + assert ( + self.user_old_login_non_staff + in self.user_old_login_non_staff_group.users.all() + ) + assert ( + self.user_old_login_non_staff + in self.user_non_staff_status_group.users.all() + ) From 85541af71acf38faca9e65a9773542e608086915 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 24 Jun 2025 18:04:46 +0200 Subject: [PATCH 06/12] refactor: contain logic in criteria types and API layer --- openedx_user_groups/api.py | 66 +++++++-------------------- openedx_user_groups/backends.py | 10 ++-- openedx_user_groups/criteria.py | 22 ++++++--- openedx_user_groups/criteria_types.py | 55 ++++++---------------- openedx_user_groups/events.py | 9 ++++ openedx_user_groups/manager.py | 25 ++++++++++ openedx_user_groups/models.py | 19 ++++++-- openedx_user_groups/utils.py | 22 +++++++++ tests/test_tasks.py | 5 +- 9 files changed, 122 insertions(+), 111 deletions(-) create mode 100644 openedx_user_groups/utils.py diff --git a/openedx_user_groups/api.py b/openedx_user_groups/api.py index b28a918..f4c45c6 100644 --- a/openedx_user_groups/api.py +++ b/openedx_user_groups/api.py @@ -13,7 +13,7 @@ from openedx_user_groups.backends import DjangoORMBackendClient from openedx_user_groups.criteria import BaseCriterionType -from openedx_user_groups.manager import load_criterion_class +from openedx_user_groups.manager import load_criterion_class_and_create_instance from openedx_user_groups.models import Criterion, Scope, UserGroup, UserGroupMembership User = get_user_model() @@ -37,27 +37,6 @@ ] -def get_scope_type_from_content_type(content_type): - """ - Map Django ContentType to scope type names used by criteria. - - Args: - content_type: Django ContentType instance - - Returns: - str: Scope type name (e.g., "course", "organization", "instance") - """ - # Mapping from Django model names to scope types - model_to_scope_mapping = { - "course": "course", # When we have actual course models - "courseoverview": "course", # edx-platform course overview model - "organization": "organization", # Organization models - } - - model_name = content_type.model - return model_to_scope_mapping.get(model_name, "instance") # Default to instance - - def get_or_create_group_and_scope( name: str, description: str, scope_context: dict ) -> tuple[UserGroup, Scope]: @@ -87,26 +66,11 @@ def get_or_create_group_and_scope( return user_group, scope -def load_criterion_class_and_create_instance( - criterion_type: str, criterion_operator: str, criterion_config: dict -): - """Create a new criterion class. - - Args: - criterion_type (str): The type of the criterion. - criterion_operator (str): The operator of the criterion. - criterion_config (dict): The configuration of the criterion. - - Returns: - BaseCriterionType: The created criterion class. - """ - criterion_class = load_criterion_class(criterion_type) - criterion_instance = criterion_class(criterion_operator, criterion_config) - return criterion_instance - - def create_group_with_criteria_from_data( - name: str, description: str, scope_context: dict, criterion_data: [dict] # TODO: should we use pydantic models instead of dicts? + name: str, + description: str, + scope_context: dict, + criterion_data: [dict], # TODO: should we use pydantic models instead of dicts? ): """Create a new user group with the given name, description, scope, and criteria. This criteria hasn't been instantiated and validated yet. @@ -129,10 +93,13 @@ def create_group_with_criteria_from_data( data["criterion_type"], data["criterion_operator"], data["criterion_config"], + scope, + DjangoORMBackendClient, + ) + criterion = Criterion.objects.create( + user_group=user_group, **criterion_instance.serialize() ) - scope_type = get_scope_type_from_content_type(scope.content_type) - assert scope_type in criterion_instance.scopes, f"Criterion '{criterion_instance.criterion_type}' does not support scope type '{scope_type}'. Supported scopes: {criterion_instance.scopes}" - Criterion.objects.create(user_group=user_group, **criterion_instance.serialize()) + return user_group @@ -148,10 +115,7 @@ def evaluate_and_update_membership_for_group(group_id: int): # Evaluatate criteria and build list of Q objects - Done by what we called "combinator" criteria_results = [] for criterion in user_group.criteria.all(): - result = criterion.criterion_instance.evaluate( - current_scope=user_group.scope, - backend_client=DjangoORMBackendClient, # TODO: for now we'd only support DjangoORMBackendClient. But I think we could pass a list of registered backend clients here. - ) + result = criterion.criterion_instance.evaluate() criteria_results.append(result) @@ -297,7 +261,11 @@ def get_group_by_id(group_id: int): Returns: UserGroup: The user group. """ - return UserGroup.objects.get(id=group_id) + return ( + UserGroup.objects.select_related("scope") + .prefetch_related("criteria") + .get(id=group_id) + ) def get_group_by_name(name: str): diff --git a/openedx_user_groups/backends.py b/openedx_user_groups/backends.py index cbcf280..f8ceb81 100644 --- a/openedx_user_groups/backends.py +++ b/openedx_user_groups/backends.py @@ -5,7 +5,7 @@ from django.contrib.auth import get_user_model from django.db.models import QuerySet -from openedx_user_groups.models import Scope +# Scope import removed to avoid circular import - using duck typing instead User = get_user_model() @@ -33,7 +33,7 @@ class DjangoORMBackendClient(BackendClient): """ @staticmethod - def get_enrollments(scope: Scope) -> QuerySet: + def get_enrollments(scope) -> QuerySet: # scope: Scope model instance """Provide an interface to get all user enrollments for a given scope. Args: @@ -48,7 +48,7 @@ def get_enrollments(scope: Scope) -> QuerySet: pass @staticmethod - def get_users(scope: Scope) -> QuerySet: + def get_users(scope) -> QuerySet: # scope: Scope model instance """Provide an interface to get all users for a given scope. For simplicity reasons, we'll consider all users in the current instance. The idea would be @@ -58,7 +58,7 @@ def get_users(scope: Scope) -> QuerySet: return User.objects.all().exclude(is_staff=True, is_superuser=True) @staticmethod - def get_grade(scope: Scope) -> QuerySet: + def get_grade(scope) -> QuerySet: # scope: Scope model instance """Provide an interface to get all grades for a given scope. This method should be implemented by the backend client. Use existent API methods to get the data for the scope. @@ -66,7 +66,7 @@ def get_grade(scope: Scope) -> QuerySet: pass @staticmethod - def get_course_progress(scope: Scope) -> QuerySet: + def get_course_progress(scope) -> QuerySet: # scope: Scope model instance """Provide an interface to get all course progress for a given scope. This method should be implemented by the backend client. Use existent API methods to get the data for the scope. diff --git a/openedx_user_groups/criteria.py b/openedx_user_groups/criteria.py index d730a4f..4b89f4f 100644 --- a/openedx_user_groups/criteria.py +++ b/openedx_user_groups/criteria.py @@ -19,6 +19,9 @@ from openedx_events.tooling import OpenEdxPublicSignal from pydantic import BaseModel, ValidationError +from openedx_user_groups.backends import BackendClient +from openedx_user_groups.utils import get_scope_type_from_content_type + logger = logging.getLogger(__name__) @@ -91,12 +94,22 @@ def __init__( self, criterion_operator: str, criterion_config: dict | BaseModel, + scope, # Scope model instance - no type hint to avoid circular imports + backend_client: BackendClient, ): if isinstance(criterion_config, BaseModel): - self.criterion_config = criterion_config # DO not validate if we're passing a pydantic model + self.criterion_config = ( + criterion_config # DO not validate if we're passing a pydantic model + ) else: self.criterion_config = self.validate_config(criterion_config) self.criterion_operator = self.validate_operator(criterion_operator) + scope_type = get_scope_type_from_content_type(scope.content_type) + assert ( + scope_type in self.scopes + ), f"Criterion '{self.criterion_type}' does not support scope type '{scope_type}'. Supported scopes: {self.scopes}" + self.scope = scope + self.backend_client = backend_client def __init_subclass__(cls, **kwargs): """Override to validate the subclass attributes.""" @@ -173,12 +186,7 @@ def config_model( return self.ConfigModel @abstractmethod - def evaluate( - self, - config: BaseModel, - operator: ComparisonOperator, - scope_context: Dict[str, Any] = None, - ) -> QuerySet: # TODO: for simplicity return a queryset. + def evaluate(self) -> QuerySet: # TODO: for simplicity return a queryset. """ Evaluate the criterion and return a Q object for filtering users. diff --git a/openedx_user_groups/criteria_types.py b/openedx_user_groups/criteria_types.py index 38f1691..d4565a3 100644 --- a/openedx_user_groups/criteria_types.py +++ b/openedx_user_groups/criteria_types.py @@ -26,19 +26,7 @@ from openedx_user_groups.backends import BackendClient from openedx_user_groups.criteria import BaseCriterionType, ComparisonOperator from openedx_user_groups.models import Scope - - -@attr.s(frozen=True) -class UserDataExtended(UserData): - is_staff = attr.ib(type=bool) - - -USER_STAFF_STATUS_CHANGED = OpenEdxPublicSignal( - event_type="org.openedx.learning.user.staff_status.changed.v1", - data={ - "user": UserDataExtended, - }, -) +from openedx_user_groups.events import USER_STAFF_STATUS_CHANGED class ManualCriterion(BaseCriterionType): @@ -58,15 +46,13 @@ class ConfigModel(BaseModel): ComparisonOperator.NOT_IN, ] - def evaluate( - self, - current_scope: Scope, - backend_client: BackendClient = None, - ) -> QuerySet: + def evaluate(self) -> QuerySet: """ Evaluate the criterion. """ - return backend_client.get_users(current_scope).filter( # Currently side-wide, but should be filtered by scope + return self.backend_client.get_users( + self.scope + ).filter( # Currently side-wide, but should be filtered by scope id__in=self.criterion_config.user_ids ) @@ -98,11 +84,7 @@ class ConfigModel(BaseModel): scopes: List[str] = ["course"] updated_by_events = [COURSE_ENROLLMENT_CREATED, COURSE_ENROLLMENT_CHANGED] - def evaluate( - self, - current_scope: Scope, - backend_client: BackendClient = None, - ) -> QuerySet: + def evaluate(self) -> QuerySet: """ Evaluate the criterion. """ @@ -111,7 +93,7 @@ def evaluate( filters["mode"] = self.criterion_config.mode if self.criterion_config.enrollment_date: filters["created__gte"] = self.criterion_config.enrollment_date - return backend_client.get_enrollments(current_scope).filter(**filters) + return self.backend_client.get_enrollments(self.scope).filter(**filters) class LastLoginCriterion(BaseCriterionType): @@ -135,11 +117,7 @@ class ConfigModel(BaseModel): ComparisonOperator.LESS_THAN_OR_EQUAL, ] - def evaluate( - self, - current_scope: Scope, - backend_client: BackendClient = None, # Dependency injection for the backend client - ) -> QuerySet: + def evaluate(self) -> QuerySet: """ Evaluate the criterion. @@ -151,6 +129,7 @@ def evaluate( # For "days since last login" logic: # - GREATER_THAN X days = last_login < (now - X days) [older than X days] # - LESS_THAN X days = last_login > (now - X days) [more recent than X days] + # TODO: extract this to a helper function (backend so it's criteria agnostic)? queryset_operator_mapping = { ComparisonOperator.EQUAL: "exact", # exactly X days ago (rarely used for datetime) ComparisonOperator.NOT_EQUAL: "exact", # not exactly X days ago @@ -165,7 +144,7 @@ def evaluate( "last_login__" + queryset_operator_mapping[self.criterion_operator]: threshold_date } - return backend_client.get_users(current_scope).filter( + return self.backend_client.get_users(self.scope).filter( **query ) # TODO: is it better to use Q objects instead? @@ -188,11 +167,7 @@ class ConfigModel(BaseModel): ] scopes: List[str] = ["course"] - def evaluate( - self, - current_scope: Scope, - backend_client: BackendClient = None, - ) -> QuerySet: + def evaluate(self) -> QuerySet: """ Evaluate the criterion. """ @@ -211,11 +186,7 @@ class UserStaffStatusCriterion(BaseCriterionType): class ConfigModel(BaseModel): is_staff: bool # True to filter for staff users, False for non-staff users - def evaluate( - self, - current_scope: Scope, - backend_client: BackendClient = None, - ) -> QuerySet: + def evaluate(self) -> QuerySet: """Evaluate the criterion based on user staff status. Args: @@ -227,6 +198,6 @@ def evaluate( Returns: Q object for filtering users """ - return backend_client.get_users(current_scope).filter( + return self.backend_client.get_users(self.scope).filter( is_staff=self.criterion_config.is_staff ) diff --git a/openedx_user_groups/events.py b/openedx_user_groups/events.py index 9386561..df81cdc 100644 --- a/openedx_user_groups/events.py +++ b/openedx_user_groups/events.py @@ -5,9 +5,15 @@ This is a temporary module that will be replaced by the events defined in the openedx-events repository. """ +import attr +from openedx_events.learning.data import UserData from openedx_events.tooling import OpenEdxPublicSignal +@attr.s(frozen=True) +class UserDataExtended(UserData): + is_staff = attr.ib(type=bool) + # .. event_type: org.openedx.learning.user.staff_status.changed.v1 # .. event_name: USER_STAFF_STATUS_CHANGED # .. event_key_field: user.id @@ -16,4 +22,7 @@ # .. event_trigger_repository: openedx/edx-platform USER_STAFF_STATUS_CHANGED = OpenEdxPublicSignal( event_type="org.openedx.learning.user.staff_status.changed.v1", + data={ + "user": UserDataExtended, + }, ) diff --git a/openedx_user_groups/manager.py b/openedx_user_groups/manager.py index 812999e..ce62a3b 100644 --- a/openedx_user_groups/manager.py +++ b/openedx_user_groups/manager.py @@ -37,6 +37,7 @@ class CriterionManager(PluginManager): "enrollment_mode": "openedx_user_groups.criteria_types:EnrollmentModeCriterion", "enrolled_with_specific_mode": "openedx_user_groups.criteria_types:EnrollmentModeCriterion", "user_staff_status": "openedx_user_groups.criteria_types:UserStaffStatusCriterion", + "manual": "openedx_user_groups.criteria_types:ManualCriterion", } @classmethod @@ -75,3 +76,27 @@ def load_criterion_class(criterion_type: str) -> BaseCriterionType: BaseCriterionType: The criterion class. """ return CriterionManager.get_criterion_class_by_type(criterion_type) + + +def load_criterion_class_and_create_instance( + criterion_type: str, + criterion_operator: str, + criterion_config: dict, + scope, # Scope model instance - no type hint to avoid circular imports + backend_client, # BackendClient instance - no type hint to avoid circular imports +): + """Create a new criterion class. + + Args: + criterion_type (str): The type of the criterion. + criterion_operator (str): The operator of the criterion. + criterion_config (dict): The configuration of the criterion. + + Returns: + BaseCriterionType: The created criterion class. + """ + criterion_class = load_criterion_class(criterion_type) + criterion_instance = criterion_class( + criterion_operator, criterion_config, scope, backend_client + ) + return criterion_instance diff --git a/openedx_user_groups/models.py b/openedx_user_groups/models.py index 80aa460..75dabd1 100644 --- a/openedx_user_groups/models.py +++ b/openedx_user_groups/models.py @@ -28,15 +28,18 @@ This module is not meant for production, it's only for POC purposes. """ + +import json + from django.contrib.auth import get_user_model from django.contrib.contenttypes.fields import GenericForeignKey from django.contrib.contenttypes.models import ContentType from django.core.exceptions import ValidationError from django.db import models -import json - +from django.utils.functional import cached_property from pydantic_core import from_json +from openedx_user_groups.backends import DjangoORMBackendClient from openedx_user_groups.manager import CriterionManager User = get_user_model() @@ -206,10 +209,18 @@ def criterion_type_template(self): @property def criterion_instance(self): + """Return the criterion instanced with the current configuration. + + Returns: + BaseCriterionType: The criterion instance. + """ return self.criterion_type_template( - self.criterion_operator, self.criterion_config + self.criterion_operator, + self.criterion_config, + self.user_group.scope, + DjangoORMBackendClient(), ) - @property + @cached_property def config(self): return from_json(self.criterion_config) diff --git a/openedx_user_groups/utils.py b/openedx_user_groups/utils.py new file mode 100644 index 0000000..418c9c4 --- /dev/null +++ b/openedx_user_groups/utils.py @@ -0,0 +1,22 @@ +"""Utility functions for the openedx_user_groups app.""" + + +def get_scope_type_from_content_type(content_type): + """ + Map Django ContentType to scope type names used by criteria. + + Args: + content_type: Django ContentType instance + + Returns: + str: Scope type name (e.g., "course", "organization", "instance") + """ + # Mapping from Django model names to scope types + model_to_scope_mapping = { + "course": "course", # When we have actual course models + "courseoverview": "course", # edx-platform course overview model + "organization": "organization", # Organization models + } + + model_name = content_type.model + return model_to_scope_mapping.get(model_name, "instance") # Default to instance diff --git a/tests/test_tasks.py b/tests/test_tasks.py index a74c21e..37d9c23 100644 --- a/tests/test_tasks.py +++ b/tests/test_tasks.py @@ -16,10 +16,7 @@ from tests.factories import * from openedx_events.learning.data import UserPersonalData -from openedx_user_groups.criteria_types import ( - USER_STAFF_STATUS_CHANGED, - UserDataExtended, -) +from openedx_user_groups.events import USER_STAFF_STATUS_CHANGED, UserDataExtended User = get_user_model() From a9904e66294b597f763f9d8ff2deaa76c50ec94e Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 25 Jun 2025 14:04:57 +0200 Subject: [PATCH 07/12] feat: add group collections for manual groups enforcing mutual exclusivity --- openedx_user_groups/api.py | 94 ++++++++++++++----- openedx_user_groups/criteria_types.py | 7 +- openedx_user_groups/events.py | 1 + .../migrations/0002_groupcollection.py | 36 +++++++ openedx_user_groups/models.py | 16 ++++ tests/factories.py | 7 ++ tests/test_api.py | 93 ++++++++++++++++++ 7 files changed, 230 insertions(+), 24 deletions(-) create mode 100644 openedx_user_groups/migrations/0002_groupcollection.py diff --git a/openedx_user_groups/api.py b/openedx_user_groups/api.py index f4c45c6..c824a51 100644 --- a/openedx_user_groups/api.py +++ b/openedx_user_groups/api.py @@ -9,12 +9,13 @@ from django.contrib.auth import get_user_model from django.db import transaction +from django.db.models import Count from django.utils import timezone from openedx_user_groups.backends import DjangoORMBackendClient from openedx_user_groups.criteria import BaseCriterionType from openedx_user_groups.manager import load_criterion_class_and_create_instance -from openedx_user_groups.models import Criterion, Scope, UserGroup, UserGroupMembership +from openedx_user_groups.models import Criterion, GroupCollection, Scope, UserGroup, UserGroupMembership User = get_user_model() @@ -27,13 +28,14 @@ "evaluate_and_update_membership_for_multiple_groups", "get_groups_for_scope", "get_group_by_id", - "get_group_by_name", "get_group_by_name_and_scope", "get_user_group_members", "update_group_name_or_description", "soft_delete_group", "hard_delete_group", "get_groups_for_user", + "create_group_collection_and_add_groups", + "evaluate_and_update_membership_for_group_collection", ] @@ -249,9 +251,6 @@ def get_groups_for_user(user_id: int): ).select_related("group") -# TODO: THESE METHODS I HAVEN'T TESTED YET - - def get_group_by_id(group_id: int): """Get a user group by its ID. @@ -268,20 +267,83 @@ def get_group_by_id(group_id: int): ) -def get_group_by_name(name: str): - """Get a user group by its name. +def create_group_collection_and_add_groups( + name: str, description: str, group_ids: [int] +): + """Create a new group collection and add groups to it. Args: - name (str): The name of the user group. + name (str): The name of the group collection. + description (str): A brief description of the group collection. + group_ids (list): The IDs of the user groups to add to the collection. Returns: - UserGroup: The user group. + GroupCollection: The created group collection. """ - return UserGroup.objects.get(name=name) + with transaction.atomic(): + group_collection = GroupCollection.objects.create( + name=name, description=description + ) + for group_id in group_ids: + group = get_group_by_id(group_id) + group_collection.user_groups.add(group) + + return group_collection + + +def get_group_collection_by_id(group_collection_id: int): + """Get a group collection by its ID. + + Args: + group_collection_id (int): The ID of the group collection. + + Returns: + GroupCollection: The group collection. + """ + return GroupCollection.objects.prefetch_related("user_groups").get( + id=group_collection_id + ) + + +def evaluate_and_update_membership_for_group_collection(group_collection_id: int): + """Evaluate the membership of a group collection and update the membership records. + + This method considers the mutual exclusivity of the groups in the collection. + + Args: + group_collection_id (int): The ID of the group collection. + + Returns: + tuple: + - GroupCollection: The group collection. + - QuerySet: The duplicates users found and removed from the group collection. + """ + with transaction.atomic(): + group_collection = get_group_collection_by_id(group_collection_id) + for group in group_collection.user_groups.all(): + evaluate_and_update_membership_for_group(group.id) + # Find duplicates in the group collection to remove them and prompt for action + duplicates = ( + User.objects.filter( + usergroupmembership__group__in=group_collection.user_groups.all() + ) + .annotate(group_count=Count("usergroupmembership__group", distinct=True)) + .filter(group_count__gt=1) + ) + if duplicates.exists(): + # TODO: Prompt for action, but for the time being remove the duplicates + for duplicate in duplicates: + duplicate.usergroupmembership_set.filter( + group__in=group_collection.user_groups.all() + ).delete() + return group_collection, duplicates + + +# TODO: THESE METHODS I HAVEN'T TESTED YET def get_group_by_name_and_scope(name: str, scope: str): - """Get a user group by its name and scope. TODO: should we allow multiple groups with the same name but different scopes? + """Get a user group by its name and scope. Args: name (str): The name of the user group. @@ -324,13 +386,3 @@ def hard_delete_group(group_id: int): def soft_delete_group(group_id: int): """Soft delete a user group. This will not delete the group, but it will prevent it from being used by disabling it.""" UserGroup.objects.filter(id=group_id).update(is_active=False) - - -def enable_group(group_id: int): - """Enable a user group. This will allow it to be used again.""" - UserGroup.objects.filter(id=group_id).update(is_active=True) - - -def disable_group(group_id: int): - """Disable a user group. This will prevent it from being used.""" - UserGroup.objects.filter(id=group_id).update(is_active=False) diff --git a/openedx_user_groups/criteria_types.py b/openedx_user_groups/criteria_types.py index d4565a3..164777f 100644 --- a/openedx_user_groups/criteria_types.py +++ b/openedx_user_groups/criteria_types.py @@ -25,8 +25,8 @@ from openedx_user_groups.backends import BackendClient from openedx_user_groups.criteria import BaseCriterionType, ComparisonOperator -from openedx_user_groups.models import Scope from openedx_user_groups.events import USER_STAFF_STATUS_CHANGED +from openedx_user_groups.models import Scope class ManualCriterion(BaseCriterionType): @@ -38,7 +38,7 @@ class ManualCriterion(BaseCriterionType): ) class ConfigModel(BaseModel): - user_ids: List[str] # Usernames or emails + usernames_or_emails: List[str] # Usernames or emails # Supported operators for this criterion type supported_operators: List[ComparisonOperator] = [ @@ -53,7 +53,8 @@ def evaluate(self) -> QuerySet: return self.backend_client.get_users( self.scope ).filter( # Currently side-wide, but should be filtered by scope - id__in=self.criterion_config.user_ids + Q(username__in=self.criterion_config.usernames_or_emails) + | Q(email__in=self.criterion_config.usernames_or_emails) ) diff --git a/openedx_user_groups/events.py b/openedx_user_groups/events.py index df81cdc..a57c0ec 100644 --- a/openedx_user_groups/events.py +++ b/openedx_user_groups/events.py @@ -14,6 +14,7 @@ class UserDataExtended(UserData): is_staff = attr.ib(type=bool) + # .. event_type: org.openedx.learning.user.staff_status.changed.v1 # .. event_name: USER_STAFF_STATUS_CHANGED # .. event_key_field: user.id diff --git a/openedx_user_groups/migrations/0002_groupcollection.py b/openedx_user_groups/migrations/0002_groupcollection.py new file mode 100644 index 0000000..6c726e4 --- /dev/null +++ b/openedx_user_groups/migrations/0002_groupcollection.py @@ -0,0 +1,36 @@ +# Generated by Django 4.2.21 on 2025-06-25 06:37 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("openedx_user_groups", "0001_initial"), + ] + + operations = [ + migrations.CreateModel( + name="GroupCollection", + fields=[ + ( + "id", + models.BigAutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("name", models.CharField(max_length=255)), + ("description", models.TextField(blank=True, null=True)), + ( + "user_groups", + models.ManyToManyField( + related_name="group_collections", + to="openedx_user_groups.usergroup", + ), + ), + ], + ), + ] diff --git a/openedx_user_groups/models.py b/openedx_user_groups/models.py index 75dabd1..819c002 100644 --- a/openedx_user_groups/models.py +++ b/openedx_user_groups/models.py @@ -224,3 +224,19 @@ def criterion_instance(self): @cached_property def config(self): return from_json(self.criterion_config) + + +class GroupCollection(models.Model): + """Represents a collection of user groups. + + Attributes: + name (str): The name of the group collection. + description (str): A brief description of the group collection. + """ + + name = models.CharField(max_length=255) + description = models.TextField(blank=True, null=True) + user_groups = models.ManyToManyField(UserGroup, related_name="group_collections") + + def __str__(self): + return self.name diff --git a/tests/factories.py b/tests/factories.py index 7a59511..a4bd337 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -90,3 +90,10 @@ class UserStaffStatusCriterionFactory(CriterionFactory): criterion_type = "user_staff_status" criterion_operator = "=" criterion_config = factory.Dict({"is_staff": False}) # Filter for non-staff users + + +class ManualCriterionFactory(CriterionFactory): + """Factory for creating ManualCriterion instances.""" + + criterion_type = "manual" + criterion_operator = "in" diff --git a/tests/test_api.py b/tests/test_api.py index b3a1239..23cd6a6 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -41,6 +41,9 @@ def setUpTestData(cls): }, } + +class UserGroupAPIGeneralPurposeMethodsTestCase(UserGroupAPITestCase): + def test_create_group_with_no_criteria(self): """Test that a group can be created with no criteria associated. @@ -287,3 +290,93 @@ def test_evaluate_membership_for_multiple_groups(self): assert groups[0].users.count() == 1 assert groups[1].users.count() == 1 + + +class UserGroupAPICollectionMethodsTestCase(UserGroupAPITestCase): + + @classmethod + def setUpTestData(cls): + """Set up test data that will be reused across all test methods.""" + super().setUpTestData() + cls.users = [ + UserFactory(username=f"user_{i}", email=f"user_{i}@example.com") + for i in range(3) + ] + cls.manual_groups = [ + create_group_with_criteria( + name=f"manual_group_{i}", + description="Manual group description", + scope_context=cls.scope_context, + criterion_data=[ + { + "criterion_type": ManualCriterionFactory.criterion_type, + "criterion_operator": ManualCriterionFactory.criterion_operator, + "criterion_config": { + "usernames_or_emails": [ + cls.users[i].username + for i in range( + i, 3 + ) # This makes user 1 and 2 in both groups + ] + }, + } + ], + ) + for i in range(2) + ] + + def test_create_group_collection_and_add_groups(self): + """Test that a group collection can be created and groups can be added to it. + + In this test case the groups haven't been evaluated yet, so they should all be empty. + + Expected Results: + - The group collection is created successfully. + - The group collection has the correct name, description, and groups. + - The groups are empty. + """ + group_collection = create_group_collection_and_add_groups( + name="Test Group Collection", + description="Test Group Collection Description", + group_ids=[group.id for group in self.manual_groups], + ) + assert group_collection is not None + assert group_collection.name == "Test Group Collection" + assert group_collection.user_groups.count() == 2 + assert group_collection.user_groups.first().users.count() == 0 + assert group_collection.user_groups.last().users.count() == 0 + + def test_evaluate_and_update_membership_with_duplicates_for_group_collection(self): + """Test that the membership of a group collection can be evaluated and updated. + + In this test case the groups have been evaluated, so they should have members. + The groups have been created with the same users, so there should be duplicates which will be removed + from the groups within the group collection. + + Expected Results: + - The group collection is evaluated successfully. + - The group collection has the correct members. + - The duplicates are removed from the groups within the group collection. + """ + group_collection = create_group_collection_and_add_groups( + name="Test Group Collection", + description="Test Group Collection Description", + group_ids=[group.id for group in self.manual_groups], + ) + group_collection, duplicates = ( + evaluate_and_update_membership_for_group_collection( + group_collection_id=group_collection.id + ) + ) + assert group_collection is not None + assert duplicates is not None + assert group_collection.user_groups.count() == 2 + assert group_collection.user_groups.first().users.count() == 1 # User 0 + assert group_collection.user_groups.first().users.first() == self.users[0] + assert ( + group_collection.user_groups.last().users.count() == 0 + ) # User 1 and 2 were duplicates, so this group is empty + # User 1 and 2 are duplicates, so they should be removed from the group collection + assert duplicates.count() == 2 + assert self.users[1] in duplicates + assert self.users[2] in duplicates From 99ac4dccca0e83aafaaae5d585a2c204d1080a06 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 26 Jun 2025 16:42:32 +0200 Subject: [PATCH 08/12] feat: add API to manage user groups (basic create & retrieve) --- openedx_user_groups/api.py | 92 ++++++++++------ openedx_user_groups/apps.py | 10 ++ openedx_user_groups/backends.py | 6 +- openedx_user_groups/criteria.py | 52 ++++++++- openedx_user_groups/criteria_types.py | 81 +++++++++++--- openedx_user_groups/manager.py | 8 ++ .../migrations/0003_scope_reference_id.py | 18 +++ .../migrations/0004_alter_scope_object_id.py | 18 +++ .../0005_remove_scope_reference_id.py | 17 +++ openedx_user_groups/models.py | 2 +- openedx_user_groups/serializers.py | 66 +++++++++++ openedx_user_groups/urls.py | 17 ++- openedx_user_groups/utils.py | 53 ++++++++- openedx_user_groups/views.py | 67 ++++++++++++ requirements/base.in | 3 + requirements/base.txt | 70 +++++++++++- requirements/dev.txt | 88 ++++++++++++++- requirements/doc.txt | 90 +++++++++++++-- requirements/quality.in | 3 +- requirements/quality.txt | 103 +++++++++++++++++- requirements/test.txt | 90 +++++++++++++++ setup.py | 9 +- test_settings.py | 3 + tests/test_api.py | 81 ++++++++------ tests/test_models.py | 87 ++++++++++----- tests/test_tasks.py | 41 ++++--- 26 files changed, 1024 insertions(+), 151 deletions(-) create mode 100644 openedx_user_groups/migrations/0003_scope_reference_id.py create mode 100644 openedx_user_groups/migrations/0004_alter_scope_object_id.py create mode 100644 openedx_user_groups/migrations/0005_remove_scope_reference_id.py create mode 100644 openedx_user_groups/serializers.py diff --git a/openedx_user_groups/api.py b/openedx_user_groups/api.py index c824a51..414e826 100644 --- a/openedx_user_groups/api.py +++ b/openedx_user_groups/api.py @@ -8,14 +8,16 @@ """ from django.contrib.auth import get_user_model +from django.contrib.contenttypes.models import ContentType from django.db import transaction from django.db.models import Count from django.utils import timezone from openedx_user_groups.backends import DjangoORMBackendClient from openedx_user_groups.criteria import BaseCriterionType -from openedx_user_groups.manager import load_criterion_class_and_create_instance +from openedx_user_groups.manager import CriterionManager, load_criterion_class_and_create_instance from openedx_user_groups.models import Criterion, GroupCollection, Scope, UserGroup, UserGroupMembership +from openedx_user_groups.utils import process_content_object User = get_user_model() @@ -40,39 +42,43 @@ def get_or_create_group_and_scope( - name: str, description: str, scope_context: dict + name: str, description: str, scope: dict ) -> tuple[UserGroup, Scope]: - """Create a new user group with the given name, description, and scope. No criteria is associated with the group. + """Get or create a user group and its scope. Args: name (str): The name of the user group. description (str): A brief description of the user group. - scope (Scope): The scope of the user group. + scope (dict): The context of the scope. Returns: - UserGroup: The created user group. + tuple: A tuple containing the user group and scope. """ with transaction.atomic(): - scope, created = Scope.objects.get_or_create( - name=scope_context[ - "name" - ], # TODO: what is this going to be? The course_key (CourseKey) as string? - content_type=scope_context["content_object"]["content_type"], - object_id=scope_context["content_object"]["object_id"], + content_object = scope.get("content_object", {}) + + content_type, object_id = process_content_object(content_object) + + scope_obj, created = Scope.objects.get_or_create( + name=scope["name"], + description=scope.get("description", ""), + content_type=content_type, + object_id=object_id, ) + user_group, _ = UserGroup.objects.get_or_create( name=name, description=description, - scope=scope, + scope=scope_obj, ) - return user_group, scope + return user_group, scope_obj def create_group_with_criteria_from_data( name: str, description: str, - scope_context: dict, - criterion_data: [dict], # TODO: should we use pydantic models instead of dicts? + scope: dict, + criteria: [dict], # TODO: should we use pydantic models instead of dicts? ): """Create a new user group with the given name, description, scope, and criteria. This criteria hasn't been instantiated and validated yet. @@ -80,17 +86,15 @@ def create_group_with_criteria_from_data( Args: name (str): The name of the user group. description (str): A brief description of the user group. - scope_context (dict): The context of the scope. - criterion_data (list): A list of criterion data. + scope (dict): The context of the scope. + criteria (list): A list of criterion data. Returns: UserGroup: The created user group. """ with transaction.atomic(): - user_group, scope = get_or_create_group_and_scope( - name, description, scope_context - ) - for data in criterion_data: + user_group, scope = get_or_create_group_and_scope(name, description, scope) + for data in criteria: criterion_instance = load_criterion_class_and_create_instance( data["criterion_type"], data["criterion_operator"], @@ -167,16 +171,16 @@ def evaluate_and_update_membership_for_multiple_groups(group_ids: [int]): def create_group_with_criteria_and_evaluate_membership( - name: str, description: str, scope_context: dict, criterion_data: dict + name: str, description: str, scope: dict, criteria: [dict] ): - """Create a new user group with the given name, description, scope, and criterion. - This criterion has been instantiated and validated. + """Create a new user group with the given name, description, scope, and criteria. + This criteria have been instantiated and validated. Args: name (str): The name of the user group. description (str): A brief description of the user group. - scope_context (dict): The context of the scope. - criterion_data (dict): The data of the criterion following the format of: + scope (dict): The context of the scope. + criteria (list): A list of criterion data following the format of: { "criterion_type": str, "criterion_operator": str, @@ -188,14 +192,14 @@ def create_group_with_criteria_and_evaluate_membership( """ with transaction.atomic(): user_group = create_group_with_criteria_from_data( - name, description, scope_context, criterion_data + name, description, scope, criteria ) evaluate_and_update_membership_for_group(user_group.id) return user_group def create_group_with_criteria( - name: str, description: str, scope_context: dict, criterion_data: [dict] + name: str, description: str, scope: dict, criteria: [dict] ): """Create a new user group with the given name, description, scope, and criteria. This criteria hasn't been instantiated and validated yet. @@ -203,8 +207,8 @@ def create_group_with_criteria( Args: name (str): The name of the user group. description (str): A brief description of the user group. - scope_context (dict): The context of the scope. - criterion_data (list): A list of criterion data following the format of: + scope (dict): The context of the scope. + criteria (list): A list of criterion data following the format of: { "criterion_type": str, "criterion_operator": str, @@ -216,7 +220,7 @@ def create_group_with_criteria( """ with transaction.atomic(): user_group = create_group_with_criteria_from_data( - name, description, scope_context, criterion_data + name, description, scope, criteria ) return user_group @@ -339,6 +343,32 @@ def evaluate_and_update_membership_for_group_collection(group_collection_id: int return group_collection, duplicates +def get_available_registered_criteria_schema(): + """Get all available with their schema for fields, operators and descriptions. + + Returns: + dict: A dictionary containing the schema for all available criteria. For example: + { + "criterion_type": { + "fields": { + "field_name": { + "type": "string", + "description": "Description of the field" + } + }, + "operators": ["operator1", "operator2"], + "description": "Description of the criterion", + "criterion_type": "criterion_type", + "supported_scopes": ["course", "organization", "instance"] + } + } + """ + return { + criterion_type: criterion_class.get_schema() + for criterion_type, criterion_class in CriterionManager.get_criterion_classes().items() + } + + # TODO: THESE METHODS I HAVEN'T TESTED YET diff --git a/openedx_user_groups/apps.py b/openedx_user_groups/apps.py index 178dce6..425fe35 100644 --- a/openedx_user_groups/apps.py +++ b/openedx_user_groups/apps.py @@ -13,6 +13,16 @@ class OpenedxUserGroupsConfig(AppConfig): name = "openedx_user_groups" default_auto_field = "django.db.models.BigAutoField" + plugin_app = { + "url_config": { + "lms.djangoapp": { + "namespace": "openedx-user-groups", + "regex": r"^openedx-user-groups/", + "relative_path": "urls", + } + } + } + def ready(self): """ Perform application initialization. diff --git a/openedx_user_groups/backends.py b/openedx_user_groups/backends.py index f8ceb81..4294c41 100644 --- a/openedx_user_groups/backends.py +++ b/openedx_user_groups/backends.py @@ -45,7 +45,11 @@ def get_enrollments(scope) -> QuerySet: # scope: Scope model instance # TODO: need an API to get enrollment objects for a given course. Currently, there is no way # of implementing unittests for this without edx-platform. Can be executed as part of the # edx-platform tests though. - pass + from common.djangoapps.student.models import CourseEnrollment + + return CourseEnrollment.objects.filter( + course_id=scope.object_id + ) # TODO: this could be a way of managing external imports. Can we standardize this? @staticmethod def get_users(scope) -> QuerySet: # scope: Scope model instance diff --git a/openedx_user_groups/criteria.py b/openedx_user_groups/criteria.py index 4b89f4f..14ced9e 100644 --- a/openedx_user_groups/criteria.py +++ b/openedx_user_groups/criteria.py @@ -104,7 +104,9 @@ def __init__( else: self.criterion_config = self.validate_config(criterion_config) self.criterion_operator = self.validate_operator(criterion_operator) - scope_type = get_scope_type_from_content_type(scope.content_type) + scope_type = get_scope_type_from_content_type( + scope.content_type + ) # TODO: we need a way of referencing courseoverview without the cognitive overload of understanding what courseoverview is? assert ( scope_type in self.scopes ), f"Criterion '{self.criterion_type}' does not support scope type '{scope_type}'. Supported scopes: {self.scopes}" @@ -243,3 +245,51 @@ def serialize(self, *args, **kwargs): "criterion_operator": self.criterion_operator, "criterion_config": self.criterion_config.model_dump(*args, **kwargs), } + + @classmethod + def get_schema(cls) -> dict: + """Return the schema for the criterion type. + + Returns: + dict: A dictionary containing the schema for the criterion type. For example: + { + "title": "Manual Criterion Configuration", + "description": "Configuration for manually specifying users by username or email", + "properties": { + "usernames_or_emails": { + "type": "array", + "items": {"type": "string"}, + "description": "List of usernames or email addresses to include in the group", + "examples": [["user1", "user2@example.com", "user3"]], + "minItems": 1 + } + }, + "operators": ["in", "not_in"], + "criterion_description": "A criterion that is used to push a given list of users to a group." + } + """ + config_schema = cls.ConfigModel.model_json_schema() + + config_schema_filtered = { + "title": config_schema.get("title", ""), + "description": config_schema.get("description", ""), + "properties": { + key: value + for key, value in config_schema.get("properties", {}).items() + if key in cls.ConfigModel.model_fields + }, + } + + schema = { + **config_schema_filtered, + "operators": ( + [op.value for op in cls.supported_operators] + if cls.supported_operators + else [] + ), + "criterion_description": cls.description, + "criterion_type": cls.criterion_type, + "supported_scopes": cls.scopes, + } + + return schema diff --git a/openedx_user_groups/criteria_types.py b/openedx_user_groups/criteria_types.py index 164777f..d5e6a35 100644 --- a/openedx_user_groups/criteria_types.py +++ b/openedx_user_groups/criteria_types.py @@ -12,7 +12,7 @@ from typing import Any, Dict, List, Optional, Type import attr -from django.db.models import Q, QuerySet +from django.db.models import Exists, OuterRef, Q, QuerySet from django.utils import timezone from openedx_events.learning.data import UserData from openedx_events.learning.signals import ( @@ -21,7 +21,7 @@ SESSION_LOGIN_COMPLETED, ) from openedx_events.tooling import OpenEdxPublicSignal -from pydantic import BaseModel +from pydantic import BaseModel, Field from openedx_user_groups.backends import BackendClient from openedx_user_groups.criteria import BaseCriterionType, ComparisonOperator @@ -38,7 +38,18 @@ class ManualCriterion(BaseCriterionType): ) class ConfigModel(BaseModel): - usernames_or_emails: List[str] # Usernames or emails + """Configuration model for manual criterion.""" + + model_config = { + "title": "Manual Criterion Configuration", + "description": "Configuration for manually specifying users by username or email", + } + + usernames_or_emails: List[str] = Field( + description="List of usernames or email addresses to include in the group", + examples=[["user1", "user2@example.com", "user3"]], + min_length=1, + ) # Supported operators for this criterion type supported_operators: List[ComparisonOperator] = [ @@ -52,7 +63,7 @@ def evaluate(self) -> QuerySet: """ return self.backend_client.get_users( self.scope - ).filter( # Currently side-wide, but should be filtered by scope + ).filter( # TODO: Currently side-wide, but should be filtered by scope Q(username__in=self.criterion_config.usernames_or_emails) | Q(email__in=self.criterion_config.usernames_or_emails) ) @@ -69,8 +80,23 @@ class CourseEnrollmentCriterion(BaseCriterionType): # TODO: should we use a single criterion with multiple attributes to filter by: mode, enrollment date, etc.? This would be an example of how we could do it, instead of having multiple criteria with specific attributes? class ConfigModel(BaseModel): - mode: Optional[str] = None - enrollment_date: Optional[datetime] = None + """Configuration model for course enrollment criterion.""" + + model_config = { + "title": "Course Enrollment Criterion Configuration", + "description": "Configuration for filtering users based on their course enrollment details", + } + + mode: Optional[str] = Field( + default=None, + description="Enrollment mode to filter by (e.g., 'audit', 'verified', 'honor')", + examples=["audit", "verified", "honor"], + ) + enrollment_date: Optional[datetime] = Field( + default=None, + description="Filter users enrolled on or after this date", + examples=["2024-01-01T00:00:00Z"], + ) supported_operators: List[ComparisonOperator] = [ ComparisonOperator.IN, @@ -86,15 +112,21 @@ class ConfigModel(BaseModel): updated_by_events = [COURSE_ENROLLMENT_CREATED, COURSE_ENROLLMENT_CHANGED] def evaluate(self) -> QuerySet: - """ - Evaluate the criterion. - """ + """Evaluate the criterion and return users based on enrollment criteria.""" filters = {} if self.criterion_config.mode: filters["mode"] = self.criterion_config.mode if self.criterion_config.enrollment_date: filters["created__gte"] = self.criterion_config.enrollment_date - return self.backend_client.get_enrollments(self.scope).filter(**filters) + + # Use Exists() for better performance - single query with subquery + # TODO: needs to be tested for performance, always try enforcing a single query if possible. + enrollments_subquery = self.backend_client.get_enrollments(self.scope).filter( + user=OuterRef("pk"), **filters + ) + return self.backend_client.get_users(self.scope).filter( + Exists(enrollments_subquery) + ) class LastLoginCriterion(BaseCriterionType): @@ -107,7 +139,18 @@ class LastLoginCriterion(BaseCriterionType): ) class ConfigModel(BaseModel): - days: int # TODO: can we use a single criterion with multiple attributes to filter by: days, country, etc.? + """Configuration model for last login criterion.""" + + model_config = { + "title": "Last Login Criterion Configuration", + "description": "Configuration for filtering users based on their last login activity", + } + + days: int = Field( + description="Number of days since last login to use for comparison", + examples=[1, 7, 30, 90], + ge=0, + ) # TODO: can we use a single criterion with multiple attributes to filter by: days, country, etc.? supported_operators: List[ComparisonOperator] = [ ComparisonOperator.EQUAL, @@ -145,9 +188,7 @@ def evaluate(self) -> QuerySet: "last_login__" + queryset_operator_mapping[self.criterion_operator]: threshold_date } - return self.backend_client.get_users(self.scope).filter( - **query - ) # TODO: is it better to use Q objects instead? + return self.backend_client.get_users(self.scope).filter(**query) class EnrollmentModeCriterion(BaseCriterionType): @@ -185,7 +226,17 @@ class UserStaffStatusCriterion(BaseCriterionType): ) class ConfigModel(BaseModel): - is_staff: bool # True to filter for staff users, False for non-staff users + """Configuration model for user staff status criterion.""" + + model_config = { + "title": "User Staff Status Criterion Configuration", + "description": "Configuration for filtering users based on their staff status", + } + + is_staff: bool = Field( + description="Whether to filter for staff users (True) or non-staff users (False)", + examples=[True, False], + ) def evaluate(self) -> QuerySet: """Evaluate the criterion based on user staff status. diff --git a/openedx_user_groups/manager.py b/openedx_user_groups/manager.py index ce62a3b..d081706 100644 --- a/openedx_user_groups/manager.py +++ b/openedx_user_groups/manager.py @@ -46,6 +46,14 @@ def get_criterion_types(cls): # TODO: should be get_available_plugins(), but for now this is the closest we're going to implement. return OrderedDict(cls._criterion_registry) + @classmethod + def get_criterion_classes(cls): + """Return list of available criterion classes.""" + return { + criterion_type: cls.get_criterion_class_by_type(criterion_type) + for criterion_type in cls.get_criterion_types() + } + @classmethod def get_criterion_type_by_type(cls, criterion_type): """Return the criterion type module path for a given name.""" diff --git a/openedx_user_groups/migrations/0003_scope_reference_id.py b/openedx_user_groups/migrations/0003_scope_reference_id.py new file mode 100644 index 0000000..29742db --- /dev/null +++ b/openedx_user_groups/migrations/0003_scope_reference_id.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.21 on 2025-06-26 07:03 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("openedx_user_groups", "0002_groupcollection"), + ] + + operations = [ + migrations.AddField( + model_name="scope", + name="reference_id", + field=models.CharField(blank=True, max_length=255, null=True), + ), + ] diff --git a/openedx_user_groups/migrations/0004_alter_scope_object_id.py b/openedx_user_groups/migrations/0004_alter_scope_object_id.py new file mode 100644 index 0000000..8cb3c5d --- /dev/null +++ b/openedx_user_groups/migrations/0004_alter_scope_object_id.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.21 on 2025-06-26 10:33 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("openedx_user_groups", "0003_scope_reference_id"), + ] + + operations = [ + migrations.AlterField( + model_name="scope", + name="object_id", + field=models.CharField(max_length=255), + ), + ] diff --git a/openedx_user_groups/migrations/0005_remove_scope_reference_id.py b/openedx_user_groups/migrations/0005_remove_scope_reference_id.py new file mode 100644 index 0000000..1b80c79 --- /dev/null +++ b/openedx_user_groups/migrations/0005_remove_scope_reference_id.py @@ -0,0 +1,17 @@ +# Generated by Django 4.2.21 on 2025-06-26 10:42 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("openedx_user_groups", "0004_alter_scope_object_id"), + ] + + operations = [ + migrations.RemoveField( + model_name="scope", + name="reference_id", + ), + ] diff --git a/openedx_user_groups/models.py b/openedx_user_groups/models.py index 819c002..e5607e5 100644 --- a/openedx_user_groups/models.py +++ b/openedx_user_groups/models.py @@ -82,7 +82,7 @@ class Scope(models.Model): ) # TODO: should we use something like: display_name? description = models.TextField(blank=True, null=True) content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE) - object_id = models.PositiveIntegerField() + object_id = models.CharField(max_length=255) content_object = GenericForeignKey( "content_type", "object_id" ) # TODO: how can we display this in a nice way? diff --git a/openedx_user_groups/serializers.py b/openedx_user_groups/serializers.py new file mode 100644 index 0000000..3d3b001 --- /dev/null +++ b/openedx_user_groups/serializers.py @@ -0,0 +1,66 @@ +"""Serializers for the openedx_user_groups app.""" + +from django.contrib.auth import get_user_model +from rest_framework import serializers + +from openedx_user_groups.models import UserGroup + +User = get_user_model() + + +class UserSerializer(serializers.ModelSerializer): + + # TODO: Add more fields here, like is_staff, is_active, etc. profile? + class Meta: + model = User + fields = ["username", "email", "first_name", "last_name"] + + +class ScopeSerializer(serializers.Serializer): + """Serializer for scope - handles both input and output.""" + + id = serializers.IntegerField(read_only=True) + name = serializers.CharField() + description = serializers.CharField(required=False, allow_null=True) + content_type = serializers.SerializerMethodField() + content_object = serializers.DictField(write_only=True, required=False) + + def get_content_type(self, obj): + """Get simplified content type information for output.""" + return { + "object_id": obj.object_id, + "content_type": str(obj.content_type), + } + + +class CriterionSerializer(serializers.Serializer): + """Serializer for criterion - handles both input and output.""" + + criterion_type = serializers.CharField() + criterion_operator = serializers.CharField() + criterion_config = serializers.DictField() + + +class UserGroupSerializer(serializers.ModelSerializer): + """Serializer for user group - handles both input and output.""" + + scope = ScopeSerializer() + criteria = CriterionSerializer(many=True, required=False) + users = UserSerializer(many=True, read_only=True) + evaluate_immediately = serializers.BooleanField( + write_only=True, required=False, default=False + ) + + class Meta: + model = UserGroup + fields = [ + "id", + "name", + "description", + "enabled", + "scope", + "last_membership_change", + "criteria", + "users", + "evaluate_immediately", + ] diff --git a/openedx_user_groups/urls.py b/openedx_user_groups/urls.py index 97fe533..f8e1910 100644 --- a/openedx_user_groups/urls.py +++ b/openedx_user_groups/urls.py @@ -2,10 +2,19 @@ URLs for openedx_user_groups. """ -from django.urls import re_path # pylint: disable=unused-import -from django.views.generic import TemplateView # pylint: disable=unused-import +from django.urls import include, re_path +from rest_framework.routers import DefaultRouter + +from openedx_user_groups.views import AvailableCriteriaView, UserGroupViewSet + +router = DefaultRouter() +router.register(r"user-groups", UserGroupViewSet, basename="usergroup") urlpatterns = [ - # TODO: Fill in URL patterns and views here. - # re_path(r'', TemplateView.as_view(template_name="openedx_user_groups/base.html")), + re_path( + r"^available-criteria/$", + AvailableCriteriaView.as_view(), + name="available-criteria", + ), + re_path(r"^", include(router.urls)), ] diff --git a/openedx_user_groups/utils.py b/openedx_user_groups/utils.py index 418c9c4..8c87302 100644 --- a/openedx_user_groups/utils.py +++ b/openedx_user_groups/utils.py @@ -1,5 +1,16 @@ """Utility functions for the openedx_user_groups app.""" +from django.conf import settings +from django.contrib.contenttypes.models import ContentType +from organizations.models import Organization + +try: + from opaque_keys.edx.keys import CourseKey + from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +except ImportError: + CourseKey = None + CourseOverview = None + def get_scope_type_from_content_type(content_type): """ @@ -13,10 +24,50 @@ def get_scope_type_from_content_type(content_type): """ # Mapping from Django model names to scope types model_to_scope_mapping = { - "course": "course", # When we have actual course models "courseoverview": "course", # edx-platform course overview model "organization": "organization", # Organization models } model_name = content_type.model return model_to_scope_mapping.get(model_name, "instance") # Default to instance + + +def process_content_object(content_object_data): + """ + Process content_object data to get the correct ContentType and object_id. + + Args: + content_object_data: Dict with content_type_model and object_id + + Returns: + tuple: (ContentType, object_id) + """ + content_type_model = content_object_data["content_type_model"] + object_id = content_object_data["object_id"] + + if content_type_model == "courseoverview": + # Validate course exists and use course key directly + course_key = CourseKey.from_string(object_id) + CourseOverview.get_from_id(course_key) # Validates existence + return ( + ContentType.objects.get( + app_label="course_overviews", model="courseoverview" + ), + object_id, + ) + elif content_type_model == "organization": + # Validate organization exists and use short_name directly + Organization.objects.get(short_name=object_id) # Validates existence + return ( + ContentType.objects.get(app_label="organizations", model="organization"), + object_id, + ) + else: + # Generic case - assume app_label equals content_type_model + # TODO: this needs support for what we're considering to be instance-level scope. + return ( + ContentType.objects.get( + app_label=content_type_model, model=content_type_model + ), + object_id, + ) diff --git a/openedx_user_groups/views.py b/openedx_user_groups/views.py index e69de29..53650bc 100644 --- a/openedx_user_groups/views.py +++ b/openedx_user_groups/views.py @@ -0,0 +1,67 @@ +"""REST API to manage user groups, their criteria and membership.""" + +from rest_framework import status +from rest_framework.decorators import action +from rest_framework.mixins import CreateModelMixin, ListModelMixin, RetrieveModelMixin +from rest_framework.response import Response +from rest_framework.views import APIView +from rest_framework.viewsets import GenericViewSet, ModelViewSet + +from openedx_user_groups.api import ( + create_group_with_criteria, + create_group_with_criteria_and_evaluate_membership, + get_available_registered_criteria_schema, + get_group_by_id, +) +from openedx_user_groups.models import UserGroup +from openedx_user_groups.serializers import UserGroupSerializer + + +class AvailableCriteriaView(APIView): + """View to get all available criteria with their schema for fields, operators and descriptions.""" + + def get(self, request): + """Get all available criteria with their schema for fields, operators and descriptions.""" + criteria_data = get_available_registered_criteria_schema() + return Response(criteria_data, status=status.HTTP_200_OK) + + +class UserGroupViewSet( + ListModelMixin, RetrieveModelMixin, CreateModelMixin, GenericViewSet +): + """ViewSet for user group operations using DRF mixins.""" + + queryset = UserGroup.objects.all() + serializer_class = UserGroupSerializer + + def create(self, request, *args, **kwargs): + """Create a new user group with criteria.""" + serializer = self.get_serializer(data=request.data) + serializer.is_valid(raise_exception=True) + + data = serializer.validated_data + + if data.get("evaluate_immediately", False): + user_group = create_group_with_criteria_and_evaluate_membership( + name=data["name"], + description=data.get("description", ""), + scope=data["scope"], + criteria=data.get("criteria", []), + ) + else: + user_group = create_group_with_criteria( + name=data["name"], + description=data.get("description", ""), + scope=data["scope"], + criteria=data.get("criteria", []), + ) + + serializer = UserGroupSerializer(user_group) + return Response(serializer.data, status=status.HTTP_201_CREATED) + + def retrieve(self, request, *args, **kwargs): + """Get a specific user group by ID.""" + group_id = kwargs.get("pk") + user_group = get_group_by_id(group_id) + serializer = UserGroupSerializer(user_group) + return Response(serializer.data, status=status.HTTP_200_OK) diff --git a/requirements/base.in b/requirements/base.in index 76f0033..4ff44c7 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -7,3 +7,6 @@ edx-django-utils # edX utilities for Django openedx-atlas openedx-events # Open edX Events library for updating user groups celery # Celery for background tasks +djangorestframework +edx-organizations +pydantic diff --git a/requirements/base.txt b/requirements/base.txt index 9cb33fe..4b5f252 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -6,6 +6,8 @@ # amqp==5.3.1 # via kombu +annotated-types==0.7.0 + # via pydantic asgiref==3.8.1 # via django attrs==25.3.0 @@ -14,8 +16,14 @@ billiard==4.2.1 # via celery celery==5.5.3 # via -r requirements/base.in +certifi==2025.6.15 + # via requests cffi==1.17.1 - # via pynacl + # via + # cryptography + # pynacl +charset-normalizer==3.4.2 + # via requests click==8.2.1 # via # celery @@ -29,32 +37,64 @@ click-plugins==1.1.1 # via celery click-repl==0.3.0 # via celery +cryptography==45.0.4 + # via pyjwt django==4.2.21 # via # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt # -r requirements/base.in # django-crum + # django-model-utils # django-waffle + # djangorestframework + # drf-jwt # edx-django-utils + # edx-drf-extensions + # edx-organizations # openedx-events django-crum==0.7.9 # via edx-django-utils +django-model-utils==5.0.0 + # via edx-organizations +django-simple-history==3.0.0 + # via + # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt + # edx-organizations django-waffle==4.2.0 - # via edx-django-utils + # via + # edx-django-utils + # edx-drf-extensions +djangorestframework==3.16.0 + # via + # -r requirements/base.in + # drf-jwt + # edx-drf-extensions + # edx-organizations dnspython==2.7.0 # via pymongo +drf-jwt==1.19.2 + # via edx-drf-extensions edx-ccx-keys==2.0.2 # via openedx-events edx-django-utils==8.0.0 # via # -r requirements/base.in + # edx-drf-extensions # openedx-events +edx-drf-extensions==10.6.0 + # via edx-organizations edx-opaque-keys[django]==3.0.0 # via # edx-ccx-keys + # edx-drf-extensions + # edx-organizations # openedx-events +edx-organizations==7.1.0 + # via -r requirements/base.in fastavro==1.11.1 # via openedx-events +idna==3.10 + # via requests kombu==5.5.4 # via celery openedx-atlas==0.7.0 @@ -65,18 +105,32 @@ packaging==25.0 # via kombu pbr==6.1.1 # via stevedore +pillow==11.2.1 + # via edx-organizations prompt-toolkit==3.0.51 # via click-repl psutil==7.0.0 # via edx-django-utils pycparser==2.22 # via cffi +pydantic==2.11.7 + # via -r requirements/base.in +pydantic-core==2.33.2 + # via pydantic +pyjwt[crypto]==2.10.1 + # via + # drf-jwt + # edx-drf-extensions pymongo==4.13.1 # via edx-opaque-keys pynacl==1.5.0 # via edx-django-utils python-dateutil==2.9.0.post0 # via celery +requests==2.32.4 + # via edx-drf-extensions +semantic-version==2.10.0 + # via edx-drf-extensions six==1.17.0 # via # edx-ccx-keys @@ -88,9 +142,19 @@ stevedore==5.4.1 # edx-django-utils # edx-opaque-keys typing-extensions==4.14.0 - # via edx-opaque-keys + # via + # edx-opaque-keys + # pydantic + # pydantic-core + # typing-inspection +typing-inspection==0.4.1 + # via pydantic tzdata==2025.2 # via kombu +urllib3==2.2.3 + # via + # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt + # requests vine==5.1.0 # via # amqp diff --git a/requirements/dev.txt b/requirements/dev.txt index c6869df..c4b79c2 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -29,6 +29,8 @@ billiard==4.2.1 # via # -r requirements/quality.txt # celery +black==25.1.0 + # via -r requirements/quality.txt build==1.2.2.post1 # via # -r requirements/pip-tools.txt @@ -39,19 +41,29 @@ cachetools==6.0.0 # tox celery==5.5.3 # via -r requirements/quality.txt +certifi==2025.6.15 + # via + # -r requirements/quality.txt + # requests cffi==1.17.1 # via # -r requirements/quality.txt + # cryptography # pynacl chardet==5.2.0 # via # -r requirements/ci.txt # diff-cover # tox +charset-normalizer==3.4.2 + # via + # -r requirements/quality.txt + # requests click==8.2.1 # via # -r requirements/pip-tools.txt # -r requirements/quality.txt + # black # celery # click-didyoumean # click-log @@ -89,6 +101,10 @@ coverage[toml]==7.8.2 # via # -r requirements/quality.txt # pytest-cov +cryptography==45.0.4 + # via + # -r requirements/quality.txt + # pyjwt ddt==1.7.2 # via -r requirements/quality.txt diff-cover==9.3.1 @@ -106,22 +122,47 @@ django==4.2.21 # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt # -r requirements/quality.txt # django-crum + # django-model-utils # django-waffle + # djangorestframework + # drf-jwt # edx-django-utils + # edx-drf-extensions # edx-i18n-tools + # edx-organizations # openedx-events django-crum==0.7.9 # via # -r requirements/quality.txt # edx-django-utils +django-model-utils==5.0.0 + # via + # -r requirements/quality.txt + # edx-organizations +django-simple-history==3.0.0 + # via + # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt + # -r requirements/quality.txt + # edx-organizations django-waffle==4.2.0 # via # -r requirements/quality.txt # edx-django-utils + # edx-drf-extensions +djangorestframework==3.16.0 + # via + # -r requirements/quality.txt + # drf-jwt + # edx-drf-extensions + # edx-organizations dnspython==2.7.0 # via # -r requirements/quality.txt # pymongo +drf-jwt==1.19.2 + # via + # -r requirements/quality.txt + # edx-drf-extensions edx-ccx-keys==2.0.2 # via # -r requirements/quality.txt @@ -129,7 +170,12 @@ edx-ccx-keys==2.0.2 edx-django-utils==8.0.0 # via # -r requirements/quality.txt + # edx-drf-extensions # openedx-events +edx-drf-extensions==10.6.0 + # via + # -r requirements/quality.txt + # edx-organizations edx-i18n-tools==1.9.0 # via -r requirements/dev.in edx-lint==5.6.0 @@ -138,7 +184,11 @@ edx-opaque-keys[django]==3.0.0 # via # -r requirements/quality.txt # edx-ccx-keys + # edx-drf-extensions + # edx-organizations # openedx-events +edx-organizations==7.1.0 + # via -r requirements/quality.txt factory-boy==3.3.3 # via -r requirements/quality.txt faker==37.3.0 @@ -154,6 +204,10 @@ filelock==3.18.0 # -r requirements/ci.txt # tox # virtualenv +idna==3.10 + # via + # -r requirements/quality.txt + # requests iniconfig==2.1.0 # via # -r requirements/quality.txt @@ -185,6 +239,10 @@ mccabe==0.7.0 # via # -r requirements/quality.txt # pylint +mypy-extensions==1.1.0 + # via + # -r requirements/quality.txt + # black openedx-atlas==0.7.0 # via -r requirements/quality.txt openedx-events==10.2.1 @@ -194,6 +252,7 @@ packaging==25.0 # -r requirements/ci.txt # -r requirements/pip-tools.txt # -r requirements/quality.txt + # black # build # kombu # pyproject-api @@ -201,16 +260,25 @@ packaging==25.0 # tox path==16.16.0 # via edx-i18n-tools +pathspec==0.12.1 + # via + # -r requirements/quality.txt + # black pbr==6.1.1 # via # -r requirements/quality.txt # stevedore +pillow==11.2.1 + # via + # -r requirements/quality.txt + # edx-organizations pip-tools==7.4.1 # via -r requirements/pip-tools.txt platformdirs==4.3.8 # via # -r requirements/ci.txt # -r requirements/quality.txt + # black # pylint # tox # virtualenv @@ -237,7 +305,7 @@ pycparser==2.22 # via # -r requirements/quality.txt # cffi -pydantic==2.11.5 +pydantic==2.11.7 # via -r requirements/quality.txt pydantic-core==2.33.2 # via @@ -247,6 +315,11 @@ pydocstyle==6.3.0 # via -r requirements/quality.txt pygments==2.19.1 # via diff-cover +pyjwt[crypto]==2.10.1 + # via + # -r requirements/quality.txt + # drf-jwt + # edx-drf-extensions pylint==3.3.7 # via # -r requirements/quality.txt @@ -306,6 +379,14 @@ pyyaml==6.0.2 # -r requirements/quality.txt # code-annotations # edx-i18n-tools +requests==2.32.4 + # via + # -r requirements/quality.txt + # edx-drf-extensions +semantic-version==2.10.0 + # via + # -r requirements/quality.txt + # edx-drf-extensions six==1.17.0 # via # -r requirements/quality.txt @@ -352,6 +433,11 @@ tzdata==2025.2 # -r requirements/quality.txt # faker # kombu +urllib3==2.2.3 + # via + # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt + # -r requirements/quality.txt + # requests vine==5.1.0 # via # -r requirements/quality.txt diff --git a/requirements/doc.txt b/requirements/doc.txt index 328c654..5a37b8a 100644 --- a/requirements/doc.txt +++ b/requirements/doc.txt @@ -12,6 +12,10 @@ amqp==5.3.1 # via # -r requirements/test.txt # kombu +annotated-types==0.7.0 + # via + # -r requirements/test.txt + # pydantic asgiref==3.8.1 # via # -r requirements/test.txt @@ -34,15 +38,19 @@ build==1.2.2.post1 # via -r requirements/doc.in celery==5.5.3 # via -r requirements/test.txt -certifi==2025.4.26 - # via requests +certifi==2025.6.15 + # via + # -r requirements/test.txt + # requests cffi==1.17.1 # via # -r requirements/test.txt # cryptography # pynacl charset-normalizer==3.4.2 - # via requests + # via + # -r requirements/test.txt + # requests click==8.2.1 # via # -r requirements/test.txt @@ -70,8 +78,11 @@ coverage[toml]==7.8.2 # via # -r requirements/test.txt # pytest-cov -cryptography==45.0.3 - # via secretstorage +cryptography==45.0.4 + # via + # -r requirements/test.txt + # pyjwt + # secretstorage ddt==1.7.2 # via -r requirements/test.txt django==4.2.21 @@ -79,17 +90,38 @@ django==4.2.21 # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt # -r requirements/test.txt # django-crum + # django-model-utils # django-waffle + # djangorestframework + # drf-jwt # edx-django-utils + # edx-drf-extensions + # edx-organizations # openedx-events django-crum==0.7.9 # via # -r requirements/test.txt # edx-django-utils +django-model-utils==5.0.0 + # via + # -r requirements/test.txt + # edx-organizations +django-simple-history==3.0.0 + # via + # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt + # -r requirements/test.txt + # edx-organizations django-waffle==4.2.0 # via # -r requirements/test.txt # edx-django-utils + # edx-drf-extensions +djangorestframework==3.16.0 + # via + # -r requirements/test.txt + # drf-jwt + # edx-drf-extensions + # edx-organizations dnspython==2.7.0 # via # -r requirements/test.txt @@ -103,6 +135,10 @@ docutils==0.21.2 # readme-renderer # restructuredtext-lint # sphinx +drf-jwt==1.19.2 + # via + # -r requirements/test.txt + # edx-drf-extensions edx-ccx-keys==2.0.2 # via # -r requirements/test.txt @@ -110,12 +146,21 @@ edx-ccx-keys==2.0.2 edx-django-utils==8.0.0 # via # -r requirements/test.txt + # edx-drf-extensions # openedx-events +edx-drf-extensions==10.6.0 + # via + # -r requirements/test.txt + # edx-organizations edx-opaque-keys[django]==3.0.0 # via # -r requirements/test.txt # edx-ccx-keys + # edx-drf-extensions + # edx-organizations # openedx-events +edx-organizations==7.1.0 + # via -r requirements/test.txt factory-boy==3.3.3 # via -r requirements/test.txt faker==37.3.0 @@ -129,7 +174,9 @@ fastavro==1.11.1 id==1.5.0 # via twine idna==3.10 - # via requests + # via + # -r requirements/test.txt + # requests imagesize==1.4.1 # via sphinx iniconfig==2.1.0 @@ -188,6 +235,10 @@ pbr==6.1.1 # via # -r requirements/test.txt # stevedore +pillow==11.2.1 + # via + # -r requirements/test.txt + # edx-organizations pluggy==1.6.0 # via # -r requirements/test.txt @@ -204,6 +255,12 @@ pycparser==2.22 # via # -r requirements/test.txt # cffi +pydantic==2.11.7 + # via -r requirements/test.txt +pydantic-core==2.33.2 + # via + # -r requirements/test.txt + # pydantic pydata-sphinx-theme==0.15.4 # via sphinx-book-theme pygments==2.19.1 @@ -214,6 +271,11 @@ pygments==2.19.1 # readme-renderer # rich # sphinx +pyjwt[crypto]==2.10.1 + # via + # -r requirements/test.txt + # drf-jwt + # edx-drf-extensions pymongo==4.13.1 # via # -r requirements/test.txt @@ -247,8 +309,10 @@ pyyaml==6.0.2 # code-annotations readme-renderer==44.0 # via twine -requests==2.32.3 +requests==2.32.4 # via + # -r requirements/test.txt + # edx-drf-extensions # id # requests-toolbelt # sphinx @@ -265,6 +329,10 @@ roman-numerals-py==3.1.0 # via sphinx secretstorage==3.3.3 # via keyring +semantic-version==2.10.0 + # via + # -r requirements/test.txt + # edx-drf-extensions six==1.17.0 # via # -r requirements/test.txt @@ -315,7 +383,14 @@ typing-extensions==4.14.0 # -r requirements/test.txt # beautifulsoup4 # edx-opaque-keys + # pydantic + # pydantic-core # pydata-sphinx-theme + # typing-inspection +typing-inspection==0.4.1 + # via + # -r requirements/test.txt + # pydantic tzdata==2025.2 # via # -r requirements/test.txt @@ -324,6 +399,7 @@ tzdata==2025.2 urllib3==2.2.3 # via # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt + # -r requirements/test.txt # requests # twine vine==5.1.0 diff --git a/requirements/quality.in b/requirements/quality.in index 1a56d7f..0c7008a 100644 --- a/requirements/quality.in +++ b/requirements/quality.in @@ -8,4 +8,5 @@ edx-lint # edX pylint rules and plugins isort # to standardize order of imports pycodestyle # PEP 8 compliance validation pydocstyle # PEP 257 compliance validation -pydantic # static type checking +pydantic +black diff --git a/requirements/quality.txt b/requirements/quality.txt index 2d6f499..6a5097c 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -9,7 +9,9 @@ amqp==5.3.1 # -r requirements/test.txt # kombu annotated-types==0.7.0 - # via pydantic + # via + # -r requirements/test.txt + # pydantic asgiref==3.8.1 # via # -r requirements/test.txt @@ -26,15 +28,27 @@ billiard==4.2.1 # via # -r requirements/test.txt # celery +black==25.1.0 + # via -r requirements/quality.in celery==5.5.3 # via -r requirements/test.txt +certifi==2025.6.15 + # via + # -r requirements/test.txt + # requests cffi==1.17.1 # via # -r requirements/test.txt + # cryptography # pynacl +charset-normalizer==3.4.2 + # via + # -r requirements/test.txt + # requests click==8.2.1 # via # -r requirements/test.txt + # black # celery # click-didyoumean # click-log @@ -65,6 +79,10 @@ coverage[toml]==7.8.2 # via # -r requirements/test.txt # pytest-cov +cryptography==45.0.4 + # via + # -r requirements/test.txt + # pyjwt ddt==1.7.2 # via -r requirements/test.txt dill==0.4.0 @@ -74,21 +92,46 @@ django==4.2.21 # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt # -r requirements/test.txt # django-crum + # django-model-utils # django-waffle + # djangorestframework + # drf-jwt # edx-django-utils + # edx-drf-extensions + # edx-organizations # openedx-events django-crum==0.7.9 # via # -r requirements/test.txt # edx-django-utils +django-model-utils==5.0.0 + # via + # -r requirements/test.txt + # edx-organizations +django-simple-history==3.0.0 + # via + # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt + # -r requirements/test.txt + # edx-organizations django-waffle==4.2.0 # via # -r requirements/test.txt # edx-django-utils + # edx-drf-extensions +djangorestframework==3.16.0 + # via + # -r requirements/test.txt + # drf-jwt + # edx-drf-extensions + # edx-organizations dnspython==2.7.0 # via # -r requirements/test.txt # pymongo +drf-jwt==1.19.2 + # via + # -r requirements/test.txt + # edx-drf-extensions edx-ccx-keys==2.0.2 # via # -r requirements/test.txt @@ -96,14 +139,23 @@ edx-ccx-keys==2.0.2 edx-django-utils==8.0.0 # via # -r requirements/test.txt + # edx-drf-extensions # openedx-events +edx-drf-extensions==10.6.0 + # via + # -r requirements/test.txt + # edx-organizations edx-lint==5.6.0 # via -r requirements/quality.in edx-opaque-keys[django]==3.0.0 # via # -r requirements/test.txt # edx-ccx-keys + # edx-drf-extensions + # edx-organizations # openedx-events +edx-organizations==7.1.0 + # via -r requirements/test.txt factory-boy==3.3.3 # via -r requirements/test.txt faker==37.3.0 @@ -114,6 +166,10 @@ fastavro==1.11.1 # via # -r requirements/test.txt # openedx-events +idna==3.10 + # via + # -r requirements/test.txt + # requests iniconfig==2.1.0 # via # -r requirements/test.txt @@ -136,6 +192,8 @@ markupsafe==3.0.2 # jinja2 mccabe==0.7.0 # via pylint +mypy-extensions==1.1.0 + # via black openedx-atlas==0.7.0 # via -r requirements/test.txt openedx-events==10.2.1 @@ -143,14 +201,23 @@ openedx-events==10.2.1 packaging==25.0 # via # -r requirements/test.txt + # black # kombu # pytest +pathspec==0.12.1 + # via black pbr==6.1.1 # via # -r requirements/test.txt # stevedore +pillow==11.2.1 + # via + # -r requirements/test.txt + # edx-organizations platformdirs==4.3.8 - # via pylint + # via + # black + # pylint pluggy==1.6.0 # via # -r requirements/test.txt @@ -169,12 +236,21 @@ pycparser==2.22 # via # -r requirements/test.txt # cffi -pydantic==2.11.5 - # via -r requirements/quality.in +pydantic==2.11.7 + # via + # -r requirements/quality.in + # -r requirements/test.txt pydantic-core==2.33.2 - # via pydantic + # via + # -r requirements/test.txt + # pydantic pydocstyle==6.3.0 # via -r requirements/quality.in +pyjwt[crypto]==2.10.1 + # via + # -r requirements/test.txt + # drf-jwt + # edx-drf-extensions pylint==3.3.7 # via # edx-lint @@ -218,6 +294,14 @@ pyyaml==6.0.2 # via # -r requirements/test.txt # code-annotations +requests==2.32.4 + # via + # -r requirements/test.txt + # edx-drf-extensions +semantic-version==2.10.0 + # via + # -r requirements/test.txt + # edx-drf-extensions six==1.17.0 # via # -r requirements/test.txt @@ -250,12 +334,19 @@ typing-extensions==4.14.0 # pydantic-core # typing-inspection typing-inspection==0.4.1 - # via pydantic + # via + # -r requirements/test.txt + # pydantic tzdata==2025.2 # via # -r requirements/test.txt # faker # kombu +urllib3==2.2.3 + # via + # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt + # -r requirements/test.txt + # requests vine==5.1.0 # via # -r requirements/test.txt diff --git a/requirements/test.txt b/requirements/test.txt index 0ee3e7d..3c90eca 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -8,6 +8,10 @@ amqp==5.3.1 # via # -r requirements/base.txt # kombu +annotated-types==0.7.0 + # via + # -r requirements/base.txt + # pydantic asgiref==3.8.1 # via # -r requirements/base.txt @@ -22,10 +26,19 @@ billiard==4.2.1 # celery celery==5.5.3 # via -r requirements/base.txt +certifi==2025.6.15 + # via + # -r requirements/base.txt + # requests cffi==1.17.1 # via # -r requirements/base.txt + # cryptography # pynacl +charset-normalizer==3.4.2 + # via + # -r requirements/base.txt + # requests click==8.2.1 # via # -r requirements/base.txt @@ -51,27 +64,56 @@ code-annotations==2.3.0 # via -r requirements/test.in coverage[toml]==7.8.2 # via pytest-cov +cryptography==45.0.4 + # via + # -r requirements/base.txt + # pyjwt ddt==1.7.2 # via -r requirements/test.in # via # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt # -r requirements/base.txt # django-crum + # django-model-utils # django-waffle + # djangorestframework + # drf-jwt # edx-django-utils + # edx-drf-extensions + # edx-organizations # openedx-events django-crum==0.7.9 # via # -r requirements/base.txt # edx-django-utils +django-model-utils==5.0.0 + # via + # -r requirements/base.txt + # edx-organizations +django-simple-history==3.0.0 + # via + # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt + # -r requirements/base.txt + # edx-organizations django-waffle==4.2.0 # via # -r requirements/base.txt # edx-django-utils + # edx-drf-extensions +djangorestframework==3.16.0 + # via + # -r requirements/base.txt + # drf-jwt + # edx-drf-extensions + # edx-organizations dnspython==2.7.0 # via # -r requirements/base.txt # pymongo +drf-jwt==1.19.2 + # via + # -r requirements/base.txt + # edx-drf-extensions edx-ccx-keys==2.0.2 # via # -r requirements/base.txt @@ -79,12 +121,21 @@ edx-ccx-keys==2.0.2 edx-django-utils==8.0.0 # via # -r requirements/base.txt + # edx-drf-extensions # openedx-events +edx-drf-extensions==10.6.0 + # via + # -r requirements/base.txt + # edx-organizations edx-opaque-keys[django]==3.0.0 # via # -r requirements/base.txt # edx-ccx-keys + # edx-drf-extensions + # edx-organizations # openedx-events +edx-organizations==7.1.0 + # via -r requirements/base.txt factory-boy==3.3.3 # via -r requirements/test.in faker==37.3.0 @@ -95,6 +146,10 @@ fastavro==1.11.1 # via # -r requirements/base.txt # openedx-events +idna==3.10 + # via + # -r requirements/base.txt + # requests iniconfig==2.1.0 # via pytest jinja2==3.1.6 @@ -118,6 +173,10 @@ pbr==6.1.1 # via # -r requirements/base.txt # stevedore +pillow==11.2.1 + # via + # -r requirements/base.txt + # edx-organizations pluggy==1.6.0 # via pytest prompt-toolkit==3.0.51 @@ -132,6 +191,17 @@ pycparser==2.22 # via # -r requirements/base.txt # cffi +pydantic==2.11.7 + # via -r requirements/base.txt +pydantic-core==2.33.2 + # via + # -r requirements/base.txt + # pydantic +pyjwt[crypto]==2.10.1 + # via + # -r requirements/base.txt + # drf-jwt + # edx-drf-extensions pymongo==4.13.1 # via # -r requirements/base.txt @@ -156,6 +226,14 @@ python-slugify==8.0.4 # via code-annotations pyyaml==6.0.2 # via code-annotations +requests==2.32.4 + # via + # -r requirements/base.txt + # edx-drf-extensions +semantic-version==2.10.0 + # via + # -r requirements/base.txt + # edx-drf-extensions six==1.17.0 # via # -r requirements/base.txt @@ -177,11 +255,23 @@ typing-extensions==4.14.0 # via # -r requirements/base.txt # edx-opaque-keys + # pydantic + # pydantic-core + # typing-inspection +typing-inspection==0.4.1 + # via + # -r requirements/base.txt + # pydantic tzdata==2025.2 # via # -r requirements/base.txt # faker # kombu +urllib3==2.2.3 + # via + # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt + # -r requirements/base.txt + # requests vine==5.1.0 # via # -r requirements/base.txt diff --git a/setup.py b/setup.py index d821e04..0ca303b 100755 --- a/setup.py +++ b/setup.py @@ -161,7 +161,7 @@ def is_requirement(line): ), include_package_data=True, install_requires=load_requirements("requirements/base.in"), - python_requires=">=3.12", + python_requires=">=3.11", license="AGPL 3.0", zip_safe=False, keywords="Python edx", @@ -174,6 +174,11 @@ def is_requirement(line): "License :: OSI Approved :: GNU Affero General Public License v3 or later (AGPLv3+)", "Natural Language :: English", "Programming Language :: Python :: 3", - "Programming Language :: Python :: 3.12", + "Programming Language :: Python :: 3.11", ], + entry_points={ + "lms.djangoapp": [ + "openedx_user_groups = openedx_user_groups.apps:OpenedxUserGroupsConfig", + ], + }, ) diff --git a/test_settings.py b/test_settings.py index 036995b..08b79d1 100644 --- a/test_settings.py +++ b/test_settings.py @@ -33,6 +33,7 @@ def root(*args): "django.contrib.messages", "django.contrib.sessions", "openedx_user_groups", + "organizations", ) LOCALE_PATHS = [ @@ -62,3 +63,5 @@ def root(*args): }, } ] + +LMS_BASE = "local.openedx.io" diff --git a/tests/test_api.py b/tests/test_api.py index 23cd6a6..a90c4ae 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -12,6 +12,7 @@ from django.contrib.contenttypes.models import ContentType from django.utils import timezone from datetime import timedelta +from organizations.models import Organization from tests.factories import * from openedx_user_groups.api import * from openedx_user_groups.models import UserGroup, Scope, Criterion @@ -23,21 +24,41 @@ class UserGroupAPITestCase(TestCase): def setUpTestData(cls): """Set up test data that will be reused across all test methods.""" cls.test_course = CourseFactory() - cls.course_content_type = ContentType.objects.get_for_model(User) - cls.test_scope = ScopeFactory( - name=cls.test_course["name"], - content_type=cls.course_content_type, - object_id=cls.test_course["id"], + + # Create a test organization + cls.test_organization = Organization.objects.create( + name="Test Organization", + short_name="TestOrg", + description="A test organization for user groups", + ) + cls.organization_content_type = ContentType.objects.get_for_model(Organization) + + # Create organization-level scope + cls.test_org_scope = ScopeFactory( + name="Test Organization Scope", + content_type=cls.organization_content_type, + object_id=cls.test_organization.id, + ) + + # Create instance-level scope (using User model as placeholder for instance) + cls.user_content_type = ContentType.objects.get_for_model(User) + cls.test_instance_scope = ScopeFactory( + name="Test Instance Scope", + content_type=cls.user_content_type, + object_id=1, # Arbitrary ID for instance level ) + cls.test_user_group_data = UserGroupFactory.build(name="At Risk Students") cls.last_login_criterion = LastLoginCriterionFactory.build() cls.enrollment_mode_criterion = EnrollmentModeCriterionFactory.build() cls.user_staff_status_criterion = UserStaffStatusCriterionFactory.build() - cls.scope_context = { - "name": cls.test_course["name"], + + # Organization scope + cls.org_scope = { + "name": "Test Organization Scope", "content_object": { - "content_type": cls.course_content_type, - "object_id": cls.test_course["id"], + "content_type_model": "organization", + "object_id": cls.test_organization.short_name, }, } @@ -57,13 +78,13 @@ def test_create_group_with_no_criteria(self): user_group, scope = get_or_create_group_and_scope( name=self.test_user_group_data.name, description=self.test_user_group_data.description, - scope_context=self.scope_context, + scope=self.org_scope, ) assert user_group is not None assert user_group.name == self.test_user_group_data.name assert user_group.description == self.test_user_group_data.description - assert scope.name == self.test_scope.name + assert scope.name == self.test_org_scope.name assert user_group.criteria.count() == 0 def test_associate_multiple_groups_with_same_scope(self): @@ -77,16 +98,16 @@ def test_associate_multiple_groups_with_same_scope(self): user_group_1, scope_1 = get_or_create_group_and_scope( name=f"{self.test_user_group_data.name}_1", description=self.test_user_group_data.description, - scope_context=self.scope_context, + scope=self.org_scope, ) user_group_2, scope_2 = get_or_create_group_and_scope( name=f"{self.test_user_group_data.name}_2", description=self.test_user_group_data.description, - scope_context=self.scope_context, + scope=self.org_scope, ) - assert scope_1.name == self.test_scope.name - assert scope_2.name == self.test_scope.name + assert scope_1.name == self.test_org_scope.name + assert scope_2.name == self.test_org_scope.name assert scope_1.name == scope_2.name def test_create_group_with_single_criterion(self): @@ -100,8 +121,8 @@ def test_create_group_with_single_criterion(self): user_group = create_group_with_criteria( name=self.test_user_group_data.name, description=self.test_user_group_data.description, - scope_context=self.scope_context, - criterion_data=[ + scope=self.org_scope, + criteria=[ { "criterion_type": self.last_login_criterion.criterion_type, "criterion_operator": self.last_login_criterion.criterion_operator, @@ -129,8 +150,8 @@ def test_create_group_with_multiple_criteria_invalid_scope(self): create_group_with_criteria( name=self.test_user_group_data.name, description=self.test_user_group_data.description, - scope_context=self.scope_context, - criterion_data=[ + scope=self.org_scope, + criteria=[ { "criterion_type": self.last_login_criterion.criterion_type, "criterion_operator": self.last_login_criterion.criterion_operator, @@ -160,14 +181,8 @@ def test_create_group_with_multiple_criteria_valid_scope(self): user_group = create_group_with_criteria( name=self.test_user_group_data.name, description=self.test_user_group_data.description, - scope_context={ - "name": self.test_course["name"], - "content_object": { - "content_type": self.course_content_type, - "object_id": self.test_course["id"], - }, - }, - criterion_data=[ + scope=self.org_scope, + criteria=[ { "criterion_type": self.last_login_criterion.criterion_type, "criterion_operator": self.last_login_criterion.criterion_operator, @@ -228,8 +243,8 @@ def test_create_group_with_criteria_and_evaluate_membership(self): user_group = create_group_with_criteria_and_evaluate_membership( name=self.test_user_group_data.name, description=self.test_user_group_data.description, - scope_context=self.scope_context, - criterion_data=[ # TODO: I'm worried about usability of this API. + scope=self.org_scope, + criteria=[ # TODO: I'm worried about usability of this API. { "criterion_type": self.last_login_criterion.criterion_type, "criterion_operator": self.last_login_criterion.criterion_operator, @@ -270,8 +285,8 @@ def test_evaluate_membership_for_multiple_groups(self): create_group_with_criteria( name=f"{self.test_user_group_data.name}_{i}", description=self.test_user_group_data.description, - scope_context=self.scope_context, - criterion_data=[ + scope=self.org_scope, + criteria=[ { "criterion_type": self.last_login_criterion.criterion_type, "criterion_operator": self.last_login_criterion.criterion_operator, @@ -306,8 +321,8 @@ def setUpTestData(cls): create_group_with_criteria( name=f"manual_group_{i}", description="Manual group description", - scope_context=cls.scope_context, - criterion_data=[ + scope=cls.org_scope, + criteria=[ { "criterion_type": ManualCriterionFactory.criterion_type, "criterion_operator": ManualCriterionFactory.criterion_operator, diff --git a/tests/test_models.py b/tests/test_models.py index 0a7f928..104c36d 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -4,12 +4,19 @@ """ import factory -from django.db import IntegrityError -from django.test import TestCase from django.contrib.auth import get_user_model from django.contrib.contenttypes.models import ContentType +from django.core.exceptions import ValidationError +from django.db import IntegrityError +from django.test import TestCase +from organizations.models import Organization + from openedx_user_groups.manager import CriterionManager -from openedx_user_groups.models import UserGroup, Criterion, Scope +from openedx_user_groups.models import Criterion, Scope, UserGroup +from tests.factories import ( + UserFactory, + UserGroupFactory, +) User = get_user_model() @@ -35,15 +42,21 @@ def setUpTestData(cls): # Create course data using the factory cls.test_course = CourseFactory() - # Use User model for ContentType (it has an existing table) - cls.course_content_type = ContentType.objects.get_for_model(User) + # Create a test organization instead of using User model + cls.test_organization = Organization.objects.create( + name="Test Organization", + short_name="TestOrg", + description="A test organization for user groups", + ) + cls.organization_content_type = ContentType.objects.get_for_model(Organization) cls.scope = Scope.objects.create( - name="Demo Course Scope", - description="Scope for the demo course", - content_type=cls.course_content_type, - object_id=cls.test_course["id"], + name="Test Organization Scope", + description="Scope for the test organization", + content_type=cls.organization_content_type, + object_id=cls.test_organization.id, ) + cls.user_group = UserGroup.objects.create( name="Test Group", description="A test group", scope=cls.scope ) @@ -63,13 +76,17 @@ def test_user_group_save_prevents_scope_change(self): - The group is not saved. - An exception is raised. """ - # Create another course data for the new scope - another_course = CourseFactory() + # Create another organization for the new scope + another_organization = Organization.objects.create( + name="Another Test Organization", + short_name="AnotherOrg", + description="Another test organization", + ) new_scope = Scope.objects.create( name="New Scope", description="Another scope", - content_type=self.course_content_type, - object_id=another_course["id"], + content_type=self.organization_content_type, + object_id=another_organization.id, ) self.user_group.scope = new_scope @@ -110,14 +127,18 @@ class TestCriterionMethods(TestCase): @classmethod def setUpTestData(cls): """Set up test data for Criterion tests.""" - # Create course data using the factory - cls.test_course = CourseFactory() - cls.course_content_type = ContentType.objects.get_for_model(User) + # Create a test organization + cls.test_organization = Organization.objects.create( + name="Test Organization", + short_name="TestOrg", + description="A test organization for user groups", + ) + cls.organization_content_type = ContentType.objects.get_for_model(Organization) cls.scope = Scope.objects.create( - name="Demo Course Scope", - content_type=cls.course_content_type, - object_id=cls.test_course["id"], + name="Test Organization Scope", + content_type=cls.organization_content_type, + object_id=cls.test_organization.id, ) cls.user_group = UserGroup.objects.create(name="Test Group", scope=cls.scope) cls.criterion = Criterion.objects.create( @@ -149,8 +170,6 @@ def test_get_available_criterion_types(self): def test_criterion_type_validation(self): """Test that invalid criterion types are rejected.""" - from django.core.exceptions import ValidationError - # Test that invalid criterion type raises ValidationError invalid_criterion = Criterion( user_group=self.user_group, @@ -177,14 +196,18 @@ class TestModelConstraints(TestCase): @classmethod def setUpTestData(cls): """Set up test data for constraint tests.""" - # Create course data using the factory - cls.test_course = CourseFactory() - cls.course_content_type = ContentType.objects.get_for_model(User) + # Create a test organization + cls.test_organization = Organization.objects.create( + name="Test Organization", + short_name="TestOrg", + description="A test organization for user groups", + ) + cls.organization_content_type = ContentType.objects.get_for_model(Organization) cls.scope = Scope.objects.create( - name="Demo Course Scope", - content_type=cls.course_content_type, - object_id=cls.test_course["id"], + name="Test Organization Scope", + content_type=cls.organization_content_type, + object_id=cls.test_organization.id, ) cls.user_group = UserGroup.objects.create(name="Test Group", scope=cls.scope) @@ -200,11 +223,15 @@ def test_user_group_unique_name_per_scope(self): def test_user_group_same_name_different_scope(self): """Test that UserGroup can have same name in different scopes.""" # Create another course data and scope - another_course = CourseFactory() + another_organization = Organization.objects.create( + name="Another Test Organization", + short_name="AnotherOrg", + description="Another test organization", + ) another_scope = Scope.objects.create( name="Another Scope", - content_type=self.course_content_type, - object_id=another_course["id"], + content_type=self.organization_content_type, + object_id=another_organization.id, ) # This should work fine - same name but different scope diff --git a/tests/test_tasks.py b/tests/test_tasks.py index 37d9c23..d1902d6 100644 --- a/tests/test_tasks.py +++ b/tests/test_tasks.py @@ -8,6 +8,7 @@ from datetime import timedelta from django.contrib.contenttypes.models import ContentType from django.contrib.auth import get_user_model +from organizations.models import Organization from openedx_user_groups.tasks import orchestrate_user_groups_updates_based_on_events from openedx_user_groups.handlers import handle_user_group_update @@ -30,23 +31,35 @@ def setUpTestData(cls): for event in BaseCriterionType.get_all_updated_by_events(): event.connect(handle_user_group_update) cls.test_course = CourseFactory() - cls.course_content_type = ContentType.objects.get_for_model(User) + + # Create a test organization + cls.test_organization = Organization.objects.create( + name="Test Organization", + short_name="TestOrg", + description="A test organization for user groups", + ) + cls.organization_content_type = ContentType.objects.get_for_model(Organization) + cls.test_scope = ScopeFactory( - name=cls.test_course["name"], - content_type=cls.course_content_type, - object_id=cls.test_course["id"], + name="Test Organization Scope", + content_type=cls.organization_content_type, + object_id=cls.test_organization.id, ) + cls.test_user_group_data = UserGroupFactory.build(name="At Risk Students") cls.last_login_criterion = LastLoginCriterionFactory.build() cls.enrollment_mode_criterion = EnrollmentModeCriterionFactory.build() cls.user_staff_status_criterion = UserStaffStatusCriterionFactory.build() - cls.scope_context = { - "name": cls.test_course["name"], + + # Organization scope + cls.scope = { + "name": "Test Organization Scope", "content_object": { - "content_type": cls.course_content_type, - "object_id": cls.test_course["id"], + "content_type_model": "organization", + "object_id": cls.test_organization.short_name, }, } + cls.user_old_login_non_staff = UserFactory( username="user_old_login_non_staff", last_login=timezone.now() - timedelta(days=2), # 2 days ago (> 1 day ago) @@ -70,8 +83,8 @@ def setUpTestData(cls): cls.user_old_login_non_staff_group = create_group_with_criteria( # Returns user_old_login_non_staff and user_old_login_non_staff_2 name="Old Login Non Staff Group", description="Old Login Non Staff Group", - scope_context=cls.scope_context, - criterion_data=[ + scope=cls.scope, + criteria=[ { "criterion_type": cls.last_login_criterion.criterion_type, "criterion_operator": cls.last_login_criterion.criterion_operator, @@ -89,8 +102,8 @@ def setUpTestData(cls): cls.user_non_staff_status_group = create_group_with_criteria( # Returns user_old_login_staff, user_old_login_non_staff, user_old_login_non_staff_2 name="Non Staff Status Group", description="Non Staff Status Group", - scope_context=cls.scope_context, - criterion_data=[ + scope=cls.scope, + criteria=[ { "criterion_type": cls.user_staff_status_criterion.criterion_type, "criterion_operator": cls.user_staff_status_criterion.criterion_operator, @@ -138,8 +151,8 @@ def test_orchestrate_updates_with_user_in_multiple_groups(self): staff_user_group = create_group_with_criteria( name="Staff User Group", description="Staff User Group", - scope_context=self.scope_context, - criterion_data=[ + scope=self.scope, + criteria=[ { "criterion_type": self.user_staff_status_criterion.criterion_type, "criterion_operator": self.user_staff_status_criterion.criterion_operator, From ee13a657611b9b533e9943d1211e9baf531c78db Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 27 Jun 2025 11:12:55 +0200 Subject: [PATCH 09/12] build: support python311 instead --- .github/workflows/ci.yml | 4 ++-- tox.ini | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 17812b8..0ee5827 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -14,7 +14,7 @@ jobs: strategy: matrix: os: [ubuntu-latest] - python-version: ["3.12"] + python-version: ["3.11"] toxenv: [quality, docs, pii_check, django42, django52] steps: - uses: actions/checkout@v4 @@ -35,7 +35,7 @@ jobs: run: tox - name: Run coverage - if: matrix.python-version == '3.12' && matrix.toxenv == 'django42' + if: matrix.python-version == '3.11' && matrix.toxenv == 'django42' uses: codecov/codecov-action@v4 with: token: ${{ secrets.CODECOV_TOKEN }} diff --git a/tox.ini b/tox.ini index a405fe5..e697cc8 100644 --- a/tox.ini +++ b/tox.ini @@ -1,5 +1,5 @@ [tox] -envlist = py312-django{42,52} +envlist = py311-django{42,52} [doc8] ; D001 = Line too long From 31ee2c04247fce4afae98d824c9c569338b18615 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 27 Jun 2025 12:43:25 +0200 Subject: [PATCH 10/12] docs: add no_pii annotation to models --- openedx_user_groups/models.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/openedx_user_groups/models.py b/openedx_user_groups/models.py index e5607e5..1e466a1 100644 --- a/openedx_user_groups/models.py +++ b/openedx_user_groups/models.py @@ -75,6 +75,7 @@ class Scope(models.Model): content_type (ForeignKey): The content type of the object that defines the scope. object_id (PositiveIntegerField): The ID of the object that defines the scope. content_object (GenericForeignKey): The object that defines the scope (e.g., course, organization). + .. no_pii: """ name = models.CharField( @@ -101,6 +102,7 @@ class UserGroup(models.Model): enabled (bool): Whether the user group is enabled. scope (str): The scope of the user group, defining the context in which it operates. users (ManyToManyField): The users that are members of the group. + .. no_pii: """ name = models.CharField(max_length=255) @@ -158,6 +160,7 @@ class UserGroupMembership(models.Model): joined_at (datetime): The timestamp when the user joined the group. left_at (datetime): The timestamp when the user left the group. is_active (bool): Whether the user is still a member of the group. + .. no_pii: """ user = models.ForeignKey(User, on_delete=models.CASCADE) @@ -180,6 +183,7 @@ class Criterion(models.Model): criterion_operator (str): The operator of the criterion. criterion_config (dict): The configuration of the criterion. group (UserGroup): The group to which the criterion belongs. + .. no_pii: """ criterion_type = models.CharField( @@ -232,6 +236,7 @@ class GroupCollection(models.Model): Attributes: name (str): The name of the group collection. description (str): A brief description of the group collection. + .. no_pii: """ name = models.CharField(max_length=255) From 4c77ee3248321ea96d74f0b8dfde3a7d897d3307 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 27 Jun 2025 12:59:13 +0200 Subject: [PATCH 11/12] build: disable pii_check for the time being --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0ee5827..ba693c9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -15,7 +15,7 @@ jobs: matrix: os: [ubuntu-latest] python-version: ["3.11"] - toxenv: [quality, docs, pii_check, django42, django52] + toxenv: [quality, docs, django42, django52] steps: - uses: actions/checkout@v4 - name: setup python From b70b36db9a8ea34de293e807574af86baf5a37c4 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 27 Jun 2025 13:01:38 +0200 Subject: [PATCH 12/12] build: disable docs check for POC (early development) --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ba693c9..3120e24 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -15,7 +15,7 @@ jobs: matrix: os: [ubuntu-latest] python-version: ["3.11"] - toxenv: [quality, docs, django42, django52] + toxenv: [quality, django42, django52] steps: - uses: actions/checkout@v4 - name: setup python