Merge pull request #686 from edx-solutions/release

Release 2016-04-06

cms/envs/aws.py

@@ -12,6 +12,7 @@ This is the default template for our main set of AWS servers.
 # pylint: disable=invalid-name
 
 import json
+import importlib
 
 from .common import *
 
@@ -210,6 +211,17 @@ STUDIO_SHORT_NAME = ENV_TOKENS.get('STUDIO_SHORT_NAME', 'Studio')
 TENDER_DOMAIN = ENV_TOKENS.get('TENDER_DOMAIN', TENDER_DOMAIN)
 TENDER_SUBDOMAIN = ENV_TOKENS.get('TENDER_SUBDOMAIN', TENDER_SUBDOMAIN)
 
+# Modules having these categories would be excluded from progress calculations
+PROGRESS_DETACHED_APPS = ['group_project_v2']
+for app in PROGRESS_DETACHED_APPS:
+    try:
+        app_config = importlib.import_module('.app_config', app)
+    except ImportError:
+        continue
+
+    detached_module_categories = getattr(app_config, 'PROGRESS_DETACHED_CATEGORIES', [])
+    PROGRESS_DETACHED_CATEGORIES.extend(detached_module_categories)
+
 # Event Tracking
 if "TRACKING_IGNORE_URL_PATTERNS" in ENV_TOKENS:
     TRACKING_IGNORE_URL_PATTERNS = ENV_TOKENS.get("TRACKING_IGNORE_URL_PATTERNS")
@@ -226,7 +238,6 @@ if FEATURES.get('AUTH_USE_CAS'):
     MIDDLEWARE_CLASSES += ('django_cas.middleware.CASMiddleware',)
     CAS_ATTRIBUTE_CALLBACK = ENV_TOKENS.get('CAS_ATTRIBUTE_CALLBACK', None)
     if CAS_ATTRIBUTE_CALLBACK:
-        import importlib
         CAS_USER_DETAILS_RESOLVER = getattr(
             importlib.import_module(CAS_ATTRIBUTE_CALLBACK['module']),
             CAS_ATTRIBUTE_CALLBACK['function']

cms/envs/common.py

@@ -42,6 +42,7 @@ from lms.envs.common import (
     # technically accessible through the CMS via legacy URLs.
     PROFILE_IMAGE_BACKEND, PROFILE_IMAGE_DEFAULT_FILENAME, PROFILE_IMAGE_DEFAULT_FILE_EXTENSION,
     PROFILE_IMAGE_SECRET_KEY, PROFILE_IMAGE_MIN_BYTES, PROFILE_IMAGE_MAX_BYTES,
+    PROGRESS_DETACHED_VERTICAL_CATEGORIES, PROGRESS_DETACHED_CATEGORIES,
     # The following setting is included as it is used to check whether to
     # display credit eligibility table on the CMS or not.
     ENABLE_CREDIT_ELIGIBILITY, YOUTUBE_API_KEY

lms/djangoapps/api_manager/courses/tests.py

@@ -145,13 +145,6 @@ class CoursesApiTests(ModuleStoreTestCase):
             display_name="Group Project2"
         )
 
-        self.course_content = ItemFactory.create(
-            category="videosequence",
-            parent_location=self.chapter.location,
-            data=self.test_data,
-            display_name="Video_Sequence",
-        )
-
         self.course_content2 = ItemFactory.create(
             category="sequential",
             parent_location=self.chapter.location,
@@ -159,13 +152,6 @@ class CoursesApiTests(ModuleStoreTestCase):
             display_name="Sequential",
         )
 
-        self.content_child = ItemFactory.create(
-            category="video",
-            parent_location=self.course_content.location,
-            data=self.test_data,
-            display_name="Video"
-        )
-
         self.content_child2 = ItemFactory.create(
             category="vertical",
             parent_location=self.course_content2.location,
@@ -173,6 +159,20 @@ class CoursesApiTests(ModuleStoreTestCase):
             display_name="Vertical Sequence"
         )
 
+        self.course_content = ItemFactory.create(
+            category="videosequence",
+            parent_location=self.content_child2.location,
+            data=self.test_data,
+            display_name="Video_Sequence",
+        )
+
+        self.content_child = ItemFactory.create(
+            category="video",
+            parent_location=self.course_content.location,
+            data=self.test_data,
+            display_name="Video"
+        )
+
         self.content_subchild = ItemFactory.create(
             category="video",
             parent_location=self.content_child2.location,
@@ -584,12 +584,13 @@ class CoursesApiTests(ModuleStoreTestCase):
         chapter = response.data['content'][0]
         self.assertEqual(chapter['category'], 'chapter')
         self.assertEqual(chapter['name'], 'Overview')
-        self.assertEqual(len(chapter['children']), 6)
+        # we should have 5 children of Overview chapter
+        # 1 sequential, 1 vertical, 1 videosequence and 2 videos
+        self.assertEqual(len(chapter['children']), 5)
 
-        sequence = chapter['children'][0]
-        self.assertEqual(sequence['category'], 'videosequence')
-        self.assertEqual(sequence['name'], 'Video_Sequence')
-        self.assertNotIn('children', sequence)
+        # Make sure one of the children should be a sequential
+        sequential = [child for child in chapter['children'] if child['category'] == 'sequential']
+        self.assertGreater(len(sequential), 0)
 
     def test_courses_tree_get_root(self):
         # query the course tree to quickly get naviation information
@@ -2341,7 +2342,7 @@ class CoursesApiTests(ModuleStoreTestCase):
             local_content_name = 'Video_Sequence{}'.format(i)
             local_content = ItemFactory.create(
                 category="videosequence",
-                parent_location=self.chapter.location,
+                parent_location=self.content_child2.location,
                 data=self.test_data,
                 display_name=local_content_name
             )

lms/djangoapps/progress/management/commands/recalculate_progress_for_users.py

+"""
+One-time data migration script -- should not need to run it again
+"""
+import logging
+from optparse import make_option
+
+from django.core.management.base import BaseCommand
+from django.conf import settings
+from django.db.models import Q
+
+from progress.models import StudentProgress, CourseModuleCompletion
+from progress.signals import is_valid_progress_module
+from student.models import CourseEnrollment
+from xmodule.modulestore.django import modulestore
+
+log = logging.getLogger(__name__)
+
+
+class Command(BaseCommand):
+    """
+    Recalculate progress entries for the specified course(s) and/or user(s)
+    """
+    help = 'Recalculate existing users progress per course'
+    option_list = BaseCommand.option_list + (
+        make_option(
+            "-c",
+            "--course_ids",
+            dest="course_ids",
+            help="List of courses for which to Recalculate progress",
+            metavar="first/course/id,second/course/id"
+        ),
+        make_option(
+            "-u",
+            "--user_ids",
+            dest="user_ids",
+            help="List of users for which to Recalculate progress",
+            metavar="1234,2468,3579"
+        ),
+    )
+
+    def handle(self, *args, **options):
+        course_ids = options.get('course_ids')
+        user_ids = options.get('user_ids')
+
+        status_summary = {'skipped': 0, 'updated': 0}
+        total_users_processed = 0
+        detached_categories = getattr(settings, 'PROGRESS_DETACHED_CATEGORIES', [])
+        cat_list = [Q(content_id__contains=item.strip()) for item in detached_categories]
+        cat_list = reduce(lambda a, b: a | b, cat_list)
+
+        # Get the list of courses from the system
+        courses = modulestore().get_courses()
+
+        # If one or more courses were specified by the caller, just use those ones...
+        if course_ids is not None:
+            filtered_courses = []
+            for course in courses:
+                if unicode(course.id) in course_ids.split(','):
+                    filtered_courses.append(course)
+            courses = filtered_courses
+
+        for course in courses:
+            users = CourseEnrollment.objects.users_enrolled_in(course.id)
+            # If one or more users were specified by the caller, just use those ones...
+            if user_ids is not None:
+                filtered_users = []
+                for user in users:
+                    if str(user.id) in user_ids.split(','):
+                        filtered_users.append(user)
+                users = filtered_users
+
+            # For each user...
+            for user in users:
+                total_users_processed += 1
+                status = 'skipped'
+                completions = CourseModuleCompletion.objects.filter(course_id=course.id, user_id=user.id)\
+                    .exclude(cat_list).values_list('content_id', flat=True).distinct()
+
+                num_completions = sum([is_valid_progress_module(content_id=content_id) for content_id in completions])
+                try:
+                    existing_record = StudentProgress.objects.get(user=user, course_id=course.id)
+
+                    if existing_record.completions != num_completions:
+                        existing_record.completions = num_completions
+                        status = 'updated'
+
+                    if status == 'updated':
+                        existing_record.save()
+
+                except StudentProgress.DoesNotExist:
+                    status = "skipped"
+
+                log_msg = 'Progress entry {} -- Course: {}, User: {}  (completions: {})'.format(
+                    status,
+                    course.id,
+                    user.id,
+                    num_completions
+                )
+
+                status_summary[status] += 1
+                log.info(log_msg)
+        print "command completed. Total users processed", total_users_processed, status_summary

lms/djangoapps/progress/management/commands/tests/test_generate_progress_entries.py

@@ -45,6 +45,18 @@ class GenerateProgressEntriesTests(ModuleStoreTestCase):
             display_name="Overview"
         )
 
+        sub_section1 = ItemFactory.create(
+            category="sequential",
+            parent_location=chapter1.location,
+            display_name="Sequential 1",
+        )
+
+        vertical1 = ItemFactory.create(
+            category="vertical",
+            parent_location=sub_section1.location,
+            display_name="Vertical 1"
+        )
+
         chapter2 = ItemFactory.create(
             category="chapter",
             parent_location=self.course.location,
@@ -52,29 +64,42 @@ class GenerateProgressEntriesTests(ModuleStoreTestCase):
             due=datetime(2014, 5, 16, 14, 30),
             display_name="Overview"
         )
+
+        sub_section2 = ItemFactory.create(
+            category="sequential",
+            parent_location=chapter2.location,
+            display_name="Sequential 2",
+        )
+
+        vertical2 = ItemFactory.create(
+            category="vertical",
+            parent_location=sub_section2.location,
+            display_name="Vertical 2"
+        )
+
         self.problem = ItemFactory.create(
-            parent_location=chapter1.location,
+            parent_location=vertical1.location,
             category='problem',
             data=StringResponseXMLFactory().build_xml(answer='bar'),
             display_name="homework problem 1",
             metadata={'rerandomize': 'always', 'graded': True, 'format': "Homework"}
         )
         self.problem2 = ItemFactory.create(
-            parent_location=chapter2.location,
+            parent_location=vertical2.location,
             category='problem',
             data=StringResponseXMLFactory().build_xml(answer='bar'),
             display_name="homework problem 2",
             metadata={'rerandomize': 'always', 'graded': True, 'format': "Homework"}
         )
         self.problem3 = ItemFactory.create(
-            parent_location=chapter2.location,
+            parent_location=vertical2.location,
             category='problem',
             data=StringResponseXMLFactory().build_xml(answer='bar'),
             display_name="lab problem 1",
             metadata={'rerandomize': 'always', 'graded': True, 'format': "Lab"}
         )
         self.problem4 = ItemFactory.create(
-            parent_location=chapter2.location,
+            parent_location=vertical2.location,
             category='problem',
             data=StringResponseXMLFactory().build_xml(answer='bar'),
             display_name="lab problem 2",

lms/djangoapps/progress/management/commands/tests/test_recalculate_progress_for_users.py

+"""
+Tests for recalculate_progress_for_users.py
+"""
+from datetime import datetime
+import uuid
+
+from django.conf import settings
+from django.test.utils import override_settings
+from django.db.models.signals import post_save
+
+from capa.tests.response_xml_factory import StringResponseXMLFactory
+from progress.management.commands import recalculate_progress_for_users
+from progress.models import StudentProgress, StudentProgressHistory, CourseModuleCompletion
+from progress.signals import handle_cmc_post_save_signal
+from student.tests.factories import UserFactory, CourseEnrollmentFactory
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, mixed_store_config
+from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
+
+MODULESTORE_CONFIG = mixed_store_config(settings.COMMON_TEST_DATA_ROOT, {}, include_xml=False)
+
+
+@override_settings(MODULESTORE=MODULESTORE_CONFIG)
+class RecalculateProgressEntriesTests(ModuleStoreTestCase):
+    """
+    Test suite for progress recalculation script
+    """
+
+    def setUp(self):
+        super(RecalculateProgressEntriesTests, self).setUp()
+
+        self.course = CourseFactory.create(
+            start=datetime(2014, 6, 16, 14, 30),
+            end=datetime(2020, 1, 16)
+        )
+        self.test_data = '<html>{}</html>'.format(str(uuid.uuid4()))
+
+        chapter1 = ItemFactory.create(
+            category="chapter",
+            parent_location=self.course.location,
+            data=self.test_data,
+            due=datetime(2014, 5, 16, 14, 30),
+            display_name="Overview"
+        )
+
+        sub_section1 = ItemFactory.create(
+            category="sequential",
+            parent_location=chapter1.location,
+            display_name="Sequential 1",
+        )
+
+        vertical1 = ItemFactory.create(
+            category="vertical",
+            parent_location=sub_section1.location,
+            display_name="Vertical 1"
+        )
+
+        chapter2 = ItemFactory.create(
+            category="chapter",
+            parent_location=self.course.location,
+            data=self.test_data,
+            due=datetime(2014, 5, 16, 14, 30),
+            display_name="Overview"
+        )
+
+        sub_section2 = ItemFactory.create(
+            category="sequential",
+            parent_location=chapter2.location,
+            display_name="Sequential 2",
+        )
+
+        vertical2 = ItemFactory.create(
+            category="vertical",
+            parent_location=sub_section2.location,
+            display_name="Vertical 2"
+        )
+
+        self.problem = ItemFactory.create(
+            parent_location=vertical1.location,
+            category='problem',
+            data=StringResponseXMLFactory().build_xml(answer='bar'),
+            display_name="homework problem 1",
+            metadata={'rerandomize': 'always', 'graded': True, 'format': "Homework"}
+        )
+        self.problem2 = ItemFactory.create(
+            parent_location=vertical2.location,
+            category='problem',
+            data=StringResponseXMLFactory().build_xml(answer='bar'),
+            display_name="homework problem 2",
+            metadata={'rerandomize': 'always', 'graded': True, 'format': "Homework"}
+        )
+        self.problem3 = ItemFactory.create(
+            parent_location=vertical2.location,
+            category='problem',
+            data=StringResponseXMLFactory().build_xml(answer='bar'),
+            display_name="lab problem 1",
+            metadata={'rerandomize': 'always', 'graded': True, 'format': "Lab"}
+        )
+        self.problem4 = ItemFactory.create(
+            parent_location=vertical2.location,
+            category='problem',
+            data=StringResponseXMLFactory().build_xml(answer='bar'),
+            display_name="lab problem 2",
+            metadata={'rerandomize': 'always', 'graded': True, 'format': "Lab"}
+        )
+        self.non_vertical_problem = ItemFactory.create(
+            parent_location=chapter1.location,
+            category='problem',
+            display_name="non vertical problem",
+        )
+
+        # Create some users and enroll them
+        self.users = [UserFactory.create(username="testuser" + str(__), profile='test') for __ in xrange(3)]
+        for user in self.users:
+            CourseEnrollmentFactory.create(user=user, course_id=self.course.id)
+        # Turn off the signalling mechanism temporarily
+        post_save.disconnect(receiver=handle_cmc_post_save_signal,
+                             sender=CourseModuleCompletion, dispatch_uid='edxapp.api_manager.post_save_cms')
+        self._generate_course_completion_test_entries()
+        post_save.connect(receiver=handle_cmc_post_save_signal,
+                          sender=CourseModuleCompletion, dispatch_uid='edxapp.api_manager.post_save_cms')
+
+    def _generate_course_completion_test_entries(self):
+        """
+        Clears existing CourseModuleCompletion entries and creates 3 for each user
+        """
+        CourseModuleCompletion.objects.all().delete()
+        for idx, user in enumerate(self.users):
+            if idx % 2 == 0:
+                StudentProgress.objects.get_or_create(
+                    user_id=user.id, course_id=self.course.id, completions=0
+                )
+            CourseModuleCompletion.objects.get_or_create(user=user,
+                                                         course_id=self.course.id,
+                                                         content_id=unicode(self.problem.location),
+                                                         stage=None)
+
+            CourseModuleCompletion.objects.get_or_create(user=user,
+                                                         course_id=self.course.id,
+                                                         content_id=unicode(self.problem2.location),
+                                                         stage=None)
+
+            CourseModuleCompletion.objects.get_or_create(user=user,
+                                                         course_id=self.course.id,
+                                                         content_id=unicode(self.problem3.location),
+                                                         stage=None)
+            CourseModuleCompletion.objects.get_or_create(user=user,
+                                                         course_id=self.course.id,
+                                                         content_id=unicode(self.non_vertical_problem.location),
+                                                         stage=None)
+
+    def test_generate_progress_entries_command(self):
+        """
+        Test the progress entry generator
+        """
+        # Set up the command context
+        course_ids = '{},bogus/course/id'.format(self.course.id)
+        user_ids = '{}'.format(self.users[0].id)
+        current_entries = StudentProgress.objects.all()
+        self.assertEqual(len(current_entries), 2)
+        current_entries = StudentProgressHistory.objects.all()
+        self.assertEqual(len(current_entries), 2)
+
+        # Run the command just for one user
+        recalculate_progress_for_users.Command().handle(user_ids=user_ids)
+
+        # Confirm the progress has been properly updated
+        current_entries = StudentProgress.objects.all()
+        self.assertEqual(len(current_entries), 2)
+        current_entries = StudentProgressHistory.objects.all()
+        self.assertEqual(len(current_entries), 3)
+        user0_entry = StudentProgress.objects.get(user=self.users[0])
+        self.assertEqual(user0_entry.completions, 3)
+
+        # Run the command across all users, but just for the specified course
+        recalculate_progress_for_users.Command().handle(course_ids=course_ids)
+
+        # The first user will be skipped this next time around because they already have a progress record
+        # and their completions have not changed
+        user0_entry = StudentProgress.objects.get(user=self.users[0])
+        self.assertEqual(user0_entry.completions, 3)
+
+        # Confirm that the progress has been properly updated
+        current_entries = StudentProgress.objects.all()
+        self.assertEqual(len(current_entries), 2)
+        current_entries = StudentProgressHistory.objects.all()
+        self.assertEqual(len(current_entries), 4)
+
+        # second user has no entry in StudentProgress so they should b skipped
+        user1_entry = StudentProgress.objects.filter(user=self.users[1])
+        self.assertEqual(len(user1_entry), 0)
+
+        # third user should have their progress updated
+        user2_entry = StudentProgress.objects.get(user=self.users[2])
+        self.assertEqual(user2_entry.completions, 3)
+
+    def test_progress_history(self):
+        """
+        Test the progress, and history
+        """
+        # Clear enteries
+        StudentProgress.objects.all().delete()
+        StudentProgressHistory.objects.all().delete()
+        self._generate_course_completion_test_entries()
+
+        current_entries = StudentProgress.objects.all()
+        self.assertEqual(len(current_entries), 3)
+        current_entries = StudentProgressHistory.objects.all()
+        # StudentProgressHistory should have 11 entries
+        # 9 entries for progress history of 3 users each with 3 completions
+        # and 2 entries for initial progress creation of 2 users
+        self.assertEqual(len(current_entries), 11)
+        user0_entry = StudentProgress.objects.get(user=self.users[0])
+        self.assertEqual(user0_entry.completions, 3)

lms/djangoapps/progress/signals.py

@@ -6,6 +6,7 @@ import logging
 
 from django.dispatch import receiver
 from django.db.models.signals import post_save, pre_save
+from django.db.models import F
 from django.core.exceptions import ObjectDoesNotExist
 from django.conf import settings
 from opaque_keys import InvalidKeyError
@@ -17,6 +18,7 @@ from edx_notifications.lib.publisher import (
     get_notification_type
 )
 from edx_notifications.data import NotificationMessage
+from openedx.core.djangoapps.content.course_metadata.utils import is_progress_detached_vertical
 
 from progress.models import StudentProgress, StudentProgressHistory, CourseModuleCompletion
 
@@ -26,16 +28,28 @@ from xmodule.modulestore.exceptions import ItemNotFoundError
 log = logging.getLogger(__name__)
 
 
-def _get_parent_content_id(html_content_id):
-    """ Gets parent block content id """
+def is_valid_progress_module(content_id):
+    """
+    Returns boolean indicating if given module is valid for marking progress
+    A valid module should be child of `vertical` and its category should be
+    one of the PROGRESS_DETACHED_CATEGORIES
+    """
     try:
-        html_usage_id = BlockUsageLocator.from_string(html_content_id)
-        html_module = modulestore().get_item(html_usage_id)
-        return unicode(html_module.parent)
+        detached_categories = getattr(settings, 'PROGRESS_DETACHED_CATEGORIES', [])
+        usage_id = BlockUsageLocator.from_string(content_id)
+        module = modulestore().get_item(usage_id)
+        if module and module.parent and module.parent.category == "vertical" and \
+                module.category not in detached_categories and not is_progress_detached_vertical(module.parent):
+            return True
+        else:
+            return False
     except (InvalidKeyError, ItemNotFoundError) as exception:
-        # something has gone wrong - the best we can do is to return original content id
-        log.warn("Error getting parent content_id for html module: %s", exception.message)
-        return html_content_id
+        log.debug("Error getting module for content_id:%s %s", content_id, exception.message)
+        return False
+    except Exception as exception:  # pylint: disable=broad-except
+        # broad except to avoid wrong calculation of progress in case of unknown exception
+        log.exception("Error getting module for content_id:%s %s", content_id, exception.message)
+        return False
 
 
 @receiver(post_save, sender=CourseModuleCompletion, dispatch_uid='edxapp.api_manager.post_save_cms')
@@ -44,15 +58,10 @@ def handle_cmc_post_save_signal(sender, instance, created, **kwargs):
     Broadcast the progress change event
     """
     content_id = unicode(instance.content_id)
-    detached_categories = getattr(settings, 'PROGRESS_DETACHED_CATEGORIES', [])
-    # HTML modules can be children of progress-detached and progress-included modules, so using parent id for
-    # progress-detached check
-    if 'html' in content_id:
-        content_id = _get_parent_content_id(content_id)
-    if created and not any(category in content_id for category in detached_categories):
+    if is_valid_progress_module(content_id):
         try:
             progress = StudentProgress.objects.get(user=instance.user, course_id=instance.course_id)
-            progress.completions += 1
+            progress.completions = F('completions') + 1
             progress.save()
         except ObjectDoesNotExist:
             progress = StudentProgress(user=instance.user, course_id=instance.course_id, completions=1)
@@ -67,10 +76,12 @@ def save_history(sender, instance, **kwargs):  # pylint: disable=no-self-argumen
     """
     Event hook for creating progress entry copies
     """
+    # since instance.completions return F() ExpressionNode we have to pull completions from db
+    progress = StudentProgress.objects.get(pk=instance.id)
     history_entry = StudentProgressHistory(
         user=instance.user,
         course_id=instance.course_id,
-        completions=instance.completions
+        completions=progress.completions
     )
     history_entry.save()
 

lms/djangoapps/progress/tests.py

@@ -52,7 +52,7 @@ class CourseModuleCompletionTests(ModuleStoreTestCase):
             mock_request,
             problem.location,
             field_data_cache,
-        )._xmodule
+        )
 
     def setUp(self):
         super(CourseModuleCompletionTests, self).setUp()
@@ -68,7 +68,7 @@ class CourseModuleCompletionTests(ModuleStoreTestCase):
         )
         self.course.always_recalculate_grades = True
         test_data = '<html>{}</html>'.format(str(uuid.uuid4()))
-        chapter1 = ItemFactory.create(
+        self.chapter1 = ItemFactory.create(
             category="chapter",
             parent_location=self.course.location,
             data=test_data,
@@ -80,20 +80,38 @@ class CourseModuleCompletionTests(ModuleStoreTestCase):
             data=test_data,
             display_name="Chapter 2"
         )
-        ItemFactory.create(
+        sub_section = ItemFactory.create(
             category="sequential",
-            parent_location=chapter1.location,
+            parent_location=self.chapter1.location,
             data=test_data,
             display_name="Sequence 1",
         )
-        ItemFactory.create(
+        sub_section2 = ItemFactory.create(
             category="sequential",
             parent_location=chapter2.location,
             data=test_data,
             display_name="Sequence 2",
         )
+        self.vertical = ItemFactory.create(
+            parent_location=sub_section.location,
+            category="vertical",
+            metadata={'graded': True, 'format': 'Homework'},
+            display_name=u"test vertical",
+        )
+        vertical2 = ItemFactory.create(
+            parent_location=sub_section2.location,
+            category="vertical",
+            metadata={'graded': True, 'format': 'Lab'},
+            display_name=u"test vertical 2",
+        )
+        vertical3 = ItemFactory.create(
+            parent_location=sub_section2.location,
+            category="vertical",
+            metadata={'graded': True, 'format': 'Lab'},
+            display_name=u"Discussion Course",
+        )
         ItemFactory.create(
-            parent_location=chapter2.location,
+            parent_location=vertical2.location,
             category='problem',
             data=StringResponseXMLFactory().build_xml(answer='foo'),
             metadata={'rerandomize': 'always'},
@@ -101,40 +119,51 @@ class CourseModuleCompletionTests(ModuleStoreTestCase):
             max_grade=45
         )
         self.problem = ItemFactory.create(
-            parent_location=chapter1.location,
+            parent_location=self.vertical.location,
             category='problem',
             data=StringResponseXMLFactory().build_xml(answer='bar'),
             display_name="homework problem 1",
             metadata={'rerandomize': 'always', 'graded': True, 'format': "Homework"}
         )
         self.problem2 = ItemFactory.create(
-            parent_location=chapter2.location,
+            parent_location=vertical2.location,
             category='problem',
             data=StringResponseXMLFactory().build_xml(answer='bar'),
             display_name="homework problem 2",
             metadata={'rerandomize': 'always', 'graded': True, 'format': "Homework"}
         )
         self.problem3 = ItemFactory.create(
-            parent_location=chapter2.location,
+            parent_location=vertical2.location,
             category='problem',
             data=StringResponseXMLFactory().build_xml(answer='bar'),
             display_name="lab problem 1",
             metadata={'rerandomize': 'always', 'graded': True, 'format': "Lab"}
         )
         self.problem4 = ItemFactory.create(
-            parent_location=chapter2.location,
+            parent_location=vertical2.location,
             category='problem',
             data=StringResponseXMLFactory().build_xml(answer='bar'),
             display_name="midterm problem 2",
             metadata={'rerandomize': 'always', 'graded': True, 'format': "Midterm Exam"}
         )
         self.problem5 = ItemFactory.create(
-            parent_location=chapter2.location,
+            parent_location=vertical2.location,
             category='problem',
             data=StringResponseXMLFactory().build_xml(answer='bar'),
             display_name="final problem 2",
             metadata={'rerandomize': 'always', 'graded': True, 'format': "Final Exam"}
         )
+        self.problem6 = ItemFactory.create(
+            parent_location=vertical3.location,
+            category='problem',
+            data=StringResponseXMLFactory().build_xml(answer='bar'),
+            display_name="Problem 6",
+        )
+        ItemFactory.create(
+            parent_location=vertical3.location,
+            category='discussion-course',
+            display_name="Course Discussion Item",
+        )
 
     def test_save_completion(self):
         """
@@ -307,6 +336,61 @@ class CourseModuleCompletionTests(ModuleStoreTestCase):
                 content_id=self.problem4.location
             )
 
+    def test_progress_calc_on_invalid_module(self):
+        """
+        Tests progress calculations for invalid modules.
+        We want to calculate progress of those module which are
+        direct children of verticals. Modules at any other level
+        of course tree should not be counted in progress.
+        """
+        self._create_course()
+        # create a module whose parent is not a vertical
+        module = ItemFactory.create(
+            parent_location=self.chapter1.location,
+            category='video',
+            data={'data': '<video display_name="Test Video" />'}
+        )
+        module = self.get_module_for_user(self.user, self.course, module)
+        module.system.publish(module, 'progress', {})
+
+        progress = StudentProgress.objects.all()
+        # assert there is no progress entry for a module whose parent is not a vertical
+        self.assertEqual(len(progress), 0)
+
+    @override_settings(PROGRESS_DETACHED_CATEGORIES=["group-project"])
+    def test_progress_calc_on_detached_module(self):
+        """
+        Tests progress calculations for modules having detached categories
+        """
+        self._create_course()
+        # create a module whose category is one of detached categories
+        module = ItemFactory.create(
+            parent_location=self.vertical.location,
+            category='group-project',
+        )
+        module = self.get_module_for_user(self.user, self.course, module)
+        module.system.publish(module, 'progress', {})
+
+        progress = StudentProgress.objects.all()
+        # assert there is no progress entry for a module whose category is in detached categories
+        self.assertEqual(len(progress), 0)
+
+    @override_settings(
+        PROGRESS_DETACHED_CATEGORIES=["group-project"],
+        PROGRESS_DETACHED_VERTICAL_CATEGORIES=["discussion-course"],
+    )
+    def test_progress_calc_on_vertical_with_detached_module(self):
+        """
+        Tests progress calculations for modules inside a vertical with detached categories
+        """
+        self._create_course()
+        module = self.get_module_for_user(self.user, self.course, self.problem6)
+        module.system.publish(module, 'progress', {})
+
+        progress = StudentProgress.objects.all()
+        # assert there is no progress entry for a module whose category is in detached categories
+        self.assertEqual(len(progress), 0)
+
     def test_receiver_on_course_deleted(self):
         self._create_course(start=datetime(2010, 1, 1, tzinfo=UTC()), end=datetime(2020, 1, 1, tzinfo=UTC()))
         module = self.get_module_for_user(self.user, self.course, self.problem)

lms/envs/aws.py

@@ -489,7 +489,6 @@ BROKER_URL = "{0}://{1}:{2}@{3}/{4}".format(CELERY_BROKER_TRANSPORT,
 STUDENT_FILEUPLOAD_MAX_SIZE = ENV_TOKENS.get("STUDENT_FILEUPLOAD_MAX_SIZE", STUDENT_FILEUPLOAD_MAX_SIZE)
 
 # Modules having these categories would be excluded from progress calculations
-PROGRESS_DETACHED_CATEGORIES = ['discussion-course', 'group-project', 'discussion-forum']
 PROGRESS_DETACHED_APPS = ['group_project_v2']
 for app in PROGRESS_DETACHED_APPS:
     try:

lms/envs/common.py

@@ -642,8 +642,10 @@ USAGE_KEY_PATTERN = r'(?P<usage_key_string>(?:i4x://?[^/]+/[^/]+/[^/]+/[^@]+(?:@
 ASSET_KEY_PATTERN = r'(?P<asset_key_string>(?:/?c4x(:/)?/[^/]+/[^/]+/[^/]+/[^@]+(?:@[^/]+)?)|(?:[^/]+))'
 USAGE_ID_PATTERN = r'(?P<usage_id>(?:i4x://?[^/]+/[^/]+/[^/]+/[^@]+(?:@[^/]+)?)|(?:[^/]+))'
 
+# Verticals having children with any of these categories would be excluded from progress calculations
+PROGRESS_DETACHED_VERTICAL_CATEGORIES = ['discussion-course', 'group-project', 'gp-v2-project']
 # Modules having these categories would be excluded from progress calculations
-PROGRESS_DETACHED_CATEGORIES = ['discussion-course', 'group-project', 'discussion-forum']
+PROGRESS_DETACHED_CATEGORIES = PROGRESS_DETACHED_VERTICAL_CATEGORIES + ['discussion-forum']
 ############################## EVENT TRACKING #################################
 
 # FIXME: Should we be doing this truncation?

openedx/core/djangoapps/content/course_metadata/test_utils.py

@@ -54,6 +54,12 @@ class UtilsTests(ModuleStoreTestCase):
             metadata={'graded': True, 'format': 'FinalExam'},
             display_name=u"test vertical 2",
         )
+        self.vertical3 = ItemFactory.create(
+            parent_location=self.sub_section2.location,
+            category="vertical",
+            metadata={'graded': True, 'format': 'Lab'},
+            display_name=u"Course Discussion",
+        )
         self.content_child1 = ItemFactory.create(
             category="html",
             parent_location=self.vertical.location,
@@ -78,9 +84,24 @@ class UtilsTests(ModuleStoreTestCase):
             data=self.test_data,
             display_name="Html component 2"
         )
+        self.content_child5 = ItemFactory.create(
+            category="discussion-course",
+            parent_location=self.vertical3.location,
+            data=self.test_data,
+            display_name="Course discussion"
+        )
+        self.content_child6 = ItemFactory.create(
+            category="html",
+            parent_location=self.vertical3.location,
+            data=self.test_data,
+            display_name="Html component 3"
+        )
         self.user = UserFactory()
 
-    @override_settings(PROGRESS_DETACHED_CATEGORIES=[])
+    @override_settings(
+        PROGRESS_DETACHED_CATEGORIES=[],
+        PROGRESS_DETACHED_VERTICAL_CATEGORIES=[],
+    )
     @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
     def test_get_course_leaf_nodes(self, module_store_type):
         """
@@ -88,9 +109,12 @@ class UtilsTests(ModuleStoreTestCase):
         """
         with modulestore().default_store(module_store_type):
             nodes = get_course_leaf_nodes(self.course.id)
-            self.assertEqual(len(nodes), 4)
+            self.assertEqual(len(nodes), 6)
 
-    @override_settings(PROGRESS_DETACHED_CATEGORIES=["group-project"])
+    @override_settings(
+        PROGRESS_DETACHED_CATEGORIES=["group-project"],
+        PROGRESS_DETACHED_VERTICAL_CATEGORIES=[],
+    )
     @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
     def test_get_course_leaf_nodes_with_detached_categories(self, module_store_type):
         """
@@ -99,16 +123,33 @@ class UtilsTests(ModuleStoreTestCase):
         with modulestore().default_store(module_store_type):
             nodes = get_course_leaf_nodes(self.course.id)
             # group-project project node should not be counted
-            self.assertEqual(len(nodes), 3)
+            self.assertEqual(len(nodes), 5)
+
+    @override_settings(
+        PROGRESS_DETACHED_CATEGORIES=["group-project"],
+        PROGRESS_DETACHED_VERTICAL_CATEGORIES=["discussion-course", "group-project"],
+    )
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
+    def test_get_course_leaf_nodes_with_detached_vertical_categories(self, module_store_type):
+        """
+        Tests get_course_leaf_nodes with detached component and vertical categories
+        """
+        with modulestore().default_store(module_store_type):
+            nodes = get_course_leaf_nodes(self.course.id)
+            # group-project project node and all children of discussion-course vertical should not be counted
+            self.assertEqual(len(nodes), 1)
 
-    @override_settings(PROGRESS_DETACHED_CATEGORIES=[])
+    @override_settings(
+        PROGRESS_DETACHED_CATEGORIES=[],
+        PROGRESS_DETACHED_VERTICAL_CATEGORIES=[],
+    )
     def test_get_course_leaf_nodes_with_orphan_nodes(self):
         """
         Tests get_course_leaf_nodes if some nodes are orphan
         """
         with modulestore().default_store(ModuleStoreEnum.Type.mongo):
             with modulestore().branch_setting(ModuleStoreEnum.Branch.draft_preferred):
-                # delete sub_section2 to make vertical2 orphan
+                # delete sub_section2 to make vertical2 and vertical3 orphan
                 store = modulestore()
                 store.delete_item(self.sub_section2.location, self.user.id)
                 nodes = get_course_leaf_nodes(self.course.id)

openedx/core/djangoapps/content/course_metadata/utils.py

@@ -16,7 +16,23 @@ def get_course_leaf_nodes(course_key):
     verticals = store.get_items(course_key, qualifiers={'category': 'vertical'})
     orphans = store.get_orphans(course_key)
     for vertical in verticals:
-        if hasattr(vertical, 'children') and vertical.location not in orphans:
+        if hasattr(vertical, 'children') and not is_progress_detached_vertical(vertical) and \
+                vertical.location not in orphans:
             nodes.extend([unit for unit in vertical.children
                           if getattr(unit, 'category') not in detached_categories])
     return nodes
+
+
+def is_progress_detached_vertical(vertical):
+    """
+    Returns boolean indicating if vertical is valid for progress calculations
+    If a vertical has any children belonging to PROGRESS_DETACHED_VERTICAL_CATEGORIES
+    it should be ignored for progress calculation
+    """
+    detached_vertical_categories = getattr(settings, 'PROGRESS_DETACHED_VERTICAL_CATEGORIES', [])
+    if not hasattr(vertical, 'children'):
+        vertical = modulestore().get_item(vertical, 1)
+    for unit in vertical.children:
+        if getattr(unit, 'category') in detached_vertical_categories:
+            return True
+    return False