Skip to content

fix: replace djchoices with Django's built-in TextChoices to fix Pyth… - #288

Open
ktyagiapphelix2u wants to merge 3 commits into
openedx:masterfrom
ktyagiapphelix2u:ktyagi/djchoice
Open

fix: replace djchoices with Django's built-in TextChoices to fix Pyth…#288
ktyagiapphelix2u wants to merge 3 commits into
openedx:masterfrom
ktyagiapphelix2u:ktyagi/djchoice

Conversation

@ktyagiapphelix2u

Copy link
Copy Markdown

Description

Replaces the deprecated djchoices library with Django's built-in models.TextChoices to resolve Python 3.11 compatibility issues.

Why

The djchoices library depends on pkg_resources (from setuptools), which is not available in Python 3.11+ by default. This was causing ModuleNotFoundError: No module named 'pkg_resources' during course-discovery migrations.

Comment thread taxonomy/choices.py


class UserGoal(DjangoChoices):
class UserGoal(models.TextChoices):

@rgopalrao-sonata-png rgopalrao-sonata-png Mar 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As explained,maintain backward compatibility with existing constant names
Issue
The migration from djchoices to Django's TextChoices changes all constant names from TitleCase to UPPER_CASE:

ProductTypes.Course → ProductTypes.COURSE
ProductTypes.Program → ProductTypes.PROGRAM
UserGoal.ChangeCareers → UserGoal.CHANGE_CAREERS
This will break the references in edx-course-discovery, since those serializers directly rely on the existing choice names.
eg:course_discovery/apps/course_metadata/search_indexes/serializers/program.py
course_discovery/apps/course_metadata/search_indexes/serializers/course_run.py

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rgopalrao-sonata-png capitalizing the names (using SCREAMING_SNAKE_CASE) is necessary when migrating from djchoices to Django's built-in TextChoices. Here's why:

Django's TextChoices follows Python's PEP 8 style guide where class constants should be in UPPERCASE with underscores
The official Django documentation shows all TextChoices members in uppercase (e.g., DRAFT, PUBLISHED)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing these values will impact Course Discovery, since the existing choice names are directly referenced there. You will need to migrate the related changes in Course Discovery as well. In this context, backward compatibility should take precedence over PEP 8 standards, to avoid breaking downstream services.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please try this approach
class ProductTypes(models.TextChoices):
COURSE = 'Course', 'Course' # New PEP 8 name
Course = COURSE

Comment thread taxonomy/choices.py
"""
User goal choices, this will be used in skills quiz.
"""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from django.db import models

class ProductTypes(models.TextChoices):
# Retain original naming for backward compatibility
Course = 'course', 'Course'
Program = 'program', 'Program'
XBlock = 'xblock', 'XBlock'
XBlockData = 'xblock_data', 'XBlockData'

class UserGoal(models.TextChoices):
# Retain original naming for backward compatibility
ChangeCareers = 'change_careers', 'I want to change careers'
GetPromoted = 'get_promoted', 'I want to get promoted'
ImproveCurrentRole = 'improve_current_role', 'I want to improve at my current role'
Other = 'other', 'Other'

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the code, Thanks.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your change will break the migration

The solution is to add the backward compatibility aliases after the class definition, outside the enum that i have implemented

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please write the test cases to check eg:# tests/test_choices.py
class TestUserGoalChoices(TestCase):
"""Test UserGoal choice field migration."""

def test_choice_values_consistency(self):
    """Ensure choice values remain the same after migration."""
    # Test old format compatibility
    assert UserGoal.CHANGE_CAREERS == ('change_careers', 'I want to change careers')
    assert UserGoal.GET_PROMOTED == ('get_promoted', 'I want to get promoted')
    assert UserGoal.IMPROVE_CURRENT_ROLE == ('improve_current_role', 'I want to improve at my current role')
    assert UserGoal.OTHER == ('other', 'Other')

also below files test cases also
test_utils/factories.py imports UserGoal for factory creation
tests/test_utilities.py uses ProductTypes in utility function tests
taxonomy/models.py uses UserGoal in model definitions

@rgopalrao-sonata-png rgopalrao-sonata-png left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the code as per the comments as it will impact course discovery and edx-catalog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants