From f2d4e0ccec379232becb8eb95dd4817ce9681564 Mon Sep 17 00:00:00 2001 From: Michael Terry Date: Mon, 2 Nov 2020 15:31:53 -0500 Subject: [PATCH] AA-408: Treat group_access as inheritable in gating transformer ContentTypeGateTransformer was not considering parent values of 'group_access' when providing its own default value for it. Also fix a few minor bugs around the place. --- common/djangoapps/course_modes/signals.py | 2 +- .../djangoapps/config_model_utils/admin.py | 8 +++--- .../content_type_gating/block_transformers.py | 25 ++++++++++++++++--- .../content_type_gating/field_override.py | 4 +-- 4 files changed, 29 insertions(+), 10 deletions(-) diff --git a/common/djangoapps/course_modes/signals.py b/common/djangoapps/course_modes/signals.py index 02dbe129fb..a9e1e502bf 100644 --- a/common/djangoapps/course_modes/signals.py +++ b/common/djangoapps/course_modes/signals.py @@ -58,7 +58,7 @@ def update_masters_access_course(sender, instance, **kwargs): # pylint: disable masters_id = getattr(settings, 'COURSE_ENROLLMENT_MODES', {}).get('masters', {}).get('id', None) verified_id = getattr(settings, 'COURSE_ENROLLMENT_MODES', {}).get('verified', {}).get('id', None) if not (masters_id and verified_id): - log.error("Missing settings.COURSE_ENROLLMENT_MODES -> verified:%s masters:%s", verified, masters) + log.error("Missing settings.COURSE_ENROLLMENT_MODES -> verified:%s masters:%s", verified_id, masters_id) return course_id = instance.course_id diff --git a/openedx/core/djangoapps/config_model_utils/admin.py b/openedx/core/djangoapps/config_model_utils/admin.py index e175e72947..e6a9891b08 100644 --- a/openedx/core/djangoapps/config_model_utils/admin.py +++ b/openedx/core/djangoapps/config_model_utils/admin.py @@ -31,6 +31,7 @@ class StackedConfigModelAdmin(ConfigurationModelAdmin): form = StackedConfigModelAdminForm raw_id_fields = ('course',) + search_fields = ('site__domain', 'org', 'org_course', 'course__id') def get_fieldsets(self, request, obj=None): return ( @@ -65,13 +66,12 @@ class StackedConfigModelAdmin(ConfigurationModelAdmin): def stackable_fields(self): return list(self.model.STACKABLE_FIELDS) - @property - def config_fields(self): - fields = super(StackedConfigModelAdmin, self).get_fields(request, obj) + def get_config_fields(self, request, obj=None): + fields = super().get_fields(request, obj) return [field for field in fields if field not in self.key_fields] def get_fields(self, request, obj=None): - return self.key_fields + self.config_fields + return self.key_fields + self.get_config_fields(request, obj) def get_displayable_field_names(self): """ diff --git a/openedx/features/content_type_gating/block_transformers.py b/openedx/features/content_type_gating/block_transformers.py index a09f47385b..6c0213b1c9 100644 --- a/openedx/features/content_type_gating/block_transformers.py +++ b/openedx/features/content_type_gating/block_transformers.py @@ -6,6 +6,7 @@ Limits access for certain users to certain types of content. from django.conf import settings +from lms.djangoapps.course_blocks.transformers.user_partitions import UserPartitionTransformer from openedx.core.djangoapps.content.block_structure.transformer import BlockStructureTransformer from openedx.features.content_type_gating.helpers import CONTENT_GATING_PARTITION_ID from openedx.features.content_type_gating.models import ContentTypeGatingConfig @@ -15,6 +16,8 @@ class ContentTypeGateTransformer(BlockStructureTransformer): """ A transformer that adds a partition condition for all graded content so that the content is only visible to verified users. + + This transformer requires that the UserPartitionTransformer also be included in your transformer list. """ WRITE_VERSION = 1 READ_VERSION = 1 @@ -48,6 +51,24 @@ class ContentTypeGateTransformer(BlockStructureTransformer): for parent_block_key in block_structure.get_parents(block_key): self._set_contains_gated_content_on_parents(block_structure, parent_block_key) + @staticmethod + def _get_block_group_access(block_structure, block_key): + """ + Gets the current group_access value for a block, supporting inheritance when possible. + In order to support inheritance, UserPartitionTransformer must also be used. + """ + # See user_partitions.py for the code that sets this field. + merged_access = block_structure.get_transformer_block_field( + block_key, UserPartitionTransformer, 'merged_group_access', None + ) + if merged_access: + current_access = merged_access.get_allowed_groups() + else: + # This fallback code has a bug if UserPartitionTranformer is not being used -- it does not consider + # inheritance from parent blocks. This is why our class docstring recommends UserPartitionTranformer. + current_access = block_structure.get_xblock_field(block_key, 'group_access') + return current_access or {} + def transform(self, usage_info, block_structure): if not ContentTypeGatingConfig.enabled_for_enrollment( user=usage_info.user, @@ -61,9 +82,7 @@ class ContentTypeGateTransformer(BlockStructureTransformer): weight_not_zero = block_structure.get_xblock_field(block_key, 'weight') != 0 problem_eligible_for_content_gating = graded and has_score and weight_not_zero if problem_eligible_for_content_gating: - current_access = block_structure.get_xblock_field(block_key, 'group_access') - if current_access is None: - current_access = {} + current_access = self._get_block_group_access(block_structure, block_key) current_access.setdefault( CONTENT_GATING_PARTITION_ID, [settings.CONTENT_TYPE_GATE_GROUP_IDS['full_access']] diff --git a/openedx/features/content_type_gating/field_override.py b/openedx/features/content_type_gating/field_override.py index eb71a509a0..d27ebd907f 100644 --- a/openedx/features/content_type_gating/field_override.py +++ b/openedx/features/content_type_gating/field_override.py @@ -56,7 +56,7 @@ class ContentTypeGatingFieldOverride(FieldOverrideProvider): if parent is not None: merged_group_access = parent.merged_group_access if merged_group_access and CONTENT_GATING_PARTITION_ID in merged_group_access: - return original_group_access + return default original_group_access.setdefault( CONTENT_GATING_PARTITION_ID, @@ -67,5 +67,5 @@ class ContentTypeGatingFieldOverride(FieldOverrideProvider): @classmethod def enabled_for(cls, course): - """This simple override provider is always enabled""" + """Check our stackable config for this specific course""" return ContentTypeGatingConfig.enabled_for_course(course_key=course.scope_ids.usage_id.course_key)