From 54c4db02ba6be857f018ef5642f5094873bcbf82 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Fri, 10 Apr 2026 16:46:45 -0500 Subject: [PATCH 01/29] feat: implement course authoring migration functionality --- openedx_authz/admin.py | 41 +++- openedx_authz/engine/utils.py | 220 ++++++++++++++++-- openedx_authz/handlers.py | 112 ++++++++- .../0008_authzcourseauthoringmigrationrun.py | 83 +++++++ openedx_authz/models/__init__.py | 1 + openedx_authz/models/migrations.py | 174 ++++++++++++++ openedx_authz/settings/common.py | 5 + openedx_authz/settings/test.py | 3 + openedx_authz/tests/test_migrations.py | 14 +- 9 files changed, 623 insertions(+), 30 deletions(-) create mode 100644 openedx_authz/migrations/0008_authzcourseauthoringmigrationrun.py create mode 100644 openedx_authz/models/migrations.py diff --git a/openedx_authz/admin.py b/openedx_authz/admin.py index e2285158..da343691 100644 --- a/openedx_authz/admin.py +++ b/openedx_authz/admin.py @@ -1,10 +1,24 @@ """Admin configuration for openedx_authz.""" +import json + from casbin_adapter.models import CasbinRule from django import forms from django.contrib import admin +from django.utils.html import format_html + +from openedx_authz.models import AuthzCourseAuthoringMigrationRun, ExtendedCasbinRule -from openedx_authz.models import ExtendedCasbinRule + +def pretty_json(value) -> str: + """Return an indented JSON representation of a value.""" + if value is None: + return "-" + try: + formatted = json.dumps(value, indent=2, ensure_ascii=False) + except (TypeError, ValueError): + return str(value) + return format_html("
{}
", formatted) class CasbinRuleForm(forms.ModelForm): @@ -48,3 +62,28 @@ class CasbinRuleAdmin(admin.ModelAdmin): # TODO: In a future, possibly we should only show an inline for the rules that # have an extended rule, and show the subject and scope information in detail. inlines = [ExtendedCasbinRuleInline] + + +@admin.register(AuthzCourseAuthoringMigrationRun) +class AuthzCourseAuthoringMigrationRunAdmin(admin.ModelAdmin): + """Admin for AuthzCourseAuthoringMigrationRun to display additional metadata.""" + + list_display = ("id", "scope_type", "scope_key", "migration_type", "status", "created_at", "updated_at") + search_fields = ("scope_type", "scope_key", "migration_type", "status") + list_filter = ("scope_type", "migration_type", "status") + readonly_fields = ( + "scope_type", + "scope_key", + "migration_type", + "status", + "pretty_metadata", + "completed_at", + "created_at", + "updated_at", + ) + fields = readonly_fields + + @admin.display(description="Metadata") + def pretty_metadata(self, obj): + """Return formatted JSON for the metadata field.""" + return pretty_json(obj.metadata) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 8603934b..020da20b 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -6,8 +6,10 @@ import logging from collections import defaultdict +from dataclasses import dataclass, field from casbin import Enforcer +from django.db import IntegrityError, transaction from django.db.models import Q from opaque_keys.edx.django.models import CourseKeyField @@ -24,6 +26,7 @@ LIBRARY_AUTHOR, LIBRARY_USER, ) +from openedx_authz.models.migrations import AuthzCourseAuthoringMigrationRun, MigrationType, ScopeType logger = logging.getLogger(__name__) @@ -34,6 +37,46 @@ COURSE_ROLE_EQUIVALENCES = {v: k for k, v in LEGACY_COURSE_ROLE_EQUIVALENCES.items()} +class MigrationErrorReason: + """String constants for categorising why a single role assignment failed during migration.""" + + # Forward (legacy → authz) reasons + UNKNOWN_ROLE = "unknown_role" + NO_SCOPE = "no_scope" + ASSIGNMENT_FAILED = "assignment_failed" + + # Rollback (authz → legacy) reasons + UNEXPECTED_SCOPE_TYPE = "unexpected_scope_type" + NO_LEGACY_EQUIVALENT = "no_legacy_equivalent" + UNEXPECTED_ERROR = "unexpected_error" + + +@dataclass +class MigrationMetadata: + """Normalised representation of a single role-assignment outcome during migration. + + Can represent both successful and failed assignments. Populate ``reason`` / + ``details`` only for failures; leave them empty for successes. + + Attributes: + subject: External key of the user whose assignment was attempted. + role: Role external key (new-style for rollback, legacy key for forward). + scope: Scope external key, or empty string when not yet determined. + reason: One of the ``MigrationErrorReason`` constants; empty for successes. + details: Optional human-readable extra context (e.g. exception message). + """ + + subject: str + role: str + scope: str = field(default="") + reason: str = field(default="") + details: str = field(default="") + + def to_dict(self) -> dict: + """Convert the migration metadata to a dictionary.""" + return {k: v for k, v in self.__dict__.items() if v} + + def migrate_policy_between_enforcers( source_enforcer: Enforcer, target_enforcer: Enforcer, @@ -187,7 +230,9 @@ def _validate_migration_input(course_id_list, org_id): ) -def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_list, org_id, delete_after_migration): +def migrate_legacy_course_roles_to_authz( + course_access_role_model, course_id_list, org_id, delete_after_migration +) -> tuple[list[MigrationMetadata], list[MigrationMetadata]]: """ Migrate legacy course role data to the new Casbin-based authorization model. This function reads legacy permissions from the CourseAccessRole model @@ -235,19 +280,22 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis .select_related("user") ) - # List to keep track of any permissions that could not be migrated - permissions_with_errors = [] - permissions_with_no_errors = [] + permissions_with_errors: list[MigrationMetadata] = [] + permissions_with_no_errors: list[MigrationMetadata] = [] + permission_ids: list[int] = [] for permission in legacy_permissions: # Migrate the permission to the new model + migration_metadata = MigrationMetadata(subject=permission.user.username, role=permission.role) role = LEGACY_COURSE_ROLE_EQUIVALENCES.get(permission.role) if role is None: # This should not happen as there are no more access_levels defined # in CourseAccessRole, log and skip logger.error(f"Unknown access level: {permission.role} for User: {permission.user}") - permissions_with_errors.append(permission) + migration_metadata.reason = MigrationErrorReason.UNKNOWN_ROLE + migration_metadata.details = f"Unknown access level: {permission.role} for User: {permission.user.username}" + permissions_with_errors.append(migration_metadata) continue if permission.course_id: @@ -259,7 +307,9 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis logger.error( f"Permission for User: {permission.user.username} has neither course_id nor org defined, skipping." ) - permissions_with_errors.append(permission) + migration_metadata.reason = MigrationErrorReason.NO_SCOPE + migration_metadata.details = f"User '{permission.user.username}' has neither course_id nor org defined" + permissions_with_errors.append(migration_metadata) continue # Permission applied to individual user @@ -279,23 +329,30 @@ def migrate_legacy_course_roles_to_authz(course_access_role_model, course_id_lis f"to Role: {role} in Scope: {permission.course_id} " "user may already have this permission assigned" ) - permissions_with_errors.append(permission) + migration_metadata.scope = scope_external_key + migration_metadata.reason = MigrationErrorReason.ASSIGNMENT_FAILED + migration_metadata.details = f"User '{permission.user.username}' may already have this permission assigned" + permissions_with_errors.append(migration_metadata) continue - permissions_with_no_errors.append(permission) + migration_metadata.scope = scope_external_key + permissions_with_no_errors.append(migration_metadata) + + permission_ids.append(permission.id) if delete_after_migration: # Only delete permissions that were successfully migrated to avoid data loss. - course_access_role_model.objects.filter(id__in=[p.id for p in permissions_with_no_errors]).delete() + course_access_role_model.objects.filter(id__in=permission_ids).delete() logger.info(f"Deleted {len(permissions_with_no_errors)} legacy permissions after successful migration.") logger.info(f"Retained {len(permissions_with_errors)} legacy permissions that had errors during migration.") return permissions_with_errors, permissions_with_no_errors +# pylint: disable=too-many-statements def migrate_authz_to_legacy_course_roles( course_access_role_model, user_subject_model, course_id_list, org_id, delete_after_migration -): +) -> tuple[list[MigrationMetadata], list[MigrationMetadata]]: """ Migrate permissions from the new Casbin-based authorization model back to the legacy CourseAccessRole model. This function reads permissions from the Casbin enforcer and creates equivalent entries in the @@ -321,10 +378,7 @@ def migrate_authz_to_legacy_course_roles( _validate_migration_input(course_id_list, org_id) role_assignments = get_all_role_assignments_per_scope_type( - scope_types=( - CourseOverviewData, - OrgCourseOverviewGlobData, - ) + scope_types=(CourseOverviewData, OrgCourseOverviewGlobData) ) # Two cases here: @@ -343,8 +397,8 @@ def migrate_authz_to_legacy_course_roles( and role_assignment.scope.course_id in course_id_list ] - roles_with_errors = [] - roles_with_no_errors = [] + roles_with_errors: list[MigrationMetadata] = [] + roles_with_no_errors: list[MigrationMetadata] = [] unassignments = defaultdict(list) user_external_keys = {assignment.subject.external_key for assignment in role_assignments} @@ -361,9 +415,24 @@ def migrate_authz_to_legacy_course_roles( role_external_key = role_assignment.roles[0].external_key scope_external_key = role_assignment.scope.external_key + migration_metadata = MigrationMetadata( + subject=user_external_key, role=role_external_key, scope=scope_external_key + ) + + legacy_role = COURSE_ROLE_EQUIVALENCES.get(role_external_key) + if legacy_role is None: + logger.error( + f"No legacy equivalent found for role: {role_external_key}, " + f"user: {user_external_key}, scope: {scope_external_key}. Skipping." + ) + migration_metadata.reason = MigrationErrorReason.NO_LEGACY_EQUIVALENT + migration_metadata.details = f"Role '{role_external_key}' has no legacy equivalent." + roles_with_errors.append(migration_metadata) + continue + course_access_role_kwargs = { "user": users_by_username[user_external_key], - "role": COURSE_ROLE_EQUIVALENCES[role_external_key], + "role": legacy_role, } if isinstance(role_assignment.scope, CourseOverviewData): @@ -378,11 +447,13 @@ def migrate_authz_to_legacy_course_roles( f"Unexpected scope type: {type(role_assignment.scope)} for RoleAssignment with " f"scope: {scope_external_key}, user: {user_external_key} and role: {role_external_key}, skipping." ) - roles_with_errors.append(role_assignment) + migration_metadata.reason = MigrationErrorReason.UNEXPECTED_SCOPE_TYPE + migration_metadata.details = f"Unexpected scope type: {type(role_assignment.scope).__name__}" + roles_with_errors.append(migration_metadata) continue course_access_role_model.objects.get_or_create(**course_access_role_kwargs) - roles_with_no_errors.append(role_assignment) + roles_with_no_errors.append(migration_metadata) logger.info( f"Successfully rolled back RoleAssignment for User: {user_external_key} " @@ -398,7 +469,9 @@ def migrate_authz_to_legacy_course_roles( f"Error rolling back RoleAssignment for User: {role_assignment.subject.external_key} " f"in Role: {role_assignment.roles[0].external_key} and Scope: {role_assignment.scope.external_key}: {e}" ) - roles_with_errors.append(role_assignment) + migration_metadata.reason = MigrationErrorReason.UNEXPECTED_ERROR + migration_metadata.details = str(e) + roles_with_errors.append(migration_metadata) # Once the loop is done, we can log summary of unassignments # and perform batch unassignment if delete_after_migration is True @@ -407,7 +480,7 @@ def migrate_authz_to_legacy_course_roles( logger.info(f"Total of {total_unassignments} role assignments unassigned after successful rollback migration.") for (role_external_key, scope), users in unassignments.items(): logger.info( - f"Unassigned Role: {role_external_key} from {len(users)} users \n" + f"Unassigned Role: {role_external_key} from {len(users)} users " f"in Scope: {scope} after successful rollback migration." ) batch_unassign_role_from_users( @@ -417,3 +490,108 @@ def migrate_authz_to_legacy_course_roles( ) return roles_with_errors, roles_with_no_errors + + +# pylint: disable=too-many-positional-arguments +def run_course_authoring_migration( + migration_type: MigrationType, + scope_type: ScopeType, + scope_key: str, + course_access_role_model, + user_subject_model=None, + course_id_list=None, + org_id=None, + delete_after_migration=True, +) -> None: + """ + Orchestrate a course authoring role migration with concurrency protection and lifecycle tracking. + + Wraps either :func:`migrate_legacy_course_roles_to_authz` (``FORWARD``) or + :func:`migrate_authz_to_legacy_course_roles` (``ROLLBACK``) with three guarantees: + + 1. Concurrency guard: an ``AuthzCourseAuthoringMigrationRun`` record is + created atomically before work begins. If an active run already exists for the + same ``(scope_type, scope_key)``, the call is skipped immediately to prevent + duplicate parallel runs. + 2. Lifecycle tracking: the run record is updated to ``COMPLETED``, + ``PARTIAL_SUCCESS``, or ``FAILED`` regardless of the outcome, with per-role + error details persisted in the record's ``metadata`` field. + 3. Transactional safety: data-migration work runs inside an inner + ``atomic()`` block so that an unexpected exception rolls back all data changes + while the tracking record (updated outside that block) is always persisted. + + Args: + migration_type (MigrationType): Direction of the migration (``FORWARD`` or ``ROLLBACK``). + scope_type (ScopeType): Granularity key dimension for the tracking record (e.g. course or org). + scope_key (str): Concrete identifier — a course-v1 key or an org name. + course_access_role_model: The ``CourseAccessRole`` model class. + user_subject_model: The ``UserSubject`` model class; required for ``ROLLBACK``, ignored otherwise. + course_id_list (list[str] | None): Restrict migration to these course-v1 keys. + org_id (str | None): Restrict migration to this org; takes precedence over ``course_id_list``. + delete_after_migration (bool): Remove successfully migrated entries from the source system. + """ + try: + with transaction.atomic(): + run = AuthzCourseAuthoringMigrationRun.create_running(migration_type, scope_type, scope_key) + except IntegrityError: + logger.warning( + "Skipping %s migration for %s:%s — an active run already exists.", migration_type, scope_type, scope_key + ) + AuthzCourseAuthoringMigrationRun.create_skipped(migration_type, scope_type, scope_key) + return + + logger.info("Started %s migration run [%s] for %s:%s", migration_type, run.id, scope_type, scope_key) + + try: + with transaction.atomic(): + if migration_type == MigrationType.FORWARD: + errors, successes = migrate_legacy_course_roles_to_authz( + course_access_role_model, course_id_list, org_id, delete_after_migration + ) + else: + errors, successes = migrate_authz_to_legacy_course_roles( + course_access_role_model, user_subject_model, course_id_list, org_id, delete_after_migration + ) + except Exception as exc: # pylint: disable=broad-exception-caught + # The inner atomic block is rolled back on exception; mark_failed() runs + # outside it so the tracking record is always persisted. + logger.exception( + "Unexpected error in migration run [%s] for %s:%s", run.id, scope_type, scope_key, exc_info=exc + ) + run.mark_failed(exception=exc) + return + + errors_by_reason: dict = defaultdict(list) + for entry in errors: + entry_dict = entry.to_dict() + errors_by_reason[entry_dict.pop("reason")].append(entry_dict) + + metadata_updates = { + "total": len(successes) + len(errors), + "success_count": len(successes), + "error_count": len(errors), + "successes": [entry.to_dict() for entry in successes], + "errors": dict(errors_by_reason), + } + + if errors: + run.mark_partial_success(metadata_updates=metadata_updates) + logger.warning( + "Partial success in %s migration run [%s] for %s:%s — successes=%d, errors=%d", + migration_type, + run.id, + scope_type, + scope_key, + len(successes), + len(errors), + ) + else: + run.mark_completed(metadata_updates=metadata_updates) + logger.info( + "Completed %s migration run [%s] for %s:%s — successes=%d", + migration_type, + run.id, + scope_type, + scope_key, + len(successes), + ) diff --git a/openedx_authz/handlers.py b/openedx_authz/handlers.py index 7701123e..fe168554 100644 --- a/openedx_authz/handlers.py +++ b/openedx_authz/handlers.py @@ -4,19 +4,33 @@ These handlers ensure proper cleanup and consistency when models are deleted. """ +from __future__ import annotations + import logging from casbin_adapter.models import CasbinRule -from django.db.models.signals import post_delete +from django.conf import settings +from django.db.models.signals import post_delete, post_save from django.dispatch import receiver from openedx_authz.api.users import unassign_all_roles_from_user +from openedx_authz.engine.utils import run_course_authoring_migration from openedx_authz.models.core import ExtendedCasbinRule +from openedx_authz.models.migrations import MigrationType, ScopeType +from openedx_authz.models.subjects import UserSubject try: + from common.djangoapps.student.models import CourseAccessRole from openedx.core.djangoapps.user_api.accounts.signals import USER_RETIRE_LMS_CRITICAL + from openedx.core.djangoapps.waffle_utils.models import WaffleFlagCourseOverrideModel, WaffleFlagOrgOverrideModel + from openedx.core.toggles import AUTHZ_COURSE_AUTHORING_FLAG except ImportError: USER_RETIRE_LMS_CRITICAL = None + WaffleFlagCourseOverrideModel = None + WaffleFlagOrgOverrideModel = None + AUTHZ_COURSE_AUTHORING_FLAG = None + CourseAccessRole = None + logger = logging.getLogger(__name__) @@ -82,3 +96,99 @@ def unassign_roles_on_user_retirement(sender, user, **kwargs): # pylint: disabl # Only register the handler if the signal is available (i.e., running in Open edX) if USER_RETIRE_LMS_CRITICAL is not None: USER_RETIRE_LMS_CRITICAL.connect(unassign_roles_on_user_retirement) + + +def handle_course_waffle_flag_change(sender, instance, **kwargs) -> None: + """ + Handle changes to course-level waffle flags. + + When the authz.enable_course_authoring flag is changed for a course, + trigger the appropriate migration run. Only trigger if automatic migration + is enabled in the settings. + + Args: + sender: The model class (WaffleFlagCourseOverrideModel) + instance: The flag override instance being saved + **kwargs: Additional keyword arguments from the signal + """ + trigger_course_authoring_migration(sender=sender, instance=instance, scope_key=str(instance.course_id)) + + +def handle_org_waffle_flag_change(sender, instance, **kwargs) -> None: + """ + Handle changes to organization-level waffle flags. + + When the authz.enable_course_authoring flag is changed for an organization, + trigger the appropriate migration run. Only trigger if automatic migration + is enabled in the settings. + + Args: + sender: The model class (WaffleFlagOrgOverrideModel) + instance: The flag override instance being saved + **kwargs: Additional keyword arguments from the signal + """ + trigger_course_authoring_migration(sender=sender, instance=instance, scope_key=str(instance.org)) + + +# Only register the handlers if the models are available (i.e., running in Open edX) +if WaffleFlagCourseOverrideModel is not None: + post_save.connect(handle_course_waffle_flag_change, sender=WaffleFlagCourseOverrideModel) + +if WaffleFlagOrgOverrideModel is not None: + post_save.connect(handle_org_waffle_flag_change, sender=WaffleFlagOrgOverrideModel) + + +def trigger_course_authoring_migration( + sender: type[WaffleFlagCourseOverrideModel | WaffleFlagOrgOverrideModel], + instance: WaffleFlagCourseOverrideModel | WaffleFlagOrgOverrideModel, + scope_key: str, +) -> None: + """ + Trigger a migration run in response to a waffle flag change. + + Determines the migration direction from the flag state, guards against + no-op saves, and delegates execution to ``run_course_authoring_migration`` + which handles tracking and concurrent-run protection. + + Args: + sender: The model class (WaffleFlagCourseOverrideModel or WaffleFlagOrgOverrideModel). + instance: The waffle flag instance that triggered the migration. + scope_key (str): Course ID or organization name. + """ + if not settings.ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION: + logger.info("ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION is set to False, skipping migration") + return + + if instance.waffle_flag != AUTHZ_COURSE_AUTHORING_FLAG.name: + return + + course_id_list, org_id, scope_type = None, None, None + filter_kwargs = {"waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG.name} + if isinstance(instance, WaffleFlagCourseOverrideModel): + filter_kwargs["course_id"] = instance.course_id + course_id_list = [scope_key] + scope_type = ScopeType.COURSE + elif isinstance(instance, WaffleFlagOrgOverrideModel): + filter_kwargs["org"] = instance.org + org_id = scope_key + scope_type = ScopeType.ORG + + prev_record = sender.objects.filter(**filter_kwargs).exclude(id=instance.id).order_by("-change_date").first() + + if prev_record and prev_record.enabled == instance.enabled: + logger.info("No change in waffle flag, skipping course migration") + return + + migration_type = MigrationType.FORWARD if instance.enabled else MigrationType.ROLLBACK + + logger.info("Triggering %s migration for %s:%s due to waffle flag change", migration_type, scope_type, scope_key) + + run_course_authoring_migration( + migration_type=migration_type, + scope_type=scope_type, + scope_key=scope_key, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=course_id_list, + org_id=org_id, + ) diff --git a/openedx_authz/migrations/0008_authzcourseauthoringmigrationrun.py b/openedx_authz/migrations/0008_authzcourseauthoringmigrationrun.py new file mode 100644 index 00000000..e1c4ceba --- /dev/null +++ b/openedx_authz/migrations/0008_authzcourseauthoringmigrationrun.py @@ -0,0 +1,83 @@ +# Generated by Django 5.2.12 on 2026-04-14 20:21 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("openedx_authz", "0007_coursescope"), + ] + + operations = [ + migrations.CreateModel( + name="AuthzCourseAuthoringMigrationRun", + fields=[ + ("id", models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")), + ( + "migration_type", + models.CharField( + choices=[("forward", "Legacy to AuthZ"), ("rollback", "AuthZ to Legacy")], + help_text="Direction of migration: forward (legacy → authz) or rollback (authz → legacy)", + max_length=20, + ), + ), + ( + "scope_type", + models.CharField( + choices=[("course", "Course"), ("org", "Organization")], + help_text="Type of scope being migrated: course or organization", + max_length=20, + ), + ), + ( + "scope_key", + models.CharField( + help_text="Identifier for the scope (e.g., course-v1:edX+DemoX+DemoCourse or org name)", + max_length=255, + ), + ), + ( + "status", + models.CharField( + choices=[ + ("running", "Running"), + ("completed", "Completed"), + ("partial_success", "Partial Success"), + ("failed", "Failed"), + ("skipped", "Skipped"), + ], + default="running", + help_text="Current status of the migration run", + max_length=20, + ), + ), + ("created_at", models.DateTimeField(auto_now_add=True, help_text="When the migration run was created")), + ( + "updated_at", + models.DateTimeField(auto_now=True, help_text="When the migration run was last updated"), + ), + ( + "completed_at", + models.DateTimeField(blank=True, help_text="When the migration run was completed", null=True), + ), + ( + "metadata", + models.JSONField( + blank=True, + default=dict, + help_text="Additional metadata about the migration run (e.g., counts, warnings, errors)", + ), + ), + ], + options={ + "verbose_name": "Course Authoring Migration Run", + "verbose_name_plural": "Course Authoring Migration Runs", + "ordering": ["-created_at"], + "indexes": [ + models.Index(fields=["scope_type", "scope_key"], name="openedx_aut_scope_t_d43a35_idx"), + models.Index(fields=["status"], name="openedx_aut_status_e34b60_idx"), + models.Index(fields=["-created_at"], name="openedx_aut_created_ab3e0a_idx"), + ], + }, + ), + ] diff --git a/openedx_authz/models/__init__.py b/openedx_authz/models/__init__.py index 4f0318a2..446dc9f9 100644 --- a/openedx_authz/models/__init__.py +++ b/openedx_authz/models/__init__.py @@ -16,5 +16,6 @@ """ from openedx_authz.models.core import * +from openedx_authz.models.migrations import * from openedx_authz.models.scopes import * from openedx_authz.models.subjects import * diff --git a/openedx_authz/models/migrations.py b/openedx_authz/models/migrations.py new file mode 100644 index 00000000..65d5dcde --- /dev/null +++ b/openedx_authz/models/migrations.py @@ -0,0 +1,174 @@ +"""Models for tracking migration runs between legacy and AuthZ systems.""" + +from django.db import IntegrityError, models, transaction +from django.utils import timezone + + +class MigrationType(models.TextChoices): + """Direction of migration.""" + + FORWARD = "forward", "Legacy to AuthZ" + ROLLBACK = "rollback", "AuthZ to Legacy" + + +class Status(models.TextChoices): + """Status of the migration task.""" + + RUNNING = "running", "Running" + COMPLETED = "completed", "Completed" + PARTIAL_SUCCESS = "partial_success", "Partial Success" + FAILED = "failed", "Failed" + SKIPPED = "skipped", "Skipped" + + +class ScopeType(models.TextChoices): + """Type of scope being migrated.""" + + COURSE = "course", "Course" + ORG = "org", "Organization" + + +class AuthzCourseAuthoringMigrationRun(models.Model): + """Track the status of course authoring migration tasks. + + This model is used to track async migrations between the legacy + CourseAccessRole system and the new AuthZ system. + + .. no_pii: + """ + + migration_type = models.CharField( + max_length=20, + choices=MigrationType, + help_text="Direction of migration: forward (legacy → authz) or rollback (authz → legacy)", + ) + + scope_type = models.CharField( + max_length=20, + choices=ScopeType, + help_text="Type of scope being migrated: course or organization", + ) + + scope_key = models.CharField( + max_length=255, + help_text="Identifier for the scope (e.g., course-v1:edX+DemoX+DemoCourse or org name)", + ) + + status = models.CharField( + max_length=20, + choices=Status, + default=Status.RUNNING, + help_text="Current status of the migration run", + ) + + created_at = models.DateTimeField( + auto_now_add=True, + help_text="When the migration run was created", + ) + + updated_at = models.DateTimeField( + auto_now=True, + help_text="When the migration run was last updated", + ) + + completed_at = models.DateTimeField( + null=True, + blank=True, + help_text="When the migration run was completed", + ) + + metadata = models.JSONField( + default=dict, + blank=True, + help_text="Additional metadata about the migration run (e.g., counts, warnings, errors)", + ) + + class Meta: + verbose_name = "Course Authoring Migration Run" + verbose_name_plural = "Course Authoring Migration Runs" + ordering = ["-created_at"] + indexes = [ + models.Index(fields=["scope_type", "scope_key"]), + models.Index(fields=["status"]), + models.Index(fields=["-created_at"]), + ] + + def save(self, *args, **kwargs) -> None: + """Enforce at most one RUNNING record per (scope_type, scope_key). + + MySQL does not support partial unique indexes, so this check is done at + the application level. select_for_update() is used to reduce (but not + fully eliminate) the race-condition window on concurrent inserts. + """ + if self.status == Status.RUNNING and self.pk is None: + with transaction.atomic(): + conflict = ( + self.__class__.objects.select_for_update() + .filter(scope_type=self.scope_type, scope_key=self.scope_key, status=Status.RUNNING) + .exists() + ) + if conflict: + raise IntegrityError( + f"Duplicate RUNNING migration run for scope {self.scope_type}:{self.scope_key}" + ) + super().save(*args, **kwargs) + + # pylint: disable=too-many-positional-arguments + @classmethod + def _create( + cls, migration_type, scope_type, scope_key, status, metadata=None + ) -> "AuthzCourseAuthoringMigrationRun": + return cls.objects.create( + migration_type=migration_type, + scope_type=scope_type, + scope_key=scope_key, + status=status, + metadata=metadata or {}, + ) + + @classmethod + def create_running( + cls, + migration_type, + scope_type, + scope_key, + metadata=None, + ) -> "AuthzCourseAuthoringMigrationRun": + """Create a migration run in running state.""" + return cls._create(migration_type, scope_type, scope_key, Status.RUNNING, metadata) + + @classmethod + def create_skipped( + cls, + migration_type, + scope_type, + scope_key, + metadata=None, + ) -> "AuthzCourseAuthoringMigrationRun": + """Create a migration run in skipped state.""" + extra = {**(metadata or {}), "skip_reason": "A concurrent migration run is already active for this scope."} + return cls._create(migration_type, scope_type, scope_key, Status.SKIPPED, extra) + + def _finalize(self, status: str, metadata_updates: dict | None = None) -> None: + """Finalize the migration run.""" + self.status = status + self.completed_at = timezone.now() + if metadata_updates: + self.metadata = {**(self.metadata or {}), **metadata_updates} + self.save(update_fields=["status", "completed_at", "updated_at", "metadata"]) + + def mark_partial_success(self, *, metadata_updates=None) -> None: + """Mark the migration run as partially successful.""" + self._finalize(Status.PARTIAL_SUCCESS, metadata_updates) + + def mark_completed(self, *, metadata_updates=None) -> None: + """Mark the migration run as completed.""" + self._finalize(Status.COMPLETED, metadata_updates) + + def mark_failed(self, *, exception=None) -> None: + """Mark the migration run as failed.""" + self._finalize(Status.FAILED, {"error": str(exception)} if exception is not None else None) + + def __str__(self) -> str: + """Return a string representation of the migration run.""" + return f"[{self.id}] {self.migration_type} {self.scope_type}:{self.scope_key} {self.status}" diff --git a/openedx_authz/settings/common.py b/openedx_authz/settings/common.py index 81e060b8..fd171efb 100644 --- a/openedx_authz/settings/common.py +++ b/openedx_authz/settings/common.py @@ -54,3 +54,8 @@ def plugin_settings(settings): # This setting defines the logging level for the Casbin enforcer. if not hasattr(settings, "CASBIN_LOG_LEVEL"): settings.CASBIN_LOG_LEVEL = "WARNING" + + # Set default ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION if not already set. + # This setting defines whether to enable automatic course migration. + if not hasattr(settings, "ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION"): + settings.ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION = False diff --git a/openedx_authz/settings/test.py b/openedx_authz/settings/test.py index ba449092..89b0eb21 100644 --- a/openedx_authz/settings/test.py +++ b/openedx_authz/settings/test.py @@ -78,3 +78,6 @@ def plugin_settings(settings): # pylint: disable=unused-argument # Use stub model for testing instead of the real content_libraries app OPENEDX_AUTHZ_CONTENT_LIBRARY_MODEL = "stubs.ContentLibrary" OPENEDX_AUTHZ_COURSE_OVERVIEW_MODEL = "stubs.CourseOverview" + +# Migration settings +ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION = False diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index fb82908e..3b8138ee 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -430,7 +430,7 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_no_deletion(self): self.assertEqual(assignments[0].roles[0], COURSE_DATA_RESEARCHER) self.assertEqual(len(permissions_with_errors), 1) - self.assertEqual(permissions_with_errors[0].user.username, self.error_user.username) + self.assertEqual(permissions_with_errors[0].subject, self.error_user.username) self.assertEqual(permissions_with_errors[0].role, "invalid-legacy-role") self.assertEqual(len(permissions_with_no_errors), 12) # 3 users for each of the 4 roles = 12 total entries @@ -581,7 +581,7 @@ def test_migrate_legacy_course_roles_to_authz_and_rollback_with_deletion(self): self.assertEqual(len(assignments), 1) self.assertEqual(assignments[0].roles[0], COURSE_DATA_RESEARCHER) self.assertEqual(len(permissions_with_errors), 1) - self.assertEqual(permissions_with_errors[0].user.username, self.error_user.username) + self.assertEqual(permissions_with_errors[0].subject, self.error_user.username) self.assertEqual(permissions_with_errors[0].role, "invalid-legacy-role") self.assertEqual(len(permissions_with_no_errors), 12) # 3 users for each of the 4 roles = 12 total entries @@ -798,7 +798,7 @@ def test_migrate_legacy_course_roles_to_authz_using_org_id(self): self.assertEqual(len(assignments), 1) self.assertEqual(assignments[0].roles[0], COURSE_DATA_RESEARCHER) self.assertEqual(len(permissions_with_errors), 1) - self.assertEqual(permissions_with_errors[0].user.username, self.error_user.username) + self.assertEqual(permissions_with_errors[0].subject, self.error_user.username) self.assertEqual(permissions_with_errors[0].role, "invalid-legacy-role") self.assertEqual(len(permissions_with_no_errors), 12) # 3 users for each of the 4 roles = 12 total entries @@ -1172,7 +1172,7 @@ def test_migrate_legacy_course_roles_to_authz_user_not_added( self.assertEqual(len(errors), 1) self.assertEqual(len(successes), 0) - self.assertEqual(errors[0].user.username, "testuser") + self.assertEqual(errors[0].subject, "testuser") @patch("openedx_authz.engine.utils.LEGACY_COURSE_ROLE_EQUIVALENCES", {"instructor": "instructor-role"}) def test_migrate_legacy_course_roles_to_authz_instance_wide_role_is_error(self): @@ -1206,7 +1206,7 @@ def test_migrate_legacy_course_roles_to_authz_instance_wide_role_is_error(self): self.assertEqual(len(errors), 1) self.assertEqual(len(successes), 0) - self.assertEqual(errors[0].user.username, "instance_user") + self.assertEqual(errors[0].subject, "instance_user") self.assertTrue(any("instance_user" in msg for msg in log.output)) @patch("openedx_authz.api.data.CourseOverview", CourseOverview) @@ -1366,7 +1366,7 @@ def test_org_id_filter_includes_glob_and_excludes_other_orgs(self): CourseAccessRole, UserSubject, course_id_list=None, org_id=self.org, delete_after_migration=False ) - migrated_users = {assignment.subject.external_key for assignment in successes} + migrated_users = {assignment.subject for assignment in successes} # glob assignment for self.org is included self.assertIn(user_glob.username, migrated_users) # assignment from the other org is excluded @@ -1405,7 +1405,7 @@ def test_course_id_list_filter_excludes_glob_and_other_courses(self): CourseAccessRole, UserSubject, course_id_list=[self.course_id], org_id=None, delete_after_migration=False ) - migrated_users = {assignment.subject.external_key for assignment in successes} + migrated_users = {assignment.subject for assignment in successes} # org-level glob is excluded even though it belongs to the same org self.assertNotIn(user_glob.username, migrated_users) # course not in the list is excluded From 50e93661484cea5c2eee0cb4e528b2640e095bba Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Wed, 15 Apr 2026 11:24:49 -0500 Subject: [PATCH 02/29] fix: set default course_id for legacy role migration --- openedx_authz/engine/utils.py | 1 + 1 file changed, 1 insertion(+) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 020da20b..65904ec5 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -440,6 +440,7 @@ def migrate_authz_to_legacy_course_roles( course_access_role_kwargs["course_id"] = scope_external_key elif isinstance(role_assignment.scope, OrgCourseOverviewGlobData): course_access_role_kwargs["org"] = role_assignment.scope.org + course_access_role_kwargs["course_id"] = CourseKeyField.Empty else: # This would only happen for course roles assigned instance-wide # which is not yet supported From fd778ec9d9e671eb4cb1262f0043c85371c42543 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Wed, 15 Apr 2026 14:35:32 -0500 Subject: [PATCH 03/29] refactor: rename 'migrations' module to 'authz_migration' --- openedx_authz/engine/utils.py | 2 +- openedx_authz/handlers.py | 2 +- openedx_authz/models/__init__.py | 2 +- openedx_authz/models/{migrations.py => authz_migration.py} | 0 4 files changed, 3 insertions(+), 3 deletions(-) rename openedx_authz/models/{migrations.py => authz_migration.py} (100%) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 65904ec5..e25280c3 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -26,7 +26,7 @@ LIBRARY_AUTHOR, LIBRARY_USER, ) -from openedx_authz.models.migrations import AuthzCourseAuthoringMigrationRun, MigrationType, ScopeType +from openedx_authz.models.authz_migration import AuthzCourseAuthoringMigrationRun, MigrationType, ScopeType logger = logging.getLogger(__name__) diff --git a/openedx_authz/handlers.py b/openedx_authz/handlers.py index fe168554..be20531b 100644 --- a/openedx_authz/handlers.py +++ b/openedx_authz/handlers.py @@ -15,8 +15,8 @@ from openedx_authz.api.users import unassign_all_roles_from_user from openedx_authz.engine.utils import run_course_authoring_migration +from openedx_authz.models.authz_migration import MigrationType, ScopeType from openedx_authz.models.core import ExtendedCasbinRule -from openedx_authz.models.migrations import MigrationType, ScopeType from openedx_authz.models.subjects import UserSubject try: diff --git a/openedx_authz/models/__init__.py b/openedx_authz/models/__init__.py index 446dc9f9..06b5d003 100644 --- a/openedx_authz/models/__init__.py +++ b/openedx_authz/models/__init__.py @@ -15,7 +15,7 @@ avoid circular dependencies. """ +from openedx_authz.models.authz_migration import * from openedx_authz.models.core import * -from openedx_authz.models.migrations import * from openedx_authz.models.scopes import * from openedx_authz.models.subjects import * diff --git a/openedx_authz/models/migrations.py b/openedx_authz/models/authz_migration.py similarity index 100% rename from openedx_authz/models/migrations.py rename to openedx_authz/models/authz_migration.py From ff40aad8e748272cfdf8f5fff51878e3fc5acf92 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Wed, 15 Apr 2026 14:38:18 -0500 Subject: [PATCH 04/29] docs: update comment to reference ADR-0013 --- openedx_authz/settings/common.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_authz/settings/common.py b/openedx_authz/settings/common.py index fd171efb..c6cabc24 100644 --- a/openedx_authz/settings/common.py +++ b/openedx_authz/settings/common.py @@ -56,6 +56,6 @@ def plugin_settings(settings): settings.CASBIN_LOG_LEVEL = "WARNING" # Set default ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION if not already set. - # This setting defines whether to enable automatic course migration. + # This setting defines whether to enable automatic course migration. See ADR-0013 for more details. if not hasattr(settings, "ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION"): settings.ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION = False From 88f1670231bef1aae20bab8b04aadc4b38e3c82e Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Wed, 15 Apr 2026 14:52:52 -0500 Subject: [PATCH 05/29] fix: handle unsupported waffle flag instances in course authoring migration --- openedx_authz/handlers.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/openedx_authz/handlers.py b/openedx_authz/handlers.py index be20531b..4109b42e 100644 --- a/openedx_authz/handlers.py +++ b/openedx_authz/handlers.py @@ -172,6 +172,9 @@ def trigger_course_authoring_migration( filter_kwargs["org"] = instance.org org_id = scope_key scope_type = ScopeType.ORG + else: + logger.error("Unsupported waffle flag instance: %s", instance) + return prev_record = sender.objects.filter(**filter_kwargs).exclude(id=instance.id).order_by("-change_date").first() From 06d340fe5ba75c261cbaf7010ec8d97490cc3470 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Wed, 15 Apr 2026 17:19:32 -0500 Subject: [PATCH 06/29] test: add unit test for handlers --- openedx_authz/settings/test.py | 2 +- openedx_authz/tests/stubs/models.py | 19 +++ openedx_authz/tests/test_handlers.py | 171 ++++++++++++++++++++++++++- 3 files changed, 187 insertions(+), 5 deletions(-) diff --git a/openedx_authz/settings/test.py b/openedx_authz/settings/test.py index 89b0eb21..5d4c50f9 100644 --- a/openedx_authz/settings/test.py +++ b/openedx_authz/settings/test.py @@ -80,4 +80,4 @@ def plugin_settings(settings): # pylint: disable=unused-argument OPENEDX_AUTHZ_COURSE_OVERVIEW_MODEL = "stubs.CourseOverview" # Migration settings -ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION = False +ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION = True diff --git a/openedx_authz/tests/stubs/models.py b/openedx_authz/tests/stubs/models.py index d443a612..38309a84 100644 --- a/openedx_authz/tests/stubs/models.py +++ b/openedx_authz/tests/stubs/models.py @@ -205,3 +205,22 @@ class CourseAccessRole(models.Model): # blank course_id implies org wide role course_id = CourseKeyField(max_length=255, db_index=True, blank=True) role = models.CharField(max_length=64, db_index=True) + + +# Waffle flag models +class WaffleFlagCourseOverrideModel(models.Model): + """Stub model representing a waffle flag course override for testing purposes.""" + + course_id = CourseKeyField(max_length=255, db_index=True) + waffle_flag = models.CharField(max_length=255, db_index=True, default="") + enabled = models.BooleanField(default=False) + change_date = models.DateTimeField(auto_now_add=True) + + +class WaffleFlagOrgOverrideModel(models.Model): + """Stub model representing a waffle flag org override for testing purposes.""" + + org = models.CharField(max_length=64, db_index=True) + waffle_flag = models.CharField(max_length=255, db_index=True, default="") + enabled = models.BooleanField(default=False) + change_date = models.DateTimeField(auto_now_add=True) diff --git a/openedx_authz/tests/test_handlers.py b/openedx_authz/tests/test_handlers.py index 1b31e1f9..e95f69c9 100644 --- a/openedx_authz/tests/test_handlers.py +++ b/openedx_authz/tests/test_handlers.py @@ -1,16 +1,31 @@ -"""Behavioral tests for the ExtendedCasbinRule deletion signal. +"""Tests for ``openedx_authz.handlers`` Coverage confirms direct deletions, cascades, bulk operations, and resilience when foreign keys -are missing so that the signal stays aligned with the cleanup guarantees in -``openedx_authz.handlers``. +are missing so that the signal stays aligned with the cleanup guarantees. + +Also covers ``trigger_course_authoring_migration`` using stub waffle model classes (Open edX +waffle models are not imported in the test environment). """ +from types import SimpleNamespace from unittest.mock import patch from casbin_adapter.models import CasbinRule -from django.test import TestCase +from ddt import data, ddt, unpack +from django.test import TestCase, override_settings +from openedx_authz.handlers import trigger_course_authoring_migration +from openedx_authz.models.authz_migration import MigrationType, ScopeType from openedx_authz.models.core import ExtendedCasbinRule, Scope, Subject +from openedx_authz.models.subjects import UserSubject +from openedx_authz.tests.stubs.models import ( + CourseAccessRole, + WaffleFlagCourseOverrideModel, + WaffleFlagOrgOverrideModel, +) + +AUTHZ_COURSE_AUTHORING_FLAG_NAME = "authz.enable_course_authoring" +OTHER_WAFFLE_FLAG_NAME = "some.other.flag" def create_casbin_rule_with_extended( # pylint: disable=too-many-positional-arguments @@ -220,3 +235,151 @@ def test_cascade_deletion_with_scope_deletion(self): self.assertFalse(CasbinRule.objects.filter(id=casbin_rule_id).exists()) self.assertFalse(Scope.objects.filter(id=scope_id).exists()) self.assertTrue(Subject.objects.filter(id=subject_id).exists()) + + +@ddt +@patch("openedx_authz.handlers.run_course_authoring_migration") +@patch.multiple( + "openedx_authz.handlers", + AUTHZ_COURSE_AUTHORING_FLAG=SimpleNamespace(name=AUTHZ_COURSE_AUTHORING_FLAG_NAME), + WaffleFlagCourseOverrideModel=WaffleFlagCourseOverrideModel, + WaffleFlagOrgOverrideModel=WaffleFlagOrgOverrideModel, + CourseAccessRole=CourseAccessRole, +) +class TestTriggerCourseAuthoringMigration(TestCase): + """ + Cover ``trigger_course_authoring_migration`` when Open edX waffle imports are absent. + + The class-level ``patch.multiple`` injects stub models and a stand-in flag into + ``openedx_authz.handlers`` so ``isinstance`` checks and flag name resolution match production. + Course and org overrides use the stub ORM (creates and queries) where the handler touches the + database. A class-level ``patch`` replaces ``run_course_authoring_migration`` so no full + migration runs; tests that also patch ``logger`` receive that mock before ``mock_run``. + """ + + @override_settings(ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION=False) + def test_skips_when_automatic_migration_setting_disabled(self, mock_run): + """When the setting is off, the handler returns before scheduling work.""" + instance = WaffleFlagCourseOverrideModel( + course_id="course-v1:org+course+run", + waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, + enabled=True, + ) + + trigger_course_authoring_migration( + sender=WaffleFlagCourseOverrideModel, + instance=instance, + scope_key=str(instance.course_id), + ) + + mock_run.assert_not_called() + + @data( + ( + WaffleFlagCourseOverrideModel, + { + "course_id": "course-v1:org+course+run_mm", + "waffle_flag": OTHER_WAFFLE_FLAG_NAME, + "enabled": True, + }, + "course-v1:org+course+run_mm", + ), + ( + WaffleFlagOrgOverrideModel, + { + "org": "test_org_waffle_mm", + "waffle_flag": OTHER_WAFFLE_FLAG_NAME, + "enabled": True, + }, + "test_org_waffle_mm", + ), + ) + @unpack + def test_skips_when_waffle_flag_name_mismatch(self, sender_model, instance_kwargs, scope_key, mock_run): + """Only the authz course authoring flag triggers migration (course and org overrides).""" + instance = sender_model(**instance_kwargs) + + trigger_course_authoring_migration(sender_model, instance, scope_key) + + mock_run.assert_not_called() + + @patch("openedx_authz.handlers.logger") + def test_logs_error_for_unsupported_instance_type(self, mock_logger, mock_run): + """Instances that are neither course nor org overrides are rejected.""" + unsupported = SimpleNamespace(waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, enabled=True, id=9) + + trigger_course_authoring_migration(WaffleFlagCourseOverrideModel, unsupported, "ignored") + + mock_run.assert_not_called() + mock_logger.error.assert_called_once() + self.assertIn("Unsupported waffle flag instance", mock_logger.error.call_args[0][0]) + + @data( + (True, MigrationType.FORWARD), + (False, MigrationType.ROLLBACK), + ) + @unpack + def test_course_scope_migration_depends_on_enabled(self, enabled, expected_migration_type, mock_run): + """Course override runs forward migration when enabled and rollback when disabled.""" + course_key = f"course-v1:test_org+handlers_course+{'on' if enabled else 'off'}" + instance = WaffleFlagCourseOverrideModel.objects.create( + course_id=course_key, + waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, + enabled=enabled, + ) + + trigger_course_authoring_migration(WaffleFlagCourseOverrideModel, instance, course_key) + + mock_run.assert_called_once_with( + migration_type=expected_migration_type, + scope_type=ScopeType.COURSE, + scope_key=course_key, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=[course_key], + org_id=None, + ) + + @data( + (True, MigrationType.FORWARD), + (False, MigrationType.ROLLBACK), + ) + @unpack + def test_org_scope_migration_depends_on_enabled(self, enabled, expected_migration_type, mock_run): + """Org override runs forward migration when enabled and rollback when disabled.""" + org_name = f"test_org_handlers_{enabled}" + instance = WaffleFlagOrgOverrideModel.objects.create( + org=org_name, + waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, + enabled=enabled, + ) + + trigger_course_authoring_migration(WaffleFlagOrgOverrideModel, instance, org_name) + + mock_run.assert_called_once_with( + migration_type=expected_migration_type, + scope_type=ScopeType.ORG, + scope_key=org_name, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=None, + org_id=org_name, + ) + + def test_skips_when_previous_record_has_same_enabled_state(self, mock_run): + """Repeated saves with the same enabled value do not trigger migration.""" + course_id = "course-v1:test_org+tcam_noop+2024" + WaffleFlagCourseOverrideModel.objects.create( + course_id=course_id, + waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, + enabled=True, + ) + instance = WaffleFlagCourseOverrideModel.objects.create( + course_id=course_id, + waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, + enabled=True, + ) + + trigger_course_authoring_migration(WaffleFlagCourseOverrideModel, instance, course_id) + + mock_run.assert_not_called() From c911f88ed22ac47ce1089cfe7a5d0e4e748f6822 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Wed, 15 Apr 2026 17:33:45 -0500 Subject: [PATCH 07/29] test: add unit tests for model --- openedx_authz/tests/test_models.py | 119 +++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) diff --git a/openedx_authz/tests/test_models.py b/openedx_authz/tests/test_models.py index 94e41a43..60371347 100644 --- a/openedx_authz/tests/test_models.py +++ b/openedx_authz/tests/test_models.py @@ -3,6 +3,7 @@ This test suite verifies the functionality of the authorization models including: - ExtendedCasbinRule model with metadata and relationships - Cascade deletion behavior across model hierarchies +- AuthzCourseAuthoringMigrationRun (migration tracking and uniqueness on RUNNING inserts) These tests use the stub ContentLibrary model from openedx_authz.tests.stubs.models instead of the real ContentLibrary model, allowing them to run without the full @@ -18,12 +19,19 @@ from casbin_adapter.models import CasbinRule from django.contrib.auth import get_user_model +from django.db import IntegrityError from django.test import TestCase from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_authz.api.data import ContentLibraryData, CourseOverviewData, UserData from openedx_authz.models import ExtendedCasbinRule, Scope, Subject +from openedx_authz.models.authz_migration import ( + AuthzCourseAuthoringMigrationRun, + MigrationType, + ScopeType, + Status, +) from openedx_authz.models.engine import PolicyCacheControl from openedx_authz.tests.stubs.models import ContentLibrary, CourseOverview @@ -311,3 +319,114 @@ def test_singleton_behavior(self): instance1.save() all_instances = PolicyCacheControl.objects.all() self.assertEqual(all_instances.count(), 1) + + +class TestAuthzCourseAuthoringMigrationRun(TestCase): + """Tests for ``AuthzCourseAuthoringMigrationRun`` lifecycle and uniqueness rules.""" + + def setUp(self): + self.scope_key = "course-v1:TestOrg+AuthzMigrationRun+2024" + + def test_create_running(self): + """``create_running`` stores RUNNING and optional metadata.""" + meta = {"batch": 1} + + run = AuthzCourseAuthoringMigrationRun.create_running( + MigrationType.FORWARD, ScopeType.COURSE, self.scope_key, metadata=meta + ) + + run.refresh_from_db() + self.assertEqual(run.migration_type, MigrationType.FORWARD) + self.assertEqual(run.scope_type, ScopeType.COURSE) + self.assertEqual(run.scope_key, self.scope_key) + self.assertEqual(run.status, Status.RUNNING) + self.assertEqual(run.metadata, meta) + + def test_create_skipped_merges_skip_reason(self): + """``create_skipped`` records SKIPPED and documents why.""" + run = AuthzCourseAuthoringMigrationRun.create_skipped( + MigrationType.ROLLBACK, ScopeType.ORG, "test_org", metadata={"note": "extra"} + ) + + self.assertEqual(run.status, Status.SKIPPED) + self.assertEqual(run.metadata["note"], "extra") + self.assertIn("skip_reason", run.metadata) + + def test_str_representation(self): + """``__str__`` includes id, migration type, scope, and status.""" + run = AuthzCourseAuthoringMigrationRun.create_running(MigrationType.FORWARD, ScopeType.COURSE, self.scope_key) + + text = str(run) + self.assertIn(str(run.id), text) + self.assertIn(MigrationType.FORWARD.value, text) + self.assertIn(self.scope_key, text) + self.assertIn(Status.RUNNING.value, text) + + def test_second_running_insert_same_scope_raises_integrity_error(self): + """At most one RUNNING row per (scope_type, scope_key) on insert.""" + AuthzCourseAuthoringMigrationRun.create_running(MigrationType.FORWARD, ScopeType.COURSE, self.scope_key) + + with self.assertRaises(IntegrityError): + AuthzCourseAuthoringMigrationRun.create_running(MigrationType.FORWARD, ScopeType.COURSE, self.scope_key) + + def test_running_allowed_after_previous_run_finished(self): + """A new RUNNING row is allowed once the prior run for that scope is no longer RUNNING.""" + first = AuthzCourseAuthoringMigrationRun.create_running(MigrationType.FORWARD, ScopeType.COURSE, self.scope_key) + first.mark_completed() + + second = AuthzCourseAuthoringMigrationRun.create_running( + MigrationType.FORWARD, ScopeType.COURSE, self.scope_key + ) + + self.assertEqual(second.status, Status.RUNNING) + self.assertNotEqual(first.pk, second.pk) + + def test_distinct_scope_type_allows_parallel_running_same_key_string(self): + """RUNNING uniqueness is per (scope_type, scope_key), not key alone.""" + key = "same-key-string" + + AuthzCourseAuthoringMigrationRun.create_running(MigrationType.FORWARD, ScopeType.COURSE, key) + other = AuthzCourseAuthoringMigrationRun.create_running(MigrationType.FORWARD, ScopeType.ORG, key) + + self.assertEqual(other.status, Status.RUNNING) + + def test_mark_completed_and_partial_success_merge_metadata(self): + """Finalizers set status, ``completed_at``, and merge metadata.""" + run = AuthzCourseAuthoringMigrationRun.create_running( + MigrationType.FORWARD, ScopeType.COURSE, self.scope_key, metadata={"a": 1} + ) + run.mark_completed(metadata_updates={"success_count": 3}) + + run.refresh_from_db() + self.assertEqual(run.status, Status.COMPLETED) + self.assertIsNotNone(run.completed_at) + self.assertEqual(run.metadata["a"], 1) + self.assertEqual(run.metadata["success_count"], 3) + + run2 = AuthzCourseAuthoringMigrationRun.create_running(MigrationType.ROLLBACK, ScopeType.ORG, "org_partial") + run2.mark_partial_success(metadata_updates={"error_count": 2}) + + run2.refresh_from_db() + self.assertEqual(run2.status, Status.PARTIAL_SUCCESS) + self.assertEqual(run2.metadata["error_count"], 2) + + def test_mark_failed_with_and_without_exception(self): + """``mark_failed`` records FAILED, optional exception string is stored when provided.""" + run = AuthzCourseAuthoringMigrationRun.create_running( + MigrationType.FORWARD, ScopeType.COURSE, f"{self.scope_key}_fail" + ) + run.mark_failed(exception=ValueError("boom")) + + run.refresh_from_db() + self.assertEqual(run.status, Status.FAILED) + self.assertEqual(run.metadata["error"], "boom") + + run2 = AuthzCourseAuthoringMigrationRun.create_running( + MigrationType.FORWARD, ScopeType.COURSE, f"{self.scope_key}_fail2" + ) + prior_meta = dict(run2.metadata) + run2.mark_failed() + + run2.refresh_from_db() + self.assertEqual(run2.status, Status.FAILED) + self.assertEqual(run2.metadata, prior_meta) From 7c1d8e5df5599dc92f57d941d44f796932690e55 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Wed, 15 Apr 2026 17:36:48 -0500 Subject: [PATCH 08/29] test: add no_pii annotation to waffle flag stub models --- openedx_authz/tests/stubs/models.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/openedx_authz/tests/stubs/models.py b/openedx_authz/tests/stubs/models.py index 38309a84..9c691d0c 100644 --- a/openedx_authz/tests/stubs/models.py +++ b/openedx_authz/tests/stubs/models.py @@ -209,7 +209,10 @@ class CourseAccessRole(models.Model): # Waffle flag models class WaffleFlagCourseOverrideModel(models.Model): - """Stub model representing a waffle flag course override for testing purposes.""" + """Stub model representing a waffle flag course override for testing purposes. + + .. no_pii: + """ course_id = CourseKeyField(max_length=255, db_index=True) waffle_flag = models.CharField(max_length=255, db_index=True, default="") @@ -218,7 +221,10 @@ class WaffleFlagCourseOverrideModel(models.Model): class WaffleFlagOrgOverrideModel(models.Model): - """Stub model representing a waffle flag org override for testing purposes.""" + """Stub model representing a waffle flag org override for testing purposes. + + .. no_pii: + """ org = models.CharField(max_length=64, db_index=True) waffle_flag = models.CharField(max_length=255, db_index=True, default="") From 69ac955bc4dd06f7033dde062efa2c91ca0929c8 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Thu, 16 Apr 2026 08:20:31 -0500 Subject: [PATCH 09/29] refactor: always return instance --- openedx_authz/engine/utils.py | 10 +++++----- openedx_authz/models/authz_migration.py | 19 ++++++++++--------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index e25280c3..af5c7961 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -503,7 +503,7 @@ def run_course_authoring_migration( course_id_list=None, org_id=None, delete_after_migration=True, -) -> None: +) -> AuthzCourseAuthoringMigrationRun: """ Orchestrate a course authoring role migration with concurrency protection and lifecycle tracking. @@ -538,8 +538,7 @@ def run_course_authoring_migration( logger.warning( "Skipping %s migration for %s:%s — an active run already exists.", migration_type, scope_type, scope_key ) - AuthzCourseAuthoringMigrationRun.create_skipped(migration_type, scope_type, scope_key) - return + return AuthzCourseAuthoringMigrationRun.create_skipped(migration_type, scope_type, scope_key) logger.info("Started %s migration run [%s] for %s:%s", migration_type, run.id, scope_type, scope_key) @@ -559,8 +558,7 @@ def run_course_authoring_migration( logger.exception( "Unexpected error in migration run [%s] for %s:%s", run.id, scope_type, scope_key, exc_info=exc ) - run.mark_failed(exception=exc) - return + return run.mark_failed(exception=exc) errors_by_reason: dict = defaultdict(list) for entry in errors: @@ -586,6 +584,7 @@ def run_course_authoring_migration( len(successes), len(errors), ) + return run else: run.mark_completed(metadata_updates=metadata_updates) logger.info( @@ -596,3 +595,4 @@ def run_course_authoring_migration( scope_key, len(successes), ) + return run diff --git a/openedx_authz/models/authz_migration.py b/openedx_authz/models/authz_migration.py index 65d5dcde..67986b5d 100644 --- a/openedx_authz/models/authz_migration.py +++ b/openedx_authz/models/authz_migration.py @@ -93,7 +93,7 @@ class Meta: models.Index(fields=["-created_at"]), ] - def save(self, *args, **kwargs) -> None: + def save(self, *args, **kwargs) -> "AuthzCourseAuthoringMigrationRun": """Enforce at most one RUNNING record per (scope_type, scope_key). MySQL does not support partial unique indexes, so this check is done at @@ -112,6 +112,7 @@ def save(self, *args, **kwargs) -> None: f"Duplicate RUNNING migration run for scope {self.scope_type}:{self.scope_key}" ) super().save(*args, **kwargs) + return self # pylint: disable=too-many-positional-arguments @classmethod @@ -149,25 +150,25 @@ def create_skipped( extra = {**(metadata or {}), "skip_reason": "A concurrent migration run is already active for this scope."} return cls._create(migration_type, scope_type, scope_key, Status.SKIPPED, extra) - def _finalize(self, status: str, metadata_updates: dict | None = None) -> None: + def _finalize(self, status: str, metadata_updates: dict | None = None) -> "AuthzCourseAuthoringMigrationRun": """Finalize the migration run.""" self.status = status self.completed_at = timezone.now() if metadata_updates: self.metadata = {**(self.metadata or {}), **metadata_updates} - self.save(update_fields=["status", "completed_at", "updated_at", "metadata"]) + return self.save(update_fields=["status", "completed_at", "updated_at", "metadata"]) - def mark_partial_success(self, *, metadata_updates=None) -> None: + def mark_partial_success(self, *, metadata_updates=None) -> "AuthzCourseAuthoringMigrationRun": """Mark the migration run as partially successful.""" - self._finalize(Status.PARTIAL_SUCCESS, metadata_updates) + return self._finalize(Status.PARTIAL_SUCCESS, metadata_updates) - def mark_completed(self, *, metadata_updates=None) -> None: + def mark_completed(self, *, metadata_updates=None) -> "AuthzCourseAuthoringMigrationRun": """Mark the migration run as completed.""" - self._finalize(Status.COMPLETED, metadata_updates) + return self._finalize(Status.COMPLETED, metadata_updates) - def mark_failed(self, *, exception=None) -> None: + def mark_failed(self, *, exception=None) -> "AuthzCourseAuthoringMigrationRun": """Mark the migration run as failed.""" - self._finalize(Status.FAILED, {"error": str(exception)} if exception is not None else None) + return self._finalize(Status.FAILED, {"error": str(exception)} if exception is not None else None) def __str__(self) -> str: """Return a string representation of the migration run.""" From 54f9704ebe0f2a31ad6b4d793d3f6ad5651b12b1 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Thu, 16 Apr 2026 10:04:35 -0500 Subject: [PATCH 10/29] test: add unit tests for course authoring migration functionality --- openedx_authz/tests/test_migrations.py | 347 +++++++++++++++++++++++++ 1 file changed, 347 insertions(+) diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index 3b8138ee..504ffda9 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -6,6 +6,7 @@ from django.contrib.auth.models import Group from django.core.management import CommandError, call_command from django.test import TestCase +from opaque_keys.edx.django.models import CourseKeyField from openedx_authz.api.data import OrgCourseOverviewGlobData from openedx_authz.api.users import ( @@ -16,6 +17,7 @@ from openedx_authz.constants.roles import ( COURSE_ADMIN, COURSE_DATA_RESEARCHER, + COURSE_EDITOR, COURSE_LIMITED_STAFF, COURSE_STAFF, LEGACY_COURSE_ROLE_EQUIVALENCES, @@ -24,9 +26,18 @@ ) from openedx_authz.engine.enforcer import AuthzEnforcer from openedx_authz.engine.utils import ( + MigrationErrorReason, + MigrationMetadata, migrate_authz_to_legacy_course_roles, migrate_legacy_course_roles_to_authz, migrate_legacy_permissions, + run_course_authoring_migration, +) +from openedx_authz.models.authz_migration import ( + AuthzCourseAuthoringMigrationRun, + MigrationType, + ScopeType, + Status, ) from openedx_authz.models.subjects import UserSubject from openedx_authz.tests.stubs.models import ( @@ -1413,3 +1424,339 @@ def test_course_id_list_filter_excludes_glob_and_other_courses(self): # library assignment in self.org is excluded — library scopes are not course scopes self.assertNotIn(user_lib.username, migrated_users) self.assertEqual(len(errors), 0) + + +class TestRunCourseAuthoringMigration(TestCase): + """Exercise ``run_course_authoring_migration`` lifecycle and outcomes using stub ``CourseAccessRole``.""" + + def setUp(self): + self.course_id = "course-v1:TestOrg+TestCourse+2024" + self.org_id = "TestOrg" + + def test_skipped_when_another_run_is_already_running_for_scope(self): + """Second call for the same scope creates a SKIPPED run and returns without migrating.""" + AuthzCourseAuthoringMigrationRun.create_running(MigrationType.FORWARD, ScopeType.COURSE, self.course_id) + + run_course_authoring_migration( + migration_type=MigrationType.FORWARD, + scope_type=ScopeType.COURSE, + scope_key=self.course_id, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=[self.course_id], + org_id=None, + delete_after_migration=False, + ) + + self.assertTrue( + AuthzCourseAuthoringMigrationRun.objects.filter(scope_key=self.course_id, status=Status.SKIPPED).exists() + ) + self.assertEqual(AuthzCourseAuthoringMigrationRun.objects.filter(scope_key=self.course_id).count(), 2) + + def test_forward_completes_when_no_legacy_rows_match(self): + """No matching ``CourseAccessRole`` rows yields a completed run with zero successes.""" + run_course_authoring_migration( + migration_type=MigrationType.FORWARD, + scope_type=ScopeType.COURSE, + scope_key=self.course_id, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=[self.course_id], + org_id=None, + delete_after_migration=False, + ) + + run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.course_id, status=Status.COMPLETED) + self.assertEqual(run.metadata.get("success_count"), 0) + self.assertEqual(run.metadata.get("error_count"), 0) + + def test_marks_failed_when_migration_input_invalid(self): + """``_validate_migration_input`` raises (e.g. non-course-v1 keys); orchestration marks FAILED.""" + invalid_scope_key = "ccx-v1:Org+Course+Run" + + run_course_authoring_migration( + migration_type=MigrationType.FORWARD, + scope_type=ScopeType.COURSE, + scope_key=invalid_scope_key, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=[invalid_scope_key], + org_id=None, + delete_after_migration=False, + ) + + run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=invalid_scope_key, status=Status.FAILED) + self.assertIn("error", run.metadata) + + def test_forward_partial_success_when_legacy_role_unknown(self): + """Unknown legacy role strings are reported as errors; run ends PARTIAL_SUCCESS.""" + user = User.objects.create_user(username="legacy_unknown_user", email="legacy_unknown_user@example.com") + CourseAccessRole.objects.create( + user=user, org=self.org_id, course_id=self.course_id, role="not_a_defined_legacy_role" + ) + + run_course_authoring_migration( + migration_type=MigrationType.FORWARD, + scope_type=ScopeType.COURSE, + scope_key=self.course_id, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=[self.course_id], + org_id=None, + delete_after_migration=False, + ) + + run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.course_id, status=Status.PARTIAL_SUCCESS) + self.assertGreaterEqual(run.metadata.get("error_count"), 1) + self.assertEqual(run.metadata.get("success_count"), 0) + self.assertIn(MigrationErrorReason.UNKNOWN_ROLE, run.metadata.get("errors")) + + def test_rollback_completes_when_no_role_assignments(self): + """Rollback with no matching authz assignments finishes as COMPLETED with empty tallies.""" + run_course_authoring_migration( + migration_type=MigrationType.ROLLBACK, + scope_type=ScopeType.COURSE, + scope_key=self.course_id, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=[self.course_id], + org_id=None, + delete_after_migration=False, + ) + + run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.course_id, status=Status.COMPLETED) + self.assertEqual(run.metadata.get("error_count"), 0) + self.assertEqual(run.metadata.get("success_count"), 0) + + def test_rollback_partial_success_when_authz_role_unknown(self): + """Roles with no legacy ``CourseAccessRole`` mapping (e.g. ``course_editor``) → PARTIAL_SUCCESS.""" + user = User.objects.create_user(username="rb_no_legacy_user", email="rb_no_legacy_user@example.com") + assign_role_to_user_in_scope(user.username, COURSE_EDITOR.external_key, self.course_id) + + run_course_authoring_migration( + migration_type=MigrationType.ROLLBACK, + scope_type=ScopeType.COURSE, + scope_key=self.course_id, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=[self.course_id], + org_id=None, + delete_after_migration=False, + ) + + run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.course_id, status=Status.PARTIAL_SUCCESS) + self.assertGreaterEqual(run.metadata.get("error_count", 0), 1) + self.assertEqual(run.metadata.get("success_count"), 0) + self.assertIn(MigrationErrorReason.NO_LEGACY_EQUIVALENT, run.metadata.get("errors", {})) + + def test_forward_partial_success_when_assignment_failed_on_duplicate(self): + """Second FORWARD over the same legacy row hits duplicate AuthZ assignment → ``ASSIGNMENT_FAILED``.""" + user = User.objects.create_user(username="dup_assign_user", email="dup_assign_user@example.com") + CourseAccessRole.objects.create(user=user, org=self.org_id, course_id=self.course_id, role="staff") + common = { + "migration_type": MigrationType.FORWARD, + "scope_type": ScopeType.COURSE, + "scope_key": self.course_id, + "course_access_role_model": CourseAccessRole, + "user_subject_model": UserSubject, + "course_id_list": [self.course_id], + "org_id": None, + "delete_after_migration": False, + } + + run_course_authoring_migration(**common) + run_obj = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.course_id, status=Status.COMPLETED) + self.assertEqual(run_obj.metadata.get("success_count"), 1) + + run_course_authoring_migration(**common) + run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.course_id, status=Status.PARTIAL_SUCCESS) + self.assertEqual(run.metadata.get("success_count"), 0) + self.assertGreaterEqual(run.metadata.get("error_count"), 1) + self.assertIn(MigrationErrorReason.ASSIGNMENT_FAILED, run.metadata.get("errors", {})) + + @patch("openedx_authz.engine.utils.migrate_legacy_course_roles_to_authz") + def test_forward_partial_success_when_no_scope(self, mock_migrate): + """``NO_SCOPE`` is surfaced when the migration body reports neither course nor org (orchestration path).""" + mock_migrate.return_value = ( + [ + MigrationMetadata( + subject="no_scope_user", + role="staff", + reason=MigrationErrorReason.NO_SCOPE, + details="neither course_id nor org", + ) + ], + [], + ) + + run_course_authoring_migration( + migration_type=MigrationType.FORWARD, + scope_type=ScopeType.COURSE, + scope_key=self.course_id, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=[self.course_id], + org_id=None, + delete_after_migration=False, + ) + + run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.course_id, status=Status.PARTIAL_SUCCESS) + self.assertIn(MigrationErrorReason.NO_SCOPE, run.metadata.get("errors", {})) + + @patch("openedx_authz.engine.utils.migrate_authz_to_legacy_course_roles") + def test_rollback_partial_success_when_unexpected_scope_type(self, mock_migrate): + """``UNEXPECTED_SCOPE_TYPE`` from rollback migration is grouped in run metadata.""" + mock_migrate.return_value = ( + [ + MigrationMetadata( + subject="u", + role="course_staff", + scope=self.course_id, + reason=MigrationErrorReason.UNEXPECTED_SCOPE_TYPE, + details="UnexpectedScope", + ) + ], + [], + ) + + run_course_authoring_migration( + migration_type=MigrationType.ROLLBACK, + scope_type=ScopeType.COURSE, + scope_key=self.course_id, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=[self.course_id], + org_id=None, + delete_after_migration=False, + ) + + run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.course_id, status=Status.PARTIAL_SUCCESS) + self.assertIn(MigrationErrorReason.UNEXPECTED_SCOPE_TYPE, run.metadata.get("errors", {})) + + @patch("openedx_authz.engine.utils.migrate_authz_to_legacy_course_roles") + def test_rollback_partial_success_when_unexpected_error(self, mock_migrate): + """Exceptions inside the rollback loop become ``UNEXPECTED_ERROR`` entries in metadata.""" + mock_migrate.return_value = ( + [ + MigrationMetadata( + subject="u", + role=COURSE_STAFF.external_key, + scope=self.course_id, + reason=MigrationErrorReason.UNEXPECTED_ERROR, + details="KeyError: 'missing'", + ) + ], + [], + ) + + run_course_authoring_migration( + migration_type=MigrationType.ROLLBACK, + scope_type=ScopeType.COURSE, + scope_key=self.course_id, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=[self.course_id], + org_id=None, + delete_after_migration=False, + ) + + run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.course_id, status=Status.PARTIAL_SUCCESS) + self.assertIn(MigrationErrorReason.UNEXPECTED_ERROR, run.metadata.get("errors", {})) + + def test_forward_completes_org_wide_legacy_when_org_id(self): + """FORWARD with ``org_id`` migrates org-level (no ``course_id``) legacy roles for that org.""" + user = User.objects.create_user(username="rcam_org_fwd_user", email="rcam_org_fwd_user@example.com") + CourseAccessRole.objects.create( + user=user, + org=self.org_id, + course_id=CourseKeyField.Empty, + role="instructor", + ) + + run_course_authoring_migration( + migration_type=MigrationType.FORWARD, + scope_type=ScopeType.ORG, + scope_key=self.org_id, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=None, + org_id=self.org_id, + delete_after_migration=False, + ) + + run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.org_id, status=Status.COMPLETED) + self.assertEqual(run.metadata.get("success_count"), 1) + self.assertEqual(run.metadata.get("error_count"), 0) + self.assertListEqual( + run.metadata.get("successes"), + [ + { + "subject": user.username, + "role": "instructor", + "scope": OrgCourseOverviewGlobData.build_external_key(self.org_id), + } + ], + ) + + def test_forward_completes_when_no_legacy_rows_for_org_id(self): + """FORWARD with ``org_id`` and no matching ``CourseAccessRole`` rows finishes with zero tallies.""" + run_course_authoring_migration( + migration_type=MigrationType.FORWARD, + scope_type=ScopeType.ORG, + scope_key=self.org_id, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=None, + org_id=self.org_id, + delete_after_migration=False, + ) + + run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.org_id, status=Status.COMPLETED) + self.assertEqual(run.metadata.get("success_count"), 0) + self.assertEqual(run.metadata.get("error_count"), 0) + + def test_rollback_completes_org_glob_assignment_when_org_id(self): + """ROLLBACK with ``org_id`` processes org glob / course scopes for that organization.""" + user = User.objects.create_user(username="rcam_org_rb_user", email="rcam_org_rb_user@example.com") + glob_scope = OrgCourseOverviewGlobData.build_external_key(self.org_id) + assign_role_to_user_in_scope(user.username, COURSE_STAFF.external_key, glob_scope) + AuthzEnforcer.get_enforcer().load_policy() + + run_course_authoring_migration( + migration_type=MigrationType.ROLLBACK, + scope_type=ScopeType.ORG, + scope_key=self.org_id, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=None, + org_id=self.org_id, + delete_after_migration=False, + ) + + run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.org_id, status=Status.COMPLETED) + self.assertGreaterEqual(run.metadata.get("success_count"), 1) + self.assertEqual(run.metadata.get("error_count"), 0) + self.assertListEqual( + run.metadata.get("successes"), + [{"subject": user.username, "role": COURSE_STAFF.external_key, "scope": glob_scope}], + ) + + def test_skipped_when_org_scope_run_already_running(self): + """Concurrent guard applies when ``scope_type`` is ORG and ``scope_key`` is the org id.""" + AuthzCourseAuthoringMigrationRun.create_running(MigrationType.FORWARD, ScopeType.ORG, self.org_id) + + run_course_authoring_migration( + migration_type=MigrationType.FORWARD, + scope_type=ScopeType.ORG, + scope_key=self.org_id, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=None, + org_id=self.org_id, + delete_after_migration=False, + ) + + self.assertTrue( + AuthzCourseAuthoringMigrationRun.objects.filter(scope_key=self.org_id, status=Status.SKIPPED).exists() + ) + self.assertEqual(AuthzCourseAuthoringMigrationRun.objects.filter(scope_key=self.org_id).count(), 2) From 5e9fe46354bdc38a06f13b0d7796dd40fd0ea335 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Thu, 16 Apr 2026 10:07:16 -0500 Subject: [PATCH 11/29] chore: reorder conditions in course authoring migration handler --- openedx_authz/handlers.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/openedx_authz/handlers.py b/openedx_authz/handlers.py index 4109b42e..f2ebee7e 100644 --- a/openedx_authz/handlers.py +++ b/openedx_authz/handlers.py @@ -155,11 +155,11 @@ def trigger_course_authoring_migration( instance: The waffle flag instance that triggered the migration. scope_key (str): Course ID or organization name. """ - if not settings.ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION: - logger.info("ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION is set to False, skipping migration") + if instance.waffle_flag != AUTHZ_COURSE_AUTHORING_FLAG.name: return - if instance.waffle_flag != AUTHZ_COURSE_AUTHORING_FLAG.name: + if not settings.ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION: + logger.info("ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION is set to False, skipping migration") return course_id_list, org_id, scope_type = None, None, None From 61131ef8510acb3f91fe60b12709d62dbcc5399a Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Thu, 16 Apr 2026 11:24:19 -0500 Subject: [PATCH 12/29] refactor: run migration run save and running guard in one transaction --- openedx_authz/models/authz_migration.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/openedx_authz/models/authz_migration.py b/openedx_authz/models/authz_migration.py index 67986b5d..3e9f8fb2 100644 --- a/openedx_authz/models/authz_migration.py +++ b/openedx_authz/models/authz_migration.py @@ -100,8 +100,8 @@ def save(self, *args, **kwargs) -> "AuthzCourseAuthoringMigrationRun": the application level. select_for_update() is used to reduce (but not fully eliminate) the race-condition window on concurrent inserts. """ - if self.status == Status.RUNNING and self.pk is None: - with transaction.atomic(): + with transaction.atomic(): + if self.status == Status.RUNNING and self.pk is None: conflict = ( self.__class__.objects.select_for_update() .filter(scope_type=self.scope_type, scope_key=self.scope_key, status=Status.RUNNING) @@ -111,7 +111,7 @@ def save(self, *args, **kwargs) -> "AuthzCourseAuthoringMigrationRun": raise IntegrityError( f"Duplicate RUNNING migration run for scope {self.scope_type}:{self.scope_key}" ) - super().save(*args, **kwargs) + super().save(*args, **kwargs) return self # pylint: disable=too-many-positional-arguments From de7da52798e2575c34b608034108b89743431696 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Thu, 16 Apr 2026 14:14:48 -0500 Subject: [PATCH 13/29] chore: bump version to 1.12.0 --- CHANGELOG.rst | 8 ++++++++ openedx_authz/__init__.py | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 49f07b03..88d17853 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -14,6 +14,14 @@ Change Log Unreleased ********** +1.12.0 - 2026-04-16 +******************* + +Added +===== + +* Add automatic course authoring migration mechanism triggered by the ``authz.enable_course_authoring`` waffle flag when it is toggled at course or organization scope. + 1.11.0 - 2026-04-16 ******************* diff --git a/openedx_authz/__init__.py b/openedx_authz/__init__.py index c6f61fe9..e04c85ea 100644 --- a/openedx_authz/__init__.py +++ b/openedx_authz/__init__.py @@ -4,6 +4,6 @@ import os -__version__ = "1.11.0" +__version__ = "1.12.0" ROOT_DIRECTORY = os.path.dirname(os.path.abspath(__file__)) From 5c7311b987f0274b1deb62986d8a5845b5cb2598 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Thu, 16 Apr 2026 14:19:51 -0500 Subject: [PATCH 14/29] refactor: remove indexes from AuthzCourseAuthoringMigrationRun model --- .../migrations/0008_authzcourseauthoringmigrationrun.py | 7 +------ openedx_authz/models/authz_migration.py | 5 ----- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/openedx_authz/migrations/0008_authzcourseauthoringmigrationrun.py b/openedx_authz/migrations/0008_authzcourseauthoringmigrationrun.py index e1c4ceba..7eaa02f0 100644 --- a/openedx_authz/migrations/0008_authzcourseauthoringmigrationrun.py +++ b/openedx_authz/migrations/0008_authzcourseauthoringmigrationrun.py @@ -1,4 +1,4 @@ -# Generated by Django 5.2.12 on 2026-04-14 20:21 +# Generated by Django 5.2.12 on 2026-04-16 20:29 from django.db import migrations, models @@ -73,11 +73,6 @@ class Migration(migrations.Migration): "verbose_name": "Course Authoring Migration Run", "verbose_name_plural": "Course Authoring Migration Runs", "ordering": ["-created_at"], - "indexes": [ - models.Index(fields=["scope_type", "scope_key"], name="openedx_aut_scope_t_d43a35_idx"), - models.Index(fields=["status"], name="openedx_aut_status_e34b60_idx"), - models.Index(fields=["-created_at"], name="openedx_aut_created_ab3e0a_idx"), - ], }, ), ] diff --git a/openedx_authz/models/authz_migration.py b/openedx_authz/models/authz_migration.py index 3e9f8fb2..1be9f8ce 100644 --- a/openedx_authz/models/authz_migration.py +++ b/openedx_authz/models/authz_migration.py @@ -87,11 +87,6 @@ class Meta: verbose_name = "Course Authoring Migration Run" verbose_name_plural = "Course Authoring Migration Runs" ordering = ["-created_at"] - indexes = [ - models.Index(fields=["scope_type", "scope_key"]), - models.Index(fields=["status"]), - models.Index(fields=["-created_at"]), - ] def save(self, *args, **kwargs) -> "AuthzCourseAuthoringMigrationRun": """Enforce at most one RUNNING record per (scope_type, scope_key). From 0ca823541c679d7b84dd4f2e49984f8a934460ad Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Fri, 17 Apr 2026 09:55:00 -0500 Subject: [PATCH 15/29] refactor: update MigrationErrorReason to use StrEnum --- openedx_authz/engine/utils.py | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index af5c7961..3cb02a26 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -7,6 +7,7 @@ import logging from collections import defaultdict from dataclasses import dataclass, field +from enum import StrEnum, auto from casbin import Enforcer from django.db import IntegrityError, transaction @@ -37,18 +38,21 @@ COURSE_ROLE_EQUIVALENCES = {v: k for k, v in LEGACY_COURSE_ROLE_EQUIVALENCES.items()} -class MigrationErrorReason: +class MigrationErrorReason(StrEnum): """String constants for categorising why a single role assignment failed during migration.""" # Forward (legacy → authz) reasons - UNKNOWN_ROLE = "unknown_role" - NO_SCOPE = "no_scope" - ASSIGNMENT_FAILED = "assignment_failed" + UNKNOWN_ROLE = auto() + NO_SCOPE = auto() + ASSIGNMENT_FAILED = auto() # Rollback (authz → legacy) reasons - UNEXPECTED_SCOPE_TYPE = "unexpected_scope_type" - NO_LEGACY_EQUIVALENT = "no_legacy_equivalent" - UNEXPECTED_ERROR = "unexpected_error" + UNEXPECTED_SCOPE_TYPE = auto() + NO_LEGACY_EQUIVALENT = auto() + UNEXPECTED_ERROR = auto() + + # Forward and rollback reasons + SKIPPED_FOR_FLAG_OVERRIDE = auto() @dataclass @@ -69,7 +73,7 @@ class MigrationMetadata: subject: str role: str scope: str = field(default="") - reason: str = field(default="") + reason: MigrationErrorReason | None = field(default=None) details: str = field(default="") def to_dict(self) -> dict: From 24285dfa107cc3dd28760eeff839af4b72ddf3d0 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Fri, 17 Apr 2026 12:46:45 -0500 Subject: [PATCH 16/29] feat: add course id exclusion logic for org migration based on authoring flag overrides --- openedx_authz/engine/utils.py | 63 +++++++++++++++++++++++++++++------ openedx_authz/handlers.py | 31 +++++++++++++++++ 2 files changed, 84 insertions(+), 10 deletions(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 3cb02a26..58195357 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -234,8 +234,13 @@ def _validate_migration_input(course_id_list, org_id): ) +# pylint: disable=too-many-statements def migrate_legacy_course_roles_to_authz( - course_access_role_model, course_id_list, org_id, delete_after_migration + course_access_role_model, + course_id_list, + org_id, + delete_after_migration, + excluded_course_ids: frozenset[str] = frozenset(), ) -> tuple[list[MigrationMetadata], list[MigrationMetadata]]: """ Migrate legacy course role data to the new Casbin-based authorization model. @@ -265,6 +270,8 @@ def migrate_legacy_course_roles_to_authz( param course_id_list: Optional list of course IDs to filter the migration. param org_id: Optional organization ID to filter the migration. param delete_after_migration: Whether to delete successfully migrated legacy permissions after migration. + param excluded_course_ids: Course ids for which migration is skipped (course-level override + opposes the org-level transition) """ _validate_migration_input(course_id_list, org_id) @@ -302,6 +309,15 @@ def migrate_legacy_course_roles_to_authz( permissions_with_errors.append(migration_metadata) continue + if permission.course_id and str(permission.course_id) in excluded_course_ids: + migration_metadata.reason = MigrationErrorReason.SKIPPED_FOR_FLAG_OVERRIDE + migration_metadata.scope = str(permission.course_id) + migration_metadata.details = ( + "Course-level authoring flag override opposes this organization-level transition" + ) + permissions_with_errors.append(migration_metadata) + continue + if permission.course_id: scope_external_key = str(permission.course_id) elif permission.org: @@ -353,9 +369,14 @@ def migrate_legacy_course_roles_to_authz( return permissions_with_errors, permissions_with_no_errors -# pylint: disable=too-many-statements +# pylint: disable=too-many-statements,too-many-positional-arguments def migrate_authz_to_legacy_course_roles( - course_access_role_model, user_subject_model, course_id_list, org_id, delete_after_migration + course_access_role_model, + user_subject_model, + course_id_list, + org_id, + delete_after_migration, + excluded_course_ids: frozenset[str] = frozenset(), ) -> tuple[list[MigrationMetadata], list[MigrationMetadata]]: """ Migrate permissions from the new Casbin-based authorization model back to the legacy CourseAccessRole model. @@ -377,7 +398,9 @@ def migrate_authz_to_legacy_course_roles( param course_id_list: Optional list of course IDs to filter the migration. param org_id: Optional organization ID to filter the migration. param delete_after_migration: Whether to unassign successfully migrated permissions - from the new model after migration. + from the new model after migration. + param excluded_course_ids: Course ids for which rollback is skipped (course-level override + opposes the org-level transition) """ _validate_migration_input(course_id_list, org_id) @@ -423,6 +446,14 @@ def migrate_authz_to_legacy_course_roles( subject=user_external_key, role=role_external_key, scope=scope_external_key ) + if isinstance(role_assignment.scope, CourseOverviewData) and scope_external_key in excluded_course_ids: + migration_metadata.reason = MigrationErrorReason.SKIPPED_FOR_FLAG_OVERRIDE + migration_metadata.details = ( + "Course-level authoring flag override opposes this organization-level transition" + ) + roles_with_errors.append(migration_metadata) + continue + legacy_role = COURSE_ROLE_EQUIVALENCES.get(role_external_key) if legacy_role is None: logger.error( @@ -503,10 +534,11 @@ def run_course_authoring_migration( scope_type: ScopeType, scope_key: str, course_access_role_model, - user_subject_model=None, - course_id_list=None, - org_id=None, - delete_after_migration=True, + user_subject_model, + course_id_list: list[str] | None, + org_id: str | None, + excluded_course_ids: frozenset[str], + delete_after_migration: bool, ) -> AuthzCourseAuthoringMigrationRun: """ Orchestrate a course authoring role migration with concurrency protection and lifecycle tracking. @@ -533,6 +565,8 @@ def run_course_authoring_migration( user_subject_model: The ``UserSubject`` model class; required for ``ROLLBACK``, ignored otherwise. course_id_list (list[str] | None): Restrict migration to these course-v1 keys. org_id (str | None): Restrict migration to this org; takes precedence over ``course_id_list``. + excluded_course_ids (frozenset[str]): For org-scoped runs, course ids + whose course-level waffle override opposes the org transition delete_after_migration (bool): Remove successfully migrated entries from the source system. """ try: @@ -550,11 +584,20 @@ def run_course_authoring_migration( with transaction.atomic(): if migration_type == MigrationType.FORWARD: errors, successes = migrate_legacy_course_roles_to_authz( - course_access_role_model, course_id_list, org_id, delete_after_migration + course_access_role_model, + course_id_list, + org_id, + delete_after_migration, + excluded_course_ids, ) else: errors, successes = migrate_authz_to_legacy_course_roles( - course_access_role_model, user_subject_model, course_id_list, org_id, delete_after_migration + course_access_role_model, + user_subject_model, + course_id_list, + org_id, + delete_after_migration, + excluded_course_ids, ) except Exception as exc: # pylint: disable=broad-exception-caught # The inner atomic block is rolled back on exception; mark_failed() runs diff --git a/openedx_authz/handlers.py b/openedx_authz/handlers.py index f2ebee7e..0d3b7196 100644 --- a/openedx_authz/handlers.py +++ b/openedx_authz/handlers.py @@ -138,6 +138,29 @@ def handle_org_waffle_flag_change(sender, instance, **kwargs) -> None: post_save.connect(handle_org_waffle_flag_change, sender=WaffleFlagOrgOverrideModel) +def get_excluded_course_ids_for_org_migration(org_id: str, new_org_enabled: bool) -> frozenset[str]: + """ + Collect course-level authoring flag overrides for an org that oppose the new org-level state. + + When the org flag changes, we need to exclude course ids that have a course-level + authoring flag override that opposes the new org-level state. + + Args: + org_id (str): Organization short name. + new_org_enabled (bool): Effective org flag value after the save. + + Returns: + frozenset[str]: course ids excluded from org migration + """ + # We only need to check the current set (active flags) + qs = WaffleFlagCourseOverrideModel.objects.current_set().filter( + enabled=not new_org_enabled, + waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG.name, + course_id__startswith=f"course-v1:{org_id}+", + ) + return frozenset(map(str, qs.values_list("course_id", flat=True))) + + def trigger_course_authoring_migration( sender: type[WaffleFlagCourseOverrideModel | WaffleFlagOrgOverrideModel], instance: WaffleFlagCourseOverrideModel | WaffleFlagOrgOverrideModel, @@ -184,6 +207,12 @@ def trigger_course_authoring_migration( migration_type = MigrationType.FORWARD if instance.enabled else MigrationType.ROLLBACK + excluded_course_ids = frozenset() + if isinstance(instance, WaffleFlagOrgOverrideModel): + excluded_course_ids = get_excluded_course_ids_for_org_migration( + org_id=scope_key, new_org_enabled=instance.enabled + ) + logger.info("Triggering %s migration for %s:%s due to waffle flag change", migration_type, scope_type, scope_key) run_course_authoring_migration( @@ -194,4 +223,6 @@ def trigger_course_authoring_migration( user_subject_model=UserSubject, course_id_list=course_id_list, org_id=org_id, + excluded_course_ids=excluded_course_ids, + delete_after_migration=True, ) From cf3faff15115bbbf589cf2a7d2107995489ad047 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Fri, 17 Apr 2026 14:56:16 -0500 Subject: [PATCH 17/29] feat: enhance course authoring migration logic with effective override state checks --- openedx_authz/handlers.py | 42 ++++++++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/openedx_authz/handlers.py b/openedx_authz/handlers.py index 0d3b7196..d56ad145 100644 --- a/openedx_authz/handlers.py +++ b/openedx_authz/handlers.py @@ -137,8 +137,13 @@ def handle_org_waffle_flag_change(sender, instance, **kwargs) -> None: if WaffleFlagOrgOverrideModel is not None: post_save.connect(handle_org_waffle_flag_change, sender=WaffleFlagOrgOverrideModel) +# Match ``WaffleFlagCourseOverrideModel.OVERRIDE_CHOICES`` / ``override_value`` in edx-platform: +# the flag is effectively forced on only when the row is enabled and ``override_choice`` is "on". +WAFFLE_OVERRIDE_FORCE_ON = "on" +WAFFLE_OVERRIDE_FORCE_OFF = "off" -def get_excluded_course_ids_for_org_migration(org_id: str, new_org_enabled: bool) -> frozenset[str]: + +def get_excluded_course_ids_for_org_migration(org_id: str, reverse_choice: str) -> frozenset[str]: """ Collect course-level authoring flag overrides for an org that oppose the new org-level state. @@ -147,17 +152,20 @@ def get_excluded_course_ids_for_org_migration(org_id: str, new_org_enabled: bool Args: org_id (str): Organization short name. - new_org_enabled (bool): Effective org flag value after the save. + reverse_choice (str): The reverse choice of the org waffle flag. Returns: frozenset[str]: course ids excluded from org migration """ - # We only need to check the current set (active flags) - qs = WaffleFlagCourseOverrideModel.objects.current_set().filter( - enabled=not new_org_enabled, - waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG.name, - course_id__startswith=f"course-v1:{org_id}+", - ) + # We only need to check the current set (active flags). Opposing overrides are rows that + # force the opposite of the org transition (Force On vs Force Off), not merely inactive rows. + filter_kwargs = { + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG.name, + "course_id__startswith": f"course-v1:{org_id}+", + "enabled": True, + "override_choice": reverse_choice, + } + qs = WaffleFlagCourseOverrideModel.objects.current_set().filter(**filter_kwargs) return frozenset(map(str, qs.values_list("course_id", flat=True))) @@ -181,6 +189,10 @@ def trigger_course_authoring_migration( if instance.waffle_flag != AUTHZ_COURSE_AUTHORING_FLAG.name: return + if not instance.enabled: + logger.info("Waffle flag is disabled, skipping migration") + return + if not settings.ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION: logger.info("ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION is set to False, skipping migration") return @@ -201,16 +213,22 @@ def trigger_course_authoring_migration( prev_record = sender.objects.filter(**filter_kwargs).exclude(id=instance.id).order_by("-change_date").first() - if prev_record and prev_record.enabled == instance.enabled: - logger.info("No change in waffle flag, skipping course migration") + if prev_record and prev_record.enabled and instance.override_choice == prev_record.override_choice: + logger.info("No change in waffle flag, skipping migration") return - migration_type = MigrationType.FORWARD if instance.enabled else MigrationType.ROLLBACK + if instance.override_choice == WAFFLE_OVERRIDE_FORCE_ON: + migration_type = MigrationType.FORWARD + reverse_choice = WAFFLE_OVERRIDE_FORCE_OFF + else: + migration_type = MigrationType.ROLLBACK + reverse_choice = WAFFLE_OVERRIDE_FORCE_ON excluded_course_ids = frozenset() if isinstance(instance, WaffleFlagOrgOverrideModel): excluded_course_ids = get_excluded_course_ids_for_org_migration( - org_id=scope_key, new_org_enabled=instance.enabled + org_id=scope_key, + reverse_choice=reverse_choice, ) logger.info("Triggering %s migration for %s:%s due to waffle flag change", migration_type, scope_type, scope_key) From fedaeb6be73c04886b0d70b18189349de7e5635b Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Sat, 18 Apr 2026 21:29:06 -0500 Subject: [PATCH 18/29] test: add WaffleStubManager and override_choice field to waffle flag models --- openedx_authz/tests/stubs/models.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/openedx_authz/tests/stubs/models.py b/openedx_authz/tests/stubs/models.py index 9c691d0c..8bbe8b42 100644 --- a/openedx_authz/tests/stubs/models.py +++ b/openedx_authz/tests/stubs/models.py @@ -208,6 +208,13 @@ class CourseAccessRole(models.Model): # Waffle flag models +class _WaffleStubManager(models.Manager): + """Minimal stand-in for ``ConfigurationModel`` managers exposing ``current_set()``.""" + + def current_set(self): + return self.get_queryset() + + class WaffleFlagCourseOverrideModel(models.Model): """Stub model representing a waffle flag course override for testing purposes. @@ -217,7 +224,9 @@ class WaffleFlagCourseOverrideModel(models.Model): course_id = CourseKeyField(max_length=255, db_index=True) waffle_flag = models.CharField(max_length=255, db_index=True, default="") enabled = models.BooleanField(default=False) + override_choice = models.CharField(max_length=3, default="on") change_date = models.DateTimeField(auto_now_add=True) + objects = _WaffleStubManager() class WaffleFlagOrgOverrideModel(models.Model): @@ -229,4 +238,6 @@ class WaffleFlagOrgOverrideModel(models.Model): org = models.CharField(max_length=64, db_index=True) waffle_flag = models.CharField(max_length=255, db_index=True, default="") enabled = models.BooleanField(default=False) + override_choice = models.CharField(max_length=3, default="on") change_date = models.DateTimeField(auto_now_add=True) + objects = _WaffleStubManager() From 406603e78ec67341b1e8e6d06eeafa043c1198c8 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Sun, 19 Apr 2026 08:26:19 -0500 Subject: [PATCH 19/29] test: enhance course authoring migration tests with ddt --- openedx_authz/tests/test_migrations.py | 492 +++++++++++++------------ 1 file changed, 253 insertions(+), 239 deletions(-) diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index 504ffda9..58879b65 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -2,6 +2,7 @@ from unittest.mock import MagicMock, patch +from ddt import data, ddt, unpack from django.contrib.auth import get_user_model from django.contrib.auth.models import Group from django.core.management import CommandError, call_command @@ -34,7 +35,9 @@ run_course_authoring_migration, ) from openedx_authz.models.authz_migration import ( - AuthzCourseAuthoringMigrationRun, + AuthzCourseAuthoringMigrationRun as MigrationRun, +) +from openedx_authz.models.authz_migration import ( MigrationType, ScopeType, Status, @@ -1426,153 +1429,155 @@ def test_course_id_list_filter_excludes_glob_and_other_courses(self): self.assertEqual(len(errors), 0) +@ddt class TestRunCourseAuthoringMigration(TestCase): """Exercise ``run_course_authoring_migration`` lifecycle and outcomes using stub ``CourseAccessRole``.""" - def setUp(self): - self.course_id = "course-v1:TestOrg+TestCourse+2024" - self.org_id = "TestOrg" - - def test_skipped_when_another_run_is_already_running_for_scope(self): + COURSE_ID = "course-v1:TestOrg+TestCourse+2024" + ORG_ID = "TestOrg" + + @data( + (MigrationType.FORWARD, ScopeType.COURSE, None, [COURSE_ID]), + (MigrationType.ROLLBACK, ScopeType.COURSE, None, [COURSE_ID]), + (MigrationType.FORWARD, ScopeType.ORG, ORG_ID, None), + (MigrationType.ROLLBACK, ScopeType.ORG, ORG_ID, None), + ) + @unpack + def test_skipped_when_another_run_is_already_running_for_scope( + self, migration_type, scope_type, org_id, course_id_list + ): """Second call for the same scope creates a SKIPPED run and returns without migrating.""" - AuthzCourseAuthoringMigrationRun.create_running(MigrationType.FORWARD, ScopeType.COURSE, self.course_id) - - run_course_authoring_migration( - migration_type=MigrationType.FORWARD, - scope_type=ScopeType.COURSE, - scope_key=self.course_id, - course_access_role_model=CourseAccessRole, - user_subject_model=UserSubject, - course_id_list=[self.course_id], - org_id=None, - delete_after_migration=False, - ) - - self.assertTrue( - AuthzCourseAuthoringMigrationRun.objects.filter(scope_key=self.course_id, status=Status.SKIPPED).exists() - ) - self.assertEqual(AuthzCourseAuthoringMigrationRun.objects.filter(scope_key=self.course_id).count(), 2) + scope_key = org_id if scope_type == ScopeType.ORG else course_id_list[0] + MigrationRun.create_running(migration_type, scope_type, scope_key) - def test_forward_completes_when_no_legacy_rows_match(self): - """No matching ``CourseAccessRole`` rows yields a completed run with zero successes.""" run_course_authoring_migration( - migration_type=MigrationType.FORWARD, - scope_type=ScopeType.COURSE, - scope_key=self.course_id, + migration_type=migration_type, + scope_type=scope_type, + scope_key=scope_key, course_access_role_model=CourseAccessRole, user_subject_model=UserSubject, - course_id_list=[self.course_id], - org_id=None, + course_id_list=course_id_list, + org_id=org_id, + excluded_course_ids=frozenset(), delete_after_migration=False, ) - run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.course_id, status=Status.COMPLETED) - self.assertEqual(run.metadata.get("success_count"), 0) - self.assertEqual(run.metadata.get("error_count"), 0) + run = MigrationRun.objects.get(scope_key=scope_key, status=Status.SKIPPED) + self.assertEqual(run.migration_type, migration_type) + self.assertEqual(run.scope_type, scope_type) + self.assertEqual(run.scope_key, scope_key) + self.assertEqual(run.metadata, {"skip_reason": "A concurrent migration run is already active for this scope."}) - def test_marks_failed_when_migration_input_invalid(self): + @data( + (MigrationType.FORWARD, ScopeType.COURSE, None, ["ccx-v1:Org+Course+Run"]), + (MigrationType.ROLLBACK, ScopeType.COURSE, None, ["ccx-v1:Org+Course+Run"]), + ) + @unpack + def test_marks_failed_when_migration_input_invalid(self, migration_type, scope_type, org_id, course_id_list): """``_validate_migration_input`` raises (e.g. non-course-v1 keys); orchestration marks FAILED.""" - invalid_scope_key = "ccx-v1:Org+Course+Run" + scope_key = org_id if scope_type == ScopeType.ORG else course_id_list[0] if course_id_list else None run_course_authoring_migration( - migration_type=MigrationType.FORWARD, - scope_type=ScopeType.COURSE, - scope_key=invalid_scope_key, + migration_type=migration_type, + scope_type=scope_type, + scope_key=scope_key, course_access_role_model=CourseAccessRole, user_subject_model=UserSubject, - course_id_list=[invalid_scope_key], - org_id=None, + course_id_list=course_id_list, + org_id=org_id, + excluded_course_ids=frozenset(), delete_after_migration=False, ) - run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=invalid_scope_key, status=Status.FAILED) + run = MigrationRun.objects.get(scope_key=scope_key, status=Status.FAILED) self.assertIn("error", run.metadata) + self.assertEqual(run.migration_type, migration_type) + self.assertEqual(run.scope_type, scope_type) + self.assertEqual(run.scope_key, scope_key) def test_forward_partial_success_when_legacy_role_unknown(self): """Unknown legacy role strings are reported as errors; run ends PARTIAL_SUCCESS.""" user = User.objects.create_user(username="legacy_unknown_user", email="legacy_unknown_user@example.com") - CourseAccessRole.objects.create( - user=user, org=self.org_id, course_id=self.course_id, role="not_a_defined_legacy_role" - ) + role = "not_a_defined_legacy_role" + CourseAccessRole.objects.create(user=user, org=self.ORG_ID, course_id=self.COURSE_ID, role=role) run_course_authoring_migration( migration_type=MigrationType.FORWARD, scope_type=ScopeType.COURSE, - scope_key=self.course_id, + scope_key=self.COURSE_ID, course_access_role_model=CourseAccessRole, user_subject_model=UserSubject, - course_id_list=[self.course_id], + course_id_list=[self.COURSE_ID], org_id=None, + excluded_course_ids=frozenset(), delete_after_migration=False, ) - run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.course_id, status=Status.PARTIAL_SUCCESS) + run = MigrationRun.objects.get(scope_key=self.COURSE_ID, status=Status.PARTIAL_SUCCESS) self.assertGreaterEqual(run.metadata.get("error_count"), 1) self.assertEqual(run.metadata.get("success_count"), 0) self.assertIn(MigrationErrorReason.UNKNOWN_ROLE, run.metadata.get("errors")) - - def test_rollback_completes_when_no_role_assignments(self): - """Rollback with no matching authz assignments finishes as COMPLETED with empty tallies.""" - run_course_authoring_migration( - migration_type=MigrationType.ROLLBACK, - scope_type=ScopeType.COURSE, - scope_key=self.course_id, - course_access_role_model=CourseAccessRole, - user_subject_model=UserSubject, - course_id_list=[self.course_id], - org_id=None, - delete_after_migration=False, - ) - - run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.course_id, status=Status.COMPLETED) - self.assertEqual(run.metadata.get("error_count"), 0) - self.assertEqual(run.metadata.get("success_count"), 0) + error = run.metadata.get("errors")[MigrationErrorReason.UNKNOWN_ROLE][0] + self.assertEqual(error["subject"], user.username) + self.assertEqual(error["role"], role) + self.assertEqual(error["details"], f"Unknown access level: {role} for User: {user.username}") def test_rollback_partial_success_when_authz_role_unknown(self): """Roles with no legacy ``CourseAccessRole`` mapping (e.g. ``course_editor``) → PARTIAL_SUCCESS.""" user = User.objects.create_user(username="rb_no_legacy_user", email="rb_no_legacy_user@example.com") - assign_role_to_user_in_scope(user.username, COURSE_EDITOR.external_key, self.course_id) + role = COURSE_EDITOR.external_key + assign_role_to_user_in_scope(user.username, role, self.COURSE_ID) run_course_authoring_migration( migration_type=MigrationType.ROLLBACK, scope_type=ScopeType.COURSE, - scope_key=self.course_id, + scope_key=self.COURSE_ID, course_access_role_model=CourseAccessRole, user_subject_model=UserSubject, - course_id_list=[self.course_id], + course_id_list=[self.COURSE_ID], org_id=None, + excluded_course_ids=frozenset(), delete_after_migration=False, ) - run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.course_id, status=Status.PARTIAL_SUCCESS) - self.assertGreaterEqual(run.metadata.get("error_count", 0), 1) + run = MigrationRun.objects.get(scope_key=self.COURSE_ID, status=Status.PARTIAL_SUCCESS) + self.assertGreaterEqual(run.metadata.get("error_count"), 1) self.assertEqual(run.metadata.get("success_count"), 0) - self.assertIn(MigrationErrorReason.NO_LEGACY_EQUIVALENT, run.metadata.get("errors", {})) + self.assertIn(MigrationErrorReason.NO_LEGACY_EQUIVALENT, run.metadata.get("errors")) + error = run.metadata.get("errors")[MigrationErrorReason.NO_LEGACY_EQUIVALENT][0] + self.assertEqual(error["subject"], user.username) + self.assertEqual(error["role"], role) + self.assertEqual(error["details"], f"Role '{role}' has no legacy equivalent.") def test_forward_partial_success_when_assignment_failed_on_duplicate(self): """Second FORWARD over the same legacy row hits duplicate AuthZ assignment → ``ASSIGNMENT_FAILED``.""" user = User.objects.create_user(username="dup_assign_user", email="dup_assign_user@example.com") - CourseAccessRole.objects.create(user=user, org=self.org_id, course_id=self.course_id, role="staff") + CourseAccessRole.objects.create(user=user, org=self.ORG_ID, course_id=self.COURSE_ID, role="staff") common = { "migration_type": MigrationType.FORWARD, "scope_type": ScopeType.COURSE, - "scope_key": self.course_id, + "scope_key": self.COURSE_ID, "course_access_role_model": CourseAccessRole, "user_subject_model": UserSubject, - "course_id_list": [self.course_id], + "course_id_list": [self.COURSE_ID], "org_id": None, + "excluded_course_ids": frozenset(), "delete_after_migration": False, } run_course_authoring_migration(**common) - run_obj = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.course_id, status=Status.COMPLETED) + run_obj = MigrationRun.objects.get(scope_key=self.COURSE_ID, status=Status.COMPLETED) self.assertEqual(run_obj.metadata.get("success_count"), 1) run_course_authoring_migration(**common) - run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.course_id, status=Status.PARTIAL_SUCCESS) + run = MigrationRun.objects.get(scope_key=self.COURSE_ID, status=Status.PARTIAL_SUCCESS) self.assertEqual(run.metadata.get("success_count"), 0) self.assertGreaterEqual(run.metadata.get("error_count"), 1) - self.assertIn(MigrationErrorReason.ASSIGNMENT_FAILED, run.metadata.get("errors", {})) + self.assertIn(MigrationErrorReason.ASSIGNMENT_FAILED, run.metadata.get("errors")) + error = run.metadata.get("errors")[MigrationErrorReason.ASSIGNMENT_FAILED][0] + self.assertEqual(error["subject"], user.username) + self.assertEqual(error["role"], "staff") + self.assertEqual(error["details"], f"User '{user.username}' may already have this permission assigned") @patch("openedx_authz.engine.utils.migrate_legacy_course_roles_to_authz") def test_forward_partial_success_when_no_scope(self, mock_migrate): @@ -1583,7 +1588,7 @@ def test_forward_partial_success_when_no_scope(self, mock_migrate): subject="no_scope_user", role="staff", reason=MigrationErrorReason.NO_SCOPE, - details="neither course_id nor org", + details="User 'no_scope_user' has neither course_id nor org defined.", ) ], [], @@ -1592,171 +1597,180 @@ def test_forward_partial_success_when_no_scope(self, mock_migrate): run_course_authoring_migration( migration_type=MigrationType.FORWARD, scope_type=ScopeType.COURSE, - scope_key=self.course_id, + scope_key=self.COURSE_ID, course_access_role_model=CourseAccessRole, user_subject_model=UserSubject, - course_id_list=[self.course_id], + course_id_list=[self.COURSE_ID], org_id=None, + excluded_course_ids=frozenset(), delete_after_migration=False, ) - run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.course_id, status=Status.PARTIAL_SUCCESS) - self.assertIn(MigrationErrorReason.NO_SCOPE, run.metadata.get("errors", {})) - - @patch("openedx_authz.engine.utils.migrate_authz_to_legacy_course_roles") - def test_rollback_partial_success_when_unexpected_scope_type(self, mock_migrate): - """``UNEXPECTED_SCOPE_TYPE`` from rollback migration is grouped in run metadata.""" - mock_migrate.return_value = ( - [ - MigrationMetadata( - subject="u", - role="course_staff", - scope=self.course_id, - reason=MigrationErrorReason.UNEXPECTED_SCOPE_TYPE, - details="UnexpectedScope", - ) - ], - [], - ) - - run_course_authoring_migration( - migration_type=MigrationType.ROLLBACK, - scope_type=ScopeType.COURSE, - scope_key=self.course_id, - course_access_role_model=CourseAccessRole, - user_subject_model=UserSubject, - course_id_list=[self.course_id], - org_id=None, - delete_after_migration=False, - ) - - run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.course_id, status=Status.PARTIAL_SUCCESS) - self.assertIn(MigrationErrorReason.UNEXPECTED_SCOPE_TYPE, run.metadata.get("errors", {})) - - @patch("openedx_authz.engine.utils.migrate_authz_to_legacy_course_roles") - def test_rollback_partial_success_when_unexpected_error(self, mock_migrate): - """Exceptions inside the rollback loop become ``UNEXPECTED_ERROR`` entries in metadata.""" - mock_migrate.return_value = ( - [ - MigrationMetadata( - subject="u", - role=COURSE_STAFF.external_key, - scope=self.course_id, - reason=MigrationErrorReason.UNEXPECTED_ERROR, - details="KeyError: 'missing'", - ) - ], - [], - ) - - run_course_authoring_migration( - migration_type=MigrationType.ROLLBACK, - scope_type=ScopeType.COURSE, - scope_key=self.course_id, - course_access_role_model=CourseAccessRole, - user_subject_model=UserSubject, - course_id_list=[self.course_id], - org_id=None, - delete_after_migration=False, - ) - - run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.course_id, status=Status.PARTIAL_SUCCESS) - self.assertIn(MigrationErrorReason.UNEXPECTED_ERROR, run.metadata.get("errors", {})) - - def test_forward_completes_org_wide_legacy_when_org_id(self): - """FORWARD with ``org_id`` migrates org-level (no ``course_id``) legacy roles for that org.""" - user = User.objects.create_user(username="rcam_org_fwd_user", email="rcam_org_fwd_user@example.com") - CourseAccessRole.objects.create( - user=user, - org=self.org_id, - course_id=CourseKeyField.Empty, - role="instructor", - ) - - run_course_authoring_migration( - migration_type=MigrationType.FORWARD, - scope_type=ScopeType.ORG, - scope_key=self.org_id, - course_access_role_model=CourseAccessRole, - user_subject_model=UserSubject, - course_id_list=None, - org_id=self.org_id, - delete_after_migration=False, - ) - - run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.org_id, status=Status.COMPLETED) - self.assertEqual(run.metadata.get("success_count"), 1) - self.assertEqual(run.metadata.get("error_count"), 0) - self.assertListEqual( - run.metadata.get("successes"), - [ - { - "subject": user.username, - "role": "instructor", - "scope": OrgCourseOverviewGlobData.build_external_key(self.org_id), - } - ], - ) - - def test_forward_completes_when_no_legacy_rows_for_org_id(self): - """FORWARD with ``org_id`` and no matching ``CourseAccessRole`` rows finishes with zero tallies.""" - run_course_authoring_migration( - migration_type=MigrationType.FORWARD, - scope_type=ScopeType.ORG, - scope_key=self.org_id, - course_access_role_model=CourseAccessRole, - user_subject_model=UserSubject, - course_id_list=None, - org_id=self.org_id, - delete_after_migration=False, - ) - - run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.org_id, status=Status.COMPLETED) - self.assertEqual(run.metadata.get("success_count"), 0) - self.assertEqual(run.metadata.get("error_count"), 0) - - def test_rollback_completes_org_glob_assignment_when_org_id(self): - """ROLLBACK with ``org_id`` processes org glob / course scopes for that organization.""" - user = User.objects.create_user(username="rcam_org_rb_user", email="rcam_org_rb_user@example.com") - glob_scope = OrgCourseOverviewGlobData.build_external_key(self.org_id) - assign_role_to_user_in_scope(user.username, COURSE_STAFF.external_key, glob_scope) - AuthzEnforcer.get_enforcer().load_policy() - - run_course_authoring_migration( - migration_type=MigrationType.ROLLBACK, - scope_type=ScopeType.ORG, - scope_key=self.org_id, - course_access_role_model=CourseAccessRole, - user_subject_model=UserSubject, - course_id_list=None, - org_id=self.org_id, - delete_after_migration=False, - ) - - run = AuthzCourseAuthoringMigrationRun.objects.get(scope_key=self.org_id, status=Status.COMPLETED) - self.assertGreaterEqual(run.metadata.get("success_count"), 1) - self.assertEqual(run.metadata.get("error_count"), 0) - self.assertListEqual( - run.metadata.get("successes"), - [{"subject": user.username, "role": COURSE_STAFF.external_key, "scope": glob_scope}], - ) - - def test_skipped_when_org_scope_run_already_running(self): - """Concurrent guard applies when ``scope_type`` is ORG and ``scope_key`` is the org id.""" - AuthzCourseAuthoringMigrationRun.create_running(MigrationType.FORWARD, ScopeType.ORG, self.org_id) - - run_course_authoring_migration( - migration_type=MigrationType.FORWARD, - scope_type=ScopeType.ORG, - scope_key=self.org_id, - course_access_role_model=CourseAccessRole, - user_subject_model=UserSubject, - course_id_list=None, - org_id=self.org_id, - delete_after_migration=False, - ) - - self.assertTrue( - AuthzCourseAuthoringMigrationRun.objects.filter(scope_key=self.org_id, status=Status.SKIPPED).exists() - ) - self.assertEqual(AuthzCourseAuthoringMigrationRun.objects.filter(scope_key=self.org_id).count(), 2) + run = MigrationRun.objects.get(scope_key=self.COURSE_ID, status=Status.PARTIAL_SUCCESS) + self.assertIn(MigrationErrorReason.NO_SCOPE, run.metadata.get("errors")) + error = run.metadata.get("errors")[MigrationErrorReason.NO_SCOPE][0] + self.assertEqual(error["subject"], "no_scope_user") + self.assertEqual(error["role"], "staff") + self.assertEqual(error["details"], "User 'no_scope_user' has neither course_id nor org defined.") + + # @patch("openedx_authz.engine.utils.migrate_authz_to_legacy_course_roles") + # def test_rollback_partial_success_when_unexpected_scope_type(self, mock_migrate): + # """``UNEXPECTED_SCOPE_TYPE`` from rollback migration is grouped in run metadata.""" + # mock_migrate.return_value = ( + # [ + # MigrationMetadata( + # subject="u", + # role="course_staff", + # scope=self.course_id, + # reason=MigrationErrorReason.UNEXPECTED_SCOPE_TYPE, + # details="UnexpectedScope", + # ) + # ], + # [], + # ) + + # run_course_authoring_migration( + # migration_type=MigrationType.ROLLBACK, + # scope_type=ScopeType.COURSE, + # scope_key=self.course_id, + # course_access_role_model=CourseAccessRole, + # user_subject_model=UserSubject, + # course_id_list=[self.course_id], + # org_id=None, + # excluded_course_ids=frozenset(), + # delete_after_migration=False, + # ) + + # run = MigrationRun.objects.get(scope_key=self.course_id, status=Status.PARTIAL_SUCCESS) + # self.assertIn(MigrationErrorReason.UNEXPECTED_SCOPE_TYPE, run.metadata.get("errors", {})) + + # @patch("openedx_authz.engine.utils.migrate_authz_to_legacy_course_roles") + # def test_rollback_partial_success_when_unexpected_error(self, mock_migrate): + # """Exceptions inside the rollback loop become ``UNEXPECTED_ERROR`` entries in metadata.""" + # mock_migrate.return_value = ( + # [ + # MigrationMetadata( + # subject="u", + # role=COURSE_STAFF.external_key, + # scope=self.course_id, + # reason=MigrationErrorReason.UNEXPECTED_ERROR, + # details="KeyError: 'missing'", + # ) + # ], + # [], + # ) + + # run_course_authoring_migration( + # migration_type=MigrationType.ROLLBACK, + # scope_type=ScopeType.COURSE, + # scope_key=self.course_id, + # course_access_role_model=CourseAccessRole, + # user_subject_model=UserSubject, + # course_id_list=[self.course_id], + # org_id=None, + # excluded_course_ids=frozenset(), + # delete_after_migration=False, + # ) + + # run = MigrationRun.objects.get(scope_key=self.course_id, status=Status.PARTIAL_SUCCESS) + # self.assertIn(MigrationErrorReason.UNEXPECTED_ERROR, run.metadata.get("errors", {})) + + # def test_forward_completes_org_wide_legacy_when_org_id(self): + # """FORWARD with ``org_id`` migrates org-level (no ``course_id``) legacy roles for that org.""" + # user = User.objects.create_user(username="rcam_org_fwd_user", email="rcam_org_fwd_user@example.com") + # CourseAccessRole.objects.create( + # user=user, + # org=self.org_id, + # course_id=CourseKeyField.Empty, + # role="instructor", + # ) + + # run_course_authoring_migration( + # migration_type=MigrationType.FORWARD, + # scope_type=ScopeType.ORG, + # scope_key=self.org_id, + # course_access_role_model=CourseAccessRole, + # user_subject_model=UserSubject, + # course_id_list=None, + # org_id=self.org_id, + # excluded_course_ids=frozenset(), + # delete_after_migration=False, + # ) + + # run = MigrationRun.objects.get(scope_key=self.org_id, status=Status.COMPLETED) + # self.assertEqual(run.metadata.get("success_count"), 1) + # self.assertEqual(run.metadata.get("error_count"), 0) + # self.assertListEqual( + # run.metadata.get("successes"), + # [ + # { + # "subject": user.username, + # "role": "instructor", + # "scope": OrgCourseOverviewGlobData.build_external_key(self.org_id), + # } + # ], + # ) + + # def test_forward_completes_when_no_legacy_rows_for_org_id(self): + # """FORWARD with ``org_id`` and no matching ``CourseAccessRole`` rows finishes with zero tallies.""" + # run_course_authoring_migration( + # migration_type=MigrationType.FORWARD, + # scope_type=ScopeType.ORG, + # scope_key=self.org_id, + # course_access_role_model=CourseAccessRole, + # user_subject_model=UserSubject, + # course_id_list=None, + # org_id=self.org_id, + # excluded_course_ids=frozenset(), + # delete_after_migration=False, + # ) + + # run = MigrationRun.objects.get(scope_key=self.org_id, status=Status.COMPLETED) + # self.assertEqual(run.metadata.get("success_count"), 0) + # self.assertEqual(run.metadata.get("error_count"), 0) + + # def test_rollback_completes_org_glob_assignment_when_org_id(self): + # """ROLLBACK with ``org_id`` processes org glob / course scopes for that organization.""" + # user = User.objects.create_user(username="rcam_org_rb_user", email="rcam_org_rb_user@example.com") + # glob_scope = OrgCourseOverviewGlobData.build_external_key(self.org_id) + # assign_role_to_user_in_scope(user.username, COURSE_STAFF.external_key, glob_scope) + # AuthzEnforcer.get_enforcer().load_policy() + + # run_course_authoring_migration( + # migration_type=MigrationType.ROLLBACK, + # scope_type=ScopeType.ORG, + # scope_key=self.org_id, + # course_access_role_model=CourseAccessRole, + # user_subject_model=UserSubject, + # course_id_list=None, + # org_id=self.org_id, + # excluded_course_ids=frozenset(), + # delete_after_migration=False, + # ) + + # run = MigrationRun.objects.get(scope_key=self.org_id, status=Status.COMPLETED) + # self.assertGreaterEqual(run.metadata.get("success_count"), 1) + # self.assertEqual(run.metadata.get("error_count"), 0) + # self.assertListEqual( + # run.metadata.get("successes"), + # [{"subject": user.username, "role": COURSE_STAFF.external_key, "scope": glob_scope}], + # ) + + # def test_skipped_when_org_scope_run_already_running(self): + # """Concurrent guard applies when ``scope_type`` is ORG and ``scope_key`` is the org id.""" + # MigrationRun.create_running(MigrationType.FORWARD, ScopeType.ORG, self.org_id) + + # run_course_authoring_migration( + # migration_type=MigrationType.FORWARD, + # scope_type=ScopeType.ORG, + # scope_key=self.org_id, + # course_access_role_model=CourseAccessRole, + # user_subject_model=UserSubject, + # course_id_list=None, + # org_id=self.org_id, + # excluded_course_ids=frozenset(), + # delete_after_migration=False, + # ) + + # self.assertTrue(MigrationRun.objects.filter(scope_key=self.org_id, status=Status.SKIPPED).exists()) + # self.assertEqual(MigrationRun.objects.filter(scope_key=self.org_id).count(), 2) From 4a00968eb97931ed3b275b411f03b80606ff45b9 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Mon, 20 Apr 2026 09:29:29 -0500 Subject: [PATCH 20/29] test: update migration tests to improve error detail handling --- openedx_authz/tests/test_migrations.py | 304 +++++++++++-------------- 1 file changed, 139 insertions(+), 165 deletions(-) diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index 58879b65..c53783af 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -9,7 +9,13 @@ from django.test import TestCase from opaque_keys.edx.django.models import CourseKeyField -from openedx_authz.api.data import OrgCourseOverviewGlobData +from openedx_authz.api.data import ( + CourseOverviewData, + OrgCourseOverviewGlobData, + RoleAssignmentData, + RoleData, + UserData, +) from openedx_authz.api.users import ( assign_role_to_user_in_scope, batch_unassign_role_from_users, @@ -1588,7 +1594,7 @@ def test_forward_partial_success_when_no_scope(self, mock_migrate): subject="no_scope_user", role="staff", reason=MigrationErrorReason.NO_SCOPE, - details="User 'no_scope_user' has neither course_id nor org defined.", + details="No scope", ) ], [], @@ -1611,166 +1617,134 @@ def test_forward_partial_success_when_no_scope(self, mock_migrate): error = run.metadata.get("errors")[MigrationErrorReason.NO_SCOPE][0] self.assertEqual(error["subject"], "no_scope_user") self.assertEqual(error["role"], "staff") - self.assertEqual(error["details"], "User 'no_scope_user' has neither course_id nor org defined.") - - # @patch("openedx_authz.engine.utils.migrate_authz_to_legacy_course_roles") - # def test_rollback_partial_success_when_unexpected_scope_type(self, mock_migrate): - # """``UNEXPECTED_SCOPE_TYPE`` from rollback migration is grouped in run metadata.""" - # mock_migrate.return_value = ( - # [ - # MigrationMetadata( - # subject="u", - # role="course_staff", - # scope=self.course_id, - # reason=MigrationErrorReason.UNEXPECTED_SCOPE_TYPE, - # details="UnexpectedScope", - # ) - # ], - # [], - # ) - - # run_course_authoring_migration( - # migration_type=MigrationType.ROLLBACK, - # scope_type=ScopeType.COURSE, - # scope_key=self.course_id, - # course_access_role_model=CourseAccessRole, - # user_subject_model=UserSubject, - # course_id_list=[self.course_id], - # org_id=None, - # excluded_course_ids=frozenset(), - # delete_after_migration=False, - # ) - - # run = MigrationRun.objects.get(scope_key=self.course_id, status=Status.PARTIAL_SUCCESS) - # self.assertIn(MigrationErrorReason.UNEXPECTED_SCOPE_TYPE, run.metadata.get("errors", {})) - - # @patch("openedx_authz.engine.utils.migrate_authz_to_legacy_course_roles") - # def test_rollback_partial_success_when_unexpected_error(self, mock_migrate): - # """Exceptions inside the rollback loop become ``UNEXPECTED_ERROR`` entries in metadata.""" - # mock_migrate.return_value = ( - # [ - # MigrationMetadata( - # subject="u", - # role=COURSE_STAFF.external_key, - # scope=self.course_id, - # reason=MigrationErrorReason.UNEXPECTED_ERROR, - # details="KeyError: 'missing'", - # ) - # ], - # [], - # ) - - # run_course_authoring_migration( - # migration_type=MigrationType.ROLLBACK, - # scope_type=ScopeType.COURSE, - # scope_key=self.course_id, - # course_access_role_model=CourseAccessRole, - # user_subject_model=UserSubject, - # course_id_list=[self.course_id], - # org_id=None, - # excluded_course_ids=frozenset(), - # delete_after_migration=False, - # ) - - # run = MigrationRun.objects.get(scope_key=self.course_id, status=Status.PARTIAL_SUCCESS) - # self.assertIn(MigrationErrorReason.UNEXPECTED_ERROR, run.metadata.get("errors", {})) - - # def test_forward_completes_org_wide_legacy_when_org_id(self): - # """FORWARD with ``org_id`` migrates org-level (no ``course_id``) legacy roles for that org.""" - # user = User.objects.create_user(username="rcam_org_fwd_user", email="rcam_org_fwd_user@example.com") - # CourseAccessRole.objects.create( - # user=user, - # org=self.org_id, - # course_id=CourseKeyField.Empty, - # role="instructor", - # ) - - # run_course_authoring_migration( - # migration_type=MigrationType.FORWARD, - # scope_type=ScopeType.ORG, - # scope_key=self.org_id, - # course_access_role_model=CourseAccessRole, - # user_subject_model=UserSubject, - # course_id_list=None, - # org_id=self.org_id, - # excluded_course_ids=frozenset(), - # delete_after_migration=False, - # ) - - # run = MigrationRun.objects.get(scope_key=self.org_id, status=Status.COMPLETED) - # self.assertEqual(run.metadata.get("success_count"), 1) - # self.assertEqual(run.metadata.get("error_count"), 0) - # self.assertListEqual( - # run.metadata.get("successes"), - # [ - # { - # "subject": user.username, - # "role": "instructor", - # "scope": OrgCourseOverviewGlobData.build_external_key(self.org_id), - # } - # ], - # ) - - # def test_forward_completes_when_no_legacy_rows_for_org_id(self): - # """FORWARD with ``org_id`` and no matching ``CourseAccessRole`` rows finishes with zero tallies.""" - # run_course_authoring_migration( - # migration_type=MigrationType.FORWARD, - # scope_type=ScopeType.ORG, - # scope_key=self.org_id, - # course_access_role_model=CourseAccessRole, - # user_subject_model=UserSubject, - # course_id_list=None, - # org_id=self.org_id, - # excluded_course_ids=frozenset(), - # delete_after_migration=False, - # ) - - # run = MigrationRun.objects.get(scope_key=self.org_id, status=Status.COMPLETED) - # self.assertEqual(run.metadata.get("success_count"), 0) - # self.assertEqual(run.metadata.get("error_count"), 0) - - # def test_rollback_completes_org_glob_assignment_when_org_id(self): - # """ROLLBACK with ``org_id`` processes org glob / course scopes for that organization.""" - # user = User.objects.create_user(username="rcam_org_rb_user", email="rcam_org_rb_user@example.com") - # glob_scope = OrgCourseOverviewGlobData.build_external_key(self.org_id) - # assign_role_to_user_in_scope(user.username, COURSE_STAFF.external_key, glob_scope) - # AuthzEnforcer.get_enforcer().load_policy() - - # run_course_authoring_migration( - # migration_type=MigrationType.ROLLBACK, - # scope_type=ScopeType.ORG, - # scope_key=self.org_id, - # course_access_role_model=CourseAccessRole, - # user_subject_model=UserSubject, - # course_id_list=None, - # org_id=self.org_id, - # excluded_course_ids=frozenset(), - # delete_after_migration=False, - # ) - - # run = MigrationRun.objects.get(scope_key=self.org_id, status=Status.COMPLETED) - # self.assertGreaterEqual(run.metadata.get("success_count"), 1) - # self.assertEqual(run.metadata.get("error_count"), 0) - # self.assertListEqual( - # run.metadata.get("successes"), - # [{"subject": user.username, "role": COURSE_STAFF.external_key, "scope": glob_scope}], - # ) - - # def test_skipped_when_org_scope_run_already_running(self): - # """Concurrent guard applies when ``scope_type`` is ORG and ``scope_key`` is the org id.""" - # MigrationRun.create_running(MigrationType.FORWARD, ScopeType.ORG, self.org_id) - - # run_course_authoring_migration( - # migration_type=MigrationType.FORWARD, - # scope_type=ScopeType.ORG, - # scope_key=self.org_id, - # course_access_role_model=CourseAccessRole, - # user_subject_model=UserSubject, - # course_id_list=None, - # org_id=self.org_id, - # excluded_course_ids=frozenset(), - # delete_after_migration=False, - # ) - - # self.assertTrue(MigrationRun.objects.filter(scope_key=self.org_id, status=Status.SKIPPED).exists()) - # self.assertEqual(MigrationRun.objects.filter(scope_key=self.org_id).count(), 2) + self.assertEqual(error["details"], "No scope") + + @patch("openedx_authz.engine.utils.migrate_authz_to_legacy_course_roles") + def test_rollback_partial_success_when_unexpected_scope_type(self, mock_migrate): + """``UNEXPECTED_SCOPE_TYPE`` from rollback migration is grouped in run metadata.""" + mock_migrate.return_value = ( + [ + MigrationMetadata( + subject="u", + role="course_staff", + scope=self.COURSE_ID, + reason=MigrationErrorReason.UNEXPECTED_SCOPE_TYPE, + details="UnexpectedScope", + ) + ], + [], + ) + + run_course_authoring_migration( + migration_type=MigrationType.ROLLBACK, + scope_type=ScopeType.COURSE, + scope_key=self.COURSE_ID, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=[self.COURSE_ID], + org_id=None, + excluded_course_ids=frozenset(), + delete_after_migration=False, + ) + + run = MigrationRun.objects.get(scope_key=self.COURSE_ID, status=Status.PARTIAL_SUCCESS) + self.assertIn(MigrationErrorReason.UNEXPECTED_SCOPE_TYPE, run.metadata.get("errors")) + error = run.metadata.get("errors")[MigrationErrorReason.UNEXPECTED_SCOPE_TYPE][0] + self.assertEqual(error["subject"], "u") + self.assertEqual(error["role"], "course_staff") + self.assertEqual(error["scope"], self.COURSE_ID) + self.assertEqual(error["details"], "UnexpectedScope") + + @patch("openedx_authz.engine.utils.get_all_role_assignments_per_scope_type") + def test_rollback_partial_success_when_unexpected_error(self, mock_role_assignments): + """Exceptions inside the rollback loop become ``UNEXPECTED_ERROR`` entries in metadata.""" + missing_username = "missing_user_no_subject" + mock_role_assignments.return_value = [ + RoleAssignmentData( + subject=UserData(external_key=missing_username), + roles=[RoleData(external_key=COURSE_STAFF.external_key)], + scope=CourseOverviewData(external_key=self.COURSE_ID), + ) + ] + + run_course_authoring_migration( + migration_type=MigrationType.ROLLBACK, + scope_type=ScopeType.COURSE, + scope_key=self.COURSE_ID, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=[self.COURSE_ID], + org_id=None, + excluded_course_ids=frozenset(), + delete_after_migration=False, + ) + + run = MigrationRun.objects.get(scope_key=self.COURSE_ID, status=Status.PARTIAL_SUCCESS) + self.assertIn(MigrationErrorReason.UNEXPECTED_ERROR, run.metadata.get("errors")) + error = run.metadata.get("errors")[MigrationErrorReason.UNEXPECTED_ERROR][0] + self.assertEqual(error["subject"], missing_username) + self.assertEqual(error["role"], COURSE_STAFF.external_key) + self.assertEqual(error["scope"], self.COURSE_ID) + self.assertEqual(error["details"], str(KeyError(missing_username))) + + def test_forward_completes_org_wide_legacy_when_org_id(self): + """FORWARD with ``org_id`` migrates org-level (no ``course_id``) legacy roles for that org.""" + user = User.objects.create_user(username="rcam_org_fwd_user", email="rcam_org_fwd_user@example.com") + CourseAccessRole.objects.create( + user=user, + org=self.ORG_ID, + course_id=CourseKeyField.Empty, + role="instructor", + ) + + run_course_authoring_migration( + migration_type=MigrationType.FORWARD, + scope_type=ScopeType.ORG, + scope_key=self.ORG_ID, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=None, + org_id=self.ORG_ID, + excluded_course_ids=frozenset(), + delete_after_migration=False, + ) + + run = MigrationRun.objects.get(scope_key=self.ORG_ID, status=Status.COMPLETED) + self.assertEqual(run.metadata.get("success_count"), 1) + self.assertEqual(run.metadata.get("error_count"), 0) + self.assertListEqual( + run.metadata.get("successes"), + [ + { + "subject": user.username, + "role": "instructor", + "scope": OrgCourseOverviewGlobData.build_external_key(self.ORG_ID), + } + ], + ) + + def test_rollback_completes_org_glob_assignment_when_org_id(self): + """ROLLBACK with ``org_id`` processes org glob / course scopes for that organization.""" + user = User.objects.create_user(username="rcam_org_rb_user", email="rcam_org_rb_user@example.com") + glob_scope = OrgCourseOverviewGlobData.build_external_key(self.ORG_ID) + assign_role_to_user_in_scope(user.username, COURSE_STAFF.external_key, glob_scope) + + run_course_authoring_migration( + migration_type=MigrationType.ROLLBACK, + scope_type=ScopeType.ORG, + scope_key=self.ORG_ID, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=None, + org_id=self.ORG_ID, + excluded_course_ids=frozenset(), + delete_after_migration=False, + ) + + run = MigrationRun.objects.get(scope_key=self.ORG_ID, status=Status.COMPLETED) + self.assertGreaterEqual(run.metadata.get("success_count"), 1) + self.assertEqual(run.metadata.get("error_count"), 0) + self.assertListEqual( + run.metadata.get("successes"), + [{"subject": user.username, "role": COURSE_STAFF.external_key, "scope": glob_scope}], + ) From f73fc6a58f7a5f986d11edf6a7715395482323da Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Mon, 20 Apr 2026 10:45:29 -0500 Subject: [PATCH 21/29] test: add new tests according handlers changes --- openedx_authz/tests/test_handlers.py | 371 +++++++++++++++++++++++---- 1 file changed, 321 insertions(+), 50 deletions(-) diff --git a/openedx_authz/tests/test_handlers.py b/openedx_authz/tests/test_handlers.py index e95f69c9..07cf8ec6 100644 --- a/openedx_authz/tests/test_handlers.py +++ b/openedx_authz/tests/test_handlers.py @@ -14,7 +14,11 @@ from ddt import data, ddt, unpack from django.test import TestCase, override_settings -from openedx_authz.handlers import trigger_course_authoring_migration +from openedx_authz.handlers import ( + WAFFLE_OVERRIDE_FORCE_OFF, + WAFFLE_OVERRIDE_FORCE_ON, + trigger_course_authoring_migration, +) from openedx_authz.models.authz_migration import MigrationType, ScopeType from openedx_authz.models.core import ExtendedCasbinRule, Scope, Subject from openedx_authz.models.subjects import UserSubject @@ -255,43 +259,33 @@ class TestTriggerCourseAuthoringMigration(TestCase): Course and org overrides use the stub ORM (creates and queries) where the handler touches the database. A class-level ``patch`` replaces ``run_course_authoring_migration`` so no full migration runs; tests that also patch ``logger`` receive that mock before ``mock_run``. - """ - @override_settings(ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION=False) - def test_skips_when_automatic_migration_setting_disabled(self, mock_run): - """When the setting is off, the handler returns before scheduling work.""" - instance = WaffleFlagCourseOverrideModel( - course_id="course-v1:org+course+run", - waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, - enabled=True, - ) - - trigger_course_authoring_migration( - sender=WaffleFlagCourseOverrideModel, - instance=instance, - scope_key=str(instance.course_id), - ) + The handler matches production waffle semantics: migration direction follows ``override_choice`` + (force on vs force off) while ``enabled`` gates whether any migration runs; org scope passes + ``excluded_course_ids`` for course-level overrides that oppose the org transition. + """ - mock_run.assert_not_called() + COURSE_KEY = "course-v1:test_org+course+run_mm" + ORG_KEY = "test_org" @data( ( WaffleFlagCourseOverrideModel, { - "course_id": "course-v1:org+course+run_mm", + "course_id": COURSE_KEY, "waffle_flag": OTHER_WAFFLE_FLAG_NAME, "enabled": True, }, - "course-v1:org+course+run_mm", + COURSE_KEY, ), ( WaffleFlagOrgOverrideModel, { - "org": "test_org_waffle_mm", + "org": ORG_KEY, "waffle_flag": OTHER_WAFFLE_FLAG_NAME, "enabled": True, }, - "test_org_waffle_mm", + ORG_KEY, ), ) @unpack @@ -303,6 +297,82 @@ def test_skips_when_waffle_flag_name_mismatch(self, sender_model, instance_kwarg mock_run.assert_not_called() + @data( + ( + WaffleFlagCourseOverrideModel, + { + "course_id": COURSE_KEY, + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": False, + "override_choice": WAFFLE_OVERRIDE_FORCE_ON, + }, + COURSE_KEY, + ), + ( + WaffleFlagOrgOverrideModel, + { + "org": ORG_KEY, + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": False, + "override_choice": WAFFLE_OVERRIDE_FORCE_ON, + }, + ORG_KEY, + ), + ) + @unpack + @patch("openedx_authz.handlers.logger") + def test_skips_when_waffle_row_disabled( + self, + sender_model, + instance_kwargs, + scope_key, + mock_logger, + mock_run, + ): # pylint: disable=too-many-positional-arguments + """When the override row is not active, the handler exits before migration (course and org).""" + instance = sender_model(**instance_kwargs) + + trigger_course_authoring_migration(sender_model, instance, scope_key) + + mock_run.assert_not_called() + mock_logger.info.assert_called_once_with("Waffle flag is disabled, skipping migration") + + @override_settings(ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION=False) + @data( + ( + WaffleFlagCourseOverrideModel, + { + "course_id": COURSE_KEY, + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": True, + }, + COURSE_KEY, + ), + ( + WaffleFlagOrgOverrideModel, + { + "org": ORG_KEY, + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": True, + }, + ORG_KEY, + ), + ) + @unpack + @patch("openedx_authz.handlers.logger") + def test_skips_when_automatic_migration_setting_disabled( + self, sender_model, instance_kwargs, scope_key, mock_logger, mock_run + ): # pylint: disable=too-many-positional-arguments + """When the setting is off, the handler returns before scheduling work (course and org).""" + instance = sender_model(**instance_kwargs) + + trigger_course_authoring_migration(sender_model, instance, scope_key) + + mock_run.assert_not_called() + mock_logger.info.assert_called_once_with( + "ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION is set to False, skipping migration" + ) + @patch("openedx_authz.handlers.logger") def test_logs_error_for_unsupported_instance_type(self, mock_logger, mock_run): """Instances that are neither course nor org overrides are rejected.""" @@ -311,75 +381,276 @@ def test_logs_error_for_unsupported_instance_type(self, mock_logger, mock_run): trigger_course_authoring_migration(WaffleFlagCourseOverrideModel, unsupported, "ignored") mock_run.assert_not_called() - mock_logger.error.assert_called_once() - self.assertIn("Unsupported waffle flag instance", mock_logger.error.call_args[0][0]) + mock_logger.error.assert_called_once_with("Unsupported waffle flag instance: %s", unsupported) @data( - (True, MigrationType.FORWARD), - (False, MigrationType.ROLLBACK), + (WAFFLE_OVERRIDE_FORCE_ON, MigrationType.FORWARD), + (WAFFLE_OVERRIDE_FORCE_OFF, MigrationType.ROLLBACK), ) @unpack - def test_course_scope_migration_depends_on_enabled(self, enabled, expected_migration_type, mock_run): - """Course override runs forward migration when enabled and rollback when disabled.""" - course_key = f"course-v1:test_org+handlers_course+{'on' if enabled else 'off'}" + def test_course_scope_migration_depends_on_override_choice( + self, override_choice, expected_migration_type, mock_run + ): + """Course override runs forward when forced on and rollback when forced off.""" instance = WaffleFlagCourseOverrideModel.objects.create( - course_id=course_key, + course_id=self.COURSE_KEY, waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, - enabled=enabled, + enabled=True, + override_choice=override_choice, ) - trigger_course_authoring_migration(WaffleFlagCourseOverrideModel, instance, course_key) + trigger_course_authoring_migration(WaffleFlagCourseOverrideModel, instance, self.COURSE_KEY) mock_run.assert_called_once_with( migration_type=expected_migration_type, scope_type=ScopeType.COURSE, - scope_key=course_key, + scope_key=self.COURSE_KEY, course_access_role_model=CourseAccessRole, user_subject_model=UserSubject, - course_id_list=[course_key], + course_id_list=[self.COURSE_KEY], org_id=None, + excluded_course_ids=frozenset(), + delete_after_migration=True, ) @data( - (True, MigrationType.FORWARD), - (False, MigrationType.ROLLBACK), + (WAFFLE_OVERRIDE_FORCE_ON, MigrationType.FORWARD), + (WAFFLE_OVERRIDE_FORCE_OFF, MigrationType.ROLLBACK), ) @unpack - def test_org_scope_migration_depends_on_enabled(self, enabled, expected_migration_type, mock_run): - """Org override runs forward migration when enabled and rollback when disabled.""" - org_name = f"test_org_handlers_{enabled}" + def test_org_scope_migration_depends_on_override_choice(self, override_choice, expected_migration_type, mock_run): + """Org override runs forward when forced on and rollback when forced off.""" instance = WaffleFlagOrgOverrideModel.objects.create( - org=org_name, + org=self.ORG_KEY, waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, - enabled=enabled, + enabled=True, + override_choice=override_choice, ) - trigger_course_authoring_migration(WaffleFlagOrgOverrideModel, instance, org_name) + trigger_course_authoring_migration(WaffleFlagOrgOverrideModel, instance, self.ORG_KEY) mock_run.assert_called_once_with( migration_type=expected_migration_type, scope_type=ScopeType.ORG, - scope_key=org_name, + scope_key=self.ORG_KEY, course_access_role_model=CourseAccessRole, user_subject_model=UserSubject, course_id_list=None, - org_id=org_name, + org_id=self.ORG_KEY, + excluded_course_ids=frozenset(), + delete_after_migration=True, ) - def test_skips_when_previous_record_has_same_enabled_state(self, mock_run): - """Repeated saves with the same enabled value do not trigger migration.""" - course_id = "course-v1:test_org+tcam_noop+2024" + @data( + ( + WAFFLE_OVERRIDE_FORCE_ON, + WAFFLE_OVERRIDE_FORCE_OFF, + MigrationType.FORWARD, + ), + ( + WAFFLE_OVERRIDE_FORCE_OFF, + WAFFLE_OVERRIDE_FORCE_ON, + MigrationType.ROLLBACK, + ), + ) + @unpack + def test_org_scope_passes_excluded_course_ids_when_course_overrides_oppose_org( + self, org_override_choice, course_override_choice, expected_migration_type, mock_run + ): + """Org migration excludes active course rows whose override opposes the org transition.""" WaffleFlagCourseOverrideModel.objects.create( - course_id=course_id, + course_id=self.COURSE_KEY, waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, enabled=True, + override_choice=course_override_choice, ) - instance = WaffleFlagCourseOverrideModel.objects.create( - course_id=course_id, + instance = WaffleFlagOrgOverrideModel.objects.create( + org=self.ORG_KEY, waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, enabled=True, + override_choice=org_override_choice, + ) + + trigger_course_authoring_migration(WaffleFlagOrgOverrideModel, instance, self.ORG_KEY) + + mock_run.assert_called_once_with( + migration_type=expected_migration_type, + scope_type=ScopeType.ORG, + scope_key=self.ORG_KEY, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=None, + org_id=self.ORG_KEY, + excluded_course_ids=frozenset({str(self.COURSE_KEY)}), + delete_after_migration=True, ) - trigger_course_authoring_migration(WaffleFlagCourseOverrideModel, instance, course_id) + @data( + ( + WaffleFlagCourseOverrideModel, + { + "course_id": COURSE_KEY, + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": True, + "override_choice": WAFFLE_OVERRIDE_FORCE_ON, + }, + COURSE_KEY, + ), + ( + WaffleFlagOrgOverrideModel, + { + "org": ORG_KEY, + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": True, + "override_choice": WAFFLE_OVERRIDE_FORCE_ON, + }, + ORG_KEY, + ), + ) + @unpack + @patch("openedx_authz.handlers.logger") + def test_skips_when_previous_enabled_record_has_same_override_choice( + self, sender_model, row_kwargs, scope_key, mock_logger, mock_run + ): # pylint: disable=too-many-positional-arguments + """Repeated history rows with the same active override choice do not trigger migration.""" + sender_model.objects.create(**row_kwargs) + instance = sender_model.objects.create(**row_kwargs) + + trigger_course_authoring_migration(sender_model, instance, scope_key) mock_run.assert_not_called() + mock_logger.info.assert_called_once_with("No change in waffle flag, skipping migration") + + @data( + ( + WaffleFlagCourseOverrideModel, + { + "course_id": COURSE_KEY, + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": False, + "override_choice": WAFFLE_OVERRIDE_FORCE_ON, + }, + { + "course_id": COURSE_KEY, + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": True, + "override_choice": WAFFLE_OVERRIDE_FORCE_ON, + }, + COURSE_KEY, + ), + ( + WaffleFlagOrgOverrideModel, + { + "org": ORG_KEY, + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": False, + "override_choice": WAFFLE_OVERRIDE_FORCE_ON, + }, + { + "org": ORG_KEY, + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": True, + "override_choice": WAFFLE_OVERRIDE_FORCE_ON, + }, + ORG_KEY, + ), + ) + @unpack + def test_runs_when_previous_record_disabled_even_if_same_override_choice( + self, sender_model, prev_kwargs, instance_kwargs, scope_key, mock_run + ): # pylint: disable=too-many-positional-arguments + """If the prior row was inactive, a new active row still triggers migration (course and org).""" + sender_model.objects.create(**prev_kwargs) + instance = sender_model.objects.create(**instance_kwargs) + + trigger_course_authoring_migration(sender_model, instance, scope_key) + + common = { + "migration_type": MigrationType.FORWARD, + "scope_key": scope_key, + "course_access_role_model": CourseAccessRole, + "user_subject_model": UserSubject, + "excluded_course_ids": frozenset(), + "delete_after_migration": True, + } + if sender_model is WaffleFlagCourseOverrideModel: + mock_run.assert_called_once_with( + **common, + scope_type=ScopeType.COURSE, + course_id_list=[scope_key], + org_id=None, + ) + else: + mock_run.assert_called_once_with( + **common, + scope_type=ScopeType.ORG, + course_id_list=None, + org_id=scope_key, + ) + + @data( + ( + WaffleFlagCourseOverrideModel, + { + "course_id": "course-v1:test_org+tcam_override_change+1", + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": True, + "override_choice": WAFFLE_OVERRIDE_FORCE_ON, + }, + { + "course_id": "course-v1:test_org+tcam_override_change+1", + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": True, + "override_choice": WAFFLE_OVERRIDE_FORCE_OFF, + }, + "course-v1:test_org+tcam_override_change+1", + ), + ( + WaffleFlagOrgOverrideModel, + { + "org": "test_org_tcam_override_change", + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": True, + "override_choice": WAFFLE_OVERRIDE_FORCE_ON, + }, + { + "org": "test_org_tcam_override_change", + "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, + "enabled": True, + "override_choice": WAFFLE_OVERRIDE_FORCE_OFF, + }, + "test_org_tcam_override_change", + ), + ) + @unpack + def test_runs_when_previous_enabled_record_has_different_override_choice( + self, sender_model, prev_kwargs, instance_kwargs, scope_key, mock_run + ): # pylint: disable=too-many-positional-arguments + """A new row that changes override choice from the last active row triggers migration (course and org).""" + sender_model.objects.create(**prev_kwargs) + instance = sender_model.objects.create(**instance_kwargs) + + trigger_course_authoring_migration(sender_model, instance, scope_key) + + common = { + "migration_type": MigrationType.ROLLBACK, + "scope_key": scope_key, + "course_access_role_model": CourseAccessRole, + "user_subject_model": UserSubject, + "excluded_course_ids": frozenset(), + "delete_after_migration": True, + } + if sender_model is WaffleFlagCourseOverrideModel: + mock_run.assert_called_once_with( + **common, + scope_type=ScopeType.COURSE, + course_id_list=[scope_key], + org_id=None, + ) + else: + mock_run.assert_called_once_with( + **common, + scope_type=ScopeType.ORG, + course_id_list=None, + org_id=scope_key, + ) From add438a605e55ff8db836b685a8e3084ccdc6d39 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Mon, 20 Apr 2026 10:53:57 -0500 Subject: [PATCH 22/29] docs: remove unexpected identation --- openedx_authz/engine/utils.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/openedx_authz/engine/utils.py b/openedx_authz/engine/utils.py index 58195357..93f27e1c 100644 --- a/openedx_authz/engine/utils.py +++ b/openedx_authz/engine/utils.py @@ -270,8 +270,8 @@ def migrate_legacy_course_roles_to_authz( param course_id_list: Optional list of course IDs to filter the migration. param org_id: Optional organization ID to filter the migration. param delete_after_migration: Whether to delete successfully migrated legacy permissions after migration. - param excluded_course_ids: Course ids for which migration is skipped (course-level override - opposes the org-level transition) + param excluded_course_ids: Course ids for which migration is skipped (course-level + override opposes the org-level transition). """ _validate_migration_input(course_id_list, org_id) @@ -397,10 +397,10 @@ def migrate_authz_to_legacy_course_roles( is intended to run within a Django migration context, where direct model imports can cause issues. param course_id_list: Optional list of course IDs to filter the migration. param org_id: Optional organization ID to filter the migration. - param delete_after_migration: Whether to unassign successfully migrated permissions - from the new model after migration. + param delete_after_migration: Whether to unassign successfully migrated permissions from + the new model after migration. param excluded_course_ids: Course ids for which rollback is skipped (course-level override - opposes the org-level transition) + opposes the org-level transition). """ _validate_migration_input(course_id_list, org_id) From 20db8d78b3dd151e3e526d635c3f3d38cff3a8b9 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Mon, 20 Apr 2026 11:03:00 -0500 Subject: [PATCH 23/29] test: add migration test to verify skipping of excluded course ids --- openedx_authz/tests/test_migrations.py | 62 ++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/openedx_authz/tests/test_migrations.py b/openedx_authz/tests/test_migrations.py index c53783af..53b63a66 100644 --- a/openedx_authz/tests/test_migrations.py +++ b/openedx_authz/tests/test_migrations.py @@ -901,6 +901,39 @@ def test_migrate_legacy_course_roles_to_authz_using_org_id(self): # After rollback, we should have 0 UserSubjects related to the course permissions self.assertEqual(len(state_after_migration_user_subjects), 0) + @patch("openedx_authz.api.data.CourseOverview", CourseOverview) + def test_migrate_legacy_course_roles_to_authz_skips_excluded_course_ids(self): + """Org-scoped migration skips course-level rows whose course_id is in excluded_course_ids.""" + errors, successes = migrate_legacy_course_roles_to_authz( + CourseAccessRole, + course_id_list=None, + org_id=self.org, + delete_after_migration=False, + excluded_course_ids=frozenset({self.course_id}), + ) + + self.assertEqual(len(successes), 0) + self.assertEqual(len(errors), 13) + + skipped = [entry for entry in errors if entry.reason == MigrationErrorReason.SKIPPED_FOR_FLAG_OVERRIDE] + self.assertEqual(len(skipped), 12) + for entry in skipped: + self.assertEqual(entry.scope, self.course_id) + self.assertEqual( + entry.details, + "Course-level authoring flag override opposes this organization-level transition", + ) + + unknown_role = [entry for entry in errors if entry.reason == MigrationErrorReason.UNKNOWN_ROLE] + self.assertEqual(len(unknown_role), 1) + self.assertEqual(unknown_role[0].subject, self.error_user.username) + + for user in self.admin_users: + assignments = get_user_role_assignments_in_scope( + user_external_key=user.username, scope_external_key=self.course_id + ) + self.assertEqual(len(assignments), 0) + @patch("openedx_authz.api.data.CourseOverview", CourseOverview) def test_migrate_authz_to_legacy_course_roles_with_no_org_and_courses(self): # Migrate from legacy CourseAccessRole to new Casbin-based model @@ -1249,6 +1282,35 @@ def test_migrate_authz_to_legacy_course_roles_user_not_added(self): self.assertEqual(len(errors), 12) self.assertEqual(len(successes), 0) + @patch("openedx_authz.api.data.CourseOverview", CourseOverview) + def test_migrate_authz_to_legacy_course_roles_skips_excluded_course_ids(self): + """Course-level assignments whose scope is listed in excluded_course_ids are skipped.""" + migrate_legacy_course_roles_to_authz( + CourseAccessRole, + course_id_list=[self.course_id], + org_id=None, + delete_after_migration=False, + ) + + errors, successes = migrate_authz_to_legacy_course_roles( + CourseAccessRole, + UserSubject, + course_id_list=[self.course_id], + org_id=None, + delete_after_migration=False, + excluded_course_ids=frozenset({self.course_id}), + ) + + self.assertEqual(len(successes), 0) + self.assertEqual(len(errors), 12) + for entry in errors: + self.assertEqual(entry.reason, MigrationErrorReason.SKIPPED_FOR_FLAG_OVERRIDE) + self.assertEqual( + entry.details, + "Course-level authoring flag override opposes this organization-level transition", + ) + self.assertEqual(entry.scope, self.course_id) + def create_library_env(self): """ Helper method to create a ContentLibrary environment for testing. From 9dd98b31602f9c9f5f40d47a27088a6a4f3efe63 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Mon, 20 Apr 2026 11:13:09 -0500 Subject: [PATCH 24/29] chore: update release date for version 1.12.0 --- CHANGELOG.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 88d17853..297f6e93 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -14,7 +14,7 @@ Change Log Unreleased ********** -1.12.0 - 2026-04-16 +1.12.0 - 2026-04-20 ******************* Added From 78812241f6cd0d5ffe93b167b7cbb2db6af01302 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Mon, 20 Apr 2026 13:47:12 -0500 Subject: [PATCH 25/29] feat: implement migration type determination logic based on current and previous record states --- openedx_authz/handlers.py | 72 +++++-- openedx_authz/tests/test_handlers.py | 289 ++++++++++++--------------- 2 files changed, 176 insertions(+), 185 deletions(-) diff --git a/openedx_authz/handlers.py b/openedx_authz/handlers.py index d56ad145..d61c31aa 100644 --- a/openedx_authz/handlers.py +++ b/openedx_authz/handlers.py @@ -7,6 +7,7 @@ from __future__ import annotations import logging +from typing import Union from casbin_adapter.models import CasbinRule from django.conf import settings @@ -137,13 +138,54 @@ def handle_org_waffle_flag_change(sender, instance, **kwargs) -> None: if WaffleFlagOrgOverrideModel is not None: post_save.connect(handle_org_waffle_flag_change, sender=WaffleFlagOrgOverrideModel) + # Match ``WaffleFlagCourseOverrideModel.OVERRIDE_CHOICES`` / ``override_value`` in edx-platform: # the flag is effectively forced on only when the row is enabled and ``override_choice`` is "on". WAFFLE_OVERRIDE_FORCE_ON = "on" WAFFLE_OVERRIDE_FORCE_OFF = "off" -def get_excluded_course_ids_for_org_migration(org_id: str, reverse_choice: str) -> frozenset[str]: +# Type Alias for better readability +WaffleOverrideRecord = Union[WaffleFlagCourseOverrideModel, WaffleFlagOrgOverrideModel] + + +def get_migration_type( + current_record: WaffleOverrideRecord, previous_record: WaffleOverrideRecord | None +) -> MigrationType | None: + """ + Determine the migration type by comparing the current and previous record states. + + A FORWARD migration occurs when a flag transitions from disabled/not-forced + to enabled and forced-on. A ROLLBACK occurs when it transitions back. + + Args: + current_record (WaffleOverrideRecord): The state of the record in the current transaction. + previous_record (WaffleOverrideRecord | None): The state of the record prior to the current transaction. + + Returns: + MigrationType.FORWARD: If the flag is newly forced on. + MigrationType.ROLLBACK: If the forced-on state is removed. + None: If there is no effective change in the flag's behavior. + """ + # 1. Handle New Records (Creation) + if not previous_record: + is_active = current_record.enabled and current_record.override_choice == WAFFLE_OVERRIDE_FORCE_ON + return MigrationType.FORWARD if is_active else None + + # 2. Extract Effective States + # A flag is "effectively active" only if enabled AND set to FORCE_ON + was_effectively_active = previous_record.enabled and previous_record.override_choice == WAFFLE_OVERRIDE_FORCE_ON + is_effectively_active = current_record.enabled and current_record.override_choice == WAFFLE_OVERRIDE_FORCE_ON + + # 3. Return None if state hasn't changed + if is_effectively_active == was_effectively_active: + return None + + # 4. Determine Transition Type + return MigrationType.FORWARD if is_effectively_active else MigrationType.ROLLBACK + + +def get_excluded_course_ids_for_org_migration(org_id: str, override_choice: str) -> frozenset[str]: """ Collect course-level authoring flag overrides for an org that oppose the new org-level state. @@ -152,13 +194,16 @@ def get_excluded_course_ids_for_org_migration(org_id: str, reverse_choice: str) Args: org_id (str): Organization short name. - reverse_choice (str): The reverse choice of the org waffle flag. + override_choice (str): The override choice of the org waffle flag. Returns: frozenset[str]: course ids excluded from org migration """ # We only need to check the current set (active flags). Opposing overrides are rows that # force the opposite of the org transition (Force On vs Force Off), not merely inactive rows. + reverse_choice = ( + WAFFLE_OVERRIDE_FORCE_ON if override_choice == WAFFLE_OVERRIDE_FORCE_OFF else WAFFLE_OVERRIDE_FORCE_OFF + ) filter_kwargs = { "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG.name, "course_id__startswith": f"course-v1:{org_id}+", @@ -170,8 +215,8 @@ def get_excluded_course_ids_for_org_migration(org_id: str, reverse_choice: str) def trigger_course_authoring_migration( - sender: type[WaffleFlagCourseOverrideModel | WaffleFlagOrgOverrideModel], - instance: WaffleFlagCourseOverrideModel | WaffleFlagOrgOverrideModel, + sender: type[WaffleOverrideRecord], + instance: WaffleOverrideRecord, scope_key: str, ) -> None: """ @@ -182,17 +227,13 @@ def trigger_course_authoring_migration( which handles tracking and concurrent-run protection. Args: - sender: The model class (WaffleFlagCourseOverrideModel or WaffleFlagOrgOverrideModel). + sender: The model class (WaffleOverrideRecord). instance: The waffle flag instance that triggered the migration. scope_key (str): Course ID or organization name. """ if instance.waffle_flag != AUTHZ_COURSE_AUTHORING_FLAG.name: return - if not instance.enabled: - logger.info("Waffle flag is disabled, skipping migration") - return - if not settings.ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION: logger.info("ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION is set to False, skipping migration") return @@ -213,22 +254,15 @@ def trigger_course_authoring_migration( prev_record = sender.objects.filter(**filter_kwargs).exclude(id=instance.id).order_by("-change_date").first() - if prev_record and prev_record.enabled and instance.override_choice == prev_record.override_choice: + migration_type = get_migration_type(instance, prev_record) + if migration_type is None: logger.info("No change in waffle flag, skipping migration") return - if instance.override_choice == WAFFLE_OVERRIDE_FORCE_ON: - migration_type = MigrationType.FORWARD - reverse_choice = WAFFLE_OVERRIDE_FORCE_OFF - else: - migration_type = MigrationType.ROLLBACK - reverse_choice = WAFFLE_OVERRIDE_FORCE_ON - excluded_course_ids = frozenset() if isinstance(instance, WaffleFlagOrgOverrideModel): excluded_course_ids = get_excluded_course_ids_for_org_migration( - org_id=scope_key, - reverse_choice=reverse_choice, + org_id=scope_key, override_choice=instance.override_choice ) logger.info("Triggering %s migration for %s:%s due to waffle flag change", migration_type, scope_type, scope_key) diff --git a/openedx_authz/tests/test_handlers.py b/openedx_authz/tests/test_handlers.py index 07cf8ec6..da014883 100644 --- a/openedx_authz/tests/test_handlers.py +++ b/openedx_authz/tests/test_handlers.py @@ -17,6 +17,7 @@ from openedx_authz.handlers import ( WAFFLE_OVERRIDE_FORCE_OFF, WAFFLE_OVERRIDE_FORCE_ON, + get_migration_type, trigger_course_authoring_migration, ) from openedx_authz.models.authz_migration import MigrationType, ScopeType @@ -252,17 +253,7 @@ def test_cascade_deletion_with_scope_deletion(self): ) class TestTriggerCourseAuthoringMigration(TestCase): """ - Cover ``trigger_course_authoring_migration`` when Open edX waffle imports are absent. - - The class-level ``patch.multiple`` injects stub models and a stand-in flag into - ``openedx_authz.handlers`` so ``isinstance`` checks and flag name resolution match production. - Course and org overrides use the stub ORM (creates and queries) where the handler touches the - database. A class-level ``patch`` replaces ``run_course_authoring_migration`` so no full - migration runs; tests that also patch ``logger`` receive that mock before ``mock_run``. - - The handler matches production waffle semantics: migration direction follows ``override_choice`` - (force on vs force off) while ``enabled`` gates whether any migration runs; org scope passes - ``excluded_course_ids`` for course-level overrides that oppose the org transition. + Runs tests for ``trigger_course_authoring_migration`` with stub waffle models. """ COURSE_KEY = "course-v1:test_org+course+run_mm" @@ -297,46 +288,6 @@ def test_skips_when_waffle_flag_name_mismatch(self, sender_model, instance_kwarg mock_run.assert_not_called() - @data( - ( - WaffleFlagCourseOverrideModel, - { - "course_id": COURSE_KEY, - "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, - "enabled": False, - "override_choice": WAFFLE_OVERRIDE_FORCE_ON, - }, - COURSE_KEY, - ), - ( - WaffleFlagOrgOverrideModel, - { - "org": ORG_KEY, - "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, - "enabled": False, - "override_choice": WAFFLE_OVERRIDE_FORCE_ON, - }, - ORG_KEY, - ), - ) - @unpack - @patch("openedx_authz.handlers.logger") - def test_skips_when_waffle_row_disabled( - self, - sender_model, - instance_kwargs, - scope_key, - mock_logger, - mock_run, - ): # pylint: disable=too-many-positional-arguments - """When the override row is not active, the handler exits before migration (course and org).""" - instance = sender_model(**instance_kwargs) - - trigger_course_authoring_migration(sender_model, instance, scope_key) - - mock_run.assert_not_called() - mock_logger.info.assert_called_once_with("Waffle flag is disabled, skipping migration") - @override_settings(ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION=False) @data( ( @@ -384,14 +335,15 @@ def test_logs_error_for_unsupported_instance_type(self, mock_logger, mock_run): mock_logger.error.assert_called_once_with("Unsupported waffle flag instance: %s", unsupported) @data( - (WAFFLE_OVERRIDE_FORCE_ON, MigrationType.FORWARD), - (WAFFLE_OVERRIDE_FORCE_OFF, MigrationType.ROLLBACK), + (WAFFLE_OVERRIDE_FORCE_ON, MigrationType.FORWARD, True), + (WAFFLE_OVERRIDE_FORCE_OFF, None, False), ) @unpack + @patch("openedx_authz.handlers.logger") def test_course_scope_migration_depends_on_override_choice( - self, override_choice, expected_migration_type, mock_run - ): - """Course override runs forward when forced on and rollback when forced off.""" + self, override_choice, expected_migration_type, expect_migration, mock_logger, mock_run + ): # pylint: disable=too-many-positional-arguments + """Course override runs forward only when forced on, force-off is a no-op for migration.""" instance = WaffleFlagCourseOverrideModel.objects.create( course_id=self.COURSE_KEY, waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, @@ -401,25 +353,32 @@ def test_course_scope_migration_depends_on_override_choice( trigger_course_authoring_migration(WaffleFlagCourseOverrideModel, instance, self.COURSE_KEY) - mock_run.assert_called_once_with( - migration_type=expected_migration_type, - scope_type=ScopeType.COURSE, - scope_key=self.COURSE_KEY, - course_access_role_model=CourseAccessRole, - user_subject_model=UserSubject, - course_id_list=[self.COURSE_KEY], - org_id=None, - excluded_course_ids=frozenset(), - delete_after_migration=True, - ) + if expect_migration: + mock_run.assert_called_once_with( + migration_type=expected_migration_type, + scope_type=ScopeType.COURSE, + scope_key=self.COURSE_KEY, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=[self.COURSE_KEY], + org_id=None, + excluded_course_ids=frozenset(), + delete_after_migration=True, + ) + else: + mock_run.assert_not_called() + mock_logger.info.assert_called_once_with("No change in waffle flag, skipping migration") @data( - (WAFFLE_OVERRIDE_FORCE_ON, MigrationType.FORWARD), - (WAFFLE_OVERRIDE_FORCE_OFF, MigrationType.ROLLBACK), + (WAFFLE_OVERRIDE_FORCE_ON, MigrationType.FORWARD, True), + (WAFFLE_OVERRIDE_FORCE_OFF, None, False), ) @unpack - def test_org_scope_migration_depends_on_override_choice(self, override_choice, expected_migration_type, mock_run): - """Org override runs forward when forced on and rollback when forced off.""" + @patch("openedx_authz.handlers.logger") + def test_org_scope_migration_depends_on_override_choice( + self, override_choice, expected_migration_type, expect_migration, mock_logger, mock_run + ): # pylint: disable=too-many-positional-arguments + """Org override runs forward only when forced on, force-off is a no-op for migration.""" instance = WaffleFlagOrgOverrideModel.objects.create( org=self.ORG_KEY, waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, @@ -429,52 +388,41 @@ def test_org_scope_migration_depends_on_override_choice(self, override_choice, e trigger_course_authoring_migration(WaffleFlagOrgOverrideModel, instance, self.ORG_KEY) - mock_run.assert_called_once_with( - migration_type=expected_migration_type, - scope_type=ScopeType.ORG, - scope_key=self.ORG_KEY, - course_access_role_model=CourseAccessRole, - user_subject_model=UserSubject, - course_id_list=None, - org_id=self.ORG_KEY, - excluded_course_ids=frozenset(), - delete_after_migration=True, - ) + if expect_migration: + mock_run.assert_called_once_with( + migration_type=expected_migration_type, + scope_type=ScopeType.ORG, + scope_key=self.ORG_KEY, + course_access_role_model=CourseAccessRole, + user_subject_model=UserSubject, + course_id_list=None, + org_id=self.ORG_KEY, + excluded_course_ids=frozenset(), + delete_after_migration=True, + ) + else: + mock_run.assert_not_called() + mock_logger.info.assert_called_once_with("No change in waffle flag, skipping migration") - @data( - ( - WAFFLE_OVERRIDE_FORCE_ON, - WAFFLE_OVERRIDE_FORCE_OFF, - MigrationType.FORWARD, - ), - ( - WAFFLE_OVERRIDE_FORCE_OFF, - WAFFLE_OVERRIDE_FORCE_ON, - MigrationType.ROLLBACK, - ), - ) - @unpack - def test_org_scope_passes_excluded_course_ids_when_course_overrides_oppose_org( - self, org_override_choice, course_override_choice, expected_migration_type, mock_run - ): - """Org migration excludes active course rows whose override opposes the org transition.""" + def test_org_scope_passes_excluded_course_ids_when_course_overrides_oppose_org(self, mock_run): + """Org forward migration excludes active course rows whose override opposes force-on.""" WaffleFlagCourseOverrideModel.objects.create( course_id=self.COURSE_KEY, waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, enabled=True, - override_choice=course_override_choice, + override_choice=WAFFLE_OVERRIDE_FORCE_OFF, ) instance = WaffleFlagOrgOverrideModel.objects.create( org=self.ORG_KEY, waffle_flag=AUTHZ_COURSE_AUTHORING_FLAG_NAME, enabled=True, - override_choice=org_override_choice, + override_choice=WAFFLE_OVERRIDE_FORCE_ON, ) trigger_course_authoring_migration(WaffleFlagOrgOverrideModel, instance, self.ORG_KEY) mock_run.assert_called_once_with( - migration_type=expected_migration_type, + migration_type=MigrationType.FORWARD, scope_type=ScopeType.ORG, scope_key=self.ORG_KEY, course_access_role_model=CourseAccessRole, @@ -588,69 +536,78 @@ def test_runs_when_previous_record_disabled_even_if_same_override_choice( org_id=scope_key, ) - @data( - ( - WaffleFlagCourseOverrideModel, - { - "course_id": "course-v1:test_org+tcam_override_change+1", - "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, - "enabled": True, - "override_choice": WAFFLE_OVERRIDE_FORCE_ON, - }, - { - "course_id": "course-v1:test_org+tcam_override_change+1", - "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, - "enabled": True, - "override_choice": WAFFLE_OVERRIDE_FORCE_OFF, - }, - "course-v1:test_org+tcam_override_change+1", - ), - ( - WaffleFlagOrgOverrideModel, - { - "org": "test_org_tcam_override_change", - "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, - "enabled": True, - "override_choice": WAFFLE_OVERRIDE_FORCE_ON, - }, - { - "org": "test_org_tcam_override_change", - "waffle_flag": AUTHZ_COURSE_AUTHORING_FLAG_NAME, - "enabled": True, - "override_choice": WAFFLE_OVERRIDE_FORCE_OFF, - }, - "test_org_tcam_override_change", - ), - ) - @unpack - def test_runs_when_previous_enabled_record_has_different_override_choice( - self, sender_model, prev_kwargs, instance_kwargs, scope_key, mock_run - ): # pylint: disable=too-many-positional-arguments - """A new row that changes override choice from the last active row triggers migration (course and org).""" - sender_model.objects.create(**prev_kwargs) - instance = sender_model.objects.create(**instance_kwargs) - trigger_course_authoring_migration(sender_model, instance, scope_key) +class TestGetMigrationType(TestCase): + """Tests for ``get_migration_type``.""" - common = { - "migration_type": MigrationType.ROLLBACK, - "scope_key": scope_key, - "course_access_role_model": CourseAccessRole, - "user_subject_model": UserSubject, - "excluded_course_ids": frozenset(), - "delete_after_migration": True, - } - if sender_model is WaffleFlagCourseOverrideModel: - mock_run.assert_called_once_with( - **common, - scope_type=ScopeType.COURSE, - course_id_list=[scope_key], - org_id=None, - ) - else: - mock_run.assert_called_once_with( - **common, - scope_type=ScopeType.ORG, - course_id_list=None, - org_id=scope_key, - ) + def create_mock_record(self, enabled: bool, choice: str): + """Helper to create a mock record object.""" + return SimpleNamespace(enabled=enabled, override_choice=choice) + + def test_creation_new_record_enabled(self): + """Case: No previous record, current is enabled and forced on.""" + current = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) + + result = get_migration_type(current, None) + + self.assertEqual(result, MigrationType.FORWARD) + + def test_creation_new_record_disabled(self): + """Case: No previous record, current is disabled.""" + current = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) + + result = get_migration_type(current, None) + + self.assertIsNone(result) + + def test_no_change_stay_active(self): + """Case: Both records are enabled and forced on (No change).""" + prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) + curr = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) + + result = get_migration_type(curr, prev) + + self.assertIsNone(result) + + prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_OFF) + curr = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_OFF) + + result = get_migration_type(curr, prev) + + self.assertIsNone(result) + + def test_no_change_stay_inactive(self): + """Case: Both records are disabled (No change).""" + prev = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) + curr = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) + + result = get_migration_type(curr, prev) + + self.assertIsNone(result) + + def test_transition_to_forward(self): + """Case: Transition from disabled to enabled/forced on.""" + prev = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) + curr = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) + + result = get_migration_type(curr, prev) + + self.assertEqual(result, MigrationType.FORWARD) + + def test_transition_to_rollback(self): + """Case: Transition from enabled/forced on to disabled.""" + prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) + curr = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) + + result = get_migration_type(curr, prev) + + self.assertEqual(result, MigrationType.ROLLBACK) + + def test_change_choice_to_rollback(self): + """Case: Enabled remains True, but choice changes from forced to something else.""" + prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) + curr = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_OFF) + + result = get_migration_type(curr, prev) + + self.assertEqual(result, MigrationType.ROLLBACK) From 96b2e5700ecbc42b9323d42e62e873fb2b6d423e Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Tue, 21 Apr 2026 09:12:12 -0500 Subject: [PATCH 26/29] chore: add django-waffle in base requirements --- openedx_authz/settings/test.py | 1 + requirements/base.in | 1 + requirements/base.txt | 5 +++-- requirements/ci.txt | 2 +- requirements/dev.txt | 6 +++--- requirements/doc.txt | 4 ++-- requirements/quality.txt | 4 ++-- requirements/test.txt | 4 ++-- 8 files changed, 15 insertions(+), 12 deletions(-) diff --git a/openedx_authz/settings/test.py b/openedx_authz/settings/test.py index 5d4c50f9..bd6d698d 100644 --- a/openedx_authz/settings/test.py +++ b/openedx_authz/settings/test.py @@ -39,6 +39,7 @@ def plugin_settings(settings): # pylint: disable=unused-argument "openedx_authz.apps.OpenedxAuthzConfig", "openedx_authz.tests.stubs.apps.StubsConfig", "organizations", + "waffle", ) MIDDLEWARE = [ diff --git a/requirements/base.in b/requirements/base.in index 99d7082a..11b452c6 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -13,3 +13,4 @@ edx-api-doc-tools # Tools for API documentation edx-django-utils # Used for RequestCache edx-drf-extensions # Extensions for Django Rest Framework used by Open edX edx-organizations # Organizations library for Open edX +django-waffle # Waffle library for feature flag management diff --git a/requirements/base.txt b/requirements/base.txt index 830c1607..1be06872 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -48,6 +48,7 @@ django-simple-history==3.11.0 # via edx-organizations django-waffle==5.0.0 # via + # -r requirements/base.in # edx-django-utils # edx-drf-extensions djangorestframework==3.17.1 @@ -84,7 +85,7 @@ edx-opaque-keys==4.0.0 # edx-organizations edx-organizations==8.0.0 # via -r requirements/base.in -idna==3.11 +idna==3.12 # via requests inflection==0.5.1 # via drf-yasg @@ -106,7 +107,7 @@ pyjwt[crypto]==2.12.1 # via # drf-jwt # edx-drf-extensions -pymongo==4.16.0 +pymongo==4.17.0 # via edx-opaque-keys pynacl==1.6.2 # via edx-django-utils diff --git a/requirements/ci.txt b/requirements/ci.txt index fb94db11..441e26a3 100644 --- a/requirements/ci.txt +++ b/requirements/ci.txt @@ -4,7 +4,7 @@ # # make upgrade # -cachetools==7.0.5 +cachetools==7.0.6 # via tox colorama==0.4.6 # via tox diff --git a/requirements/dev.txt b/requirements/dev.txt index 98a0f9ed..19a8d456 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -23,7 +23,7 @@ build==1.4.3 # via # -r requirements/pip-tools.txt # pip-tools -cachetools==7.0.5 +cachetools==7.0.6 # via # -r requirements/ci.txt # tox @@ -169,7 +169,7 @@ filelock==3.29.0 # python-discovery # tox # virtualenv -idna==3.11 +idna==3.12 # via # -r requirements/quality.txt # requests @@ -287,7 +287,7 @@ pylint-plugin-utils==0.9.0 # -r requirements/quality.txt # pylint-celery # pylint-django -pymongo==4.16.0 +pymongo==4.17.0 # via # -r requirements/quality.txt # edx-opaque-keys diff --git a/requirements/doc.txt b/requirements/doc.txt index babb2ad5..6b91f191 100644 --- a/requirements/doc.txt +++ b/requirements/doc.txt @@ -143,7 +143,7 @@ edx-organizations==8.0.0 # via -r requirements/test.txt id==1.6.1 # via twine -idna==3.11 +idna==3.12 # via # -r requirements/test.txt # requests @@ -236,7 +236,7 @@ pyjwt[crypto]==2.12.1 # -r requirements/test.txt # drf-jwt # edx-drf-extensions -pymongo==4.16.0 +pymongo==4.17.0 # via # -r requirements/test.txt # edx-opaque-keys diff --git a/requirements/quality.txt b/requirements/quality.txt index 0432e279..91afebf2 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -133,7 +133,7 @@ edx-opaque-keys==4.0.0 # edx-organizations edx-organizations==8.0.0 # via -r requirements/test.txt -idna==3.11 +idna==3.12 # via # -r requirements/test.txt # requests @@ -212,7 +212,7 @@ pylint-plugin-utils==0.9.0 # via # pylint-celery # pylint-django -pymongo==4.16.0 +pymongo==4.17.0 # via # -r requirements/test.txt # edx-opaque-keys diff --git a/requirements/test.txt b/requirements/test.txt index 7ea01398..f48ebd0c 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -116,7 +116,7 @@ edx-opaque-keys==4.0.0 # edx-organizations edx-organizations==8.0.0 # via -r requirements/base.txt -idna==3.11 +idna==3.12 # via # -r requirements/base.txt # requests @@ -164,7 +164,7 @@ pyjwt[crypto]==2.12.1 # -r requirements/base.txt # drf-jwt # edx-drf-extensions -pymongo==4.16.0 +pymongo==4.17.0 # via # -r requirements/base.txt # edx-opaque-keys From 3a7c4b71eaf57006cad6283686431e2ef7029805 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Tue, 21 Apr 2026 09:12:30 -0500 Subject: [PATCH 27/29] feat: add effective state evaluation for waffle flags in migration logic --- openedx_authz/handlers.py | 60 ++++++++++++++++++++++++++++----------- 1 file changed, 43 insertions(+), 17 deletions(-) diff --git a/openedx_authz/handlers.py b/openedx_authz/handlers.py index d61c31aa..45205893 100644 --- a/openedx_authz/handlers.py +++ b/openedx_authz/handlers.py @@ -13,6 +13,7 @@ from django.conf import settings from django.db.models.signals import post_delete, post_save from django.dispatch import receiver +from waffle.models import Flag from openedx_authz.api.users import unassign_all_roles_from_user from openedx_authz.engine.utils import run_course_authoring_migration @@ -149,39 +150,61 @@ def handle_org_waffle_flag_change(sender, instance, **kwargs) -> None: WaffleOverrideRecord = Union[WaffleFlagCourseOverrideModel, WaffleFlagOrgOverrideModel] +def get_effective_state(record: WaffleOverrideRecord | None, global_flag_enabled: bool) -> bool: + """ + Determine the actual active state of the feature by evaluating the local override + against the global flag state. + + Args: + record (WaffleOverrideRecord | None): The waffle flag record to evaluate. + global_flag_enabled (bool): The state of the global flag. + + Returns: + bool: True if the feature is active, False otherwise. + """ + # If there is no override, or the override is disabled, the state falls back to the global flag. + if not record or not record.enabled: + return global_flag_enabled + + # If there is an active override, it dictates the state, ignoring the global flag. + if record.override_choice == WAFFLE_OVERRIDE_FORCE_ON: + return True + if record.override_choice == WAFFLE_OVERRIDE_FORCE_OFF: + return False + + # Safety fallback (in case override_choice is corrupted or empty) + return global_flag_enabled + + def get_migration_type( - current_record: WaffleOverrideRecord, previous_record: WaffleOverrideRecord | None + current_record: WaffleOverrideRecord, + previous_record: WaffleOverrideRecord | None, + global_flag_enabled: bool, ) -> MigrationType | None: """ - Determine the migration type by comparing the current and previous record states. + Determine the migration type by comparing the effective state before and after the transaction. - A FORWARD migration occurs when a flag transitions from disabled/not-forced - to enabled and forced-on. A ROLLBACK occurs when it transitions back. + This accounts for the global flag state, meaning a transition could be triggered by + removing a FORCE_OFF override when the global flag is ON. Args: current_record (WaffleOverrideRecord): The state of the record in the current transaction. previous_record (WaffleOverrideRecord | None): The state of the record prior to the current transaction. + global_flag_enabled (bool): The state of the global flag. Returns: MigrationType.FORWARD: If the flag is newly forced on. MigrationType.ROLLBACK: If the forced-on state is removed. None: If there is no effective change in the flag's behavior. """ - # 1. Handle New Records (Creation) - if not previous_record: - is_active = current_record.enabled and current_record.override_choice == WAFFLE_OVERRIDE_FORCE_ON - return MigrationType.FORWARD if is_active else None - - # 2. Extract Effective States - # A flag is "effectively active" only if enabled AND set to FORCE_ON - was_effectively_active = previous_record.enabled and previous_record.override_choice == WAFFLE_OVERRIDE_FORCE_ON - is_effectively_active = current_record.enabled and current_record.override_choice == WAFFLE_OVERRIDE_FORCE_ON + was_effectively_active = get_effective_state(previous_record, global_flag_enabled) + is_effectively_active = get_effective_state(current_record, global_flag_enabled) - # 3. Return None if state hasn't changed + # If the effective behavior hasn't changed, we don't need to do anything if is_effectively_active == was_effectively_active: return None - # 4. Determine Transition Type + # If it is now effectively active (and wasn't before), migrate forward. Otherwise, rollback. return MigrationType.FORWARD if is_effectively_active else MigrationType.ROLLBACK @@ -254,9 +277,12 @@ def trigger_course_authoring_migration( prev_record = sender.objects.filter(**filter_kwargs).exclude(id=instance.id).order_by("-change_date").first() - migration_type = get_migration_type(instance, prev_record) + global_flag = Flag.objects.filter(name=AUTHZ_COURSE_AUTHORING_FLAG.name).first() + global_flag_enabled = bool(global_flag and global_flag.everyone) + + migration_type = get_migration_type(instance, prev_record, global_flag_enabled) if migration_type is None: - logger.info("No change in waffle flag, skipping migration") + logger.info("No effective change in waffle flag behavior, skipping migration") return excluded_course_ids = frozenset() From fb803acf6fe9877add12c6580595db2e89f8fbdc Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Tue, 21 Apr 2026 09:21:56 -0500 Subject: [PATCH 28/29] refactor: update test assertions for waffle flag migration logic --- openedx_authz/tests/test_handlers.py | 142 +++++++++++++++++++++------ 1 file changed, 110 insertions(+), 32 deletions(-) diff --git a/openedx_authz/tests/test_handlers.py b/openedx_authz/tests/test_handlers.py index da014883..81879f64 100644 --- a/openedx_authz/tests/test_handlers.py +++ b/openedx_authz/tests/test_handlers.py @@ -367,7 +367,7 @@ def test_course_scope_migration_depends_on_override_choice( ) else: mock_run.assert_not_called() - mock_logger.info.assert_called_once_with("No change in waffle flag, skipping migration") + mock_logger.info.assert_called_once_with("No effective change in waffle flag behavior, skipping migration") @data( (WAFFLE_OVERRIDE_FORCE_ON, MigrationType.FORWARD, True), @@ -402,7 +402,7 @@ def test_org_scope_migration_depends_on_override_choice( ) else: mock_run.assert_not_called() - mock_logger.info.assert_called_once_with("No change in waffle flag, skipping migration") + mock_logger.info.assert_called_once_with("No effective change in waffle flag behavior, skipping migration") def test_org_scope_passes_excluded_course_ids_when_course_overrides_oppose_org(self, mock_run): """Org forward migration excludes active course rows whose override opposes force-on.""" @@ -467,7 +467,7 @@ def test_skips_when_previous_enabled_record_has_same_override_choice( trigger_course_authoring_migration(sender_model, instance, scope_key) mock_run.assert_not_called() - mock_logger.info.assert_called_once_with("No change in waffle flag, skipping migration") + mock_logger.info.assert_called_once_with("No effective change in waffle flag behavior, skipping migration") @data( ( @@ -538,76 +538,154 @@ def test_runs_when_previous_record_disabled_even_if_same_override_choice( class TestGetMigrationType(TestCase): - """Tests for ``get_migration_type``.""" + """Tests for ``get_migration_type`` (effective state includes ``global_flag_enabled``).""" def create_mock_record(self, enabled: bool, choice: str): """Helper to create a mock record object.""" return SimpleNamespace(enabled=enabled, override_choice=choice) - def test_creation_new_record_enabled(self): - """Case: No previous record, current is enabled and forced on.""" + def test_creation_new_record_enabled_global_off(self): + """No prior row: enabling FORCE_ON migrates forward when the global flag is off.""" current = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) - result = get_migration_type(current, None) + result = get_migration_type(current, None, global_flag_enabled=False) self.assertEqual(result, MigrationType.FORWARD) - def test_creation_new_record_disabled(self): - """Case: No previous record, current is disabled.""" - current = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) + def test_creation_new_record_enabled_global_on(self): + """No prior row: FORCE_ON is already the effective state when global is on — no migration.""" + current = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) - result = get_migration_type(current, None) + result = get_migration_type(current, None, global_flag_enabled=True) self.assertIsNone(result) - def test_no_change_stay_active(self): - """Case: Both records are enabled and forced on (No change).""" + def test_creation_new_record_disabled_matches_global(self): + """Disabled row defers to global, no previous row means same effective state as global.""" + current = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) + + self.assertIsNone(get_migration_type(current, None, global_flag_enabled=False)) + self.assertIsNone(get_migration_type(current, None, global_flag_enabled=True)) + + def test_no_change_stay_active_force_on(self): + """Both enabled FORCE_ON — effective stays on.""" prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) curr = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) - result = get_migration_type(curr, prev) - - self.assertIsNone(result) + self.assertIsNone(get_migration_type(curr, prev, global_flag_enabled=False)) + self.assertIsNone(get_migration_type(curr, prev, global_flag_enabled=True)) + def test_no_change_stay_active_force_off(self): + """Both enabled FORCE_OFF — effective stays off.""" prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_OFF) curr = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_OFF) - result = get_migration_type(curr, prev) - - self.assertIsNone(result) + self.assertIsNone(get_migration_type(curr, prev, global_flag_enabled=False)) + self.assertIsNone(get_migration_type(curr, prev, global_flag_enabled=True)) def test_no_change_stay_inactive(self): - """Case: Both records are disabled (No change).""" + """Both rows disabled — both follow global, so no effective change.""" prev = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) curr = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) - result = get_migration_type(curr, prev) + self.assertIsNone(get_migration_type(curr, prev, global_flag_enabled=False)) + self.assertIsNone(get_migration_type(curr, prev, global_flag_enabled=True)) - self.assertIsNone(result) - - def test_transition_to_forward(self): - """Case: Transition from disabled to enabled/forced on.""" + def test_transition_disabled_to_enabled_force_on_global_off(self): + """Row becomes active FORCE_ON while global is off — effective off → on.""" prev = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) curr = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) - result = get_migration_type(curr, prev) + result = get_migration_type(curr, prev, global_flag_enabled=False) self.assertEqual(result, MigrationType.FORWARD) - def test_transition_to_rollback(self): - """Case: Transition from enabled/forced on to disabled.""" + def test_transition_disabled_to_enabled_force_on_global_on(self): + """Previously inactive row followed global (on), turning FORCE_ON on stays on — no op.""" + prev = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) + curr = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) + + result = get_migration_type(curr, prev, global_flag_enabled=True) + + self.assertIsNone(result) + + def test_transition_enabled_force_on_to_disabled_global_off(self): + """FORCE_ON row disabled, global off — effective on → off (rollback).""" prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) curr = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) - result = get_migration_type(curr, prev) + result = get_migration_type(curr, prev, global_flag_enabled=False) self.assertEqual(result, MigrationType.ROLLBACK) - def test_change_choice_to_rollback(self): - """Case: Enabled remains True, but choice changes from forced to something else.""" + def test_transition_enabled_force_on_to_disabled_global_on(self): + """FORCE_ON row disabled but global still on — effective stays on, no migration.""" + prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) + curr = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_ON) + + result = get_migration_type(curr, prev, global_flag_enabled=True) + + self.assertIsNone(result) + + def test_change_choice_force_on_to_force_off(self): + """Enabled FORCE_ON → FORCE_OFF — effective on → off.""" prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) curr = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_OFF) - result = get_migration_type(curr, prev) + self.assertEqual( + get_migration_type(curr, prev, global_flag_enabled=False), + MigrationType.ROLLBACK, + ) + self.assertEqual( + get_migration_type(curr, prev, global_flag_enabled=True), + MigrationType.ROLLBACK, + ) + + def test_change_choice_force_off_to_force_on(self): + """Enabled FORCE_OFF → FORCE_ON — effective off → on.""" + prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_OFF) + curr = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_ON) + + self.assertEqual( + get_migration_type(curr, prev, global_flag_enabled=False), + MigrationType.FORWARD, + ) + self.assertEqual( + get_migration_type(curr, prev, global_flag_enabled=True), + MigrationType.FORWARD, + ) + + def test_remove_force_off_override_when_global_on(self): + """Deleting FORCE_OFF behavior by disabling row restores global on — forward migration.""" + prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_OFF) + curr = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_OFF) + + result = get_migration_type(curr, prev, global_flag_enabled=True) + + self.assertEqual(result, MigrationType.FORWARD) + + def test_remove_force_off_override_when_global_off(self): + """Disabling FORCE_OFF row while global off — still off, no migration.""" + prev = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_OFF) + curr = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_OFF) + + result = get_migration_type(curr, prev, global_flag_enabled=False) + + self.assertIsNone(result) + + def test_add_force_off_override_when_global_on(self): + """New active FORCE_OFF while global on — effective on → off.""" + prev = self.create_mock_record(False, WAFFLE_OVERRIDE_FORCE_OFF) + curr = self.create_mock_record(True, WAFFLE_OVERRIDE_FORCE_OFF) + + result = get_migration_type(curr, prev, global_flag_enabled=True) self.assertEqual(result, MigrationType.ROLLBACK) + + def test_unknown_override_choice_follows_global(self): + """Non on/off choice falls back to global — toggling only matters vs that baseline.""" + prev = self.create_mock_record(True, "unset") + curr = self.create_mock_record(True, "unset") + + self.assertIsNone(get_migration_type(curr, prev, global_flag_enabled=False)) + self.assertIsNone(get_migration_type(curr, prev, global_flag_enabled=True)) From 11782ac501b7b9467be11381f7a8938be502f526 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Tue, 21 Apr 2026 09:29:06 -0500 Subject: [PATCH 29/29] docs: clarify docstring for effective state evaluation --- openedx_authz/handlers.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/openedx_authz/handlers.py b/openedx_authz/handlers.py index 45205893..755d485e 100644 --- a/openedx_authz/handlers.py +++ b/openedx_authz/handlers.py @@ -152,8 +152,9 @@ def handle_org_waffle_flag_change(sender, instance, **kwargs) -> None: def get_effective_state(record: WaffleOverrideRecord | None, global_flag_enabled: bool) -> bool: """ - Determine the actual active state of the feature by evaluating the local override - against the global flag state. + Return whether the feature is effectively active for the override and global flag. + + An enabled override forces on or off, otherwise the result follows the global flag. Args: record (WaffleOverrideRecord | None): The waffle flag record to evaluate.