Skip to content

Commit

Permalink
Merge branch 'master' into bleach#33209
Browse files Browse the repository at this point in the history
  • Loading branch information
irtazaakram authored Jun 11, 2024
2 parents 68b0526 + b98cbd4 commit b8008a8
Show file tree
Hide file tree
Showing 18 changed files with 150 additions and 24 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/compile-python-requirements.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jobs:
- name: Set up Python environment
uses: actions/setup-python@v5
with:
python-version: "3.11"
python-version: "3.8"

- name: Run make compile-requirements
env:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/upgrade-one-python-dependency.yml
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ jobs:
- name: Set up Python environment
uses: actions/setup-python@v5
with:
python-version: "3.11"
python-version: "3.8"

- name: Update any pinned dependencies
env:
Expand Down
2 changes: 1 addition & 1 deletion cms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -948,7 +948,7 @@
'openedx.core.djangoapps.cache_toolbox.middleware.CacheBackedAuthenticationMiddleware',

'common.djangoapps.student.middleware.UserStandingMiddleware',
'openedx.core.djangoapps.contentserver.middleware.StaticContentServer',
'openedx.core.djangoapps.contentserver.middleware.StaticContentServerMiddleware',

'django.contrib.messages.middleware.MessageMiddleware',
'common.djangoapps.track.middleware.TrackMiddleware',
Expand Down
3 changes: 3 additions & 0 deletions cms/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@
path('heartbeat', include('openedx.core.djangoapps.heartbeat.urls')),
path('i18n/', include('django.conf.urls.i18n')),

# Course assets
path('', include('openedx.core.djangoapps.contentserver.urls')),

# User API endpoints
path('api/user/', include('openedx.core.djangoapps.user_api.urls')),

Expand Down
2 changes: 1 addition & 1 deletion lms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -2287,7 +2287,7 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring
'openedx.core.djangoapps.safe_sessions.middleware.EmailChangeMiddleware',

'common.djangoapps.student.middleware.UserStandingMiddleware',
'openedx.core.djangoapps.contentserver.middleware.StaticContentServer',
'openedx.core.djangoapps.contentserver.middleware.StaticContentServerMiddleware',

# Adds user tags to tracking events
# Must go before TrackMiddleware, to get the context set up
Expand Down
16 changes: 9 additions & 7 deletions lms/templates/problem.html
Original file line number Diff line number Diff line change
Expand Up @@ -68,21 +68,23 @@ <h3 class="hd hd-3 problem-header" id="${ short_id }-problem-title" aria-describ
</span>
<span class="sr">(${submit_disabled_cta['description']})</span>
% else:
<form class="submit-cta" method="post" action="${submit_disabled_cta['link']}">
<form class="submit-cta" method="post" action="${submit_disabled_cta.get('link')}">
% if submit_disabled_cta.get('link'):
<input type="hidden" id="csrf_token" name="csrfmiddlewaretoken" value="${csrf_token}">
% for form_name, form_value in submit_disabled_cta['form_values'].items():
<input type="hidden" name="${form_name}" value="${form_value}">
% endfor
<button class="submit-cta-link-button btn-link btn-small">
${submit_disabled_cta['link_name']}
</button>
<span class="submit-cta-description" tabindex="0" role="note" aria-label="description">
<span data-tooltip="${submit_disabled_cta['description']}" data-tooltip-show-on-click="true"
class="fa fa-info-circle fa-lg" aria-hidden="true">
</span>
% endif
<span class="submit-cta-description" tabindex="0" role="note" aria-label="description">
<span data-tooltip="${submit_disabled_cta['description']}" data-tooltip-show-on-click="true"
class="fa fa-info-circle fa-lg" aria-hidden="true">
</span>
<span class="sr">(${submit_disabled_cta['description']})</span>
</form>
</span>
<span class="sr">(${submit_disabled_cta['description']})</span>
</form>
% endif
% endif
<div class="submission-feedback ${'cta-enabled' if submit_disabled_cta else ''}" id="submission_feedback_${short_id}">
Expand Down
2 changes: 1 addition & 1 deletion lms/templates/vert_module.html
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ <h2 class="hd hd-2 unit-title">${unit_title}</h2>
<div class="banner-cta-button">
<button class="btn btn-outline-primary" onclick="emit_event(${vertical_banner_cta['event_data']})">${vertical_banner_cta['link_name']}</button>
</div>
% else:
% elif vertical_banner_cta.get('link'):
<div class="banner-cta-button">
<form method="post" action="${vertical_banner_cta['link']}">
<input type="hidden" id="csrf_token" name="csrfmiddlewaretoken" value="${csrf_token}">
Expand Down
3 changes: 3 additions & 0 deletions lms/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,9 @@

path('i18n/', include('django.conf.urls.i18n')),

# Course assets
path('', include('openedx.core.djangoapps.contentserver.urls')),

# Enrollment API RESTful endpoints
path('api/enrollment/v1/', include('openedx.core.djangoapps.enrollments.urls')),

Expand Down
32 changes: 31 additions & 1 deletion openedx/core/djangoapps/contentserver/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
)
from django.utils.deprecation import MiddlewareMixin
from edx_django_utils.monitoring import set_custom_attribute
from edx_toggles.toggles import WaffleFlag
from opaque_keys import InvalidKeyError
from opaque_keys.edx.locator import AssetLocator

Expand All @@ -32,19 +33,45 @@

log = logging.getLogger(__name__)

# .. toggle_name: content_server.use_view
# .. toggle_implementation: WaffleFlag
# .. toggle_default: False
# .. toggle_description: Deployment flag for switching asset serving from a middleware
# to a view. Intended to be used once in each environment to test the cutover and
# ensure there are no errors or changes in behavior. Once this has been tested,
# the middleware can be fully converted to a view.
# .. toggle_use_cases: temporary
# .. toggle_creation_date: 2024-05-02
# .. toggle_target_removal_date: 2024-07-01
# .. toggle_tickets: https://github.com/openedx/edx-platform/issues/34702
CONTENT_SERVER_USE_VIEW = WaffleFlag('content_server.use_view', module_name=__name__)

# TODO: Soon as we have a reasonable way to serialize/deserialize AssetKeys, we need
# to change this file so instead of using course_id_partial, we're just using asset keys

HTTP_DATE_FORMAT = "%a, %d %b %Y %H:%M:%S GMT"


class StaticContentServer(MiddlewareMixin):
class StaticContentServerMiddleware(MiddlewareMixin):
"""
Shim to maintain old pattern of serving course assets from a middleware. See views.py.
"""
def process_request(self, request):
"""Intercept asset request or allow view to handle it, depending on config."""
if CONTENT_SERVER_USE_VIEW.is_enabled():
return
else:
set_custom_attribute('content_server.handled_by.middleware', True)
return IMPL.process_request(request)


class StaticContentServer():
"""
Serves course assets to end users. Colloquially referred to as "contentserver."
"""
def is_asset_request(self, request):
"""Determines whether the given request is an asset request"""
# Don't change this without updating urls.py! See docstring of views.py.
return (
request.path.startswith('/' + XASSET_LOCATION_TAG + '/')
or
Expand Down Expand Up @@ -295,6 +322,9 @@ def load_asset_from_location(self, location):
return content


IMPL = StaticContentServer()


def parse_range_header(header_value, content_length):
"""
Returns the unit and a list of (start, end) tuples of ranges.
Expand Down
16 changes: 16 additions & 0 deletions openedx/core/djangoapps/contentserver/urls.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
"""
URL patterns for course asset serving.
"""

from django.urls import path, re_path

from . import views

# These patterns are incomplete and do not capture the variable
# components of the URLs. That's because the view itself is separately
# parsing the paths, for historical reasons. See docstring on views.py.
urlpatterns = [
path("c4x/", views.course_assets_view),
re_path("^asset-v1:", views.course_assets_view),
re_path("^assets/courseware/", views.course_assets_view),
]
58 changes: 58 additions & 0 deletions openedx/core/djangoapps/contentserver/views.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
"""
Views for serving course assets.
Historically, this was implemented as a *middleware* (StaticContentServer) that
intercepted requests with paths matching certain patterns, rather than using
urlpatterns and a view. There wasn't any good reason for this, as far as I can
tell. It causes some problems for telemetry: When the code-owner middleware asks
Django what view handled the request, it does so by looking at the result of the
`resolve` utility, but these URLs get a Resolver404 (because there's no
registered urlpattern).
We'd like to turn this into a proper view:
https://github.com/openedx/edx-platform/issues/34702
The first step, seen here, is to have urlpatterns (redundant with the
middleware's `is_asset_request` method) and a view, but the view just calls into
the same code the middleware uses. The implementation of the middleware has been
moved into StaticContentServerImpl, leaving the middleware as just a shell
around the latter.
A waffle flag chooses whether to allow the middleware to handle the request, or
whether to pass the request along to the view. Why? Because we might be relying
by accident on some weird behavior inherent to misusing a middleware this way,
and we need a way to quickly switch back if we encounter problems.
If the view works, we can move all of StaticContentServerImpl directly into the
view and drop the middleware and the waffle flag.
"""
from django.http import HttpResponseNotFound
from django.views.decorators.http import require_safe
from edx_django_utils.monitoring import set_custom_attribute

from .middleware import CONTENT_SERVER_USE_VIEW, IMPL


@require_safe
def course_assets_view(request):
"""
Serve course assets to end users. Colloquially referred to as "contentserver."
"""
set_custom_attribute('content_server.handled_by.view', True)

if not CONTENT_SERVER_USE_VIEW.is_enabled():
# Should never happen; keep track of occurrences.
set_custom_attribute('content_server.view.called_when_disabled', True)
# But handle the request anyhow.

# We'll delegate request handling to an instance of the middleware
# until we can verify that the behavior is identical when requests
# come all the way through to the view.
response = IMPL.process_request(request)

if response is None:
# Shouldn't happen
set_custom_attribute('content_server.view.no_response_from_impl', True)
return HttpResponseNotFound()
else:
return response
10 changes: 8 additions & 2 deletions openedx/features/course_experience/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,13 +141,19 @@ def get_start_block(block):
return get_start_block(first_child)


def dates_banner_should_display(course_key, user):
def dates_banner_should_display(course_key, user, allow_warning=False):
"""
Return whether or not the reset banner should display,
determined by whether or not a course has any past-due,
incomplete sequentials and which enrollment mode is being
dealt with for the current user and course.
Args:
course_key (CourseKey)
user (User)
allow_warning (bool): whether to ignore relative_dates_disable_reset_flag, in order to render
warnings for past-due incomplete units.
Returns:
(missed_deadlines, missed_gated_content):
missed_deadlines is True if the user has missed any graded content deadlines
Expand All @@ -156,7 +162,7 @@ def dates_banner_should_display(course_key, user):
if not RELATIVE_DATES_FLAG.is_enabled(course_key):
return False, False

if RELATIVE_DATES_DISABLE_RESET_FLAG.is_enabled(course_key):
if RELATIVE_DATES_DISABLE_RESET_FLAG.is_enabled(course_key) and not allow_warning:
# The `missed_deadlines` value is ignored by `reset_course_deadlines` views. Instead, they check the value of
# `missed_gated_content` to determine if learners can reset the deadlines by themselves.
# We could have added this logic directly to `reset_self_paced_schedule`, but this function is used in other
Expand Down
16 changes: 12 additions & 4 deletions openedx/features/personalized_learner_schedules/call_to_action.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from xmodule.util.misc import is_xblock_an_assignment
from openedx.core.djangolib.markup import HTML, Text
from openedx.core.lib.mobile_utils import is_request_from_mobile_app
from openedx.features.course_experience import RELATIVE_DATES_DISABLE_RESET_FLAG
from openedx.features.course_experience.url_helpers import is_request_from_learning_mfe
from openedx.features.course_experience.utils import dates_banner_should_display

Expand All @@ -37,7 +38,10 @@ def get_ctas(self, xblock, category, completed):
request = get_current_request()

course_key = xblock.scope_ids.usage_id.context_key
missed_deadlines, missed_gated_content = dates_banner_should_display(course_key, request.user)
missed_deadlines, missed_gated_content = dates_banner_should_display(
course_key, request.user, allow_warning=True
)

# Not showing in the missed_gated_content case because those learners are not eligible
# to shift due dates.
if missed_gated_content:
Expand All @@ -52,13 +56,13 @@ def get_ctas(self, xblock, category, completed):
# xblock is a capa problem, and the submit button is disabled. Check if it's because of a personalized
# schedule due date being missed, and if so, we can offer to shift it.
if self._is_block_shiftable(xblock, category):
ctas.append(self._make_reset_deadlines_cta(xblock, category, is_learning_mfe))
ctas.append(self._make_deadlines_cta(course_key, xblock, category, is_learning_mfe))

elif category == self.VERTICAL_BANNER and not completed and missed_deadlines:
# xblock is a vertical, so we'll check all the problems inside it. If there are any that will show a
# a "shift dates" CTA under CAPA_SUBMIT_DISABLED, then we'll also show the same CTA as a vertical banner.
if any(self._is_block_shiftable(item, category) for item in xblock.get_children()):
ctas.append(self._make_reset_deadlines_cta(xblock, category, is_learning_mfe))
ctas.append(self._make_deadlines_cta(course_key, xblock, category, is_learning_mfe))

return ctas

Expand Down Expand Up @@ -107,11 +111,15 @@ def _log_past_due_warning(name):
PersonalizedLearnerScheduleCallToAction.past_due_class_warnings.add(name)

@classmethod
def _make_reset_deadlines_cta(cls, xblock, category, is_learning_mfe=False):
def _make_deadlines_cta(cls, course_key, xblock, category, is_learning_mfe=False):
"""
Constructs a call to action object containing the necessary information for the view
"""
from lms.urls import RESET_COURSE_DEADLINES_NAME

if RELATIVE_DATES_DISABLE_RESET_FLAG.is_enabled(course_key):
return {"description": _("The deadline to complete this assignment has passed.")}

course_key = xblock.scope_ids.usage_id.context_key

cta_data = {
Expand Down
2 changes: 1 addition & 1 deletion requirements/constraints.txt
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ click>=8.0,<9.0
# The team that owns this package will manually bump this package rather than having it pulled in automatically.
# This is to allow them to better control its deployment and to do it in a process that works better
# for them.
edx-enterprise==4.19.11
edx-enterprise==4.19.14

# Stay on LTS version, remove once this is added to common constraint
Django<5.0
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/base.txt
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,7 @@ edx-drf-extensions==10.3.0
# edx-when
# edxval
# openedx-learning
edx-enterprise==4.19.11
edx-enterprise==4.19.14
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/kernel.in
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/development.txt
Original file line number Diff line number Diff line change
Expand Up @@ -743,7 +743,7 @@ edx-drf-extensions==10.3.0
# edx-when
# edxval
# openedx-learning
edx-enterprise==4.19.11
edx-enterprise==4.19.14
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/doc.txt
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/doc.txt
Original file line number Diff line number Diff line change
Expand Up @@ -538,7 +538,7 @@ edx-drf-extensions==10.3.0
# edx-when
# edxval
# openedx-learning
edx-enterprise==4.19.11
edx-enterprise==4.19.14
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/base.txt
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/testing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,7 @@ edx-drf-extensions==10.3.0
# edx-when
# edxval
# openedx-learning
edx-enterprise==4.19.11
edx-enterprise==4.19.14
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/base.txt
Expand Down

0 comments on commit b8008a8

Please sign in to comment.