From 9f356b90d117767932efdf8a8c118458b759b777 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Wed, 8 Sep 2021 06:54:19 +0000 Subject: [PATCH] feat!: Add data model for course app API [BD-38] [TNL-8506] [BB-4470] (#28165) * feat!: Add data model for course app API The current course apps API makes individual queries to determine if a course app is enabled, which can be inefficient. With this change we now have a Django model that contains this information, allowing us to make bulk queries about all the course apps for a course in the API. It also adds a new signal handler that initialises the status of all course apps in a course on course publish. * Use celery tasks and a management commands to make cache async * Review feedback * update log messages use separate celery task for each course task --- openedx/core/djangoapps/course_apps/api.py | 23 ++++++- openedx/core/djangoapps/course_apps/apps.py | 4 ++ .../core/djangoapps/course_apps/handlers.py | 36 +++++++++++ .../course_apps/management/__init__.py | 0 .../management/commands/__init__.py | 0 .../commands/cache_course_app_status.py | 22 +++++++ .../course_apps/migrations/0001_initial.py | 62 +++++++++++++++++++ openedx/core/djangoapps/course_apps/models.py | 61 +++++++++++++++++- .../course_apps/rest_api/v1/views.py | 11 +++- .../core/djangoapps/course_apps/signals.py | 8 +++ openedx/core/djangoapps/course_apps/tasks.py | 48 ++++++++++++++ .../djangoapps/course_apps/tests/test_api.py | 31 ++++++++++ 12 files changed, 300 insertions(+), 6 deletions(-) create mode 100644 openedx/core/djangoapps/course_apps/handlers.py create mode 100644 openedx/core/djangoapps/course_apps/management/__init__.py create mode 100644 openedx/core/djangoapps/course_apps/management/commands/__init__.py create mode 100644 openedx/core/djangoapps/course_apps/management/commands/cache_course_app_status.py create mode 100644 openedx/core/djangoapps/course_apps/migrations/0001_initial.py create mode 100644 openedx/core/djangoapps/course_apps/signals.py create mode 100644 openedx/core/djangoapps/course_apps/tasks.py diff --git a/openedx/core/djangoapps/course_apps/api.py b/openedx/core/djangoapps/course_apps/api.py index 4ce3f8bff1..acd2c8f552 100644 --- a/openedx/core/djangoapps/course_apps/api.py +++ b/openedx/core/djangoapps/course_apps/api.py @@ -3,8 +3,10 @@ Python APIs for Course Apps. """ from django.contrib.auth import get_user_model from opaque_keys.edx.keys import CourseKey +from openedx.core.djangoapps.course_apps.models import CourseAppStatus from .plugins import CourseAppsPluginManager +from .signals import COURSE_APP_STATUS_INIT User = get_user_model() @@ -22,8 +24,20 @@ def is_course_app_enabled(course_key: CourseKey, app_id: str) -> bool: Returns: True or False depending on if the course app is enabled or not. """ - course_app = CourseAppsPluginManager.get_plugin(app_id) - is_enabled = course_app.is_enabled(course_key) + try: + course_app_status = CourseAppStatus.objects.get(course_key=course_key, app_id=app_id) + is_enabled = course_app_status.enabled + except CourseAppStatus.DoesNotExist: + course_app = CourseAppsPluginManager.get_plugin(app_id) + is_enabled = course_app.is_enabled(course_key) + # If the status doesn't exist it means this is an existing course so + # dispatch an initialisation signal to make sure the next request is + # direct from the database. + COURSE_APP_STATUS_INIT.send( + sender=app_id, + course_key=course_key, + is_enabled=is_enabled, + ) return is_enabled @@ -42,4 +56,9 @@ def set_course_app_enabled(course_key: CourseKey, app_id: str, enabled: bool, us """ course_app = CourseAppsPluginManager.get_plugin(app_id) enabled = course_app.set_enabled(course_key, user=user, enabled=enabled) + CourseAppStatus.update_status_for_course_app( + course_key=course_key, + app_id=app_id, + enabled=enabled, + ) return enabled diff --git a/openedx/core/djangoapps/course_apps/apps.py b/openedx/core/djangoapps/course_apps/apps.py index ef03761e6d..87c1bde6ad 100644 --- a/openedx/core/djangoapps/course_apps/apps.py +++ b/openedx/core/djangoapps/course_apps/apps.py @@ -22,3 +22,7 @@ class CourseAppsConfig(AppConfig): } }, } + + def ready(self): + # Connect signal handlers + from . import handlers # pylint: disable=unused-import diff --git a/openedx/core/djangoapps/course_apps/handlers.py b/openedx/core/djangoapps/course_apps/handlers.py new file mode 100644 index 0000000000..fc932af71e --- /dev/null +++ b/openedx/core/djangoapps/course_apps/handlers.py @@ -0,0 +1,36 @@ +""" +Signal handlers for course apps. +""" +from django.dispatch import receiver +from opaque_keys.edx.keys import CourseKey +from xmodule.modulestore.django import SignalHandler + +from .models import CourseAppStatus +from .signals import COURSE_APP_STATUS_INIT +from .tasks import update_course_apps_status + + +@receiver(SignalHandler.course_published) +def update_course_apps(sender, course_key, **kwargs): # pylint: disable=unused-argument + """ + Whenever the course is published, update the status of course apps in the + django models to match their status in the course. + """ + update_course_apps_status.delay(str(course_key)) + + +# pylint: disable=unused-argument +@receiver(COURSE_APP_STATUS_INIT) +def initialize_course_app_status(sender: str, course_key: CourseKey, is_enabled: bool, **kwargs): + """ + Create a new CourseAppStatus object when a course app is accessed for the first time. + + For existing courses CourseAppStatus object might not exist. This handler creates a new + CourseAppStatus object in such cases. + + Args: + sender (str): The sender is the app_id for the app being accessed for the first time. + course_key (CourseKey): The key for the course run the app is associated with + is_enabled (bool): The status of the course app + """ + CourseAppStatus.update_status_for_course_app(course_key=course_key, app_id=sender, enabled=is_enabled) diff --git a/openedx/core/djangoapps/course_apps/management/__init__.py b/openedx/core/djangoapps/course_apps/management/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openedx/core/djangoapps/course_apps/management/commands/__init__.py b/openedx/core/djangoapps/course_apps/management/commands/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openedx/core/djangoapps/course_apps/management/commands/cache_course_app_status.py b/openedx/core/djangoapps/course_apps/management/commands/cache_course_app_status.py new file mode 100644 index 0000000000..ad1d1a774f --- /dev/null +++ b/openedx/core/djangoapps/course_apps/management/commands/cache_course_app_status.py @@ -0,0 +1,22 @@ +""" +Management command to cache course apps status in the databse. +""" + +from textwrap import dedent + +from django.core.management import BaseCommand + +from openedx.core.djangoapps.course_apps.tasks import cache_all_course_apps_status + + +class Command(BaseCommand): + """ + Command to cache status of course apps to the database to speed up queries. + + This can be run as a one-off command. If new plugins are installed or existing plugins are uninstalled, the + status of those will be cached on first access so re-running this isn't necessary. + """ + help = dedent(__doc__) + + def handle(self, *args, **options): + cache_all_course_apps_status.delay() diff --git a/openedx/core/djangoapps/course_apps/migrations/0001_initial.py b/openedx/core/djangoapps/course_apps/migrations/0001_initial.py new file mode 100644 index 0000000000..2376f9dd9d --- /dev/null +++ b/openedx/core/djangoapps/course_apps/migrations/0001_initial.py @@ -0,0 +1,62 @@ +# Generated by Django 2.2.24 on 2021-07-13 07:48 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import django.utils.timezone +import model_utils.fields +import opaque_keys.edx.django.models +import simple_history.models + + +class Migration(migrations.Migration): + + initial = True + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.CreateModel( + name='CourseAppStatus', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('created', model_utils.fields.AutoCreatedField(default=django.utils.timezone.now, editable=False, verbose_name='created')), + ('modified', model_utils.fields.AutoLastModifiedField(default=django.utils.timezone.now, editable=False, verbose_name='modified')), + ('app_id', models.CharField(db_index=True, max_length=32)), + ('enabled', models.BooleanField(default=False)), + ('course_key', opaque_keys.edx.django.models.CourseKeyField(db_index=True, max_length=255)), + ], + ), + migrations.CreateModel( + name='HistoricalCourseAppStatus', + fields=[ + ('id', models.IntegerField(auto_created=True, blank=True, db_index=True, verbose_name='ID')), + ('created', model_utils.fields.AutoCreatedField(default=django.utils.timezone.now, editable=False, verbose_name='created')), + ('modified', model_utils.fields.AutoLastModifiedField(default=django.utils.timezone.now, editable=False, verbose_name='modified')), + ('app_id', models.CharField(db_index=True, max_length=32)), + ('enabled', models.BooleanField(default=False)), + ('course_key', opaque_keys.edx.django.models.CourseKeyField(db_index=True, max_length=255)), + ('history_id', models.AutoField(primary_key=True, serialize=False)), + ('history_date', models.DateTimeField()), + ('history_change_reason', models.CharField(max_length=100, null=True)), + ('history_type', models.CharField(choices=[('+', 'Created'), ('~', 'Changed'), ('-', 'Deleted')], max_length=1)), + ('history_user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to=settings.AUTH_USER_MODEL)), + ], + options={ + 'verbose_name': 'historical course app status', + 'ordering': ('-history_date', '-history_id'), + 'get_latest_by': 'history_date', + }, + bases=(simple_history.models.HistoricalChanges, models.Model), + ), + migrations.AddIndex( + model_name='courseappstatus', + index=models.Index(fields=['app_id', 'course_key'], name='course_apps_app_id_b3df8c_idx'), + ), + migrations.AddConstraint( + model_name='courseappstatus', + constraint=models.UniqueConstraint(fields=('app_id', 'course_key'), name='unique_app_config_for_course'), + ), + ] diff --git a/openedx/core/djangoapps/course_apps/models.py b/openedx/core/djangoapps/course_apps/models.py index 327a8dd707..49868910d0 100644 --- a/openedx/core/djangoapps/course_apps/models.py +++ b/openedx/core/djangoapps/course_apps/models.py @@ -1,3 +1,62 @@ """ -Models for course apps. +Models to store course apps data. """ +from typing import Dict + +from django.db import models +from model_utils.models import TimeStampedModel +from opaque_keys.edx.django.models import CourseKeyField +from opaque_keys.edx.keys import CourseKey +from simple_history.models import HistoricalRecords + + +class CourseAppStatus(TimeStampedModel): + """ + CourseAppStatus stores the enabled/disabled status for course apps. + + .. no_pii: + """ + + app_id = models.CharField(max_length=32, db_index=True) + enabled = models.BooleanField(default=False) + course_key = CourseKeyField(max_length=255, db_index=True) + + history = HistoricalRecords() + + @classmethod + def get_all_app_status_data_for_course(cls, course_key: CourseKey) -> Dict[str, bool]: + """ + Get a dictionary containing the status of all apps linked to the course. + + Args: + course_key (CourseKey): the course id for which app status is needed + + Returns: + A dictionary where the keys are app ids and the values are booleans + indicating if the app with that id is enabled + """ + return dict( + cls.objects.filter(course_key=course_key).values("app_id", "enabled") + ) + + @classmethod + def update_status_for_course_app(cls, course_key: CourseKey, app_id: str, enabled: bool): + """ + Creates or updates a status entry for the specified app and course. + + Args: + course_key (CourseKey): the course id for which the app status is to + be set + app_id (str): id for course app to update + enabled (bool): enabled status to set for app + """ + CourseAppStatus.objects.update_or_create( + course_key=course_key, + app_id=app_id, + defaults={'enabled': enabled} + ) + + class Meta: + constraints = [models.UniqueConstraint(fields=("app_id", "course_key"), name="unique_app_config_for_course")] + indexes = [models.Index(fields=("app_id", "course_key"))] + app_label = "course_apps" diff --git a/openedx/core/djangoapps/course_apps/rest_api/v1/views.py b/openedx/core/djangoapps/course_apps/rest_api/v1/views.py index 35bdb707f5..5695b0e887 100644 --- a/openedx/core/djangoapps/course_apps/rest_api/v1/views.py +++ b/openedx/core/djangoapps/course_apps/rest_api/v1/views.py @@ -14,6 +14,7 @@ from rest_framework.request import Request from rest_framework.response import Response from common.djangoapps.student.auth import has_studio_write_access +from openedx.core.djangoapps.course_apps.models import CourseAppStatus from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, validate_course_key, verify_course_exists from ...api import is_course_app_enabled, set_course_app_enabled @@ -60,9 +61,10 @@ class CourseAppSerializer(serializers.Serializer): # pylint: disable=abstract-m def to_representation(self, instance: CourseApp) -> Dict: course_key = self.context.get("course_key") request = self.context.get("request") + app_status = self.context.get("app_status") data = { "id": instance.app_id, - "enabled": is_course_app_enabled(course_key, instance.app_id), + "enabled": app_status.get(instance.app_id, is_course_app_enabled(course_key, instance.app_id)), "name": instance.name, "description": instance.description, "allowed_operations": instance.get_allowed_operations(course_key, request.user), @@ -133,11 +135,13 @@ class CourseAppsView(DeveloperErrorViewMixin, views.APIView): """ course_key = CourseKey.from_string(course_id) course_apps = CourseAppsPluginManager.get_apps_available_for_course(course_key) + course_apps_status = CourseAppStatus.get_all_app_status_data_for_course(course_key) serializer = CourseAppSerializer( course_apps, many=True, context={ "course_key": course_key, + "app_status": course_apps_status, "request": request, } ) @@ -192,12 +196,13 @@ class CourseAppsView(DeveloperErrorViewMixin, views.APIView): course_app = None if not course_app or not course_app.is_available(course_key): raise ValidationError({"id": "Invalid app ID"}) - set_course_app_enabled(course_key=course_key, app_id=app_id, enabled=enabled, user=request.user) + is_enabled = set_course_app_enabled(course_key=course_key, app_id=app_id, enabled=enabled, user=request.user) serializer = CourseAppSerializer( course_app, context={ "course_key": course_key, "request": request, - } + "app_status": {app_id: is_enabled}, + }, ) return Response(serializer.data) diff --git a/openedx/core/djangoapps/course_apps/signals.py b/openedx/core/djangoapps/course_apps/signals.py new file mode 100644 index 0000000000..393cf9f995 --- /dev/null +++ b/openedx/core/djangoapps/course_apps/signals.py @@ -0,0 +1,8 @@ +""" +Signals for course apps. +""" +from django.dispatch import Signal + +# A signal that's dispatched when the status for a course app that's available for a course +# isn't present in the `CourseAppStatus` table. +COURSE_APP_STATUS_INIT = Signal(providing_args=["course_key", "is_enabled"]) diff --git a/openedx/core/djangoapps/course_apps/tasks.py b/openedx/core/djangoapps/course_apps/tasks.py new file mode 100644 index 0000000000..1448c6e3df --- /dev/null +++ b/openedx/core/djangoapps/course_apps/tasks.py @@ -0,0 +1,48 @@ +""" +This file contains celery tasks for course apps. +""" + +from celery import shared_task +from celery.utils.log import get_task_logger +from edx_django_utils.monitoring import set_code_owner_attribute +from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.locator import LibraryLocator + +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.djangoapps.course_apps.models import CourseAppStatus +from openedx.core.djangoapps.course_apps.plugins import CourseAppsPluginManager + +log = get_task_logger(__name__) + + +@shared_task(name='openedx.core.djangoapps.course_apps.tasks.cache_all_course_apps_status') +@set_code_owner_attribute +def cache_all_course_apps_status(): + """ + Create CourseAppStatus entries for all course apps, across all courses to speed up queries. + """ + for idx, course_key in enumerate(CourseOverview.objects.values_list('id', flat=True)): + if isinstance(course_key, LibraryLocator): + continue + update_course_apps_status.delay(str(course_key)) + if (idx + 1) % 100 == 0: + log.info("Cached course app status for %s courses successfully", idx + 1) + log.info("Cached course app status for all courses successfully") + + +@shared_task(name='openedx.core.djangoapps.course_apps.tasks.update_course_apps_status') +@set_code_owner_attribute +def update_course_apps_status(course_key_str: str): + """ + Create CourseAppStatus entries for apps available for the specified course. + """ + course_key = CourseKey.from_string(course_key_str) + course_apps = CourseAppsPluginManager.get_apps_available_for_course(course_key) + log.info("Caching course apps status for course with id: %s", course_key) + for course_app in course_apps: + is_enabled = course_app.is_enabled(course_key=course_key) + CourseAppStatus.update_status_for_course_app( + course_key=course_key, + app_id=course_app.app_id, + enabled=is_enabled, + ) diff --git a/openedx/core/djangoapps/course_apps/tests/test_api.py b/openedx/core/djangoapps/course_apps/tests/test_api.py index 3f4868783e..a8a72672ea 100644 --- a/openedx/core/djangoapps/course_apps/tests/test_api.py +++ b/openedx/core/djangoapps/course_apps/tests/test_api.py @@ -7,6 +7,7 @@ from unittest.mock import Mock import ddt from django.test import TestCase from opaque_keys.edx.locator import CourseLocator +from openedx.core.djangoapps.course_apps.models import CourseAppStatus from .utils import make_test_course_app from ..api import is_course_app_enabled, set_course_app_enabled @@ -38,6 +39,34 @@ class CourseAppsPythonAPITest(TestCase): assert is_course_app_enabled(self.course_key, "test-app") == enabled mock_is_enabled.assert_called() + def test_plugin_init(self, get_plugin): + """ + Test that the when get is called for the first time, a new CourseAppStatus object is created automatically. + """ + CourseApp = make_test_course_app(is_available=True) + get_plugin.return_value = CourseApp + # Make sure that a status doesn't already exist in the database for this app + assert not CourseAppStatus.objects.filter(course_key=self.course_key, app_id="test-app").exists() + # Test that the default value of false is returned + assert not is_course_app_enabled(self.course_key, "test-app") + # Test that a status object is created automatically + assert CourseAppStatus.objects.filter(course_key=self.course_key, app_id="test-app").exists() + + @ddt.data(True, False) + def test_plugin_enabled_for_existing(self, enabled, get_plugin): + """ + Test that if an existing model exists, we use that instead of calling is_enabled. + """ + CourseApp = make_test_course_app(is_available=True) + get_plugin.return_value = CourseApp + # Set contradictory value in existing CourseAppStatus to ensure that the `is_enabled` value is + # being used. + mock_is_enabled = Mock(return_value=not enabled) + CourseAppStatus.objects.create(course_key=self.course_key, app_id="test-app", enabled=enabled) + with mock.patch.object(CourseApp, "is_enabled", mock_is_enabled, create=True): + assert is_course_app_enabled(self.course_key, "test-app") == enabled + mock_is_enabled.assert_not_called() + @ddt.data(True, False) def test_set_plugin_enabled(self, enabled, get_plugin): """ @@ -46,6 +75,8 @@ class CourseAppsPythonAPITest(TestCase): CourseApp = make_test_course_app(is_available=True) get_plugin.return_value = CourseApp mock_set_enabled = Mock(return_value=enabled) + assert not CourseAppStatus.objects.filter(course_key=self.course_key, app_id="test-app").exists() with mock.patch.object(CourseApp, "set_enabled", mock_set_enabled, create=True): assert set_course_app_enabled(self.course_key, "test-app", enabled, Mock()) == enabled mock_set_enabled.assert_called() + assert CourseAppStatus.objects.get(course_key=self.course_key, app_id="test-app").enabled == enabled