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

Refactor: Moved a number of edx-platform imports to figures.compat #313

Merged
merged 1 commit into from
Dec 19, 2020

Conversation

johnbaldwin
Copy link
Contributor

@johnbaldwin johnbaldwin commented Dec 18, 2020

Improved abstraction: This commit refactors how Figures imports edx-platform classes. It centralizes imports to figures.compat. The rest of Figures then imports the platform classes from figures.compat.

This is preliminary work to help abstract platform dependencies for Open edX named release structures and improve future testing efforts

Also bumped Figures release version to 0.4.dev7

Copy link
Contributor

@OmarIthawi OmarIthawi left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks John!

@@ -1,7 +1,9 @@
"""Figures compatability module

This module serves to provide a common access point to functionality that
differs from different named Open edX releases
This module serves to provide a common access point to edx-platform objects.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: The module can now be called openedx instead of compat to reflect the newer name imho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I'll do that during another refactor

@@ -116,6 +116,8 @@ class SiteDailyMetrics(TimeStampedModel):
# Should change this to default value of 0
mau = models.IntegerField(blank=True, null=True)

# TODO: Add field for number of CDMs reported
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This doesn't seem to be related to the pull request overall. Was it added by mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, on purpose, a reminder while I was in the code

@@ -241,3 +241,25 @@ def student_modules_for_course_enrollment(ce):
Relies on the fact that course_ids are globally unique
"""
return StudentModule.objects.filter(student=ce.user, course_id=ce.course_id)


def site_certificates(site):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This function doesn't seem to be both unused and unrelated to the pull request. Was it added by mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not added by mistake, just a little extra that I remembered I wanted in there as I was refactoring

Comment on lines 34 to 36
# from openedx.core.djangoapps.content.course_overviews.models import (
# CourseOverview,
# )
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Probably not needed.

Suggested change
# from openedx.core.djangoapps.content.course_overviews.models import (
# CourseOverview,
# )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this dead code. Thanks!

Improved abstraction: This commit refactors how Figures imports edx-platform
classes. It centralizes imports to figures.compat. The rest of Figures then
imports the platform classes from figures.compat.

This is preliminary work to help abstract platform dependencies for Open
edX named release structures and improve future testing efforts
@johnbaldwin johnbaldwin force-pushed the john/0.4-refactor-platform-imports branch from a55cd9b to 5c2dcd2 Compare December 19, 2020 17:32
@johnbaldwin johnbaldwin merged commit 61d26b3 into master Dec 19, 2020
@johnbaldwin johnbaldwin deleted the john/0.4-refactor-platform-imports branch December 19, 2020 22:51
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