diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 3b2f533e9e17..68beeec8448c 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -3,7 +3,7 @@ # default code owner -* @omarithawi +* @bryanlandia # default set of reviewers -* @omarithawi @melvinsoft @thraxil @shadinaif +* @amirtds @xscrio diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index ccd1c9b49c28..cbad39a52923 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -18,7 +18,7 @@ jobs: PULL_REQUEST_BODY: | This is an automated pull request from branch `hawthorn/main` into `hawthorn/prod` (production). Please review the changes and merge this pull request _before_ running the Tahoe production Cloud Build deployment. - PULL_REQUEST_REVIEWERS: "johnbaldwin amirtds bryanlandia" + PULL_REQUEST_REVIEWERS: "amirtds bryanlandia xscrio" hawthorn-to-juniper-sync: name: PullRequestAction runs-on: ubuntu-latest @@ -35,5 +35,5 @@ jobs: This is meant for making sure all of our Hawthorn changes gets merge into Juniper otherwise Juniper would stall. If tests passes merge this pull request. If there are merge conflicts, it needs to be resolved manually in a seperate pull request. - PULL_REQUEST_REVIEWERS: "melvinsoft shadinaif OmarIthawi thraxil" + PULL_REQUEST_REVIEWERS: "bryanlandia amirtds xscrio" diff --git a/.github/workflows/report_conflicts.yml b/.github/workflows/report_conflicts.yml index 1239066f1f87..96ff1955f55d 100644 --- a/.github/workflows/report_conflicts.yml +++ b/.github/workflows/report_conflicts.yml @@ -2,13 +2,13 @@ on: [pull_request] name: 'Merge conflicts' jobs: - report_master: - name: 'koa, lilac, maple, nutmeg and master' + report: + name: 'nutmeg and master' uses: appsembler/action-conflict-counter/.github/workflows/report-via-comment.yml@main with: local_base_branch: ${{ github.base_ref }} - upstream_repo: 'https://github.com/edx/edx-platform.git' - upstream_branches: 'open-release/koa.master,open-release/lilac.master,open-release/maple.master,open-release/nutmeg.master,master' + upstream_repo: 'https://github.com/openedx/edx-platform.git' + upstream_branches: 'open-release/nutmeg.master,master' exclude_paths: 'cms/static/js/,conf/locale/,lms/static/js/,package.json,package-lock.json,.github/' secrets: custom_github_token: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/sync_nutmeg_with_juniper.yml b/.github/workflows/sync_nutmeg_with_juniper.yml index f92283f378d1..c1dbb1130b64 100644 --- a/.github/workflows/sync_nutmeg_with_juniper.yml +++ b/.github/workflows/sync_nutmeg_with_juniper.yml @@ -9,7 +9,7 @@ jobs: runs-on: ubuntu-latest steps: - name: pull-request-action - uses: vsoch/pull-request-action@1.0.19 + uses: vsoch/pull-request-action@1.1.1 env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} BRANCH_PREFIX: "main" diff --git a/.github/workflows/sync_prod_with_main.yml b/.github/workflows/sync_prod_with_main.yml index 4927eb46f65d..358f6072bf33 100644 --- a/.github/workflows/sync_prod_with_main.yml +++ b/.github/workflows/sync_prod_with_main.yml @@ -9,10 +9,10 @@ jobs: runs-on: ubuntu-latest steps: - name: pull-request-action - uses: vsoch/pull-request-action@1.0.19 + uses: vsoch/pull-request-action@1.1.1 env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} BRANCH_PREFIX: "main" PULL_REQUEST_BRANCH: "prod" PULL_REQUEST_TITLE: "Update from `main` (production)" - PULL_REQUEST_REVIEWERS: "melvinsoft OmarIthawi thraxil shadinaif" + PULL_REQUEST_REVIEWERS: "VladyslavTy daniilly" diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index a110bf2f8fd2..b96fe22dde49 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -24,6 +24,8 @@ jobs: - lms-1 - lms-2 - mte + - legacy-amc-tests + - db-migrations - studio steps: @@ -32,6 +34,8 @@ jobs: uses: actions/setup-python@v2 with: python-version: ${{ matrix.python-version }} + env: + PIP_TRUSTED_HOST: "pypi.python.org pypi.org files.pythonhosted.org" - name: Install dependencies # TODO: Remove tox-pip-version once we upgrade to Koa+, or whenever we have addressed pip 20.3 strict issues. run: | diff --git a/Dockerfile.tutor b/Dockerfile.tutor index deb29f5c1b9f..fe47d6ea685d 100644 --- a/Dockerfile.tutor +++ b/Dockerfile.tutor @@ -17,23 +17,25 @@ RUN pip install -r ./requirements/edx/base.txt \ # Sync with `edx-configs` `appsembler/tahoe/us/juniper/prod/files/server-vars.yml` RUN echo "Installing pip packages:" \ - && pip install openedx-scorm-xblock==10.4.0 \ - && pip install xblock-launchcontainer==2.3.1 \ + && pip install xblock-launchcontainer==4.0.0 \ && pip install xblock-prismjs==0.1.4 \ && pip install xblock-problem-builder==4.1.9 \ && echo \ + && pip install https://github.com/appsembler/openedx-scorm-xblock/archive/refs/tags/v15.1.0-appsembler-tahoe-compat.tar.gz \ && pip install https://github.com/appsembler/pdfXBlock/archive/v0.3.1.tar.gz \ && pip install https://github.com/edx/xblock-free-text-response/archive/4149cc450.tar.gz \ && pip install https://github.com/pmitros/FeedbackXBlock/archive/v1.1.tar.gz \ && pip install https://github.com/ubc/ubcpi/archive/1.0.0.tar.gz \ && echo \ - && pip install course-access-groups==0.5.4 \ - && pip install figures==0.4.1 \ + && pip install course-access-groups==0.6.1 \ + && pip install figures==0.4.4 \ && pip install tahoe-figures-plugins==0.1.1 \ && pip install tahoe-lti==0.3.0 \ - && pip install tahoe-scorm==0.1.2 \ + && pip install tahoe-scorm==0.1.4 \ + && pip install xblock-grade-fetcher==0.5.0 \ + && pip install django-manage-admins==0.1.0 \ && echo \ - && pip install https://github.com/appsembler/openedx-completion-aggregator/archive/3.0.3-2021-may-18-bug-fixes.tar.gz \ + && pip install https://github.com/appsembler/openedx-completion-aggregator/archive/3.0.3-2023-mar-27-revert-use-of-task-track.tar.gz \ && echo "Finished installing pip packages." EXPOSE 8000 diff --git a/cms/djangoapps/appsembler/apps.py b/cms/djangoapps/appsembler/apps.py new file mode 100644 index 000000000000..1f7c35a3b84c --- /dev/null +++ b/cms/djangoapps/appsembler/apps.py @@ -0,0 +1,14 @@ +""" +Appsembler CMS App Configuration +""" + + +from django.apps import AppConfig + + +class CMSAppsemblerConfig(AppConfig): + """ + Application Configuration for Badges. + """ + name = u'appsembler' + plugin_app = {} diff --git a/cms/djangoapps/appsembler/management/__init__.py b/cms/djangoapps/appsembler/management/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/cms/djangoapps/appsembler/management/commands/__init__.py b/cms/djangoapps/appsembler/management/commands/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/cms/djangoapps/appsembler/management/commands/cms_remove_stray_courses.py b/cms/djangoapps/appsembler/management/commands/cms_remove_stray_courses.py new file mode 100644 index 000000000000..2c6897fff6da --- /dev/null +++ b/cms/djangoapps/appsembler/management/commands/cms_remove_stray_courses.py @@ -0,0 +1,100 @@ +""" +Command to remove courses without associated organization. + +This command is intended as a follow-up step after `remove_site` but can be run independently. +""" + +from django.core.management.base import BaseCommand, CommandError +from django.conf import settings +from opaque_keys.edx.keys import CourseKey + +from xmodule.contentstore.django import contentstore +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.django import modulestore + +from contentstore.utils import delete_course + +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.djangoapps.appsembler.sites.deletion_utils import ( + confirm_deletion, +) + + +def get_deletable_course_keys_from_mongo(): + """ + Get keys of courses without active organization. + """ + mongodb_course_keys = {str(mongodb_course.id) for mongodb_course in modulestore().get_course_summaries()} + mysql_course_keys = {str(mysql_course_key) for mysql_course_key in CourseOverview.get_all_course_keys()} + return list(mongodb_course_keys - mysql_course_keys) + + +def delete_course_and_assets(course_key): + """ + Delete all courses without active organization. + """ + course_key_obj = CourseKey.from_string(course_key) + delete_course(course_key_obj, ModuleStoreEnum.UserID.mgmt_command, keep_instructors=False) + contentstore().delete_all_course_assets(course_key_obj) + + +def cms_remove_stray_courses(commit, limit): + """ + Remove all courses from mongodb that has no CourseOverview entry in MySQL. + """ + course_keys = get_deletable_course_keys_from_mongo() + if limit: + course_keys = course_keys[:limit] + + if not course_keys: + raise CommandError('No courses found to delete.') + + str_course_list = [str(course_key) for course_key in course_keys] + print('Preparing to delete:') + print('\n'.join(str_course_list)) + commit = confirm_deletion( + question='Do you confirm to delete the courses from CMS?', + commit=commit, + ) + + for course_key in course_keys: + if commit: + print('Deleting course: {}'.format(course_key)) + delete_course_and_assets(course_key) + else: + print('[Dry run] deleting course: {}'.format(course_key)) + + print('Finished removing deletable courses') + + +class Command(BaseCommand): + help = "Delete courses that don't belong to organization in `get_active_organizations()`." + + def add_arguments(self, parser): + parser.add_argument( + '--limit', + dest='limit', + default=1, + type=int, + help='Max courses to delete, use 0 to delete all courses.', + ) + + parser.add_argument( + '--commit', + dest='commit', + action='store_true', + help='Remove courses, otherwise only the log will be printed.', + ) + + parser.add_argument( + '--dry-run', + dest='commit', + action='store_false', + help='Do not remove courses, only print the logs.', + ) + + def handle(self, *args, **options): + if settings.ROOT_URLCONF != 'cms.urls': + raise CommandError('This command can only be run in CMS.') + + cms_remove_stray_courses(commit=options.get('commit'), limit=options['limit']) diff --git a/cms/djangoapps/appsembler/tests/test_deletion_command.py b/cms/djangoapps/appsembler/tests/test_deletion_command.py new file mode 100644 index 000000000000..cf95a801ffa2 --- /dev/null +++ b/cms/djangoapps/appsembler/tests/test_deletion_command.py @@ -0,0 +1,35 @@ +""" + +""" + +from django.core.management import call_command, CommandError + +from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory + + +class DeletionCommandTestCase(ModuleStoreTestCase): + def test_cms_remove_stray_courses_command_no_courses(self): + """ + Raise CommandError if there's no courses to delete. + """ + with self.assertRaises(CommandError): + call_command('cms_remove_stray_courses') + + def test_cms_remove_stray_courses_command(self): + """ + Removes all courses that has only MongoDB entry. + """ + CourseFactory.create() + call_command('cms_remove_stray_courses') + + def test_cms_remove_stray_courses_command_non_to_delete(self): + """ + Should not remove courses from MongoDB if it has a MySQL CourseOverview entry. + """ + course = CourseFactory.create() + CourseOverviewFactory.create(id=course.id) + + with self.assertRaises(CommandError): + call_command('cms_remove_stray_courses') diff --git a/cms/djangoapps/appsembler/tests/test_studio_logout_view.py b/cms/djangoapps/appsembler/tests/test_studio_logout_view.py new file mode 100644 index 000000000000..d6c243cf13f2 --- /dev/null +++ b/cms/djangoapps/appsembler/tests/test_studio_logout_view.py @@ -0,0 +1,145 @@ +""" +Tests for APPSEMBLER_MULTI_TENANT_EMAILS in Studio logout. + +Special note: + +This test module needs to patch `cms.urls.urlpatterns` to include urlpatterns +from `cms.djangoapps.appsembler.urls`. This works by overriding the +`doango.conf.settings.ROOT_URLCONF` with `django.test.utils.override_settings` +at the TestCase class level with the `urlpatterns` list declared in the module +containing the TestCase class. + +For this test module, we've added a `urlpatterns` module level variable and +assigned it the value of `cms.urls.urlpatterns` then appended the conditionally +included urlpatterns we need to run the tests. + +Then we add `@override_settings(ROOT_URLCONF=__name__)` to the TestClass + +There are other ways to do this. However, this is simple and does not require +our code to explicitly hack `sys.modules` reloading +""" +from unittest.mock import Mock, patch + +from django.conf import settings +from django.conf.urls import include, url +from django.contrib import auth +from django.contrib.auth.models import AnonymousUser +from django.urls import reverse +from django.test import RequestFactory, TestCase +from django.test.utils import override_settings +from rest_framework import status +from tahoe_sites.api import add_user_to_organization, create_tahoe_site + +from student.tests.factories import UserFactory +import cms.urls +from cms.djangoapps.appsembler.views import get_logout_redirect_url + + +# Set the urlpatterns we want to use for our tests in this module only +urlpatterns = cms.urls.urlpatterns + [ + url(r'', include('cms.djangoapps.appsembler.urls')) +] + + +@override_settings(ROOT_URLCONF=__name__) # the module that contains `urlpatterns` +@override_settings(LOGOUT_REDIRECT_URL='home') # ensure that we have a value for LOGOUT_REDIRECT_URL +@patch.dict('django.conf.settings.FEATURES', {'TAHOE_STUDIO_LOCAL_LOGIN': True}) +class TestStudioLogoutView(TestCase): + """ + Testing the APPSEMBLER_MULTI_TENANT_EMAILS feature when enabled in Studio. + """ + BLUE = 'blue1' + EMAIL = 'customer@example.com' + PASSWORD = 'xyz' + DOMAIN = 'testdomain.com' + SHORT_NAME = 'testdomain' + + def setUp(self): + super(TestStudioLogoutView, self).setUp() + self.url = reverse('logout') + self.user = UserFactory.create(email=self.EMAIL, password=self.PASSWORD) + add_user_to_organization( + user=self.user, + organization=create_tahoe_site(domain=self.DOMAIN, short_name=self.SHORT_NAME)['organization'] + ) + self.request = RequestFactory() + self.request.is_secure = Mock(return_value=False) + self.lms_url = 'http://{site_domain}/logout'.format(site_domain=self.DOMAIN) + + def test_logout_must_be_authenticated(self): + """ + Test logout from studio must be authenticated + """ + response = self.client.get(self.url) + assert response.status_code == status.HTTP_302_FOUND + assert not response.content + assert '?next=/logout' in response.url + + def test_logout_normal_user(self): + """ + Test logout from studio for normal users (meaning that they are linked to an organization). It is expected + that a logout then redirect to LMS will be performed + """ + self.client.login(username=self.user.username, password=self.PASSWORD) + assert auth.get_user(self.client).is_authenticated + + response = self.client.get(self.url) + assert not auth.get_user(self.client).is_authenticated + + assert response.status_code == status.HTTP_302_FOUND + assert not response.content + assert response.url == self.lms_url + + def test_logout_staff_user(self): + """ + Test logout from studio for staff users (meaning that they are not linked to any organization). It is expected + that a logout then a redirect to settings.LOGOUT_REDIRECT_URL will be performed + """ + # Not necessary to set the user as staff, the thing we need to test is when it lacks a link to an organization + user = UserFactory.create(email=self.EMAIL, password=self.PASSWORD) + + self.client.login(username=user.username, password=self.PASSWORD) + assert auth.get_user(self.client).is_authenticated + + response = self.client.get(self.url) + assert not auth.get_user(self.client).is_authenticated + + assert response.status_code == status.HTTP_302_FOUND + assert not response.content + assert response.url == reverse(settings.LOGOUT_REDIRECT_URL) + + def test_get_logout_redirect_url_no_request(self): + """ + Verify that get_logout_redirect_url will return settings.LOGOUT_REDIRECT_URL if the request is None + """ + assert get_logout_redirect_url(request=None) == reverse(settings.LOGOUT_REDIRECT_URL) + + def test_get_logout_redirect_url_no_user(self): + """ + Verify that get_logout_redirect_url will return settings.LOGOUT_REDIRECT_URL if no user is logged in + """ + assert not hasattr(self.request, 'user') + assert get_logout_redirect_url(request=self.request) == reverse(settings.LOGOUT_REDIRECT_URL) + + def test_get_logout_redirect_url_anonymous(self): + """ + Verify that get_logout_redirect_url will return settings.LOGOUT_REDIRECT_URL if the user is anonymous + """ + self.request.user = AnonymousUser() + assert get_logout_redirect_url(request=self.request) == reverse(settings.LOGOUT_REDIRECT_URL) + + def test_get_logout_redirect_url_user(self): + """ + Verify that get_logout_redirect_url will return the LMS URL related to the user + """ + self.request.user = self.user + assert get_logout_redirect_url(request=self.request) == self.lms_url + + def test_get_logout_redirect_url_staff(self): + """ + Verify that get_logout_redirect_url will return settings.LOGOUT_REDIRECT_URL if the user is not linked to + any organization (staff users and superusers) + """ + user = UserFactory.create(email=self.EMAIL, password=self.PASSWORD) + self.request.user = user + assert get_logout_redirect_url(request=self.request) == reverse(settings.LOGOUT_REDIRECT_URL) diff --git a/cms/djangoapps/appsembler/urls.py b/cms/djangoapps/appsembler/urls.py index 200913eeb344..6562882ef50c 100644 --- a/cms/djangoapps/appsembler/urls.py +++ b/cms/djangoapps/appsembler/urls.py @@ -5,13 +5,10 @@ We have this code in the Appsembler CMS app to help isolate custom code """ -from django.conf import settings from django.urls import path -from django.contrib.auth.views import LogoutView -from .views import LoginView +from .views import LoginView, StudioLogoutView urlpatterns = [ path('login/', LoginView.as_view(), name='login'), - path('logout/', LogoutView.as_view( - next_page=settings.LOGOUT_REDIRECT_URL), name='logout'), + path('logout/', StudioLogoutView.as_view(), name='logout'), ] diff --git a/cms/djangoapps/appsembler/views.py b/cms/djangoapps/appsembler/views.py index c3faf55bc4f0..77297ca02e9f 100644 --- a/cms/djangoapps/appsembler/views.py +++ b/cms/djangoapps/appsembler/views.py @@ -9,6 +9,9 @@ import logging from django.conf import settings from django.contrib.auth import authenticate, get_user_model, login +from django.contrib.auth.decorators import login_required +from django.contrib.auth.views import LogoutView as DjangoLogoutView +from django.core.exceptions import ObjectDoesNotExist from django.db.models import Q from django.http import HttpResponseServerError from django.shortcuts import redirect @@ -18,7 +21,11 @@ from django.views import View from django.views.decorators.clickjacking import xframe_options_deny from django.views.decorators.csrf import csrf_protect -from tahoe_sites.api import deprecated_get_admin_users_queryset_by_email +from tahoe_sites.api import ( + deprecated_get_admin_users_queryset_by_email, + get_organization_for_user, + get_site_by_organization, +) from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.user_authn.utils import is_safe_login_or_logout_redirect @@ -274,3 +281,39 @@ def log_multiple_objects_returned(self): def render_login_page_with_error(self, error_code): return render_login_page( login_error_message=self.error_messages[error_code]) + + +def get_logout_redirect_url(request): + """ + Return logout redirect url using the site related to the given user if possible. + Otherwise, return settings.LOGOUT_REDIRECT_URL + + :return: logout redirect url or settings.LOGOUT_REDIRECT_URL + """ + user = getattr(request, 'user', None) + if not user or not user.is_authenticated: + return reverse(settings.LOGOUT_REDIRECT_URL) + + try: + organization = get_organization_for_user(user=user) + except ObjectDoesNotExist: + return reverse(settings.LOGOUT_REDIRECT_URL) + site = get_site_by_organization(organization=organization) + + return '{protocol}://{site_domain}/logout'.format( + protocol='https' if request.is_secure() else 'http', + site_domain=site.domain + ) + + +class StudioLogoutView(View): + """ + Studio Logout View + """ + @method_decorator(csrf_protect) + @method_decorator(login_required) + def get(self, request): + """ + Perform logout from studio, and redirect to LMS home page + """ + return DjangoLogoutView.as_view(next_page=get_logout_redirect_url(request))(request) diff --git a/cms/djangoapps/contentstore/tasks.py b/cms/djangoapps/contentstore/tasks.py index 636c3f22e2b9..98fd0e931c66 100644 --- a/cms/djangoapps/contentstore/tasks.py +++ b/cms/djangoapps/contentstore/tasks.py @@ -311,7 +311,7 @@ def async_migrate_transcript_subtask(*args, **kwargs): # pylint: disable=unused command_run=command_run, edx_video_id=edx_video_id, language_code=language_code, - transcript_content=transcript_content, + transcript_content=transcript_content.encode(), file_format=Transcript.SJSON, force_update=force_update, ) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 5a8f75c76621..bd194177c10f 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -1114,7 +1114,7 @@ def settings_handler(request, course_key_string): 'upgrade_deadline': upgrade_deadline, } if is_prerequisite_courses_enabled(): - courses, in_process_course_actions = get_courses_accessible_to_user(request) + courses, in_process_course_actions = get_courses_accessible_to_user(request, course_module.location.org) # exclude current course from the list of available courses courses = (course for course in courses if course.id != course_key) if courses: diff --git a/cms/envs/common.py b/cms/envs/common.py index 3e77bc28d3b4..95e57cd3abf6 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1461,6 +1461,9 @@ # CMS specific user task handling 'cms_user_tasks.apps.CmsUserTasksConfig', + # Appsembler customization app for CMS + 'appsembler.apps.CMSAppsemblerConfig', + # Unusual migrations 'database_fixups', diff --git a/cms/static/js/certificates/models/certificate.js b/cms/static/js/certificates/models/certificate.js index a440d569d606..d0e6ae6e9f35 100644 --- a/cms/static/js/certificates/models/certificate.js +++ b/cms/static/js/certificates/models/certificate.js @@ -43,7 +43,7 @@ define([ initialize: function(attributes, options) { // Set up the initial state of the attributes set for this model instance this.canBeEmpty = options && options.canBeEmpty; - if (options.add) { + if (options.add && !attributes.signatories) { // Ensure at least one child Signatory model is defined for any new Certificate model attributes.signatories = new SignatoryModel({certificate: this}); } diff --git a/common/djangoapps/student/helpers.py b/common/djangoapps/student/helpers.py index 3c47510b5af1..a2ff280c7aa3 100644 --- a/common/djangoapps/student/helpers.py +++ b/common/djangoapps/student/helpers.py @@ -6,6 +6,7 @@ import json import logging import mimetypes +import re import urllib.parse from collections import OrderedDict from datetime import datetime @@ -18,6 +19,7 @@ from django.core.exceptions import PermissionDenied from django.core.validators import ValidationError from django.db import IntegrityError, transaction +from django.shortcuts import redirect from django.urls import NoReverseMatch, reverse from django.utils.translation import ugettext as _ from pytz import UTC @@ -65,6 +67,8 @@ EMAIL_EXISTS_MSG_FMT = _("An account with the Email '{email}' already exists.") USERNAME_EXISTS_MSG_FMT = _("An account with the Public Username '{username}' already exists.") +COURSE_URL_PATTERN = re.compile(r'courses/course-v1:[^/]+/course') + log = logging.getLogger(__name__) @@ -224,7 +228,8 @@ def check_verify_status_by_course(user, course_enrollments): # Query string parameters that can be passed to the "finish_auth" view to manage # things like auto-enrollment. -POST_AUTH_PARAMS = ('course_id', 'enrollment_action', 'course_mode', 'email_opt_in', 'purchase_workflow') +POST_AUTH_PARAMS = ('course_id', 'enrollment_action', 'course_mode', 'email_opt_in', 'purchase_workflow', + 'hide_elements') def get_next_url_for_login_page(request): @@ -301,6 +306,7 @@ def _get_redirect_to(request_host, request_headers, request_params, request_is_h redirect url if safe else None """ redirect_to = request_params.get('next') + redirect_to = sanitize_next_parameter(redirect_to) header_accept = request_headers.get('HTTP_ACCEPT', '') accepts_text_html = any( mime_type in header_accept @@ -700,3 +706,50 @@ def get_resume_urls_for_enrollments(user, enrollments): url_to_block = '' resume_course_urls[enrollment.course_id] = url_to_block return resume_course_urls + + +def sanitize_next_parameter(next_param): + """ + Check the next parameter pattern and update the + symbol to its ASCII equivalent. + """ + if not next_param: + return next_param + + if COURSE_URL_PATTERN.match(next_param): + # Sometimes the course id received with incorrect encoding/decoding, we need to + # replace it to the correct pattern: + # course-v1:test-sandbox PREP-CORE C -> course-v1:test-sandbox+PREP-CORE+C + # + # Note: We are not expect to have any spaces in course URL + + if ' ' in next_param: + next_param = next_param.replace(' ', '+') + elif '%20' in next_param: + next_param = next_param.replace('%20', '+') + + sanitized_next_parameter = re.sub(r'\+', '%2B', next_param) + + log.info( + u"The course-like next parameter was detected '%(next_param)s'" + u" this will be replaced with sanitized version: '%(sanitized_next_parameter)s'", + { + "next_param": next_param, + "sanitized_next_parameter": sanitized_next_parameter, + } + ) + return sanitized_next_parameter + + return next_param + + +def add_hide_elements_cookie_to_redirect(redirect_to): + if 'hide_elements' in redirect_to: + # Perform the redirect and set the cookie only if 'hide_elements' is present + response = redirect(redirect_to) + + # Set a cookie to indicate that elements should be hidden + response.set_cookie('hideElements', 'true', max_age=86400) + + return response + else: + return redirect(redirect_to) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 37b208311ae2..9db611db6d7c 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -1656,7 +1656,10 @@ def unenroll_by_email(cls, email, course_id): RequestCache('get_enrollment').clear() try: - user = User.objects.get(email=email) + if settings.FEATURES.get('APPSEMBLER_MULTI_TENANT_EMAILS', False): + user = CourseEnrollment.get_user_by_email_within_organization(email) + else: + user = User.objects.get(email=email) return cls.unenroll(user, course_id) except User.DoesNotExist: log.error( diff --git a/common/djangoapps/student/tests/test_helpers.py b/common/djangoapps/student/tests/test_helpers.py index 655d9b763b36..f069910296f8 100644 --- a/common/djangoapps/student/tests/test_helpers.py +++ b/common/djangoapps/student/tests/test_helpers.py @@ -14,7 +14,7 @@ from testfixtures import LogCapture from openedx.core.djangoapps.site_configuration.tests.test_util import with_site_configuration_context -from student.helpers import get_next_url_for_login_page +from student.helpers import get_next_url_for_login_page, sanitize_next_parameter LOGGER_NAME = "student.helpers" @@ -156,3 +156,49 @@ def test_custom_tahoe_site_redirect_lms(self): 'LOGIN_REDIRECT_URL': '' # Falsy or empty URLs should not be used }): assert '/dashboard' == get_next_url_for_login_page(request), 'Falsy url should default to dashboard' + + def test_sanitize_next_param(self): + # Valid URL with plus - change the plus symbol to ASCII code + next_param = 'courses/course-v1:abc-sandbox+ACC-PTF+C/course' + expected_result = 'courses/course-v1:abc-sandbox%2BACC-PTF%2BC/course' + self.assertEqual(sanitize_next_parameter(next_param), expected_result) + + # Valid URL without plus - keep the next_param as it is + next_param = 'courses/course-v1:abc-sandbox/course' + self.assertEqual(sanitize_next_parameter(next_param), next_param) + + # Empty string - keep the next_param as it is + next_param = '' + self.assertEqual(sanitize_next_parameter(next_param), next_param) + + # None input - keep the next_param as it is + next_param = None + self.assertEqual(sanitize_next_parameter(next_param), next_param) + + # Invalid pattern - keep the next_param as it is + next_param = 'some/other/path' + self.assertEqual(sanitize_next_parameter(next_param), next_param) + + # Invalid URL with space - replace the ' ' with '+' and encode it + expected_result = 'courses/course-v1:abc-sandbox%2BACC-PTF%2BC/course' + + next_param = 'courses/course-v1:abc-sandbox ACC-PTF C/course' + self.assertEqual(sanitize_next_parameter(next_param), expected_result) + + next_param = 'courses/course-v1:abc-sandbox ACC-PTF+C/course' + self.assertEqual(sanitize_next_parameter(next_param), expected_result) + + next_param = 'courses/course-v1:abc-sandbox+ACC-PTF C/course' + self.assertEqual(sanitize_next_parameter(next_param), expected_result) + + # Invalid URL with encoded space - replace the '%20' with '+' and encode it + expected_result = 'courses/course-v1:abc-sandbox%2BACC-PTF%2BC/course' + + next_param = 'courses/course-v1:abc-sandbox%20ACC-PTF%20C/course' + self.assertEqual(sanitize_next_parameter(next_param), expected_result) + + next_param = 'courses/course-v1:abc-sandbox%20ACC-PTF+C/course' + self.assertEqual(sanitize_next_parameter(next_param), expected_result) + + next_param = 'courses/course-v1:abc-sandbox+ACC-PTF%20C/course' + self.assertEqual(sanitize_next_parameter(next_param), expected_result) diff --git a/common/djangoapps/third_party_auth/settings.py b/common/djangoapps/third_party_auth/settings.py index 234dba9857d0..09d6a9cef812 100644 --- a/common/djangoapps/third_party_auth/settings.py +++ b/common/djangoapps/third_party_auth/settings.py @@ -57,7 +57,9 @@ def apply_settings(django_settings): 'third_party_auth.pipeline.get_username', 'third_party_auth.pipeline.set_pipeline_timeout', 'third_party_auth.pipeline.ensure_user_information', - 'social_core.pipeline.user.create_user', + # wrap social_core.pipeline.user.create_user so we can test selectively disabling + 'openedx.core.djangoapps.appsembler.tahoe_idp.tpa_pipeline.wrapped_social_core_create_user', + # 'social_core.pipeline.user.create_user', 'social_core.pipeline.social_auth.associate_user', 'social_core.pipeline.social_auth.load_extra_data', 'social_core.pipeline.user.user_details', diff --git a/common/djangoapps/track/views/segmentio.py b/common/djangoapps/track/views/segmentio.py index 82aa2923752e..a61c5fdf5a5e 100644 --- a/common/djangoapps/track/views/segmentio.py +++ b/common/djangoapps/track/views/segmentio.py @@ -289,8 +289,8 @@ def send_event(request, method, **params): segment_key = helpers.get_current_site_configuration().get_secret_value('SEGMENT_KEY') if segment_key: data['writeKey'] = segment_key - data['messageId'] = 'ajs-' + uuid.uuid4().hex - site_response = requests.post(url, json=data) + data['messageId'] = 'ajs-next-' + uuid.uuid4().hex + site_response = requests.post(url, json=data) # noqa: F841 return HttpResponse( main_response.content, status=main_response.status_code, diff --git a/common/lib/xmodule/xmodule/js/src/video/01_initialize.js b/common/lib/xmodule/xmodule/js/src/video/01_initialize.js index 6051cc428621..98adaf6af182 100644 --- a/common/lib/xmodule/xmodule/js/src/video/01_initialize.js +++ b/common/lib/xmodule/xmodule/js/src/video/01_initialize.js @@ -90,6 +90,7 @@ function(VideoPlayer, i18n, moment, _) { _youtubeApiDeferred = null, _oldOnYouTubeIframeAPIReady; + const setupOnYouTubeIframeAPIReadyMaxCalls=5; Initialize.prototype = methodsDict; @@ -163,14 +164,16 @@ function(VideoPlayer, i18n, moment, _) { // so that it resolves our Deferred object, which will call all of the // OnYouTubeIframeAPIReady callbacks. // - // If this global function is already defined, we store it first, and make - // sure that it gets executed when our Deferred object is resolved. - setupOnYouTubeIframeAPIReady = function() { + + let setupOnYouTubeIframeAPIReady = function() { + + // If this global function is already defined, we store it first, and make + // sure that it gets executed when our Deferred object is resolved. _oldOnYouTubeIframeAPIReady = window.onYouTubeIframeAPIReady || undefined; window.onYouTubeIframeAPIReady = function() { - window.onYouTubeIframeAPIReady.resolve(); - }; + _youtubeApiDeferred.resolve(); + } window.onYouTubeIframeAPIReady.resolve = _youtubeApiDeferred.resolve; window.onYouTubeIframeAPIReady.done = _youtubeApiDeferred.done; @@ -201,6 +204,7 @@ function(VideoPlayer, i18n, moment, _) { window.YT.ready(onYTApiReady); }); } + } else { video = VideoPlayer(state); diff --git a/common/lib/xmodule/xmodule/video_module/video_module.py b/common/lib/xmodule/xmodule/video_module/video_module.py index 1884a1388c9b..eea74231c91c 100644 --- a/common/lib/xmodule/xmodule/video_module/video_module.py +++ b/common/lib/xmodule/xmodule/video_module/video_module.py @@ -63,6 +63,8 @@ from .video_utils import create_youtube_string, format_xml_exception_message, get_poster, rewrite_video_url from .video_xfields import VideoFields +from openedx.core.djangoapps.appsembler.sites import utils as appsembler_site_utils + # The following import/except block for edxval is temporary measure until # edxval is a proper XBlock Runtime Service. # @@ -393,6 +395,10 @@ def get_html(self, view=STUDENT_VIEW): # it anymore; therefore we force-disable it in this case (when controls aren't visible). autoadvance_this_video = self.auto_advance and autoadvance_enabled + scheme = "https" if settings.HTTPS == "on" else "http" + lms_base_url = appsembler_site_utils.get_lms_link_from_course_key(settings.LMS_ROOT_URL, self.course_id) + lms_root_url = "{}://{}".format(scheme, lms_base_url) if "http" not in lms_base_url else lms_base_url + metadata = { 'saveStateEnabled': view != PUBLIC_VIEW, 'saveStateUrl': self.ajax_url + '/save_user_state', @@ -418,7 +424,7 @@ def get_html(self, view=STUDENT_VIEW): 'transcriptLanguages': sorted_languages, 'ytTestTimeout': settings.YOUTUBE['TEST_TIMEOUT'], 'ytApiUrl': settings.YOUTUBE['API'], - 'lmsRootURL': settings.LMS_ROOT_URL, + 'lmsRootURL': lms_root_url, 'ytMetadataEndpoint': ( # In the new runtime, get YouTube metadata via a handler. The handler supports anonymous users and # can work in sandboxed iframes. In the old runtime, the JS will call the LMS's yt_video_metadata diff --git a/conf/locale/ar/LC_MESSAGES/django.mo b/conf/locale/ar/LC_MESSAGES/django.mo index c05c00998def..a73163947863 100644 Binary files a/conf/locale/ar/LC_MESSAGES/django.mo and b/conf/locale/ar/LC_MESSAGES/django.mo differ diff --git a/conf/locale/ar/LC_MESSAGES/django.po b/conf/locale/ar/LC_MESSAGES/django.po index 75cc4f5f81b2..1a154de88d00 100644 --- a/conf/locale/ar/LC_MESSAGES/django.po +++ b/conf/locale/ar/LC_MESSAGES/django.po @@ -16825,7 +16825,7 @@ msgstr "استعراض عملية التقييم في Studio" #: lms/templates/courseware/progress.html msgid "Course Progress for '{username}' ({email})" -msgstr "تقدم الدورة ل '{اسم المستخدم}' ({عنوان البريد الالكتروني})" +msgstr "تقدم الدورة ل '{username}' ({email})" #: lms/templates/courseware/progress.html msgid "View Certificate" diff --git a/lms/djangoapps/courseware/models.py b/lms/djangoapps/courseware/models.py index d6dae57db4c5..09d64eb9fa63 100644 --- a/lms/djangoapps/courseware/models.py +++ b/lms/djangoapps/courseware/models.py @@ -37,6 +37,21 @@ log = logging.getLogger("edx.courseware") +def should_update_student_module_modified_on_save(): + """ + Fixes RED-3616 to avoid updating StudentModule.modified after regrade or any celery automated updates. + + The modified field is relied upon for Monthly Active Users calculations, so any celery task updates it would be + confused with leaner activity. + + Hack: This duplicates the `common.djangoapps.track.shim:is_celery_worker` helper function, but hopefully we'll + be removing this soon enough that code duplication won't be a problem. + """ + is_celery_worker = getattr(settings, 'IS_CELERY_WORKER', False) + is_hack_enabled = settings.FEATURES.get('TAHOE_STUDENT_MODULES_DISABLE_MODIFIED_IN_CELERY', True) + return not (is_celery_worker and is_hack_enabled) + + def chunks(items, chunk_size): """ Yields the values from items in chunks of size chunk_size @@ -119,7 +134,12 @@ class Meta(object): done = models.CharField(max_length=8, choices=DONE_TYPES, default='na') created = models.DateTimeField(auto_now_add=True, db_index=True) - modified = models.DateTimeField(auto_now=True, db_index=True) + + if should_update_student_module_modified_on_save(): + modified = models.DateTimeField(auto_now=True, db_index=True) + else: + # Fixes RED-3616 to modified on celery. + modified = models.DateTimeField(auto_now_add=True, db_index=True) @classmethod def all_submitted_problems_read_only(cls, course_id): diff --git a/lms/djangoapps/courseware/tests/test_tahoe_student_module_hack.py b/lms/djangoapps/courseware/tests/test_tahoe_student_module_hack.py new file mode 100644 index 000000000000..7ea880d33060 --- /dev/null +++ b/lms/djangoapps/courseware/tests/test_tahoe_student_module_hack.py @@ -0,0 +1,84 @@ +""" +Tests for the RED-3616 hack/fix for MAU calculations depending on StudentModule.modified. +""" + +from importlib import reload + +import pytest +from freezegun import freeze_time + + +def import_fresh_models(): + """ + Import `lms.djangoapps.courseware.models` and reload it to react to features. + """ + from lms.djangoapps.courseware import models as courseware_models + from lms.djangoapps.courseware.tests import factories as courseware_factories + reload(courseware_models) + reload(courseware_factories) + return { + 'courseware_models': courseware_models, + 'courseware_factories': courseware_factories, + } + + +def test_is_untouched_by_default_in_celery(settings): + settings.IS_CELERY_WORKER = True + courseware_models = import_fresh_models()['courseware_models'] + assert not courseware_models.should_update_student_module_modified_on_save(), 'Should be enabled for celery' + + +def test_in_updated_by_default_in_http_requests(settings): + settings.IS_CELERY_WORKER = False + courseware_models = import_fresh_models()['courseware_models'] + assert courseware_models.should_update_student_module_modified_on_save(), 'Should be disabled in http requests' + + +def test_can_be_updated_in_celery_if_needed(settings): + """ + TAHOE_STUDENT_MODULES_DISABLE_MODIFIED_IN_CELERY is on by default but can be turned off via lms FEATURES. + """ + settings.IS_CELERY_WORKER = True + settings.FEATURES = { + **settings.FEATURES, + 'TAHOE_STUDENT_MODULES_DISABLE_MODIFIED_IN_CELERY': False, + } + courseware_models = import_fresh_models()['courseware_models'] + assert courseware_models.should_update_student_module_modified_on_save(), 'The feature is configurable' + + +@pytest.mark.django_db +def test_new_student_module_with_in_celery(settings): + """ + Ensure that `StudentModule.modified` isn't updated when saving from within a celery task. + """ + settings.IS_CELERY_WORKER = True + courseware_factories = import_fresh_models()['courseware_factories'] + + with freeze_time('2012-01-14'): + student_module = courseware_factories.StudentModuleFactory.create() + assert student_module.created.year == 2012 + assert student_module.modified.year == 2012 + + with freeze_time('2020-12-20'): + student_module.save() + assert student_module.created.year == 2012 + assert student_module.modified.year == 2012, 'Should not touch `modified` during in celery' + + +@pytest.mark.django_db +def test_new_student_module_with_in_http_requests(settings): + """ + Ensure that `StudentModule.modified` _is updated_ when saving from within an HTTP request. + """ + courseware_factories = import_fresh_models()['courseware_factories'] + + with freeze_time('2012-01-14'): + student_module = courseware_factories.StudentModuleFactory.create() + assert student_module.created.year == 2012 + assert student_module.modified.year == 2012 + + with freeze_time('2020-12-20'): + student_module.save() + assert student_module.created.year == 2012 + assert student_module.modified.year == 2020, 'Should update `modified` outside celery' diff --git a/lms/static/js/discovery/models/search_state.js b/lms/static/js/discovery/models/search_state.js index cd37f938ea91..e727938cd81f 100644 --- a/lms/static/js/discovery/models/search_state.js +++ b/lms/static/js/discovery/models/search_state.js @@ -11,7 +11,7 @@ return Backbone.Model.extend({ page: 0, - pageSize: 20, + pageSize: 100, // Tahoe: fix a bug related to Course Access Groups - RED-3598 searchTerm: '', terms: {}, jqhxr: null, diff --git a/lms/static/js/student_account/components/StudentAccountDeletion.jsx b/lms/static/js/student_account/components/StudentAccountDeletion.jsx index 30713689bb12..0ed88a0f9f98 100644 --- a/lms/static/js/student_account/components/StudentAccountDeletion.jsx +++ b/lms/static/js/student_account/components/StudentAccountDeletion.jsx @@ -112,18 +112,10 @@ export class StudentAccountDeletion extends React.Component { {bodyDeletion} {bodyDeletion2}

-

-