feat: skill validation configuration to disable validation for course or organization #4295
Conversation
muhammad-ammar
force-pushed
the
ammar/skill-validation-configuration
branch
from
9fbb393
to
ae26e4d
Compare
There was a problem hiding this comment.
@muhammad-ammar Please add test for the new provider method here https://github.com/openedx/course-discovery/blob/master/course_discovery/apps/taxonomy_support/tests/test_providers.py
Once that is done 👍🏼
muhammad-ammar
force-pushed
the
ammar/skill-validation-configuration
branch
2 times, most recently
from
0d44ae0
to
2ae1212
Compare
| class SkillValidationConfiguration(TimeStampedModel): | ||
| """ | ||
| Model to store the configuration for disabling skill validation for a course or organization. | ||
| """ |
There was a problem hiding this comment.
why do we need this model in here as opposed to taxonomy-connector?
There was a problem hiding this comment.
Adding the new model in taxonomy_support app in course discovery will give us below benefits
- Make taxonomy-connector independent of skill validation disable logic
- This will make integration smoother in case we want to install taxonomy-connector in any other service
- Better user experience and constraints validation in django admin due to foreign key relationships with
CourseandOrganizationmodels- This eliminates the need for users to manually copy and paste the course key and organization key, reducing the likelihood of errors
There was a problem hiding this comment.
Make taxonomy-connector independent of skill validation disable logic
How so? Isn't this linked to taxonomy-connector?
Better user experience and constraints validation in django admin due to foreign key relationships with Course and Organization models
While I get this point, it does not alone warrant adding this model to discovery. At this point, we are trying to avoid adding many unrelated items in Discovery. Let's see what any other team members have to say.
There was a problem hiding this comment.
How so? Isn't this linked to taxonomy-connector?
This is not directly related to the taxonomy-connector. The logic to disable skill validation lies with the service that intends to utilize the taxonomy. Specifically, in this case, the data (Course and Organization models) required by the taxonomy-connector are part of course-discovery. This is where the providers come into play. Providers act as intermediaries between the taxonomy-connector and consumers, providing the required data. Moving this model into the taxonomy would force consumers of the taxonomy-connector to disable skill validation using course or organization, which would be undesirable for them.
There was a problem hiding this comment.
Rather than keeping a tightly coupled model with Course and Org, can't we just keep something like a uuid in the model and keep it in Taxonomy instead? Making a specific model in the consuming app kinda kills the purpose of an add-on service like Taxonomy i.e. we keep a lot of outside info in Discovery. Having this model in Taxonomy should make more sense if we can make it more generic by using an identifier like uuid.
There was a problem hiding this comment.
@Ali-D-Akbar Can you add more details regarding the uuid approach? Any prior art?
There was a problem hiding this comment.
A model that keeps track of uuids of different products (Course, Programs etc) for which we want to disable validation. Something like this:
class DisableSkillValidationConfiguration(TimeStampedModel):
product_uuid = models.UUIDField(blank=False, null=False, default=uuid4, editable=True, verbose_name=_('Product UUID'))
I renamed it DisableSkillValidationConfiguration so the name shows the model's exact purpose.
Since you're not using the organization as shown in the PR, I haven't added any config in the model but feel free to add some identifier for that and make a logic around this accordingly.
There was a problem hiding this comment.
In our case the product_uuid will be course uuid, right? So the admins have to copy and paste a uuid in django admin, correct? And to find if the skill validation is disable for a course/courseun/org, we will find, lets say courserun against a courserun key and then see if uuid of the courserun exists in DisableSkillValidationConfiguration model, correct?
muhammad-ammar
force-pushed
the
ammar/skill-validation-configuration
branch
2 times, most recently
from
9309d68
to
9e0f2b4
Compare
muhammad-ammar
force-pushed
the
ammar/skill-validation-configuration
branch
from
9e0f2b4
to
d0b6283
Compare
muhammad-ammar
force-pushed
the
ammar/skill-validation-configuration
branch
from
d0b6283
to
bce3283
Compare
| """ | ||
|
|
||
| course = models.ForeignKey( | ||
| Course, |
There was a problem hiding this comment.
How does it differ across draft and non-draft? I see the filtering is applied on admin but should it not be on field level as well?
| return False, None | ||
|
|
||
| @classmethod | ||
| def is_disabled(cls, course_run_key) -> bool: |
There was a problem hiding this comment.
Why is course_run_key coming in here? It should be Course key as Taxonomy skills are calculated on Course level. The validation model is linked with Course, so it makes sense to provide course key here.
There was a problem hiding this comment.
This will be a courserun key. This key will be passed via an api from a lms xblock. We find course key of a courserun key and then see if the skill validation is disabled for that course?
There was a problem hiding this comment.
Yes, I would advocate getting course key in Taxonomy connector or whatever the service using this. This method should be taking course key in here -- for sake of consistency.
| @@ -138,3 +144,61 @@ def test_get_video_xblocks_content(self): | |||
| provider = get_xblock_metadata_provider() | |||
| xblocks = provider.get_xblocks(block_ids) | |||
| assert 'Should not be included in tagging content.' not in xblocks[0].content | |||
|
|
|||
|
|
|||
| @pytest.mark.django_db | |||
There was a problem hiding this comment.
p.s. is this really needed -- considering we are using Django testcase already?
| assert get_course_metadata_provider().skill_validation_disabled( | ||
| self.courserun_with_skill_validation_disabled.key | ||
| ) is True | ||
| assert get_course_metadata_provider().skill_validation_disabled( | ||
| self.course_with_skill_validation_enabled.key | ||
| ) is False | ||
|
|
||
| # skill validation should be disabled for all courses runs of an orgranization | ||
| assert get_course_metadata_provider().skill_validation_disabled( | ||
| self.courserun_with_org_skill_validation_disabled_1.key | ||
| ) is True | ||
| assert get_course_metadata_provider().skill_validation_disabled( | ||
| self.courserun_with_org_skill_validation_disabled_2.key | ||
| ) is True |
There was a problem hiding this comment.
nit: it would be better to have these in separate unit tests, even though they will be one-liner.
|
@DawoudSheraz @Ali-D-Akbar Closing this in favor of #4300 |
Description: Add a new model and related taxonomy provider method to disable skill validation for a course or an organization.
JIRA: https://2u-internal.atlassian.net/browse/ENT-8457
Dependency: openedx/taxonomy-connector#193