From 8289c321be1dccb41870cb85d95332149653f7f1 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Tue, 8 Dec 2015 14:01:49 -0500 Subject: [PATCH] Update CourseOverview for Course Catalog Added the following fields to CourseOverview: announcement catalog_visibility course_video_url effort short_description Now requires CourseOverview models for each course in the system: Introduced a CourseOverviewGeneratedHistory model to keep track of when the CourseOverview table was seeded. The seeding can be done preemptively by calling the generate_course_overview management command. Otherwise, it is lazily computed, when needed. --- .../commands/generate_course_overview.py | 22 +-- .../tests/test_generate_course_overview.py | 2 +- .../0002_add_course_catalog_fields.py | 39 +++++ .../0003_courseoverviewgeneratedhistory.py | 28 ++++ .../migrations/0004_courseoverview_org.py | 19 +++ .../content/course_overviews/models.py | 148 +++++++++++++++++- .../content/course_overviews/tests.py | 118 ++++++++++---- 7 files changed, 327 insertions(+), 49 deletions(-) create mode 100644 openedx/core/djangoapps/content/course_overviews/migrations/0002_add_course_catalog_fields.py create mode 100644 openedx/core/djangoapps/content/course_overviews/migrations/0003_courseoverviewgeneratedhistory.py create mode 100644 openedx/core/djangoapps/content/course_overviews/migrations/0004_courseoverview_org.py diff --git a/openedx/core/djangoapps/content/course_overviews/management/commands/generate_course_overview.py b/openedx/core/djangoapps/content/course_overviews/management/commands/generate_course_overview.py index ea2ee5115d..04cfd5e4ea 100644 --- a/openedx/core/djangoapps/content/course_overviews/management/commands/generate_course_overview.py +++ b/openedx/core/djangoapps/content/course_overviews/management/commands/generate_course_overview.py @@ -32,11 +32,13 @@ class Command(BaseCommand): ) def handle(self, *args, **options): - course_keys = [] if options['all']: - course_keys = [course.id for course in modulestore().get_courses()] + # Have CourseOverview generate course overviews for all + # the courses in the system. + CourseOverview.get_all_courses(force_reseeding=True) else: + course_keys = [] if len(args) < 1: raise CommandError('At least one course or --all must be specified.') try: @@ -44,17 +46,7 @@ class Command(BaseCommand): except InvalidKeyError: log.fatal('Invalid key specified.') - if not course_keys: - log.fatal('No courses specified.') + if not course_keys: + log.fatal('No courses specified.') - log.info('Generating course overview for %d courses.', len(course_keys)) - log.debug('Generating course overview(s) for the following courses: %s', course_keys) - - for course_key in course_keys: - try: - CourseOverview.get_from_id(course_key) - except Exception as ex: # pylint: disable=broad-except - log.exception('An error occurred while generating course overview for %s: %s', unicode( - course_key), ex.message) - - log.info('Finished generating course overviews.') + CourseOverview.get_select_courses(course_keys) diff --git a/openedx/core/djangoapps/content/course_overviews/management/commands/tests/test_generate_course_overview.py b/openedx/core/djangoapps/content/course_overviews/management/commands/tests/test_generate_course_overview.py index 12ba8acafc..7178b1c89c 100644 --- a/openedx/core/djangoapps/content/course_overviews/management/commands/tests/test_generate_course_overview.py +++ b/openedx/core/djangoapps/content/course_overviews/management/commands/tests/test_generate_course_overview.py @@ -64,7 +64,7 @@ class TestGenerateCourseOverview(ModuleStoreTestCase): self.command.handle('not/found', all=False) self.assertTrue(mock_log.fatal.called) - @patch('openedx.core.djangoapps.content.course_overviews.management.commands.generate_course_overview.log') + @patch('openedx.core.djangoapps.content.course_overviews.models.log') def test_not_found_key(self, mock_log): """ Test keys not found are logged. diff --git a/openedx/core/djangoapps/content/course_overviews/migrations/0002_add_course_catalog_fields.py b/openedx/core/djangoapps/content/course_overviews/migrations/0002_add_course_catalog_fields.py new file mode 100644 index 0000000000..7d6f48e301 --- /dev/null +++ b/openedx/core/djangoapps/content/course_overviews/migrations/0002_add_course_catalog_fields.py @@ -0,0 +1,39 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('course_overviews', '0001_initial'), + ] + + operations = [ + migrations.AddField( + model_name='courseoverview', + name='announcement', + field=models.DateTimeField(null=True), + ), + migrations.AddField( + model_name='courseoverview', + name='catalog_visibility', + field=models.TextField(null=True), + ), + migrations.AddField( + model_name='courseoverview', + name='course_video_url', + field=models.TextField(null=True), + ), + migrations.AddField( + model_name='courseoverview', + name='effort', + field=models.TextField(null=True), + ), + migrations.AddField( + model_name='courseoverview', + name='short_description', + field=models.TextField(null=True), + ), + ] diff --git a/openedx/core/djangoapps/content/course_overviews/migrations/0003_courseoverviewgeneratedhistory.py b/openedx/core/djangoapps/content/course_overviews/migrations/0003_courseoverviewgeneratedhistory.py new file mode 100644 index 0000000000..807f7b62c6 --- /dev/null +++ b/openedx/core/djangoapps/content/course_overviews/migrations/0003_courseoverviewgeneratedhistory.py @@ -0,0 +1,28 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models +import django.utils.timezone +import model_utils.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('course_overviews', '0002_add_course_catalog_fields'), + ] + + operations = [ + migrations.CreateModel( + name='CourseOverviewGeneratedHistory', + fields=[ + ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)), + ('created', model_utils.fields.AutoCreatedField(default=django.utils.timezone.now, verbose_name='created', editable=False)), + ('modified', model_utils.fields.AutoLastModifiedField(default=django.utils.timezone.now, verbose_name='modified', editable=False)), + ('num_courses', models.IntegerField(null=True)), + ], + options={ + 'abstract': False, + }, + ), + ] diff --git a/openedx/core/djangoapps/content/course_overviews/migrations/0004_courseoverview_org.py b/openedx/core/djangoapps/content/course_overviews/migrations/0004_courseoverview_org.py new file mode 100644 index 0000000000..73faff4d6e --- /dev/null +++ b/openedx/core/djangoapps/content/course_overviews/migrations/0004_courseoverview_org.py @@ -0,0 +1,19 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('course_overviews', '0003_courseoverviewgeneratedhistory'), + ] + + operations = [ + migrations.AddField( + model_name='courseoverview', + name='org', + field=models.TextField(default=b'outdated_entry', max_length=255), + ), + ] diff --git a/openedx/core/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py index 6e07db130d..e018df6bdb 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/openedx/core/djangoapps/content/course_overviews/models.py @@ -2,19 +2,21 @@ Declaration of CourseOverview model """ import json -from django.db import models, transaction +import logging +from django.db import models, transaction from django.db.models.fields import BooleanField, DateTimeField, DecimalField, TextField, FloatField, IntegerField from django.db.utils import IntegrityError +from django.template import defaultfilters from django.utils.translation import ugettext from lms.djangoapps import django_comment_client from model_utils.models import TimeStampedModel - from opaque_keys.edx.keys import CourseKey +from openedx.core.djangoapps.models.course_details import CourseDetails from util.date_utils import strftime_localized from xmodule import course_metadata_utils -from xmodule.course_module import CourseDescriptor +from xmodule.course_module import CourseDescriptor, DEFAULT_START_DATE from xmodule.error_module import ErrorDescriptor from xmodule.modulestore.django import modulestore from xmodule_django.models import CourseKeyField, UsageKeyField @@ -22,20 +24,26 @@ from xmodule_django.models import CourseKeyField, UsageKeyField from ccx_keys.locator import CCXLocator +log = logging.getLogger(__name__) + + class CourseOverview(TimeStampedModel): """ Model for storing and caching basic information about a course. This model contains basic course metadata such as an ID, display name, image URL, and any other information that would be necessary to display - a course as part of a user dashboard or enrollment API. + a course as part of: + user dashboard (enrolled courses) + course catalog (courses to enroll in) + course about (meta data about the course) """ class Meta(object): app_label = 'course_overviews' # IMPORTANT: Bump this whenever you modify this model and/or add a migration. - VERSION = 2 + VERSION = 3 # Cache entry versioning. version = IntegerField() @@ -43,6 +51,7 @@ class CourseOverview(TimeStampedModel): # Course identification id = CourseKeyField(db_index=True, primary_key=True, max_length=255) _location = UsageKeyField(max_length=255) + org = TextField(max_length=255, default='outdated_entry') display_name = TextField(null=True) display_number_with_default = TextField() display_org_with_default = TextField() @@ -51,6 +60,7 @@ class CourseOverview(TimeStampedModel): start = DateTimeField(null=True) end = DateTimeField(null=True) advertised_start = TextField(null=True) + announcement = DateTimeField(null=True) # URLs course_image_url = TextField() @@ -82,6 +92,12 @@ class CourseOverview(TimeStampedModel): invitation_only = BooleanField(default=False) max_student_enrollments_allowed = IntegerField(null=True) + # Catalog information + catalog_visibility = TextField(null=True) + short_description = TextField(null=True) + course_video_url = TextField(null=True) + effort = TextField(null=True) + @classmethod def _create_from_course(cls, course): """ @@ -99,6 +115,8 @@ class CourseOverview(TimeStampedModel): from lms.djangoapps.certificates.api import get_active_web_certificate from openedx.core.lib.courses import course_image_url + log.info('Creating course overview for %s.', unicode(course.id)) + # Workaround for a problem discovered in https://openedx.atlassian.net/browse/TNL-2806. # If the course has a malformed grading policy such that # course._grading_policy['GRADE_CUTOFFS'] = {}, then @@ -125,6 +143,7 @@ class CourseOverview(TimeStampedModel): version=cls.VERSION, id=course.id, _location=course.location, + org=course.location.org, display_name=display_name, display_number_with_default=course.display_number_with_default, display_org_with_default=course.display_org_with_default, @@ -132,6 +151,7 @@ class CourseOverview(TimeStampedModel): start=start, end=end, advertised_start=course.advertised_start, + announcement=course.announcement, course_image_url=course_image_url(course), facebook_url=course.facebook_url, @@ -156,6 +176,11 @@ class CourseOverview(TimeStampedModel): enrollment_domain=course.enrollment_domain, invitation_only=course.invitation_only, max_student_enrollments_allowed=max_student_enrollments_allowed, + + catalog_visibility=course.catalog_visibility, + short_description=CourseDetails.fetch_about_attribute(course.id, 'short_description'), + effort=CourseDetails.fetch_about_attribute(course.id, 'effort'), + course_video_url=CourseDetails.fetch_video_url(course.id), ) @classmethod @@ -343,6 +368,42 @@ class CourseOverview(TimeStampedModel): strftime_localized ) + @property + def sorting_score(self): + """ + Returns a tuple that can be used to sort the courses according + the how "new" they are. The "newness" score is computed using a + heuristic that takes into account the announcement and + (advertised) start dates of the course if available. + + The lower the number the "newer" the course. + """ + return course_metadata_utils.sorting_score(self.start, self.advertised_start, self.announcement) + + @property + def start_type(self): + """ + Returns the type of the course's 'start' field. + """ + if self.advertised_start: + return u'string' + elif self.start != DEFAULT_START_DATE: + return u'timestamp' + else: + return u'empty' + + @property + def start_display(self): + """ + Returns the display value for the course's start date. + """ + if self.advertised_start: + return self.advertised_start + elif self.start != DEFAULT_START_DATE: + return defaultfilters.date(self.start, "DATE_FORMAT") + else: + return None + def may_certify(self): """ Returns whether it is acceptable to show the student a certificate @@ -361,6 +422,72 @@ class CourseOverview(TimeStampedModel): """ return json.loads(self._pre_requisite_courses_json) + @classmethod + def get_select_courses(cls, course_keys): + """ + Returns CourseOverview objects for the given course_keys. + """ + course_overviews = [] + + log.info('Generating course overview for %d courses.', len(course_keys)) + log.debug('Generating course overview(s) for the following courses: %s', course_keys) + + for course_key in course_keys: + try: + course_overviews.append(CourseOverview.get_from_id(course_key)) + except Exception as ex: # pylint: disable=broad-except + log.exception( + 'An error occurred while generating course overview for %s: %s', + unicode(course_key), + ex.message, + ) + + log.info('Finished generating course overviews.') + + return course_overviews + + @classmethod + def get_all_courses(cls, force_reseeding=False, org=None): + """ + Returns all CourseOverview objects in the database. + + Arguments: + force_reseeding (bool): Optional parameter. + + If True, the modulestore is used as the source of truth for + the list of courses, even if the CourseOverview table was + previously seeded. However, only non-existing CourseOverview + entries or those with older data model versions or will get + populated. + + If False, the list of courses is retrieved from the + CourseOverview table if it was previously seeded, falling + back to the modulestore if it wasn't seeded. + + org (string): Optional parameter that allows filtering + by organization. + """ + if force_reseeding or not CourseOverviewGeneratedHistory.objects.first(): + # Seed the CourseOverview table with data for all + # courses in the system. + course_keys = [course.id for course in modulestore().get_courses()] + course_overviews = cls.get_select_courses(course_keys) + num_courses = len(course_overviews) + CourseOverviewGeneratedHistory.objects.create(num_courses=num_courses) + if org: + course_overviews = [c for c in course_overviews if c.org == org] + + else: + # Note: If a newly created course is not returned in this QueryList, + # make sure the "publish" signal was emitted when the course was + # created. For tests using CourseFactory, use emit_signals=True. + # Or pass True for force_reseeding. + course_overviews = CourseOverview.objects.all() + if org: + course_overviews = course_overviews.filter(org=org) + + return course_overviews + @classmethod def get_all_course_keys(cls): """ @@ -389,3 +516,14 @@ class CourseOverviewTab(models.Model): """ tab_id = models.CharField(max_length=50) course_overview = models.ForeignKey(CourseOverview, db_index=True, related_name="tabs") + + +class CourseOverviewGeneratedHistory(TimeStampedModel): + """ + Model for keeping track of when CourseOverview Models are + generated/seeded. + """ + num_courses = IntegerField(null=True) + + def __unicode__(self): + return self.num_courses diff --git a/openedx/core/djangoapps/content/course_overviews/tests.py b/openedx/core/djangoapps/content/course_overviews/tests.py index b3869be9b7..25a03bb5b5 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests.py +++ b/openedx/core/djangoapps/content/course_overviews/tests.py @@ -11,8 +11,14 @@ import pytz from django.utils import timezone from lms.djangoapps.certificates.api import get_active_web_certificate +from openedx.core.djangoapps.models.course_details import CourseDetails from openedx.core.lib.courses import course_image_url from xmodule.course_metadata_utils import DEFAULT_START_DATE +from xmodule.course_module import ( + CATALOG_VISIBILITY_CATALOG_AND_ABOUT, + CATALOG_VISIBILITY_ABOUT, + CATALOG_VISIBILITY_NONE, +) from xmodule.error_module import ErrorDescriptor from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore @@ -96,6 +102,7 @@ class CourseOverviewTestCase(ModuleStoreTestCase): 'enrollment_domain', 'invitation_only', 'max_student_enrollments_allowed', + 'catalog_visibility', ] for attribute_name in fields_to_test: course_value = getattr(course, attribute_name) @@ -124,45 +131,49 @@ class CourseOverviewTestCase(ModuleStoreTestCase): self.assertEqual(cache_miss_value, cache_hit_value) # Other values to test - # Note: we test the start and end attributes here instead of in - # fields_to_test, because I ran into trouble while testing datetimes + + # Note: we test the time-related attributes here instead of in + # fields_to_test, because we run into trouble while testing datetimes # for equality. When writing and reading dates from databases, the # resulting values are often off by fractions of a second. So, as a # workaround, we simply test if the start and end times are the same # number of seconds from the Unix epoch. + time_field_accessor = lambda object, field_name: get_seconds_since_epoch(getattr(object, field_name)) + + # The course about fields are accessed through the CourseDetail + # class for the course module, and stored as attributes on the + # CourseOverview objects. + course_about_accessor = lambda object, field_name: CourseDetails.fetch_about_attribute(object.id, field_name) + others_to_test = [ + ('start', time_field_accessor, time_field_accessor), + ('end', time_field_accessor, time_field_accessor), + ('enrollment_start', time_field_accessor, time_field_accessor), + ('enrollment_end', time_field_accessor, time_field_accessor), + ('announcement', time_field_accessor, time_field_accessor), + + ('short_description', course_about_accessor, getattr), + ('effort', course_about_accessor, getattr), ( - course_image_url(course), - course_overview_cache_miss.course_image_url, - course_overview_cache_hit.course_image_url + 'video', + lambda c, __: CourseDetails.fetch_video_url(c.id), + lambda c, __: c.course_video_url, ), ( - get_active_web_certificate(course) is not None, - course_overview_cache_miss.has_any_active_web_certificate, - course_overview_cache_hit.has_any_active_web_certificate + 'course_image_url', + lambda c, __: course_image_url(c), + getattr, ), ( - get_seconds_since_epoch(course.start), - get_seconds_since_epoch(course_overview_cache_miss.start), - get_seconds_since_epoch(course_overview_cache_hit.start), - ), - ( - get_seconds_since_epoch(course.end), - get_seconds_since_epoch(course_overview_cache_miss.end), - get_seconds_since_epoch(course_overview_cache_hit.end), - ), - ( - get_seconds_since_epoch(course.enrollment_start), - get_seconds_since_epoch(course_overview_cache_miss.enrollment_start), - get_seconds_since_epoch(course_overview_cache_hit.enrollment_start), - ), - ( - get_seconds_since_epoch(course.enrollment_end), - get_seconds_since_epoch(course_overview_cache_miss.enrollment_end), - get_seconds_since_epoch(course_overview_cache_hit.enrollment_end), + 'has_any_active_web_certificate', + lambda c, field_name: get_active_web_certificate(c) is not None, + getattr, ), ] - for (course_value, cache_miss_value, cache_hit_value) in others_to_test: + for attribute_name, course_accessor, course_overview_accessor in others_to_test: + course_value = course_accessor(course, attribute_name) + cache_miss_value = course_overview_accessor(course_overview_cache_miss, attribute_name) + cache_hit_value = course_overview_accessor(course_overview_cache_hit, attribute_name) self.assertEqual(course_value, cache_miss_value) self.assertEqual(cache_miss_value, cache_hit_value) @@ -178,6 +189,7 @@ class CourseOverviewTestCase(ModuleStoreTestCase): "display_name": "Test Course", # Display name provided "start": LAST_WEEK, # In the middle of the course "end": NEXT_WEEK, + "announcement": LAST_MONTH, # Announcement date provided "advertised_start": "2015-01-01 11:22:33", # Parse-able advertised_start "pre_requisite_courses": [ # Has pre-requisites 'course-v1://edX+test1+run1', @@ -194,6 +206,7 @@ class CourseOverviewTestCase(ModuleStoreTestCase): "pre_requisite_courses": [], # No pre-requisites "static_asset_path": "my/relative/path", # Relative asset path "certificates_show_before_end": False, + "catalog_visibility": CATALOG_VISIBILITY_CATALOG_AND_ABOUT, }, { "display_name": "", # Empty display name @@ -203,6 +216,7 @@ class CourseOverviewTestCase(ModuleStoreTestCase): "pre_requisite_courses": [], # No pre-requisites "static_asset_path": "", # Empty asset path "certificates_show_before_end": False, + "catalog_visibility": CATALOG_VISIBILITY_ABOUT, }, { # # Don't set display name @@ -212,6 +226,7 @@ class CourseOverviewTestCase(ModuleStoreTestCase): "pre_requisite_courses": [], # No pre-requisites "static_asset_path": None, # No asset path "certificates_show_before_end": False, + "catalog_visibility": CATALOG_VISIBILITY_NONE, } ], [ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split] @@ -325,7 +340,7 @@ class CourseOverviewTestCase(ModuleStoreTestCase): course_overview = CourseOverview._create_from_course(course) # pylint: disable=protected-access self.assertEqual(course_overview.lowest_passing_grade, None) - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 1), (ModuleStoreEnum.Type.split, 3, 4)) + @ddt.data((ModuleStoreEnum.Type.mongo, 4, 4), (ModuleStoreEnum.Type.split, 3, 4)) @ddt.unpack def test_versioning(self, modulestore_type, min_mongo_calls, max_mongo_calls): """ @@ -425,3 +440,50 @@ class CourseOverviewTestCase(ModuleStoreTestCase): # knows how to write, it's not going to overwrite what's there. unmodified_overview = CourseOverview.get_from_id(course.id) self.assertEqual(unmodified_overview.version, 11) + + def test_get_select_courses(self): + course_ids = [CourseFactory.create().id for __ in range(3)] + select_course_ids = course_ids[:len(course_ids) - 1] # all items except the last + self.assertSetEqual( + {course_overview.id for course_overview in CourseOverview.get_select_courses(select_course_ids)}, + set(select_course_ids), + ) + + def test_get_all_courses(self): + course_ids = [CourseFactory.create().id for __ in range(3)] + self.assertSetEqual( + {course_overview.id for course_overview in CourseOverview.get_all_courses()}, + set(course_ids), + ) + + with mock.patch( + 'openedx.core.djangoapps.content.course_overviews.models.CourseOverview.get_from_id' + ) as mock_get_from_id: + CourseOverview.get_all_courses() + self.assertFalse(mock_get_from_id.called) + + CourseOverview.get_all_courses(force_reseeding=True) + self.assertTrue(mock_get_from_id.called) + + def test_get_all_courses_by_org(self): + org_courses = [] # list of lists of courses + for index in range(2): + org_courses.append([ + CourseFactory.create(org='test_org_' + unicode(index)) + for __ in range(3) + ]) + + self.assertSetEqual( + {c.id for c in CourseOverview.get_all_courses(org='test_org_0', force_reseeding=True)}, + {c.id for c in org_courses[0]}, + ) + + self.assertSetEqual( + {c.id for c in CourseOverview.get_all_courses(org='test_org_1')}, + {c.id for c in org_courses[1]}, + ) + + self.assertSetEqual( + {c.id for c in CourseOverview.get_all_courses()}, + {c.id for c in org_courses[0] + org_courses[1]}, + )