-
Notifications
You must be signed in to change notification settings - Fork 172
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
fix: allow both active and marketable runs in update_course_ai_translations #4441
Conversation
@@ -51,10 +51,15 @@ def handle(self, *args, **options): | |||
|
|||
course_runs = CourseRun.objects.all() | |||
|
|||
if options['active']: | |||
if options['active'] and options['marketable']: | |||
course_runs = CourseRun.objects.filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[curious] Did you try course_runs.active().marketable()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that be an intersection of objects instead of Union? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch, I did not realize we were doing a union here. Could we try course_runs.marketable().union(course_runs.active())
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like we can.
marketable_course_run.translation_languages, | ||
[{'code': 'es', 'label': 'Spanish'}] | ||
) | ||
self.assertListEqual(non_active_course_run.translation_languages, []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also have a non-marketable but active
run with the same assertion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the active run is actually the non-marketable. Lemme rename that.
95008a1
to
5aa8c81
Compare
active_non_marketable_course_run.translation_languages, | ||
[{'code': 'fr', 'label': 'French'}] | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a non-marketable non-active
run and assert that the translation_languages are empty?
) | ||
self.assertListEqual( | ||
active_non_marketable_course_run.translation_languages, | ||
[{'code': 'fr', 'label': 'French'}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mock_get_translations
returns Spanish, why is this French?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, the marketable course was also active somehow. Updated the test by adding end date to past.
c8a1904
to
f6801e5
Compare
f6801e5
to
2a03ea0
Compare
PROD-4175
Adding an improvement to allow filtering both active and marketable runs.