Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: return override fields from properties when they are populated #4434

Merged
merged 4 commits into from
Sep 16, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@ def get_products(self, product_type, product_source):
subject_translations
)
elif product_type == 'degree':
queryset = Program.objects.marketable().exclude(degree__isnull=True).select_related('partner', 'type')
queryset = Program.objects.marketable().exclude(degree__isnull=True) \
.select_related('partner', 'type', 'primary_subject_override', 'language_override')

if product_source:
queryset = queryset.filter(product_source__slug=product_source)
Expand Down Expand Up @@ -164,12 +165,12 @@ def get_transformed_data(self, product, product_type):
})
elif product_type == 'degree':
data.update({
"Subjects": ", ".join(subject.name for subject in product.subjects),
"Subjects": ", ".join(subject.name for subject in product.active_subjects),
"Subjects Spanish": ", ".join(
translation.name for subject in product.subjects
for translation in subject.spanish_translations
),
"Languages": ", ".join(language.code for language in product.languages),
"Languages": ", ".join(language.code for language in product.active_languages),
"Marketing Image": product.card_image.url if product.card_image else "",
})

Expand Down Expand Up @@ -198,7 +199,7 @@ def handle(self, *args, **options):
raise CommandError('No products found for the given criteria.')
products_count = products.count()

logger.info(f'Fetched {products_count} courses from the database')
logger.info(f'Fetched {products_count} {product_type}s from the database')
if output_csv:
with open(output_csv, 'w', newline='') as output_file:
output_writer = self.write_csv_header(output_file)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@
from course_discovery.apps.course_metadata.models import Course, CourseType, ProgramType
from course_discovery.apps.course_metadata.tests.factories import (
CourseFactory, CourseRunFactory, CourseTypeFactory, DegreeFactory, OrganizationFactory, PartnerFactory,
ProgramTypeFactory, SeatFactory, SourceFactory
ProgramTypeFactory, SeatFactory, SourceFactory, SubjectFactory
)
from course_discovery.apps.ietf_language_tags.models import LanguageTag


class PopulateProductCatalogCommandTests(TestCase):
Expand Down Expand Up @@ -218,6 +219,48 @@ def test_populate_product_catalog_excludes_non_marketable_degrees(self):
self.assertEqual(len(matching_rows), 1,
f"Marketable degree '{marketable_degree.title}' should be in the CSV")

def test_populate_product_catalog_with_degrees_having_overrides(self):
"""
Test that the populate_product_catalog command includes the overridden subjects and languages for degrees.
"""
degree = DegreeFactory.create(
product_source=self.source,
partner=self.partner,
additional_metadata=None,
type=self.program_type,
status=ProgramStatus.Active,
marketing_slug="valid-marketing-slug",
title="Marketable Degree",
authoring_organizations=[self.organization],
card_image=factory.django.ImageField(),
primary_subject_override=SubjectFactory(name='Subject1'),
language_override=LanguageTag.objects.get(code='es'),
)

with NamedTemporaryFile() as output_csv:
call_command(
"populate_product_catalog",
product_type="degree",
output_csv=output_csv.name,
product_source="edx",
gspread_client_flag=False,
)

with open(output_csv.name, "r") as output_csv_file:
csv_reader = csv.DictReader(output_csv_file)
rows = list(csv_reader)

matching_rows = [
row for row in rows if row["UUID"] == str(degree.uuid.hex)
]
self.assertEqual(len(matching_rows), 1)

row = matching_rows[0]
self.assertEqual(row["UUID"], str(degree.uuid.hex))
self.assertEqual(row["Title"], degree.title)
self.assertIn(degree.primary_subject_override.name, row["Subjects"])
self.assertEqual(row["Languages"], degree.language_override.code)

@mock.patch(
"course_discovery.apps.course_metadata.management.commands.populate_product_catalog.Command.get_products"
)
Expand Down Expand Up @@ -309,8 +352,8 @@ def test_get_transformed_data_for_degree(self):
org.logo_image.url for org in product_authoring_orgs if org.logo_image
),
"Organizations Abbr": ", ".join(org.key for org in product_authoring_orgs),
"Languages": ", ".join(language.code for language in product.languages),
"Subjects": ", ".join(subject.name for subject in product.subjects),
"Languages": ", ".join(language.code for language in product.active_languages),
"Subjects": ", ".join(subject.name for subject in product.active_subjects),
"Subjects Spanish": ", ".join(
translation.name for subject in product.subjects
for translation in subject.spanish_translations
Expand Down
29 changes: 29 additions & 0 deletions course_discovery/apps/course_metadata/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -3518,6 +3518,16 @@ def course_run_statuses(self):
def languages(self):
return {course_run.language for course_run in self.course_runs if course_run.language is not None}

@property
def active_languages(self):
"""
:return: The list of languages; It gives preference to the language_override over the languages
extracted from the course runs.
"""
if self.language_override:
return {self.language_override}
Copy link
Contributor

Choose a reason for hiding this comment

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

what's with dict operator?

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1, it should be a list instead of a set

Copy link
Contributor

Choose a reason for hiding this comment

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

 @property
    def languages(self):
        return {course_run.language for course_run in self.course_runs if course_run.language is not None}

The return type of languages is a set, so we're returning a set in active_languages as well to maintain consistency in the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, makes sense.

return self.languages

@property
def transcript_languages(self):
languages = [course_run.transcript_languages.all() for course_run in self.course_runs]
Expand All @@ -3540,6 +3550,25 @@ def subjects(self):
common_others = [s for s, _ in Counter(course_subjects).most_common() if s not in common_primary]
return common_primary + common_others

@property
def active_subjects(self):
"""
:return: The list of subjects; the first subject should be the most common primary subjects of its courses,
other subjects should be collected and ranked by frequency among the courses.

Note: This method gives preference to the primary_subject_override over the primary subject of the courses.
"""
subjects = self.subjects

if self.primary_subject_override:
if self.primary_subject_override not in subjects:
subjects = [self.primary_subject_override] + subjects
else:
subjects = [self.primary_subject_override] + \
[subject for subject in subjects if subject != self.primary_subject_override]
Comment on lines +3563 to +3568
Copy link
Contributor

Choose a reason for hiding this comment

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

wont this code be simple if we make subjects list set, adding primary override (if present) and convert it back to list?

Copy link
Contributor

Choose a reason for hiding this comment

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

While converting the subjects list to a set would simplify the code by automatically handling duplicates, we need to ensure the order of subjects is preserved, especially since the primary subject override should always appear first.
The current approach ensures both:

  1. The primary subject is explicitly placed at the front.
  2. Order is maintained for the rest of the subjects.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not remove the inner if else and swap it with the assignment in the else? That would be equivalent to the current implementation?


return subjects

@property
def topics(self):
"""
Expand Down
59 changes: 59 additions & 0 deletions course_discovery/apps/course_metadata/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -3519,6 +3519,65 @@ def test_program_duration_override(self):
self.program.program_duration_override = ''
assert self.program.program_duration_override is not None

def test_active_subjects_with_no_override(self):
"""
Test that active_subjects returns the subjects from the associated courses
when no primary_subject_override is set.
"""

subject1 = SubjectFactory.create(name='Subject 1')
subject2 = SubjectFactory.create(name='Subject 2')
course1 = CourseFactory.create(subjects=[subject1])
course2 = CourseFactory.create(subjects=[subject2])
program = ProgramFactory.create(primary_subject_override=None, courses=[course1, course2])

expected_subjects = [subject1, subject2]
self.assertEqual(program.active_subjects, expected_subjects)

def test_active_subjects_with_primary_subject_override(self):
"""
Test that active_subjects includes the primary_subject_override at the beginning
when it is set.
"""
primary_subject_override = SubjectFactory.create(name='Primary Subject')
other_subject = SubjectFactory.create(name='Other Subject')
course = CourseFactory.create(subjects=[other_subject])

program = ProgramFactory.create(primary_subject_override=primary_subject_override, courses=[course])

expected_subjects = [primary_subject_override, other_subject]
self.assertEqual(program.active_subjects, expected_subjects)

def test_active_languages_with_no_override(self):
"""
Test that active_languages returns the languages from the associated courses
when no language_override is set.
"""

language_en = LanguageTag.objects.create(code='en', name='English')
language_fr = LanguageTag.objects.get(code='fr')

course_run1 = CourseRunFactory.create(language=language_en)
course_run2 = CourseRunFactory.create(language=language_fr)

program = ProgramFactory.create(language_override=None, courses=[course_run1.course, course_run2.course])

expected_languages = {language_en, language_fr}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

replace set with list

self.assertEqual(program.active_languages, expected_languages)

def test_active_languages_with_language_override(self):
"""
Test that active_languages returns the language_override when it is set.
"""

language_es = LanguageTag.objects.get(code='es')
language_de = LanguageTag.objects.get(code='de')
course_run = CourseRunFactory.create(language=language_de)
program = ProgramFactory.create(language_override=language_es, courses=[course_run.course])

expected_languages = {language_es}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

replace set with list

self.assertEqual(program.active_languages, expected_languages)


class ProgramSubscriptionTests(TestCase):

Expand Down
Loading