-
Notifications
You must be signed in to change notification settings - Fork 2
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
test(emeritus_api): add more tests for emeritus API ingestion #3032
Conversation
a3c7d4e
to
97e2e1c
Compare
Waiting for #3048 as both have common tests. |
ec0412a
to
b9801a1
Compare
b9801a1
to
0433b93
Compare
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.
Requested a couple of changes and the draft revision thing we discussed over call.
self.stdout.write( | ||
self.style.SUCCESS( | ||
f"External course sync successful for {vendor_name}." | ||
) | ||
) |
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.
Should we also log this stat as part of log_stats
?
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.
This is not related to the stats, this is more of a success log. Kept it here intentionally. Do you think we should move this?
if is_updated: | ||
is_draft = course_page.has_unpublished_changes | ||
revision = latest_revision.save_revision() | ||
if not is_draft: | ||
revision.publish() |
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 also move this into a small method and call it from everywhere we need to run this logic?
@@ -136,6 +166,11 @@ def test_create_or_update_emeritus_course_page( | |||
course_index_page = CourseIndexPageFactory.create(parent=home_page, title="Courses") | |||
course = CourseFactory.create(is_external=True) | |||
|
|||
if test_image_name_without_extension: | |||
emeritus_course_data["image_name"] = emeritus_course_data["image_name"].split( |
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.
While reviewing this test, We can improve the logic around matching the image name during sync. e.g. imagine an image name with two or three dots in it e.g. EMERITUS_COURSE.1.png
out command would not handle this situation. So, we should split and remove only the last .<ext>
part of an image name.
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.
👍
What are the relevant tickets?
#3028
Description (What does it do?)
Adds more tests for the external course sync to make it more robust. Also, makes some refactoring to show filtered stats and make the object creation more consistent.
How can this be tested?
./manage.py sync_external_course_runs --vendor emeritus
are cleaner and much more accurate.live + draft