From 86c66f5534362fa61d09d9b01b23c4931316a735 Mon Sep 17 00:00:00 2001 From: Awais Date: Thu, 12 Nov 2015 15:16:57 +0500 Subject: [PATCH] Adding cache implementation for the programs api. ECOM-2832 --- ...__add_field_programsapiconfig_cache_ttl.py | 73 ++++++++ openedx/core/djangoapps/programs/models.py | 15 ++ .../djangoapps/programs/tests/test_models.py | 29 ++- .../djangoapps/programs/tests/test_views.py | 174 +++++++++++------- openedx/core/djangoapps/programs/utils.py | 27 +++ openedx/core/djangoapps/programs/views.py | 35 +++- 6 files changed, 286 insertions(+), 67 deletions(-) create mode 100644 openedx/core/djangoapps/programs/migrations/0002_auto__add_field_programsapiconfig_cache_ttl.py diff --git a/openedx/core/djangoapps/programs/migrations/0002_auto__add_field_programsapiconfig_cache_ttl.py b/openedx/core/djangoapps/programs/migrations/0002_auto__add_field_programsapiconfig_cache_ttl.py new file mode 100644 index 0000000000..5f7b5cff94 --- /dev/null +++ b/openedx/core/djangoapps/programs/migrations/0002_auto__add_field_programsapiconfig_cache_ttl.py @@ -0,0 +1,73 @@ +# -*- coding: utf-8 -*- +from south.utils import datetime_utils as datetime +from south.db import db +from south.v2 import SchemaMigration +from django.db import models + + +class Migration(SchemaMigration): + + def forwards(self, orm): + # Adding field 'ProgramsApiConfig.cache_ttl' + db.add_column('programs_programsapiconfig', 'cache_ttl', + self.gf('django.db.models.fields.PositiveIntegerField')(default=0), + keep_default=False) + + + def backwards(self, orm): + # Deleting field 'ProgramsApiConfig.cache_ttl' + db.delete_column('programs_programsapiconfig', 'cache_ttl') + + + models = { + 'auth.group': { + 'Meta': {'object_name': 'Group'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), + 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) + }, + 'auth.permission': { + 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, + 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) + }, + 'auth.user': { + 'Meta': {'object_name': 'User'}, + 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) + }, + 'contenttypes.contenttype': { + 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, + 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) + }, + 'programs.programsapiconfig': { + 'Meta': {'ordering': "('-change_date',)", 'object_name': 'ProgramsApiConfig'}, + 'api_version_number': ('django.db.models.fields.IntegerField', [], {}), + 'cache_ttl': ('django.db.models.fields.PositiveIntegerField', [], {'default': '0'}), + 'change_date': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), + 'changed_by': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'null': 'True', 'on_delete': 'models.PROTECT'}), + 'enable_student_dashboard': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'enabled': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'internal_service_url': ('django.db.models.fields.URLField', [], {'max_length': '200'}), + 'public_service_url': ('django.db.models.fields.URLField', [], {'max_length': '200'}) + } + } + + complete_apps = ['programs'] diff --git a/openedx/core/djangoapps/programs/models.py b/openedx/core/djangoapps/programs/models.py index 846b227717..bf866acdf0 100644 --- a/openedx/core/djangoapps/programs/models.py +++ b/openedx/core/djangoapps/programs/models.py @@ -6,6 +6,7 @@ from urlparse import urljoin from django.db.models import BooleanField, IntegerField, URLField from django.utils.translation import ugettext_lazy as _ +from django.db import models from config_models.models import ConfigurationModel @@ -20,6 +21,15 @@ class ProgramsApiConfig(ConfigurationModel): public_service_url = URLField(verbose_name=_("Public Service URL")) api_version_number = IntegerField(verbose_name=_("API Version")) enable_student_dashboard = BooleanField(verbose_name=_("Enable Student Dashboard Displays")) + cache_ttl = models.PositiveIntegerField( + verbose_name=_("Cache Time To Live"), + default=0, + help_text=_( + "Specified in seconds. Enable caching by setting this to a value greater than 0." + ) + ) + + PROGRAMS_API_CACHE_KEY = "programs.api.data" @property def internal_api_url(self): @@ -42,3 +52,8 @@ class ProgramsApiConfig(ConfigurationModel): be enabled or not. """ return self.enabled and self.enable_student_dashboard + + @property + def is_cache_enabled(self): + """Whether responses from the Programs API will be cached.""" + return self.enabled and self.cache_ttl > 0 diff --git a/openedx/core/djangoapps/programs/tests/test_models.py b/openedx/core/djangoapps/programs/tests/test_models.py index da2182689c..82c398c723 100644 --- a/openedx/core/djangoapps/programs/tests/test_models.py +++ b/openedx/core/djangoapps/programs/tests/test_models.py @@ -1,15 +1,15 @@ """ Tests for models supporting Program-related functionality. """ - +import ddt from mock import patch - from django.test import TestCase from openedx.core.djangoapps.programs.models import ProgramsApiConfig from openedx.core.djangoapps.programs.tests.mixins import ProgramsApiConfigMixin +@ddt.ddt @patch('config_models.models.cache.get', return_value=None) # during tests, make every cache get a miss. class ProgramsApiConfigTest(ProgramsApiConfigMixin, TestCase): """ @@ -70,3 +70,28 @@ class ProgramsApiConfigTest(ProgramsApiConfigMixin, TestCase): self.create_config(enabled=True, enable_student_dashboard=True) self.assertTrue(ProgramsApiConfig.current().is_student_dashboard_enabled) + + @ddt.data( + (True, 0), + (False, 0), + (False, 1), + ) + @ddt.unpack + def test_is_cache_enabled_returns_false(self, enabled, cache_ttl, _mock_cache): + """Verify that the method 'is_cache_enabled' returns false if + 'cache_ttl' value is 0 or config is not enabled. + """ + self.assertFalse(ProgramsApiConfig.current().is_cache_enabled) + + self.create_config( + enabled=enabled, + cache_ttl=cache_ttl + ) + self.assertFalse(ProgramsApiConfig.current().is_cache_enabled) + + def test_is_cache_enabled_returns_true(self, _mock_cache): + """"Verify that is_cache_enabled returns True when Programs is enabled + and the cache TTL is greater than 0." + """ + self.create_config(enabled=True, cache_ttl=10) + self.assertTrue(ProgramsApiConfig.current().is_cache_enabled) diff --git a/openedx/core/djangoapps/programs/tests/test_views.py b/openedx/core/djangoapps/programs/tests/test_views.py index 9291e028e6..05aa626199 100644 --- a/openedx/core/djangoapps/programs/tests/test_views.py +++ b/openedx/core/djangoapps/programs/tests/test_views.py @@ -1,12 +1,12 @@ """ Tests for the Programs. """ +from unittest import skipUnless +import ddt from mock import patch from provider.oauth2.models import Client from provider.constants import CONFIDENTIAL -from unittest import skipUnless - from django.conf import settings from django.test import TestCase @@ -14,8 +14,12 @@ from openedx.core.djangoapps.programs.views import get_course_programs_for_dashb from openedx.core.djangoapps.programs.tests.mixins import ProgramsApiConfigMixin from student.tests.factories import UserFactory +# Explicitly import the cache from ConfigurationModel so we can reset it after each test +from config_models.models import cache + @skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') +@ddt.ddt class TestGetXSeriesPrograms(ProgramsApiConfigMixin, TestCase): """ Tests for the Programs views. @@ -26,6 +30,7 @@ class TestGetXSeriesPrograms(ProgramsApiConfigMixin, TestCase): self.create_config(enabled=True, enable_student_dashboard=True) Client.objects.get_or_create(name="programs", client_type=CONFIDENTIAL) self.user = UserFactory() + cache.clear() self.programs_api_response = { "results": [ { @@ -68,6 +73,66 @@ class TestGetXSeriesPrograms(ProgramsApiConfigMixin, TestCase): ] } + self.expected_output = { + 'edX/DemoX_1/Run_1': { + 'category': 'xseries', + 'status': 'active', + 'subtitle': 'Dummy program 1 for testing', + 'name': 'First Program', + 'course_codes': [ + { + 'organization': {'display_name': 'Test Organization 1', 'key': 'edX'}, + 'display_name': 'Demo XSeries Program 1', + 'key': 'TEST_A', + 'run_modes': [ + {'sku': '', 'mode_slug': 'ABC_1', 'course_key': 'edX/DemoX_1/Run_1'}, + {'sku': '', 'mode_slug': 'ABC_2', 'course_key': 'edX/DemoX_2/Run_2'}, + ] + } + ], + 'organization': {'display_name': 'Test Organization 1', 'key': 'edX'}, + 'marketing_slug': 'fake-marketing-slug-xseries-1', + }, + 'edX/DemoX_2/Run_2': { + 'category': 'xseries', + 'status': 'active', + 'subtitle': 'Dummy program 1 for testing', + 'name': 'First Program', + 'course_codes': [ + { + 'organization': {'display_name': 'Test Organization 1', 'key': 'edX'}, + 'display_name': 'Demo XSeries Program 1', + 'key': 'TEST_A', + 'run_modes': [ + {'sku': '', 'mode_slug': 'ABC_1', 'course_key': 'edX/DemoX_1/Run_1'}, + {'sku': '', 'mode_slug': 'ABC_2', 'course_key': 'edX/DemoX_2/Run_2'}, + ] + } + ], + 'organization': {'display_name': 'Test Organization 1', 'key': 'edX'}, + 'marketing_slug': 'fake-marketing-slug-xseries-1', + }, + } + + self.edx_prg_run = { + 'category': 'xseries', + 'status': 'active', + 'subtitle': 'Dummy program 2 for testing', + 'name': 'Second Program', + 'course_codes': [ + { + 'organization': {'display_name': 'Test Organization 2', 'key': 'edX'}, + 'display_name': 'Demo XSeries Program 2', + 'key': 'TEST_B', + 'run_modes': [ + {'sku': '', 'mode_slug': 'XYZ_1', 'course_key': 'edX/Program/Program_Run'}, + ] + } + ], + 'organization': {'display_name': 'Test Organization 2', 'key': 'edX'}, + 'marketing_slug': 'fake-marketing-slug-xseries-2', + } + def test_get_course_programs_with_valid_user_and_courses(self): """ Test that the method 'get_course_programs_for_dashboard' returns only matching courses from the xseries programs in the expected format. @@ -81,48 +146,9 @@ class TestGetXSeriesPrograms(ProgramsApiConfigMixin, TestCase): self.user, ['edX/DemoX_1/Run_1', 'edX/DemoX_2/Run_2', 'valid/edX/Course'] ) - expected_output = { - 'edX/DemoX_1/Run_1': { - 'category': 'xseries', - 'status': 'active', - 'subtitle': 'Dummy program 1 for testing', - 'name': 'First Program', - 'course_codes': [ - { - 'organization': {'display_name': 'Test Organization 1', 'key': 'edX'}, - 'display_name': 'Demo XSeries Program 1', - 'key': 'TEST_A', - 'run_modes': [ - {'sku': '', 'mode_slug': 'ABC_1', 'course_key': 'edX/DemoX_1/Run_1'}, - {'sku': '', 'mode_slug': 'ABC_2', 'course_key': 'edX/DemoX_2/Run_2'}, - ] - } - ], - 'organization': {'display_name': 'Test Organization 1', 'key': 'edX'}, - 'marketing_slug': 'fake-marketing-slug-xseries-1', - }, - 'edX/DemoX_2/Run_2': { - 'category': 'xseries', - 'status': 'active', - 'subtitle': 'Dummy program 1 for testing', - 'name': 'First Program', - 'course_codes': [ - { - 'organization': {'display_name': 'Test Organization 1', 'key': 'edX'}, - 'display_name': 'Demo XSeries Program 1', - 'key': 'TEST_A', - 'run_modes': [ - {'sku': '', 'mode_slug': 'ABC_1', 'course_key': 'edX/DemoX_1/Run_1'}, - {'sku': '', 'mode_slug': 'ABC_2', 'course_key': 'edX/DemoX_2/Run_2'}, - ] - } - ], - 'organization': {'display_name': 'Test Organization 1', 'key': 'edX'}, - 'marketing_slug': 'fake-marketing-slug-xseries-1', - }, - } + self.assertTrue(mock_get.called) - self.assertEqual(expected_output, programs) + self.assertEqual(self.expected_output, programs) self.assertEqual(sorted(programs.keys()), ['edX/DemoX_1/Run_1', 'edX/DemoX_2/Run_2']) # now test with user having multiple courses across two different @@ -132,26 +158,9 @@ class TestGetXSeriesPrograms(ProgramsApiConfigMixin, TestCase): self.user, ['edX/DemoX_1/Run_1', 'edX/DemoX_2/Run_2', 'edX/Program/Program_Run', 'valid/edX/Course'] ) - expected_output['edX/Program/Program_Run'] = { - 'category': 'xseries', - 'status': 'active', - 'subtitle': 'Dummy program 2 for testing', - 'name': 'Second Program', - 'course_codes': [ - { - 'organization': {'display_name': 'Test Organization 2', 'key': 'edX'}, - 'display_name': 'Demo XSeries Program 2', - 'key': 'TEST_B', - 'run_modes': [ - {'sku': '', 'mode_slug': 'XYZ_1', 'course_key': 'edX/Program/Program_Run'}, - ] - } - ], - 'organization': {'display_name': 'Test Organization 2', 'key': 'edX'}, - 'marketing_slug': 'fake-marketing-slug-xseries-2', - } + self.expected_output['edX/Program/Program_Run'] = self.edx_prg_run self.assertTrue(mock_get.called) - self.assertEqual(expected_output, programs) + self.assertEqual(self.expected_output, programs) self.assertEqual( sorted(programs.keys()), ['edX/DemoX_1/Run_1', 'edX/DemoX_2/Run_2', 'edX/Program/Program_Run'] @@ -245,3 +254,44 @@ class TestGetXSeriesPrograms(ProgramsApiConfigMixin, TestCase): program ) self.assertEqual(programs, {}) + + @ddt.data(0, 1) + def test_get_course_programs_with_cache(self, ttl): + """ Test that the method 'get_course_programs_for_dashboard' with + cache_ttl greater than 0 saves the programs into cache and does not + hit the api again until the cached data expires. + """ + self.create_config(enabled=True, enable_student_dashboard=True, cache_ttl=ttl) + # Mock the request call + with patch('slumber.Resource.get') as mock_get: + mock_get.return_value = self.programs_api_response + + # First test with user having multiple courses in a single xseries + programs = get_course_programs_for_dashboard( + self.user, + ['edX/DemoX_1/Run_1', 'edX/DemoX_2/Run_2', 'valid/edX/Course'] + ) + + self.assertTrue(mock_get.called) + self.assertEqual(self.expected_output, programs) + self.assertEqual(sorted(programs.keys()), ['edX/DemoX_1/Run_1', 'edX/DemoX_2/Run_2']) + + # Now test with user having multiple courses across two different + # xseries + mock_get.reset_mock() + programs = get_course_programs_for_dashboard( + self.user, + ['edX/DemoX_1/Run_1', 'edX/DemoX_2/Run_2', 'edX/Program/Program_Run', 'valid/edX/Course'] + ) + self.expected_output['edX/Program/Program_Run'] = self.edx_prg_run + # If cache_ttl value is 0 than cache will be considered as disabled. + # And mocked method will be call again + if ttl == 0: + self.assertTrue(mock_get.called) + else: + self.assertFalse(mock_get.called) + self.assertEqual(self.expected_output, programs) + self.assertEqual( + sorted(programs.keys()), + ['edX/DemoX_1/Run_1', 'edX/DemoX_2/Run_2', 'edX/Program/Program_Run'] + ) diff --git a/openedx/core/djangoapps/programs/utils.py b/openedx/core/djangoapps/programs/utils.py index c3b852dcad..40ab607d9f 100644 --- a/openedx/core/djangoapps/programs/utils.py +++ b/openedx/core/djangoapps/programs/utils.py @@ -1,6 +1,7 @@ """ Helper methods for Programs. """ +from django.core.cache import cache from edx_rest_api_client.client import EdxRestApiClient from openedx.core.djangoapps.programs.models import ProgramsApiConfig @@ -20,3 +21,29 @@ def programs_api_client(api_url, jwt_access_token): api_url, jwt=jwt_access_token ) + + +def is_cache_enabled_for_programs(): # pylint: disable=invalid-name + """Returns a Boolean indicating whether responses from the Programs API + will be cached. + """ + return ProgramsApiConfig.current().is_cache_enabled + + +def set_cached_programs_response(programs_data): + """ Set cache value for the programs data with specific ttl. + + Arguments: + programs_data (dict): Programs data in dictionary format + """ + cache.set( + ProgramsApiConfig.PROGRAMS_API_CACHE_KEY, + programs_data, + ProgramsApiConfig.current().cache_ttl + ) + + +def get_cached_programs_response(): + """ Get programs data from cache against cache key.""" + cache_key = ProgramsApiConfig.PROGRAMS_API_CACHE_KEY + return cache.get(cache_key) diff --git a/openedx/core/djangoapps/programs/views.py b/openedx/core/djangoapps/programs/views.py index f3e404cfbb..06dab71484 100644 --- a/openedx/core/djangoapps/programs/views.py +++ b/openedx/core/djangoapps/programs/views.py @@ -6,7 +6,13 @@ import logging from openedx.core.djangoapps.util.helpers import get_id_token from openedx.core.djangoapps.programs.models import ProgramsApiConfig -from openedx.core.djangoapps.programs.utils import programs_api_client, is_student_dashboard_programs_enabled +from openedx.core.djangoapps.programs.utils import ( + programs_api_client, + is_student_dashboard_programs_enabled, + is_cache_enabled_for_programs, + get_cached_programs_response, + set_cached_programs_response, +) log = logging.getLogger(__name__) @@ -37,6 +43,12 @@ def get_course_programs_for_dashboard(user, course_keys): # pylint: disable=in # unicode-ify the course keys for efficient lookup course_keys = map(unicode, course_keys) + # If cache config is enabled then get the response from cache first. + if is_cache_enabled_for_programs(): + cached_programs = get_cached_programs_response() + if cached_programs is not None: + return _get_user_course_programs(cached_programs, course_keys) + # get programs slumber-based client 'EdxRestApiClient' try: api_client = programs_api_client(ProgramsApiConfig.current().internal_api_url, get_id_token(user, CLIENT_NAME)) @@ -56,14 +68,31 @@ def get_course_programs_for_dashboard(user, course_keys): # pylint: disable=in log.warning("No programs found for the user '%s'.", user.id) return course_programs + # If cache config is enabled than set the cache. + if is_cache_enabled_for_programs(): + set_cached_programs_response(programs) + + return _get_user_course_programs(programs, course_keys) + + +def _get_user_course_programs(programs, users_enrolled_course_keys): # pylint: disable=invalid-name + """ Parse the raw programs according to the users enrolled courses and + return the matched course runs. + + Arguments: + programs (list): List containing the programs data. + users_enrolled_course_keys (list) : List of course keys in which the user is enrolled. + """ + # reindex the result from pgm -> course code -> course run - # to + # to # course run -> program, ignoring course runs not present in the dashboard enrollments + course_programs = {} for program in programs: try: for course_code in program['course_codes']: for run in course_code['run_modes']: - if run['course_key'] in course_keys: + if run['course_key'] in users_enrolled_course_keys: course_programs[run['course_key']] = program except KeyError: log.exception('Unable to parse Programs API response: %r', program)