-
Notifications
You must be signed in to change notification settings - Fork 14
fix: fixed the lms link imports to avoid a runtime error #166
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
base: master
Are you sure you want to change the base?
Changes from all commits
6cf2cd1
e4d7859
bdc932d
46e7e3d
08c58d5
a03dac8
2913fb0
85a68b4
794d752
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,3 +22,5 @@ | |
| } | ||
|
|
||
| SECRET_KEY = 'fake-key' | ||
|
|
||
| LMS_ROOT_URL = "https://example.com" | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,44 @@ | ||||||||||||||||
| """Utilities for feedback app""" | ||||||||||||||||
|
|
||||||||||||||||
| # feedbackblock/feedback/utils.py | ||||||||||||||||
| from urllib.parse import urlencode, urlparse, urlunparse | ||||||||||||||||
| from opaque_keys.edx.keys import UsageKey | ||||||||||||||||
| from django.conf import settings | ||||||||||||||||
|
Comment on lines
+4
to
+6
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please follow the style guide for imports
Suggested change
|
||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| def _(text): | ||||||||||||||||
| """Dummy `gettext` replacement to make string extraction tools scrape strings marked for translation""" | ||||||||||||||||
| return text | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| def get_lms_link_for_item(location, preview=False): | ||||||||||||||||
| """ | ||||||||||||||||
| Returns an LMS link to the course with a jump_to to the provided location. | ||||||||||||||||
| """ | ||||||||||||||||
| assert isinstance(location, UsageKey) | ||||||||||||||||
|
|
||||||||||||||||
| try: | ||||||||||||||||
| # pylint: disable=import-outside-toplevel | ||||||||||||||||
| from openedx.core.djangoapps.site_configuration.models import SiteConfiguration | ||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. generally speaking, it is a bad pattern for plugins to import code from edx-platform, as it creates coupling between something that is supposed to be stable (the plugin) and something that is inherently unstable (edx-platform modules). However, I don't know of any better way to let this block access SiteConfiguration other than adding an new XBlock runtime service, and I don't think a new SiteConfig runtime service is a good idea either given that we want to move away from SiteConfig. So, long story short, this is a bad pattern, but I'm OK with it for this PR given that there's no better way and we need a quick fix for this bug. |
||||||||||||||||
|
|
||||||||||||||||
| except ImportError: | ||||||||||||||||
| return None | ||||||||||||||||
|
|
||||||||||||||||
| lms_base = SiteConfiguration.get_value_for_org( | ||||||||||||||||
| location.org, | ||||||||||||||||
| "LMS_ROOT_URL", | ||||||||||||||||
| settings.LMS_ROOT_URL | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| if lms_base is None: | ||||||||||||||||
| return None | ||||||||||||||||
|
|
||||||||||||||||
| query_string = '' | ||||||||||||||||
| if preview: | ||||||||||||||||
| query_string = urlencode({'preview': '1'}) | ||||||||||||||||
|
|
||||||||||||||||
| url_parts = list(urlparse(lms_base)) | ||||||||||||||||
| url_parts[2] = f'/courses/{location.course_key}/jump_to/{location}' | ||||||||||||||||
| url_parts[4] = query_string | ||||||||||||||||
|
|
||||||||||||||||
| return urlunparse(url_parts) | ||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,62 @@ | ||||||||||||||||||||||||||
| from feedback.utils import get_lms_link_for_item | ||||||||||||||||||||||||||
| from opaque_keys.edx.keys import UsageKey | ||||||||||||||||||||||||||
| from unittest.mock import Mock | ||||||||||||||||||||||||||
| import sys | ||||||||||||||||||||||||||
| import types | ||||||||||||||||||||||||||
|
Comment on lines
+1
to
+5
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. import style
Suggested change
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| def test_get_lms_link_importerror(monkeypatch): | ||||||||||||||||||||||||||
| location = Mock(spec=UsageKey) | ||||||||||||||||||||||||||
| location.org = "edX" | ||||||||||||||||||||||||||
| location.course_key = "course-v1:edX+DemoX+2024" | ||||||||||||||||||||||||||
| location.__str__ = lambda self=location: "dummy" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if "openedx.core.djangoapps.site_configuration.models" in sys.modules: | ||||||||||||||||||||||||||
| del sys.modules["openedx.core.djangoapps.site_configuration.models"] | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| monkeypatch.setattr("feedback.utils.settings", types.SimpleNamespace(LMS_ROOT_URL="https://example.com")) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| result = get_lms_link_for_item(location) | ||||||||||||||||||||||||||
| assert result is None | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| def test_get_lms_link_with_null_lms_base(monkeypatch): | ||||||||||||||||||||||||||
| location = Mock(spec=UsageKey) | ||||||||||||||||||||||||||
| location.org = "edX" | ||||||||||||||||||||||||||
| location.course_key = "dummy" | ||||||||||||||||||||||||||
| location.__str__ = lambda self=location: "dummy" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| class MockSiteConfiguration: | ||||||||||||||||||||||||||
| @staticmethod | ||||||||||||||||||||||||||
| def get_value_for_org(org, key, default): | ||||||||||||||||||||||||||
| return None # simulate LMS base not set | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| monkeypatch.setitem( | ||||||||||||||||||||||||||
| sys.modules, | ||||||||||||||||||||||||||
| "openedx.core.djangoapps.site_configuration.models", | ||||||||||||||||||||||||||
| types.SimpleNamespace(SiteConfiguration=MockSiteConfiguration) | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| result = get_lms_link_for_item(location) | ||||||||||||||||||||||||||
| assert result is None | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| def test_get_lms_link_with_preview(monkeypatch): | ||||||||||||||||||||||||||
| location = Mock(spec=UsageKey) | ||||||||||||||||||||||||||
| location.org = "edX" | ||||||||||||||||||||||||||
| location.course_key = "course-v1:edX+DemoX+2024" | ||||||||||||||||||||||||||
| location.__str__ = lambda self=location: "dummy" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| class MockSiteConfiguration: | ||||||||||||||||||||||||||
| @staticmethod | ||||||||||||||||||||||||||
| def get_value_for_org(org, key, default): | ||||||||||||||||||||||||||
| return "https://fallback.com" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| monkeypatch.setitem( | ||||||||||||||||||||||||||
| sys.modules, | ||||||||||||||||||||||||||
| "openedx.core.djangoapps.site_configuration.models", | ||||||||||||||||||||||||||
| types.SimpleNamespace(SiteConfiguration=MockSiteConfiguration) | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| result = get_lms_link_for_item(location, preview=True) | ||||||||||||||||||||||||||
| assert result == "https://fallback.com/courses/course-v1:edX+DemoX+2024/jump_to/dummy?preview=1" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
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.
comment is not necessary