feat: return override fields from properties when they are populated #4434
Conversation
AfaqShuaib09
force-pushed
the
afaq/prod-4167-3
branch
2 times, most recently
from
b237980
to
7aff413
Compare
hamza-56
force-pushed
the
afaq/prod-4167-3
branch
from
1c3f1c3
to
0c132e6
Compare
hamza-56
force-pushed
the
afaq/prod-4167-3
branch
from
0c132e6
to
de88767
Compare
hamza-56
force-pushed
the
afaq/prod-4167-3
branch
from
6b590de
to
8b5f4b7
Compare
hamza-56
force-pushed
the
afaq/prod-4167-3
branch
3 times, most recently
from
96c3510
to
0f7245f
Compare
hamza-56
force-pushed
the
afaq/prod-4167-3
branch
from
0f7245f
to
c06627c
Compare
| extracted from the course runs. | ||
| """ | ||
| if self.language_override: | ||
| return {self.language_override} |
There was a problem hiding this comment.
what's with dict operator?
There was a problem hiding this comment.
Shouldn't this be a list?
There was a problem hiding this comment.
+1, it should be a list instead of a set
There was a problem hiding this comment.
@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.
| course_run = CourseRunFactory.create(language=language_de) | ||
| program = ProgramFactory.create(language_override=language_es, courses=[course_run.course]) | ||
|
|
||
| expected_languages = {language_es} |
There was a problem hiding this comment.
replace set with list
|
|
||
| program = ProgramFactory.create(language_override=None, courses=[course_run1.course, course_run2.course]) | ||
|
|
||
| expected_languages = {language_en, language_fr} |
There was a problem hiding this comment.
replace set with list
| 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] |
There was a problem hiding this comment.
wont this code be simple if we make subjects list set, adding primary override (if present) and convert it back to list?
There was a problem hiding this comment.
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:
- The primary subject is explicitly placed at the front.
- Order is maintained for the rest of the subjects.
There was a problem hiding this comment.
Why not remove the inner if else and swap it with the assignment in the else? That would be equivalent to the current implementation?
| 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] |
There was a problem hiding this comment.
Why not remove the inner if else and swap it with the assignment in the else? That would be equivalent to the current implementation?
PROD-4176
Design methods for subjects and languages, giving preference to overrides rather than using the simple method values.