Merge pull request #683 from edx-solutions/ziafazal/YONK-309-ignore-verticals

ziafazal/YONK-309-ignore-verticals: ignore some verticals from progress calculations

cms/envs/aws.py

@@ -212,7 +212,6 @@ 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:

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/progress/signals.py

@@ -18,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
 
@@ -38,7 +39,7 @@ def is_valid_progress_module(content_id):
         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:
+                module.category not in detached_categories and not is_progress_detached_vertical(module.parent):
             return True
         else:
             return False

lms/djangoapps/progress/tests.py

@@ -104,6 +104,12 @@ class CourseModuleCompletionTests(ModuleStoreTestCase):
             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=vertical2.location,
             category='problem',
@@ -147,6 +153,17 @@ class CourseModuleCompletionTests(ModuleStoreTestCase):
             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):
         """
@@ -358,6 +375,22 @@ class CourseModuleCompletionTests(ModuleStoreTestCase):
         # 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