From 9560e0826e7bcdf0504d963aea05c9df911913f8 Mon Sep 17 00:00:00 2001 From: muhammad-ammar Date: Tue, 26 Mar 2024 12:18:12 +0500 Subject: [PATCH] feat: skill validation exemption --- CHANGELOG.rst | 4 + taxonomy/__init__.py | 2 +- taxonomy/admin.py | 8 ++ taxonomy/api/v1/views.py | 11 +- .../migrations/0036_auto_20240325_1631.py | 33 +++++ taxonomy/models.py | 136 ++++++++++++++++++ taxonomy/providers/course_metadata.py | 36 +++++ taxonomy/validators/course_metadata.py | 37 +++++ test_utils/factories.py | 9 +- test_utils/providers.py | 53 ++++++- tests/test_models.py | 80 ++++++++++- tests/test_views.py | 11 +- 12 files changed, 414 insertions(+), 6 deletions(-) create mode 100644 taxonomy/migrations/0036_auto_20240325_1631.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 17c8495a..0eb6b807 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -13,6 +13,10 @@ Change Log Unreleased +[1.50.0] - 2024-03-27 +--------------------- +* feat: Skill validation can be disbaled for a course or an organization + [1.46.2] - 2024-02-14 --------------------- * feat: Optimized finalize_xblockskill_tags command for memory via chunking diff --git a/taxonomy/__init__.py b/taxonomy/__init__.py index f7a49b10..4d14b545 100644 --- a/taxonomy/__init__.py +++ b/taxonomy/__init__.py @@ -15,6 +15,6 @@ # 2. MINOR version when you add functionality in a backwards compatible manner, and # 3. PATCH version when you make backwards compatible bug fixes. # More details can be found at https://semver.org/ -__version__ = '1.46.2' +__version__ = '1.50.0' default_app_config = 'taxonomy.apps.TaxonomyConfig' # pylint: disable=invalid-name diff --git a/taxonomy/admin.py b/taxonomy/admin.py index 3135bad4..140fe3b9 100644 --- a/taxonomy/admin.py +++ b/taxonomy/admin.py @@ -32,6 +32,7 @@ Translation, XBlockSkillData, XBlockSkills, + SkillValidationConfiguration ) from taxonomy.views import JobSkillsView @@ -289,3 +290,10 @@ def job_name(self, obj): Name of the related job. """ return obj.job.name + + +@admin.register(SkillValidationConfiguration) +class SkillValidationConfiguratonAdmin(admin.ModelAdmin): + """ + Admin view for SkillValidationConfiguration model. + """ diff --git a/taxonomy/api/v1/views.py b/taxonomy/api/v1/views.py index ec2dec82..776330f3 100644 --- a/taxonomy/api/v1/views.py +++ b/taxonomy/api/v1/views.py @@ -6,7 +6,7 @@ from django_filters.rest_framework import DjangoFilterBackend from rest_framework import permissions from rest_framework.filters import OrderingFilter -from rest_framework.generics import RetrieveAPIView, ListAPIView +from rest_framework.generics import ListAPIView, RetrieveAPIView from rest_framework.mixins import ListModelMixin, RetrieveModelMixin from rest_framework.response import Response from rest_framework.views import APIView @@ -35,6 +35,7 @@ SkillCategory, SkillsQuiz, SkillSubCategory, + SkillValidationConfiguration, XBlockSkillData, XBlockSkills, ) @@ -292,6 +293,8 @@ def get(self, request, job_id): class XBlockSkillsViewSet(TaxonomyAPIViewSetMixin, RetrieveModelMixin, ListModelMixin, GenericViewSet): """ ViewSet to list and retrieve all XBlockSkills in the system. + + If skill validation is disabled for a course, then return an empty queryset. """ serializer_class = XBlocksSkillsSerializer permission_classes = (permissions.IsAuthenticated, ) @@ -302,6 +305,12 @@ def get_queryset(self): """ Get all the xblocks skills with prefetch_related objects. """ + skill_validation_disabled = SkillValidationConfiguration.is_disabled( + self.request.query_params.get('course_key') + ) + if skill_validation_disabled: + return XBlockSkills.objects.none() + return XBlockSkills.objects.prefetch_related( Prefetch( 'skills', diff --git a/taxonomy/migrations/0036_auto_20240325_1631.py b/taxonomy/migrations/0036_auto_20240325_1631.py new file mode 100644 index 00000000..1173a6ba --- /dev/null +++ b/taxonomy/migrations/0036_auto_20240325_1631.py @@ -0,0 +1,33 @@ +# Generated by Django 3.2.22 on 2024-03-25 16:31 + +from django.db import migrations, models +import django.utils.timezone +import model_utils.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('taxonomy', '0035_auto_20231013_0324'), + ] + + operations = [ + migrations.CreateModel( + name='SkillValidationConfiguration', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('created', model_utils.fields.AutoCreatedField(default=django.utils.timezone.now, editable=False, verbose_name='created')), + ('modified', model_utils.fields.AutoLastModifiedField(default=django.utils.timezone.now, editable=False, verbose_name='modified')), + ('course_key', models.CharField(blank=True, help_text='The course, for which skill validation is disabled.', max_length=255, null=True, unique=True)), + ('organization', models.CharField(blank=True, help_text='The organization, for which skill validation is disabled.', max_length=255, null=True, unique=True)), + ], + options={ + 'verbose_name': 'Skill Validation Configuration', + 'verbose_name_plural': 'Skill Validation Configurations', + }, + ), + migrations.AddConstraint( + model_name='skillvalidationconfiguration', + constraint=models.CheckConstraint(check=models.Q(models.Q(('course_key__isnull', False), ('organization__isnull', True)), models.Q(('course_key__isnull', True), ('organization__isnull', False)), _connector='OR'), name='either_course_or_org'), + ), + ] diff --git a/taxonomy/models.py b/taxonomy/models.py index 03e01482..defd2418 100644 --- a/taxonomy/models.py +++ b/taxonomy/models.py @@ -4,17 +4,24 @@ """ from __future__ import unicode_literals +import logging import uuid +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey from solo.models import SingletonModel from django.core.exceptions import ValidationError from django.db import models +from django.db.models import Q from django.utils.translation import gettext_lazy as _ from model_utils.models import TimeStampedModel from taxonomy.choices import UserGoal +from taxonomy.providers.utils import get_course_metadata_provider + +LOGGER = logging.getLogger(__name__) class Skill(TimeStampedModel): @@ -1144,3 +1151,132 @@ class Meta: app_label = 'taxonomy' verbose_name = 'B2C Job Allow List entry' verbose_name_plural = 'B2C Job Allow List entries' + + +class SkillValidationConfiguration(TimeStampedModel): + """ + Model to store the configuration for disabling skill validation for a course or organization. + """ + + course_key = models.CharField( + max_length=255, + null=True, + blank=True, + unique=True, + help_text=_('The course, for which skill validation is disabled.'), + ) + organization = models.CharField( + max_length=255, + null=True, + blank=True, + unique=True, + help_text=_('The organization, for which skill validation is disabled.'), + ) + + def __str__(self): + """ + Create a human-readable string representation of the object. + """ + message = '' + + if self.course_key: + message = f'Skill validation disabled for course: {self.course_key}' + elif self.organization: + message = f'Skill validation disabled for organization: {self.organization}' + + return message + + class Meta: + """ + Meta configuration for SkillValidationConfiguration model. + """ + + constraints = [ + models.CheckConstraint( + check=( + Q(course_key__isnull=False) & + Q(organization__isnull=True) + ) | ( + Q(course_key__isnull=True) & + Q(organization__isnull=False) + ), + name='either_course_or_org', + # This only work on django >= 4.1 + # violation_error_message='Select either course or organization.' + ), + ] + + verbose_name = 'Skill Validation Configuration' + verbose_name_plural = 'Skill Validation Configurations' + + def clean(self): + """Override to add custom validation for course and organization fields.""" + if self.course_key: + if not get_course_metadata_provider().is_valid_course(self.course_key): + raise ValidationError({ + 'course_key': f'Course with key {self.course_key} does not exist.' + }) + + if self.organization: + if not get_course_metadata_provider().is_valid_organization(self.organization): + raise ValidationError({ + 'organization': f'Organization with key {self.organization} does not exist.' + }) + + # pylint: disable=no-member + def validate_constraints(self, exclude=None): + """ + Validate all constraints defined in Meta.constraints. + + NOTE: We override this method only to return a human readable message. + We should remove this override once taxonomy-connector is updated to django 4.1 + On django >= 4.1, add violation_error_message in models.CheckConstraint with an appropriate message. + """ + try: + super().validate_constraints(exclude=exclude) + except ValidationError as ex: + raise ValidationError({'__all__': 'Add either course key or organization.'}) from ex + + def save(self, *args, **kwargs): + """Override to ensure that custom validation is always called.""" + self.full_clean() + return super().save(*args, **kwargs) + + @staticmethod + def is_valid_course_run_key(course_run_key): + """ + Check if the given course run key is in valid format. + + Arguments: + course_run_key (str): Course run key + """ + try: + return True, CourseKey.from_string(course_run_key) + except InvalidKeyError: + LOGGER.error('[TAXONOMY_SKILL_VALIDATION_CONFIGURATION] Invalid course_run key: [%s]', course_run_key) + + return False, None + + @classmethod + def is_disabled(cls, course_run_key) -> bool: + """ + Check if skill validation is disabled for the given course run key. + + Arguments: + course_run_key (str): Course run key + + Returns: + bool: True if skill validation is disabled for the given course run key. + """ + is_valid_course_run_key, course_run_locator = cls.is_valid_course_run_key(course_run_key) + if not is_valid_course_run_key: + return False + + if cls.objects.filter(organization=course_run_locator.org).exists(): + return True + + course_key = get_course_metadata_provider().get_course_key(course_run_key) + if course_key and cls.objects.filter(course_key=course_key).exists(): + return True + + return False diff --git a/taxonomy/providers/course_metadata.py b/taxonomy/providers/course_metadata.py index 7cbe2bd3..16c83bfb 100644 --- a/taxonomy/providers/course_metadata.py +++ b/taxonomy/providers/course_metadata.py @@ -46,3 +46,39 @@ def get_all_courses(self): 4. short_description: Course's short description 5. full_description: Course's full description """ + + @abstractmethod + def get_course_key(self, course_run_key): + """ + Get the course key for the given course run key. + + Arguments: + course_run_key(str): Course run key + + Returns: + str: course key + """ + + @abstractmethod + def is_valid_course(self, course_key): + """ + Validate the course key. + + Arguments: + course_key(str): course key + + Returns: + bool: True if course is valid, False otherwise + """ + + @abstractmethod + def is_valid_organization(self, organization_key): + """ + Validate the organization. + + Arguments: + organization(str): organization key + + Returns: + bool: True if organization is valid, False otherwise + """ diff --git a/taxonomy/validators/course_metadata.py b/taxonomy/validators/course_metadata.py index 70b38c95..8b458207 100644 --- a/taxonomy/validators/course_metadata.py +++ b/taxonomy/validators/course_metadata.py @@ -4,6 +4,8 @@ All host platform must run this validator to make sure providers are working as expected. """ +import inspect + from taxonomy.providers.utils import get_course_metadata_provider @@ -23,6 +25,11 @@ def __init__(self, test_courses): self.test_courses = test_courses self.course_metadata_provider = get_course_metadata_provider() + @property + def provider_class_name(self): + """Return the name of the provider class.""" + return self.course_metadata_provider.__class__.__name__ + def validate(self): """ Validate CourseMetadataProvider implements the interface as expected. @@ -32,6 +39,9 @@ def validate(self): """ self.validate_get_courses() self.validate_get_all_courses() + self.validate_get_course_key() + self.validate_is_valid_course() + self.validate_is_valid_organization() def validate_get_courses(self): """ @@ -60,3 +70,30 @@ def validate_get_all_courses(self): assert 'title' in course assert 'short_description' in course assert 'full_description' in course + + def validate_get_course_key(self): + """ + Validate `get_course_key` attribute is a callable and has the correct signature. + """ + get_course_key = getattr(self.course_metadata_provider, 'get_course_key') # pylint: disable=literal-used-as-attribute + assert callable(get_course_key) + assert_msg = f'Invalid method signature for {self.provider_class_name}.get_course_key' + assert str(inspect.signature(get_course_key)) == '(course_run_key)', assert_msg + + def validate_is_valid_course(self): + """ + Validate `is_valid_course` attribute is a callable and has the correct signature. + """ + is_valid_course = getattr(self.course_metadata_provider, 'is_valid_course') # pylint: disable=literal-used-as-attribute + assert callable(is_valid_course) + assert_msg = f'Invalid method signature for {self.provider_class_name}.is_valid_course' + assert str(inspect.signature(is_valid_course)) == '(course_key)', assert_msg + + def validate_is_valid_organization(self): + """ + Validate `is_valid_organization` attribute is a callable and has the correct signature. + """ + is_valid_organization = getattr(self.course_metadata_provider, 'is_valid_organization') # pylint: disable=literal-used-as-attribute + assert callable(is_valid_organization) + assert_msg = f'Invalid method signature for {self.provider_class_name}.is_valid_organization' + assert str(inspect.signature(is_valid_organization)) == '(organization_key)', assert_msg diff --git a/test_utils/factories.py b/test_utils/factories.py index c2e3a711..5a7a5816 100644 --- a/test_utils/factories.py +++ b/test_utils/factories.py @@ -11,6 +11,7 @@ from taxonomy.choices import UserGoal from taxonomy.models import ( + B2CJobAllowList, CourseRunXBlockSkillsTracker, CourseSkills, Industry, @@ -27,10 +28,10 @@ SkillCategory, SkillsQuiz, SkillSubCategory, + SkillValidationConfiguration, Translation, XBlockSkillData, XBlockSkills, - B2CJobAllowList, ) FAKER = FakerFactory.create() @@ -411,3 +412,9 @@ class Meta: model = B2CJobAllowList job = factory.SubFactory(JobFactory) + + +class SkillValidationConfigurationFactory(factory.django.DjangoModelFactory): + + class Meta: + model = SkillValidationConfiguration diff --git a/test_utils/providers.py b/test_utils/providers.py index e61cf511..31aedf03 100644 --- a/test_utils/providers.py +++ b/test_utils/providers.py @@ -3,12 +3,15 @@ An implementation of providers to be used in tests. """ +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey + from taxonomy.providers import ( CourseMetadataProvider, CourseRunMetadataProvider, ProgramMetadataProvider, XBlockContent, - XBlockMetadataProvider + XBlockMetadataProvider, ) from taxonomy.providers.course_run_metadata import CourseRunContent from test_utils.mocks import MockCourse, MockCourseRun, MockProgram, MockXBlock @@ -95,6 +98,54 @@ def get_all_courses(self): 'full_description': course.full_description, } + def get_course_key(self, course_run_key): + """ + Get the course key for the given course run key. + + Arguments: + course_run_key(str): Course run key + + Returns: + str: course key + """ + try: + locator = CourseKey.from_string(course_run_key) + return '{org}+{course}'.format(org=locator.org, course=locator.course) + except InvalidKeyError: + return None + + def is_valid_course(self, course_key): + """ + Validate the course key. + + Arguments: + course_key(str): course key + + Returns: + bool: True if course is valid, False otherwise + """ + courses = self.mock_courses + for course in courses: + if course.key == course_key: + return True + return False + + def is_valid_organization(self, organization_key): + """ + Validate the organization. + + Arguments: + organization_key(str): organization key + + Returns: + bool: True if organization is valid, False otherwise + """ + courses = self.mock_courses + for course in courses: + if course.key.startswith(f'{organization_key}+'): + return True + return False + class DiscoveryProgramMetadataProvider(ProgramMetadataProvider): """ diff --git a/tests/test_models.py b/tests/test_models.py index fb7af407..ca84d603 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -9,12 +9,15 @@ from django.conf import settings from django.core.exceptions import ValidationError +from django.db.utils import IntegrityError from django.test import TestCase -from taxonomy.models import Industry, Job, JobPostings, B2CJobAllowList +from taxonomy.models import B2CJobAllowList, Industry, Job, JobPostings, SkillValidationConfiguration from taxonomy.signals.handlers import generate_job_description from taxonomy.utils import generate_and_store_job_description from test_utils import factories +from test_utils.mocks import MockCourse, MockCourseRun +from test_utils.providers import DiscoveryCourseMetadataProvider @mark.django_db @@ -731,3 +734,78 @@ def test_string_representation(self): assert expected_str == allowList_entry.__str__() assert expected_repr == allowList_entry.__repr__() + + +@mark.django_db +class SkillValidationConfigurationTests(TestCase): + """ + Tests for the ``SkillValidationConfiguration`` model. + """ + def setUp(self): + super().setUp() + + self.mock_course_run = MockCourseRun(course_key='RichX+GoldX', course_run_key='course-v1:RichX+GoldX+1T2024') + self.courses = [MockCourse() for _ in range(3)] + self.courses[0].key = self.mock_course_run.course_key + self.provider_patcher = patch('taxonomy.models.get_course_metadata_provider') + self.mock_provider = self.provider_patcher.start() + self.mock_provider.return_value = DiscoveryCourseMetadataProvider(self.courses) + + def tearDown(self): + super().tearDown() + + self.provider_patcher.stop() + + def test_model_obj_creation_with_course_and_org(self): + """ + Verify that an `IntegrityError` is raised if user try to create a + SkillValidationConfiguration with both course and organization. + """ + with pytest.raises(IntegrityError) as raised_exception: + factories.SkillValidationConfigurationFactory( + course_key=self.courses[0].key, + organization=self.courses[0].key.split('+')[0] + ) + + assert raised_exception.value.args[0] == 'CHECK constraint failed: either_course_or_org' + + def test_model_obj_creation_with_wrong_course_key(self): + """ + Verify that a `ValidationError` is raised if user try to create a + SkillValidationConfiguration with incorrect course key. + """ + with pytest.raises(ValidationError) as raised_exception: + factories.SkillValidationConfigurationFactory(course_key='MAx+WoWx') + + assert raised_exception.value.message_dict == {'course_key': ['Course with key MAx+WoWx does not exist.']} + + def test_model_obj_creation_with_wrong_org_key(self): + """ + Verify that a `ValidationError` is raised if user try to create a + SkillValidationConfiguration with incorrect organization key. + """ + with pytest.raises(ValidationError) as raised_exception: + factories.SkillValidationConfigurationFactory(organization='MAx') + + assert raised_exception.value.message_dict == {'organization': ['Organization with key MAx does not exist.']} + + def test_model_is_disabled_with_course_key(self): + """ + Verify that a `is_disabled` work as expected if a skill validation is disabled for a course. + """ + factories.SkillValidationConfigurationFactory(course_key=self.mock_course_run.course_key) + assert SkillValidationConfiguration.is_disabled(course_run_key=self.mock_course_run.course_run_key) + + def test_model_is_disabled_with_org_key(self): + """ + Verify that a `is_disabled` work as expected if a skill validation is disabled for an organization. + """ + organization = self.courses[0].key.split('+')[0] + factories.SkillValidationConfigurationFactory(organization=organization) + assert SkillValidationConfiguration.is_disabled(course_run_key=self.mock_course_run.course_run_key) + + def test_model_is_disabled_with_wrong_course_run_key_format(self): + """ + Verify that a `is_disabled` work as expected if a course_run_key is in wrong format. + """ + assert SkillValidationConfiguration.is_disabled(course_run_key='blah blah blah') is False diff --git a/tests/test_views.py b/tests/test_views.py index b96c8de9..62d28ece 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -15,7 +15,7 @@ from django.test import Client, RequestFactory, TestCase from django.urls import reverse -from taxonomy.models import JobPath, JobSkills, Skill, SkillCategory +from taxonomy.models import JobPath, JobSkills, Skill, SkillCategory, SkillValidationConfiguration from taxonomy.utils import generate_and_store_job_to_job_description from taxonomy.views import JobSkillsView, admin from test_utils.factories import ( @@ -617,6 +617,15 @@ def test_xblocks_api_filtering(self): }) self._verify_xblocks_data(api_response, self.xblock_skills[:1], verified=False) + def test_xblocks_api_with_skill_validation_disabled(self): + """ + Verify that xblocks API return no result if skill validation is disabled for a course. + """ + with mock.patch.object(SkillValidationConfiguration, 'is_disabled', return_value=True): + api_response = self.client.get(self.view_url, {"course_key": 'course-v1:edX+M12+1T2024'}) + assert api_response.status_code == 200 + assert api_response.json() == [] + @mark.django_db class TestJobPathAPIView(TestCase):