Merge pull request #677 from edx-solutions/ziafazal/YONK-309-vertical-progress

Ziafazal/yonk 309 progress calculation. @bradenmacdonald Thanks for reviewing merging it after fixing `options_list` and problematic `if` statement.

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,18 @@ 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_CATEGORIES = ['discussion-course', 'group-project', 'discussion-forum']
+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 +239,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']

lms/djangoapps/api_manager/courses/tests.py

@@ -144,13 +144,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,
@@ -158,13 +151,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,
@@ -172,6 +158,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,
@@ -418,12 +418,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
@@ -2115,7 +2116,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
@@ -26,16 +27,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:
+            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 +57,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 +75,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,32 @@ 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",
+        )
         ItemFactory.create(
-            parent_location=chapter2.location,
+            parent_location=vertical2.location,
             category='problem',
             data=StringResponseXMLFactory().build_xml(answer='foo'),
             metadata={'rerandomize': 'always'},
@@ -101,35 +113,35 @@ 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",
@@ -307,6 +319,45 @@ 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)
+
     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)