Merge pull request #1107 from rdmorganiser/progress_conditions_datasets

Fix progress for conditional questions in datasets

rdmo/projects/progress.py

@@ -8,42 +8,54 @@
 from rdmo.questions.models import Page, Question, QuestionSet
 
 
-def resolve_conditions(project, values):
-    # get all conditions for this catalog
-    pages_conditions_subquery = Page.objects.filter_by_catalog(project.catalog) \
+def get_catalog_conditions(catalog):
+    pages_conditions_subquery = Page.objects.filter_by_catalog(catalog) \
                                             .filter(conditions=OuterRef('pk'))
-    questionsets_conditions_subquery = QuestionSet.objects.filter_by_catalog(project.catalog) \
+    questionsets_conditions_subquery = QuestionSet.objects.filter_by_catalog(catalog) \
                                                           .filter(conditions=OuterRef('pk'))
-    questions_conditions_subquery = Question.objects.filter_by_catalog(project.catalog) \
+    questions_conditions_subquery = Question.objects.filter_by_catalog(catalog) \
                                                     .filter(conditions=OuterRef('pk'))
 
-    catalog_conditions = Condition.objects.annotate(has_page=Exists(pages_conditions_subquery)) \
-                                          .annotate(has_questionset=Exists(questionsets_conditions_subquery)) \
-                                          .annotate(has_question=Exists(questions_conditions_subquery)) \
-                                          .filter(Q(has_page=True) | Q(has_questionset=True) | Q(has_question=True)) \
-                                          .distinct().select_related('source', 'target_option')
+    return Condition.objects.annotate(
+        has_page=Exists(pages_conditions_subquery),
+        has_questionset=Exists(questionsets_conditions_subquery),
+        has_question=Exists(questions_conditions_subquery)
+    ).filter(
+        Q(has_page=True) | Q(has_questionset=True) | Q(has_question=True)
+    ).distinct().select_related('source', 'target_option')
+
+
+def resolve_conditions(catalog, values, sets):
+    # resolve all conditions and return for each condition the set_prefix and set_index for which it resolved true
+    conditions = defaultdict(set)
+    if sets:
+        for condition in get_catalog_conditions(catalog):
+            conditions[condition.id] = {
+                (set_prefix, set_index)
+                for set_prefix, set_index in chain.from_iterable(sets.values())
+                if condition.resolve(values, set_prefix=set_prefix, set_index=set_index)
+            }
 
-    # evaluate conditions
-    conditions = set()
-    for condition in catalog_conditions:
-        if condition.resolve(values):
-            conditions.add(condition.id)
-
-    # return all true conditions for this project
     return conditions
 
 
+def compute_sets(values):
+    # compute sets from values (including empty values)
+    sets = defaultdict(set)
+    for attribute, set_prefix, set_index in values.distinct_list():
+        sets[attribute].add((set_prefix, set_index))
+    return sets
+
+
 def compute_navigation(section, project, snapshot=None):
     # get all values for this project and snapshot
     values = project.values.filter(snapshot=snapshot).select_related('attribute', 'option')
 
-    # get true conditions
-    conditions = resolve_conditions(project, values)
-
     # compute sets from values (including empty values)
-    sets = defaultdict(lambda: defaultdict(list))
-    for attribute, set_prefix, set_index in values.distinct_list():
-        sets[attribute][set_prefix].append(set_index)
+    sets = compute_sets(values)
+
+    # resolve all conditions to get a dict mapping conditions to set_indexes
+    conditions = resolve_conditions(project.catalog, values, sets)
 
     # query distinct, non empty set values
     values_list = values.exclude_empty().distinct_list()
@@ -63,13 +75,24 @@ def compute_navigation(section, project, snapshot=None):
 
         for page in catalog_section.elements:
             pages_conditions = {page.id for page in page.conditions.all()}
-            show = bool(not pages_conditions or pages_conditions.intersection(conditions))
+
+            # show only pages with resolved conditions, but show all pages without conditions
+            if pages_conditions:
+                # check if any valuesets for set_prefix = '' resolved
+                # for non collection pages restrict further to set_index = 0
+                show = any(
+                    (set_prefix == '') and (page.is_collection or set_index == 0)
+                    for page_condition in pages_conditions
+                    for set_prefix, set_index in conditions[page_condition]
+                )
+            else:
+                show = True
 
             # count the total number of questions, taking sets and conditions into account
             counts = count_questions(page, sets, conditions)
 
             # filter the values_list for the attributes, and compute the total sum of counts
-            count = len(tuple(filter(lambda value: value[0] in counts.keys(), values_list)))
+            count = sum(1 for value in values_list if value[0] in counts)
             total = sum(counts.values())
 
             navigation_section['count'] += count
@@ -94,23 +117,20 @@ def compute_progress(project, snapshot=None):
     # get all values for this project and snapshot
     values = project.values.filter(snapshot=snapshot).select_related('attribute', 'option')
 
-    # get true conditions
-    conditions = resolve_conditions(project, values)
-
     # compute sets from values (including empty values)
-    sets = defaultdict(lambda: defaultdict(list))
-    for attribute, set_prefix, set_index in values.distinct_list():
-        sets[attribute][set_prefix].append(set_index)
+    sets = compute_sets(values)
+
+    # resolve all conditions to get a dict mapping conditions to set_indexes
+    conditions = resolve_conditions(project.catalog, values, sets)
 
     # query distinct, non empty set values
     values_list = values.exclude_empty().distinct_list()
 
-
     # count the total number of questions, taking sets and conditions into account
     counts = count_questions(project.catalog, sets, conditions)
 
     # filter the values_list for the attributes, and compute the total sum of counts
-    count = len(tuple(filter(lambda value: value[0] in counts.keys(), values_list)))
+    count = sum(1 for value in values_list if value[0] in counts)
     total = sum(counts.values())
 
     return count, total
@@ -119,39 +139,66 @@ def compute_progress(project, snapshot=None):
 def count_questions(element, sets, conditions):
     counts = defaultdict(int)
 
-    # obtain the maximum number of set-distinct values for the questions in this page or
-    # question set this number is how often each question is displayed and we will use
-    # this number to determine how often a question needs to be counted
-    if isinstance(element, (Page, QuestionSet)) and element.is_collection:
-        counted_sets = set()
+    if isinstance(element, (Page, QuestionSet)):
+        # obtain the maximum number of set-distinct values for the questions in this page or
+        # question set. this number is how often each question is displayed and we will use
+        # this number to determine how often a question needs to be counted
+        element_sets = set()
 
         # count the sets for the id attribute of the page or question
         if element.attribute is not None:
-            # nested loop over the separate set_index lists in sets[element.attribute.id]
-            for set_index in chain.from_iterable(sets[element.attribute.id].values()):
-                counted_sets.add(set_index)
+            # nested loop over the separate set_prefix, set_index lists in sets[element.attribute.id]
+            for set_prefix, set_index in sets[element.attribute.id]:
+                element_sets.add((set_prefix, set_index))
 
         # count the sets for the questions in the page or question
         for child in element.elements:
             if isinstance(child, Question):
                 if child.attribute is not None:
-                    # nested loop over the separate set_index lists in sets[element.attribute.id]
-                    for set_index in chain.from_iterable(sets[child.attribute.id].values()):
-                        counted_sets.add(set_index)
+                    # nested loop over the separate set_prefix, set_index lists in sets[element.attribute.id]
+                    for set_prefix, set_index in sets[child.attribute.id]:
+                        element_sets.add((set_prefix, set_index))
 
-        set_count = len(counted_sets)
-    else:
-        set_count = 1
+        # look for the elements conditions
+        element_conditions = {condition.id for condition in element.conditions.all()}
+
+        # if this element has conditions: check if those conditions resolve for the found sets
+        if element_conditions:
+            # compute the intersection of the conditions of this child with the full set of conditions
+            element_condition_intersection = {
+                (set_prefix, set_index)
+                for condition_id, condition in conditions.items()
+                for set_prefix, set_index in conditions[condition_id]
+                if condition_id in element_conditions
+            }
+
+            resolved_sets = element_sets.intersection(element_condition_intersection)
+            if resolved_sets:
+                set_count = len(resolved_sets) if element.is_collection else 1
+            else:
+                # return immediately and do not consider children, this page/questionset is hidden
+                return counts
+        else:
+            set_count = len(element_sets) if element.is_collection else 1
 
     # loop over all children of this element
     for child in element.elements:
-        # look for the elements conditions
+        # look for the child element's conditions
         if isinstance(child, (Page, QuestionSet, Question)):
             child_conditions = {condition.id for condition in child.conditions.all()}
         else:
-            child_conditions = []
+            child_conditions = set()
+
+        # compute the intersection of the conditions of this child with the full set of conditions
+        child_condition_intersection = {
+            (set_prefix, set_index)
+            for condition_id, condition in conditions.items()
+            for set_prefix, set_index in conditions[condition_id]
+            if condition_id in child_conditions
+        }
 
-        if not child_conditions or child_conditions.intersection(conditions):
+        # check if the element either has no condition or its conditions intersect with the full set of conditions
+        if not child_conditions or child_condition_intersection:
             if isinstance(child, Question):
                 # for regular questions add the set_count to the counts dict, since the
                 # question should be answered in every set
@@ -160,10 +207,17 @@ def count_questions(element, sets, conditions):
                 # use the max function, since the same attribute could appear twice in the tree
                 if child.attribute is not None:
                     if child.is_optional:
-                        child_count = sum(len(set_indexes) for set_indexes in sets[child.attribute.id].values())
+                        child_count = len(sets[child.attribute.id])
                         counts[child.attribute.id] = max(counts[child.attribute.id], child_count)
                     else:
-                        counts[child.attribute.id] = max(counts[child.attribute.id], set_count)
+                        if child_condition_intersection:
+                            # update the set_count for the current question (child element)
+                            # count only the sets that have conditions resolved to true
+                            current_count = len(element_sets.intersection(child_condition_intersection))
+                        else:
+                            current_count = set_count
+
+                        counts[child.attribute.id] = max(counts[child.attribute.id], current_count)
             else:
                 # for everything else, call this function recursively
                 counts.update(count_questions(child, sets, conditions))

rdmo/projects/tests/test_navigation.py

@@ -24,27 +24,27 @@
     'http://example.com/terms/questions/catalog/set/individual-collection': (9, 10, True),
     'http://example.com/terms/questions/catalog/set/collection-single': (14, 18, True),
     'http://example.com/terms/questions/catalog/set/collection-collection': (16, 20, True),
-    'http://example.com/terms/questions/catalog/conditions': (15, 23),
+    'http://example.com/terms/questions/catalog/conditions': (7, 15),
     'http://example.com/terms/questions/catalog/conditions/input': (2, 2, True),
     'http://example.com/terms/questions/catalog/conditions/text_contains': (1, 1, True),
-    'http://example.com/terms/questions/catalog/conditions/text_empty': (1, 1, False),
+    'http://example.com/terms/questions/catalog/conditions/text_empty': (0, 0, False),
     'http://example.com/terms/questions/catalog/conditions/text_equal': (1, 1, True),
-    'http://example.com/terms/questions/catalog/conditions/text_greater_than': (1, 1, False),
-    'http://example.com/terms/questions/catalog/conditions/text_greater_than_equal': (1, 1, False),
-    'http://example.com/terms/questions/catalog/conditions/text_lesser_than': (1, 1, False),
-    'http://example.com/terms/questions/catalog/conditions/text_lesser_than_equal': (1, 1, False),
+    'http://example.com/terms/questions/catalog/conditions/text_greater_than': (0, 0, False),
+    'http://example.com/terms/questions/catalog/conditions/text_greater_than_equal': (0, 0, False),
+    'http://example.com/terms/questions/catalog/conditions/text_lesser_than': (0, 0, False),
+    'http://example.com/terms/questions/catalog/conditions/text_lesser_than_equal': (0, 0, False),
     'http://example.com/terms/questions/catalog/conditions/text_not_empty': (1, 1, True),
-    'http://example.com/terms/questions/catalog/conditions/text_not_equal': (1, 1, False),
-    'http://example.com/terms/questions/catalog/conditions/option_empty': (1, 1, False),
+    'http://example.com/terms/questions/catalog/conditions/text_not_equal': (0, 0, False),
+    'http://example.com/terms/questions/catalog/conditions/option_empty': (0, 0, False),
     'http://example.com/terms/questions/catalog/conditions/option_equal': (1, 1, True),
     'http://example.com/terms/questions/catalog/conditions/option_not_empty': (1, 1, True),
-    'http://example.com/terms/questions/catalog/conditions/option_not_equal': (1, 1, False),
+    'http://example.com/terms/questions/catalog/conditions/option_not_equal': (0, 0, False),
     'http://example.com/terms/questions/catalog/conditions/set': (0, 2, True),
     'http://example.com/terms/questions/catalog/conditions/set_set': (0, 2, True),
     'http://example.com/terms/questions/catalog/conditions/optionset': (0, 2, True),
     'http://example.com/terms/questions/catalog/conditions/text_set': (0, 2, True),
-    'http://example.com/terms/questions/catalog/blocks': (9, 12),
-    'http://example.com/terms/questions/catalog/blocks/set': (9, 12, True),
+    'http://example.com/terms/questions/catalog/blocks': (9, 18),
+    'http://example.com/terms/questions/catalog/blocks/set': (9, 18, True),
 }
 
 

rdmo/projects/tests/test_progress.py

@@ -6,7 +6,7 @@
 projects = [1, 11]
 
 results_map = {
-    1: (58, 88),
+    1: (58, 94),
     11: (0, 36)
 }
 

rdmo/projects/tests/test_viewset_project_progress.py

@@ -119,7 +119,7 @@ def test_progress_post_unchanged(db, client):
 
     project = Project.objects.get(id=1)
     project.progress_count = progress_count = 58  # the progress in the fixture is not up-to-date
-    project.progress_total = progress_total = 88
+    project.progress_total = progress_total = 94
     project.save()
     project.refresh_from_db()
     project_updated = project.updated