Use the Open edX named releases to conditionally import platform objects #289
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
OmarIthawi
approved these changes
Dec 4, 2020
figures/compat.py
Outdated
| from courseware.models import StudentModule # noqa pylint: disable=unused-import,import-error | ||
|
|
||
| try: | ||
| if RELEASE_LINE == 'juniper': | ||
| from lms.djangoapps.courseware.courses import get_course_by_id # noqa pylint: disable=unused-import,import-error |
There was a problem hiding this comment.
Should work fine in Koa but we can make it future proof. See 👇🏼
$ git checkout open-release/koa.master
$ git grep get_course_by_id
lms/djangoapps/discussion/django_comment_client/base/views.py:from lms.djangoapps.courseware.courses import get_course_by_id
OmarIthawi
reviewed
Dec 4, 2020
figures/compat.py
Outdated
| from courseware.models import StudentModule # noqa pylint: disable=unused-import,import-error | ||
|
|
||
| try: | ||
| if RELEASE_LINE == 'juniper': |
There was a problem hiding this comment.
Should work fine in Koa but we can make it future proof. See 👇🏼
Suggested change
| if RELEASE_LINE == 'juniper': | |
| if RELEASE_LINE in ['juniper', 'koa']: |
OmarIthawi
reviewed
Dec 4, 2020
figures/compat.py
Outdated
| from lms.djangoapps.certificates.models import GeneratedCertificate # noqa pylint: disable=unused-import,import-error | ||
|
|
||
|
|
||
| try: | ||
| if RELEASE_LINE == 'juniper': |
There was a problem hiding this comment.
Works with Koa as well.
Suggested change
| if RELEASE_LINE == 'juniper': | |
| if RELEASE_LINE in ['juniper', 'koa']: |
# koa
lms/djangoapps/instructor_analytics/basic.py:from lms.djangoapps.courseware.models import StudentModule
OmarIthawi
reviewed
Dec 4, 2020
| try: | ||
| from opaque_keys.edx.django.models import CourseKeyField # noqa pylint: disable=unused-import,import-error | ||
| except ImportError: | ||
| if RELEASE_LINE == 'ginkgo': |
There was a problem hiding this comment.
This is future-proof. Great!
OmarIthawi
reviewed
Dec 4, 2020
figures/compat.py
Outdated
| from lms.djangoapps.courseware.courses import get_course_by_id # noqa pylint: disable=unused-import,import-error | ||
| except ImportError: | ||
| # Backward compatibily for pre-Juniper releases | ||
| else: # Backward compatibily for pre-Juniper releases |
There was a problem hiding this comment.
This import is not future proof.
Those imports would work but are not future-proof. To fix it needs to be restructured in the following form from the oldest to the most recent release:
if ginkgo or hawthorn:
import # old import
else: # all future releases
import # future proof
OmarIthawi
approved these changes
Dec 4, 2020
There was a problem hiding this comment.
I forgot to specify that all my previous comments where non-blockers. LGTM. Please feel free to merge. Also, thanks for making the issue #290.
This commit exists to address the problem that a RuntimeError can be raised instead of an ImportError when importing objects from ed-platform. The options to fix are check for ImportError AND RuntimeError. However, we opt (at least for now) to check the Open edX named release to conditionally import objects when the namespace has changed between named releases. Here is an example of the error when only ImportError is handled: ``` RuntimeError: Conflicting 'studentmodule' models in application 'courseware': <class 'courseware.models.StudentModule'> and <class 'lms.djangoapps.courseware.models.StudentModule'>. ``` NOTE: This may break Juniper compat as it was not tested in a live Juniper devstack This commit also addresses #290 We are missing test coverage for figures.compat.course_grade, but this requires some thinking because for the ginkgo test case, we need to mock all the imports as we run this test agnostically (this may be the only test that doesn't or is very close to not relying on figures 'mocks' test infrastructure. To test 'course_grade' we need to check that CourseGradeFactory().create(...) is called for Ginkgo, and 'read' is called for everyone else. BUT we are already mocking CourseGradeFactory so the import doesn't fail and attempts I made so far to get "course_grade_factory_mock.created.assert_called()" have failed. So I ran out of time and punting until later to get test coverage here Also, we've mashed up a chain of mocks for three tests. We are effectively running each test to run each of the supported Open edX platform releases: Ginkgo, Hawthorn, Juniper. This could use some rethinking...
johnbaldwin
force-pushed
the
john/fix-compat-checks-for-junniper
branch
from
519f8a7
to
6324588
Compare
|
@OmarIthawi Thanks and I'm merging this in. Take another look. I believe I addressed your comments. Only thing I think needs to be addressed is getting test coverage for |
OmarIthawi
reviewed
Dec 7, 2020
| try: | ||
| from openedx.core.release import RELEASE_LINE | ||
| except ImportError: | ||
| # we are pre-ginkgo | ||
| RELEASE_LINE = None | ||
| raise UnsuportedOpenedXRelease( |
|
Thanks @johnbaldwin! I think this is pretty good. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit exists to address the problem that a RuntimeError can be
raised instead of an ImportError when importing objects from
ed-platform.
The options to fix are check for ImportError AND RuntimeError. However,
we opt (at least for now) to check the Open edX named release to
conditionally import objects when the namespace has changed between
named releases.
Here is an example of the error when only ImportError is handled:
NOTE: This may break Juniper compat as it was not tested in a live
Juniper devstack
NOTE: See @OmarIthawi 's comments and this issue: #290