diff --git a/.github/workflows/check-consistent-dependencies.yml b/.github/workflows/check-consistent-dependencies.yml index c2fc5fd1d5c3..b2466b883070 100644 --- a/.github/workflows/check-consistent-dependencies.yml +++ b/.github/workflows/check-consistent-dependencies.yml @@ -7,13 +7,6 @@ name: Consistent Python dependencies on: pull_request: - paths: - - 'requirements/**' - push: - branches: - - master - paths: - - 'requirements/**' defaults: run: @@ -25,16 +18,37 @@ jobs: runs-on: ubuntu-22.04 steps: + # Only run remaining steps if there are changes to requirements/** + - name: "Decide whether to short-circuit" + env: + GH_TOKEN: "${{ github.token }}" + PR_URL: "${{ github.event.pull_request.html_url }}" + run: | + paths=$(gh pr diff "$PR_URL" --name-only) + echo $'Paths touched in PR:\n'"$paths" + + # The ^"? is because git may quote weird file paths + matched="$(echo "$paths" | grep -P '^"?requirements/' || true)" + echo $'Relevant paths:\n'"$matched" + if [[ -n "$matched" ]]; then + echo "RELEVANT=true" >> "$GITHUB_ENV" + fi + - uses: actions/checkout@v3 + if: ${{ env.RELEVANT == 'true' }} - uses: actions/setup-python@v4 + if: ${{ env.RELEVANT == 'true' }} with: python-version: '3.8' - - run: | + - name: "Recompile requirements" + if: ${{ env.RELEVANT == 'true' }} + run: | make compile-requirements - name: Fail if compiling requirements caused changes + if: ${{ env.RELEVANT == 'true' }} run: | SUMMARY_HELP=$(cat <<'EOMARKDOWN' # Inconsistent Python dependencies diff --git a/.github/workflows/compile-python-requirements.yml b/.github/workflows/compile-python-requirements.yml new file mode 100644 index 000000000000..18e68aa0ad6a --- /dev/null +++ b/.github/workflows/compile-python-requirements.yml @@ -0,0 +1,72 @@ +name: Recompile Python dependencies + +on: + workflow_dispatch: + inputs: + branch: + description: 'Target branch to create requirements PR against' + required: true + default: 'master' + type: string + +defaults: + run: + shell: bash # making this explicit opts into -e -o pipefail + +jobs: + recompile-python-dependencies: + runs-on: ubuntu-20.04 + + steps: + - name: Check out target branch + uses: actions/checkout@v3 + with: + ref: "${{ inputs.branch }}" + + - name: Set up Python environment + uses: actions/setup-python@v4 + with: + python-version: "3.8" + + - name: Run make compile-requirements + env: + PACKAGE: "${{ inputs.package }}" + run: | + make compile-requirements + + - name: PR preflight + run: | + if git diff --exit-code; then + # Fail early (and avoid quiet failure of create-pull-request action) + echo "Error: No changes, so not creating PR." | tee -a "$GITHUB_STEP_SUMMARY" + exit 1 + fi + + - name: Make a PR + id: make-pr + uses: peter-evans/create-pull-request@v5 + with: + branch: "${{ github.triggering_actor }}/compile-python-deps" + branch-suffix: short-commit-hash + add-paths: requirements + commit-message: | + feat: Recompile Python dependencies + + Commit generated by workflow `${{ github.workflow_ref }}` + title: "chore: Recompile Python dependencies" + body: >- + PR generated by workflow `${{ github.workflow_ref }}` + on behalf of @${{ github.triggering_actor }}. + assignees: "${{ github.triggering_actor }}" + reviewers: "${{ github.triggering_actor }}" + + - name: Job summary + env: + PR_URL: "${{ steps.make-pr.outputs.pull-request-url }}" + run: | + if [[ -z "$PR_URL" ]]; then + echo "PR not created; see log for more information" | tee -a "$GITHUB_STEP_SUMMARY" + exit 1 + else + echo "PR created or updated: $PR_URL" | tee -a "$GITHUB_STEP_SUMMARY" + fi diff --git a/.github/workflows/migrations-check.yml b/.github/workflows/migrations-check.yml index e9a6834c5257..18505b8f1b8b 100644 --- a/.github/workflows/migrations-check.yml +++ b/.github/workflows/migrations-check.yml @@ -15,8 +15,17 @@ jobs: matrix: os: [ ubuntu-20.04 ] python-version: [ 3.8 ] + # 'pinned' is used to install the latest patch version of Django + # within the global constraint i.e. Django==3.2.21 in current case + # because we have global constraint of Django<4.2 + django-version: ["pinned", "4.2"] mongo-version: ["4"] mysql-version: ["5.7", "8"] + # excluding mysql5.7 with Django 4.2 since Django 4.2 has + # dropped support for MySQL<8 + exclude: + - django-version: "4.2" + mysql-version: "5.7" services: mongo: image: mongo:${{ matrix.mongo-version }} @@ -92,6 +101,14 @@ jobs: - name: Install Python dependencies run: | make dev-requirements + if [[ "${{ matrix.django-version }}" != "pinned" ]]; then + pip install "django~=${{ matrix.django-version }}.0" + pip check # fail if this test-reqs/Django combination is broken + fi + + - name: list installed package versions + run: | + sudo pip freeze - name: Run Tests env: @@ -103,3 +120,19 @@ jobs: ./manage.py lms migrate echo "Running the CMS migrations." ./manage.py cms migrate + + # This job aggregates test results. It's the required check for branch protection. + # https://github.com/marketplace/actions/alls-green#why + # https://github.com/orgs/community/discussions/33579 + success: + name: Migrations checks successful + if: always() + needs: + - check_migrations + runs-on: ubuntu-latest + steps: + - name: Decide whether the needed jobs succeeded or failed + # uses: re-actors/alls-green@v1.2.1 + uses: re-actors/alls-green@13b4244b312e8a314951e03958a2f91519a6a3c9 + with: + jobs: ${{ toJSON(needs) }} diff --git a/.github/workflows/pylint-checks.yml b/.github/workflows/pylint-checks.yml index 50f4bf6fccc3..94a1110cfff5 100644 --- a/.github/workflows/pylint-checks.yml +++ b/.github/workflows/pylint-checks.yml @@ -20,7 +20,7 @@ jobs: - module-name: openedx-1 path: "--django-settings-module=lms.envs.test openedx/core/types/ openedx/core/djangoapps/ace_common/ openedx/core/djangoapps/agreements/ openedx/core/djangoapps/api_admin/ openedx/core/djangoapps/auth_exchange/ openedx/core/djangoapps/bookmarks/ openedx/core/djangoapps/cache_toolbox/ openedx/core/djangoapps/catalog/ openedx/core/djangoapps/ccxcon/ openedx/core/djangoapps/commerce/ openedx/core/djangoapps/common_initialization/ openedx/core/djangoapps/common_views/ openedx/core/djangoapps/config_model_utils/ openedx/core/djangoapps/content/ openedx/core/djangoapps/content_libraries/ openedx/core/djangoapps/content_staging/ openedx/core/djangoapps/contentserver/ openedx/core/djangoapps/cookie_metadata/ openedx/core/djangoapps/cors_csrf/ openedx/core/djangoapps/course_apps/ openedx/core/djangoapps/course_date_signals/ openedx/core/djangoapps/course_groups/ openedx/core/djangoapps/courseware_api/ openedx/core/djangoapps/crawlers/ openedx/core/djangoapps/credentials/ openedx/core/djangoapps/credit/ openedx/core/djangoapps/dark_lang/ openedx/core/djangoapps/debug/ openedx/core/djangoapps/demographics/ openedx/core/djangoapps/discussions/ openedx/core/djangoapps/django_comment_common/ openedx/core/djangoapps/embargo/ openedx/core/djangoapps/enrollments/ openedx/core/djangoapps/external_user_ids/ openedx/core/djangoapps/zendesk_proxy/ openedx/core/djangolib/ openedx/core/lib/ openedx/core/tests/ openedx/core/djangoapps/course_live/" - module-name: openedx-2 - path: "--django-settings-module=lms.envs.test openedx/core/djangoapps/geoinfo/ openedx/core/djangoapps/header_control/ openedx/core/djangoapps/heartbeat/ openedx/core/djangoapps/lang_pref/ openedx/core/djangoapps/models/ openedx/core/djangoapps/monkey_patch/ openedx/core/djangoapps/oauth_dispatch/ openedx/core/djangoapps/olx_rest_api/ openedx/core/djangoapps/password_policy/ openedx/core/djangoapps/plugin_api/ openedx/core/djangoapps/plugins/ openedx/core/djangoapps/profile_images/ openedx/core/djangoapps/programs/ openedx/core/djangoapps/safe_sessions/ openedx/core/djangoapps/schedules/ openedx/core/djangoapps/service_status/ openedx/core/djangoapps/session_inactivity_timeout/ openedx/core/djangoapps/signals/ openedx/core/djangoapps/site_configuration/ openedx/core/djangoapps/system_wide_roles/ openedx/core/djangoapps/theming/ openedx/core/djangoapps/user_api/ openedx/core/djangoapps/user_authn/ openedx/core/djangoapps/util/ openedx/core/djangoapps/verified_track_content/ openedx/core/djangoapps/video_config/ openedx/core/djangoapps/video_pipeline/ openedx/core/djangoapps/waffle_utils/ openedx/core/djangoapps/xblock/ openedx/core/djangoapps/xmodule_django/ openedx/core/tests/ openedx/features/ openedx/testing/ openedx/tests/ openedx/core/djangoapps/learner_pathway/ openedx/core/djangoapps/notifications/ openedx/core/djangoapps/staticfiles/" + path: "--django-settings-module=lms.envs.test openedx/core/djangoapps/geoinfo/ openedx/core/djangoapps/header_control/ openedx/core/djangoapps/heartbeat/ openedx/core/djangoapps/lang_pref/ openedx/core/djangoapps/models/ openedx/core/djangoapps/monkey_patch/ openedx/core/djangoapps/oauth_dispatch/ openedx/core/djangoapps/olx_rest_api/ openedx/core/djangoapps/password_policy/ openedx/core/djangoapps/plugin_api/ openedx/core/djangoapps/plugins/ openedx/core/djangoapps/profile_images/ openedx/core/djangoapps/programs/ openedx/core/djangoapps/safe_sessions/ openedx/core/djangoapps/schedules/ openedx/core/djangoapps/service_status/ openedx/core/djangoapps/session_inactivity_timeout/ openedx/core/djangoapps/signals/ openedx/core/djangoapps/site_configuration/ openedx/core/djangoapps/system_wide_roles/ openedx/core/djangoapps/theming/ openedx/core/djangoapps/user_api/ openedx/core/djangoapps/user_authn/ openedx/core/djangoapps/util/ openedx/core/djangoapps/verified_track_content/ openedx/core/djangoapps/video_config/ openedx/core/djangoapps/video_pipeline/ openedx/core/djangoapps/waffle_utils/ openedx/core/djangoapps/xblock/ openedx/core/djangoapps/xmodule_django/ openedx/core/tests/ openedx/features/ openedx/testing/ openedx/tests/ openedx/core/djangoapps/learner_pathway/ openedx/core/djangoapps/notifications/ openedx/core/djangoapps/staticfiles/ openedx/core/djangoapps/content_tagging/" - module-name: common path: "--django-settings-module=lms.envs.test common" - module-name: cms diff --git a/.github/workflows/semgrep.yml b/.github/workflows/semgrep.yml new file mode 100644 index 000000000000..f5ff2f63e5c8 --- /dev/null +++ b/.github/workflows/semgrep.yml @@ -0,0 +1,45 @@ +# Finds code problems by structural pattern matching. +# +# New rules can be added to test_root/semgrep/ and they should be picked up +# automatically. See https://semgrep.dev/docs/ for documentation. + +name: Semgrep code quality + +on: + pull_request: + push: + branches: + - master + +jobs: + run_semgrep: + name: Semgrep analysis + runs-on: "${{ matrix.os }}" + strategy: + matrix: + os: [ "ubuntu-20.04" ] + python-version: [ "3.8" ] + + steps: + - uses: actions/checkout@v3 + with: + fetch-depth: 1 + + - uses: actions/setup-python@v4 + with: + python-version: "${{ matrix.python-version }}" + + - name: Install semgrep + run: | + make pre-requirements + pip-sync requirements/edx/semgrep.txt + + - name: Run semgrep + env: + # Peg this to some reasonable value so that semgrep's rewrapping + # of messages doesn't break up lines in an unpredictable manner: + # https://github.com/returntocorp/semgrep/issues/8608 + COLUMNS: 80 + run: | + semgrep scan --config test_root/semgrep/ --error --quiet \ + -- lms cms common openedx diff --git a/.github/workflows/unit-test-shards.json b/.github/workflows/unit-test-shards.json index fffb216bc68b..50e54ea22402 100644 --- a/.github/workflows/unit-test-shards.json +++ b/.github/workflows/unit-test-shards.json @@ -200,6 +200,7 @@ "openedx-4": { "settings": "cms.envs.test", "paths": [ + "openedx/core/djangoapps/content_tagging/", "openedx/core/djangoapps/geoinfo/", "openedx/core/djangoapps/header_control/", "openedx/core/djangoapps/heartbeat/", diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 6ec56ec0b203..cf20ff1f0e3f 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -17,7 +17,7 @@ jobs: - "3.8" django-version: - "pinned" - #- "4.0" + - "4.2" # When updating the shards, remember to make the same changes in # .github/workflows/unit-tests-gh-hosted.yml shard_name: diff --git a/.github/workflows/upgrade-one-python-dependency.yml b/.github/workflows/upgrade-one-python-dependency.yml index 2ac704e9993e..dbc7eb94487a 100644 --- a/.github/workflows/upgrade-one-python-dependency.yml +++ b/.github/workflows/upgrade-one-python-dependency.yml @@ -2,32 +2,32 @@ name: Upgrade one Python dependency on: workflow_dispatch: - inputs: - branch: - description: 'Target branch to create requirements PR against' - required: true - default: 'master' - type: string - package: - description: 'Name of package to upgrade' - required: true - type: string - version: - description: 'Version number to upgrade to in constraints.txt (only needed if pinned)' - default: '' - type: string - change_desc: - description: | - Description of change, for commit message and PR. (What does the new version add or fix?) - default: '' - type: string + inputs: + branch: + description: 'Target branch to create requirements PR against' + required: true + default: 'master' + type: string + package: + description: 'Name of package to upgrade' + required: true + type: string + version: + description: 'Version number to upgrade to in constraints.txt (only needed if pinned)' + default: '' + type: string + change_desc: + description: | + Description of change, for commit message and PR. (What does the new version add or fix?) + default: '' + type: string defaults: run: shell: bash # making this explicit opts into -e -o pipefail jobs: - upgrade-one-python-dependency-workflow: + upgrade-one-python-dependency: runs-on: ubuntu-20.04 steps: @@ -92,8 +92,9 @@ jobs: ${{ env.body_prefix }}Commit generated by workflow `${{ github.workflow_ref }}` title: "feat: Upgrade Python dependency ${{ inputs.package }}" - body: | - ${{ env.body_prefix }}PR generated by workflow `${{ github.workflow_ref }}` on behalf of @${{ github.triggering_actor }}. + body: >- + ${{ env.body_prefix }}PR generated by workflow `${{ github.workflow_ref }}` + on behalf of @${{ github.triggering_actor }}. assignees: "${{ github.triggering_actor }}" reviewers: "${{ github.triggering_actor }}" diff --git a/Dockerfile b/Dockerfile index da215f08dcd7..b9da9d6eb4a4 100644 --- a/Dockerfile +++ b/Dockerfile @@ -147,10 +147,11 @@ COPY . . # Install Python requirements again in order to capture local projects RUN pip install -e . -USER app - # Production target FROM base as production + +USER app + ENV EDX_PLATFORM_SETTINGS='docker-production' ENV SERVICE_VARIANT "${SERVICE_VARIANT}" ENV SERVICE_PORT "${SERVICE_PORT}" @@ -167,9 +168,15 @@ CMD gunicorn \ # Development target FROM base as development -COPY --from=builder-development /edx/app/edxapp/venvs/edxapp /edx/app/edxapp/venvs/edxapp +RUN apt-get update && \ + apt-get -y install --no-install-recommends \ + # wget is used in Makefile for common_constraints.txt + wget \ + && \ + apt-get clean all && \ + rm -rf /var/lib/apt/* -USER root +COPY --from=builder-development /edx/app/edxapp/venvs/edxapp /edx/app/edxapp/venvs/edxapp RUN ln -s "$(pwd)/lms/envs/devstack-experimental.yml" "$LMS_CFG" RUN ln -s "$(pwd)/cms/envs/devstack-experimental.yml" "$CMS_CFG" diff --git a/Makefile b/Makefile index 32314eef18f0..bc5a79712e29 100644 --- a/Makefile +++ b/Makefile @@ -102,6 +102,7 @@ REQ_FILES = \ requirements/edx/testing \ requirements/edx/development \ requirements/edx/assets \ + requirements/edx/semgrep \ scripts/xblock/requirements define COMMON_CONSTRAINTS_TEMP_COMMENT @@ -111,7 +112,7 @@ endef COMMON_CONSTRAINTS_TXT=requirements/common_constraints.txt .PHONY: $(COMMON_CONSTRAINTS_TXT) $(COMMON_CONSTRAINTS_TXT): - wget -O "$(@)" https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt || touch "$(@)" + wget -O "$(@)" https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt printf "$(COMMON_CONSTRAINTS_TEMP_COMMENT)" | cat - $(@) > temp && mv temp $(@) compile-requirements: export CUSTOM_COMPILE_COMMAND=make upgrade diff --git a/README.rst b/README.rst index f8cc99f40d0c..d8d3a880c54b 100644 --- a/README.rst +++ b/README.rst @@ -25,6 +25,11 @@ platform. Functionally, the edx-platform repository provides two services: * CMS (Content Management Service), which powers Open edX Studio, the platform's learning content authoring environment; and * LMS (Learning Management Service), which delivers learning content. +Documentation +************* + +Documentation can be found at https://docs.openedx.org/projects/edx-platform. + Getting Started *************** @@ -81,11 +86,6 @@ and other rich community resources. .. _Open edX site: https://openedx.org -Documentation -************* - -Documentation can be found at https://docs.openedx.org. - Getting Help ************ diff --git a/cms/djangoapps/contentstore/asset_storage_handlers.py b/cms/djangoapps/contentstore/asset_storage_handlers.py index 1c9ba46702b9..8808dc4f0b53 100644 --- a/cms/djangoapps/contentstore/asset_storage_handlers.py +++ b/cms/djangoapps/contentstore/asset_storage_handlers.py @@ -32,7 +32,7 @@ from xmodule.modulestore.exceptions import ItemNotFoundError # lint-amnesty, pylint: disable=wrong-import-order from .exceptions import AssetNotFoundException, AssetSizeTooLargeException -from .utils import reverse_course_url, get_files_uploads_url +from .utils import reverse_course_url, get_files_uploads_url, get_response_format, request_response_format_is_json from .toggles import use_new_files_uploads_page @@ -73,8 +73,8 @@ def handle_assets(request, course_key_string=None, asset_key_string=None): if not has_course_author_access(request.user, course_key): raise PermissionDenied() - response_format = _get_response_format(request) - if _request_response_format_is_json(request, response_format): + response_format = get_response_format(request) + if request_response_format_is_json(request, response_format): if request.method == 'GET': return _assets_json(request, course_key) @@ -133,14 +133,6 @@ def get_asset_usage_path(request, course_key, asset_key_string): return JsonResponse({'usage_locations': usage_locations}) -def _get_response_format(request): - return request.GET.get('format') or request.POST.get('format') or 'html' - - -def _request_response_format_is_json(request, response_format): - return response_format == 'json' or 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json') - - def _asset_index(request, course_key): ''' Display an editable asset library. @@ -575,7 +567,7 @@ def _get_thumbnail_asset_key(asset, course_key): # TODO: this method needs improvement. These view decorators should be at the top in an actual view method, -# but this is just a method called by the asset_handler. The asset_handler used by the public studio content API +# but this is just a method called by the asset_handler. The asset_handler used by the public CMS API # just ignores all of this stuff. @require_http_methods(('DELETE', 'POST', 'PUT')) @login_required diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index c7a39be8f793..c2cc876f19da 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -147,7 +147,7 @@ def xblock_type_display_name(xblock, default_display_name=None): elif category == 'vertical': return _('Unit') elif category == 'problem': - # The problem XBlock's display_name.default is not helpful ("Blank Advanced Problem") but changing it could have + # The problem XBlock's display_name.default is not helpful ("Blank Problem") but changing it could have # too many ripple effects in other places, so we have a special case for capa problems here. # Note: With a ProblemBlock instance, we could actually check block.problem_types to give a more specific # description like "Multiple Choice Problem", but that won't work if our 'block' argument is just the block_type diff --git a/cms/djangoapps/contentstore/rest_api/v1/serializers/__init__.py b/cms/djangoapps/contentstore/rest_api/v1/serializers/__init__.py index c60d33bc4448..a5c349799824 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/serializers/__init__.py +++ b/cms/djangoapps/contentstore/rest_api/v1/serializers/__init__.py @@ -13,3 +13,7 @@ ProctoringErrorsSerializer ) from .settings import CourseSettingsSerializer +from .xblock import XblockSerializer +from .videos import VideoUploadSerializer, VideoImageSerializer +from .transcripts import TranscriptSerializer +from .assets import AssetSerializer diff --git a/cms/djangoapps/contentstore/rest_api/v1/serializers/assets.py b/cms/djangoapps/contentstore/rest_api/v1/serializers/assets.py new file mode 100644 index 000000000000..e44c31b511ed --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/serializers/assets.py @@ -0,0 +1,13 @@ +""" +API Serializers for assets +""" +from rest_framework import serializers +from .common import StrictSerializer + + +class AssetSerializer(StrictSerializer): + """ + Strict Serializer for file assets. + """ + file = serializers.FileField(required=False, allow_null=True) + locked = serializers.BooleanField(required=False, allow_null=True) diff --git a/cms/djangoapps/contentstore/rest_api/v1/serializers/common.py b/cms/djangoapps/contentstore/rest_api/v1/serializers/common.py index bc2f8d2da6a2..362504ccb0c1 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/serializers/common.py +++ b/cms/djangoapps/contentstore/rest_api/v1/serializers/common.py @@ -17,3 +17,36 @@ class CourseCommonSerializer(serializers.Serializer): rerun_link = serializers.CharField() run = serializers.CharField() url = serializers.CharField() + + +class StrictSerializer(serializers.Serializer): + """ + Serializers that validates strong parameters, i.e. that no extra fields are passed in. + The serializer inheriting from this may throw a ValidationError and can be called in a try/catch + block that will return a 400 response on ValidationError. + """ + def to_internal_value(self, data): + """ + raise validation error if there are any unexpected fields. + """ + # Transform and validate the expected fields + ret = super().to_internal_value(data) + + # Get the list of valid fields from the serializer + valid_fields = set(self.fields.keys()) + + # Check for unexpected fields + extra_fields = set(data.keys()) - valid_fields + if extra_fields: + # Check if these unexpected fields are due to nested serializers + for field_name in list(extra_fields): + if isinstance(self.fields.get(field_name), serializers.BaseSerializer): + extra_fields.remove(field_name) + + # If there are still unexpected fields left, raise an error + if extra_fields: + raise serializers.ValidationError( + {field: ["This field is not expected."] for field in extra_fields} + ) + + return ret diff --git a/cms/djangoapps/contentstore/rest_api/v1/serializers/transcripts.py b/cms/djangoapps/contentstore/rest_api/v1/serializers/transcripts.py new file mode 100644 index 000000000000..2b72f1ff441e --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/serializers/transcripts.py @@ -0,0 +1,15 @@ +""" +API Serializers for transcripts +""" +from rest_framework import serializers +from .common import StrictSerializer + + +class TranscriptSerializer(StrictSerializer): + """ + Strict Serializer for video transcripts. + """ + file = serializers.FileField() + edx_video_id = serializers.CharField() + language_code = serializers.CharField(required=False, allow_null=True) + new_language_code = serializers.CharField(required=False, allow_null=True) diff --git a/cms/djangoapps/contentstore/rest_api/v1/serializers/videos.py b/cms/djangoapps/contentstore/rest_api/v1/serializers/videos.py new file mode 100644 index 000000000000..c08856d1b511 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/serializers/videos.py @@ -0,0 +1,29 @@ +""" +API Serializers for videos +""" +from rest_framework import serializers +from .common import StrictSerializer + + +class FileSpecSerializer(StrictSerializer): + """ Strict Serializer for file specs """ + file_name = serializers.CharField() + content_type = serializers.ChoiceField(choices=['video/mp4', 'video/webm', 'video/ogg']) + + +class VideoUploadSerializer(StrictSerializer): + """ + Strict Serializer for video upload urls. + Note that these are not actually video uploads but endpoints to generate an upload url for AWS + and generating a video placeholder without performing an actual upload. + """ + files = serializers.ListField( + child=FileSpecSerializer() + ) + + +class VideoImageSerializer(StrictSerializer): + """ + Strict Serializer for video imgage files. + """ + file = serializers.ImageField() diff --git a/cms/djangoapps/contentstore/rest_api/v1/serializers/xblock.py b/cms/djangoapps/contentstore/rest_api/v1/serializers/xblock.py new file mode 100644 index 000000000000..522a34e0777d --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/serializers/xblock.py @@ -0,0 +1,79 @@ +""" +API Serializers for xblocks +""" +from rest_framework import serializers +from .common import StrictSerializer + +# The XblockSerializer is designed to be scalable and generic. As such, its structure +# should remain as general as possible. Avoid indiscriminately adding fields to it, +# especially those that are xblock-specific. In the future, we aim to develop a solution +# that can generate serializer fields dynamically based on the xblock definitions. + + +class XblockSerializer(StrictSerializer): + """ + A serializer for xblocks that enforces strict validation. + + The serializer ensures: + 1. All top-level fields have the expected data types. + 2. No unexpected fields are passed in. + + Note: The current list of fields is not exhaustive. It is primarily designed + to support the CMS API demo. While optional fields have been added, they were + chosen based on ease of discovery, not comprehensiveness. + """ + id = serializers.CharField(required=False, allow_null=True) + parent_locator = serializers.CharField(required=False, allow_null=True) + display_name = serializers.CharField(required=False, allow_null=True) + category = serializers.CharField(required=False, allow_null=True) + data = serializers.CharField(required=False, allow_null=True) + metadata = serializers.DictField(required=False, allow_null=True) + has_changes = serializers.BooleanField(required=False, allow_null=True) + children = serializers.ListField(required=False, allow_null=True) + fields = serializers.DictField(required=False, allow_null=True) + has_children = serializers.BooleanField(required=False, allow_null=True) + video_sharing_enabled = serializers.BooleanField(required=False, allow_null=True) + video_sharing_options = serializers.CharField(required=False, allow_null=True) + video_sharing_doc_url = serializers.CharField(required=False, allow_null=True) + edited_on = serializers.CharField(required=False, allow_null=True) + published = serializers.BooleanField(required=False, allow_null=True) + published_on = serializers.JSONField(required=False, allow_null=True) + studio_url = serializers.CharField(required=False, allow_null=True) + released_to_students = serializers.BooleanField(required=False, allow_null=True) + release_date = serializers.JSONField(required=False, allow_null=True) + nullout = serializers.JSONField(required=False, allow_null=True) + graderType = serializers.JSONField(required=False, allow_null=True) + visibility_state = serializers.CharField(required=False, allow_null=True) + has_explicit_staff_lock = serializers.BooleanField( + required=False, allow_null=True + ) + start = serializers.CharField(required=False, allow_null=True) + graded = serializers.BooleanField(required=False, allow_null=True) + due_date = serializers.CharField(required=False, allow_null=True) + due = serializers.JSONField(required=False, allow_null=True) + relative_weeks_due = serializers.JSONField(required=False, allow_null=True) + format = serializers.JSONField(required=False, allow_null=True) + course_graders = serializers.ListField(required=False, allow_null=True) + actions = serializers.DictField(required=False, allow_null=True) + explanatory_message = serializers.Field(required=False, allow_null=True) + group_access = serializers.DictField(required=False, allow_null=True) + user_partitions = serializers.ListField(required=False, allow_null=True) + show_correctness = serializers.CharField(required=False, allow_null=True) + discussion_enabled = serializers.BooleanField(required=False, allow_null=True) + ancestor_has_staff_lock = serializers.BooleanField(required=False, allow_null=True) + user_partition_info = serializers.DictField(required=False, allow_null=True) + summary_configuration_enabled = serializers.JSONField(required=False, allow_null=True) + isPrereq = serializers.BooleanField(required=False, allow_null=True) + prereqUsageKey = serializers.CharField(required=False, allow_null=True) + prereqMinScore = serializers.IntegerField(required=False, allow_null=True) + prereqMinCompletion = serializers.IntegerField(required=False, allow_null=True) + publish = serializers.ChoiceField( + required=False, + allow_null=True, + choices=['make_public', 'republish', 'discard_changes'] + ) + duplicate_source_locator = serializers.CharField(required=False, allow_null=True) + move_source_locator = serializers.CharField(required=False, allow_null=True) + target_index = serializers.IntegerField(required=False, allow_null=True) + boilerplate = serializers.JSONField(required=False, allow_null=True) + staged_content = serializers.CharField(required=False, allow_null=True) diff --git a/cms/djangoapps/contentstore/rest_api/v1/urls.py b/cms/djangoapps/contentstore/rest_api/v1/urls.py index 286424f5e1fd..9c1bef0c90de 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/urls.py +++ b/cms/djangoapps/contentstore/rest_api/v1/urls.py @@ -74,7 +74,7 @@ videos.VideosView.as_view(), name='studio_content_videos_uploads' ), re_path( - fr'^videos/images/{settings.COURSE_ID_PATTERN}/{VIDEO_ID_PATTERN}?$', + fr'^videos/images/{settings.COURSE_ID_PATTERN}/{VIDEO_ID_PATTERN}$', videos.VideoImagesView.as_view(), name='studio_content_videos_images' ), re_path( diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/assets.py b/cms/djangoapps/contentstore/rest_api/v1/views/assets.py index 3508020a85b4..481d850ca83d 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/assets.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/assets.py @@ -1,5 +1,5 @@ """ -Public rest API endpoints for the Studio Content API Assets. +Public rest API endpoints for the CMS API Assets. """ import logging from rest_framework.generics import RetrieveUpdateDestroyAPIView, CreateAPIView @@ -14,6 +14,11 @@ from cms.djangoapps.contentstore.asset_storage_handlers import handle_assets import cms.djangoapps.contentstore.toggles as contentstore_toggles +from cms.djangoapps.contentstore.rest_api.v1.serializers import AssetSerializer +from .utils import validate_request_with_serializer +from rest_framework.parsers import (MultiPartParser, FormParser, JSONParser) +from openedx.core.lib.api.parsers import TypedFileUploadParser + log = logging.getLogger(__name__) toggles = contentstore_toggles @@ -21,10 +26,12 @@ @view_auth_classes() class AssetsView(DeveloperErrorViewMixin, RetrieveUpdateDestroyAPIView, CreateAPIView): """ - public rest API endpoints for the Studio Content API Assets. + public rest API endpoints for the CMS API Assets. course_key: required argument, needed to authorize course authors and identify the asset. asset_key_string: required argument, needed to identify the asset. """ + serializer_class = AssetSerializer + parser_classes = (JSONParser, MultiPartParser, FormParser, TypedFileUploadParser) def dispatch(self, request, *args, **kwargs): # TODO: probably want to refactor this to a decorator. @@ -44,11 +51,13 @@ def retrieve(self, request, course_key): # pylint: disable=arguments-differ @csrf_exempt @course_author_access_required + @validate_request_with_serializer def create(self, request, course_key): # pylint: disable=arguments-differ return handle_assets(request, course_key.html_id()) @course_author_access_required @expect_json_in_class_view + @validate_request_with_serializer def update(self, request, course_key, asset_key_string): # pylint: disable=arguments-differ return handle_assets(request, course_key.html_id(), asset_key_string) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_assets.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_assets.py index 9d1bee5fd727..61fa331d6cf4 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_assets.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_assets.py @@ -1,14 +1,16 @@ """ -Tests for the xblock view of the Studio Content API. This tests only the view itself, +Tests for the xblock view of the CMS API. This tests only the view itself, not the underlying Xblock service. It checks that the assets_handler method of the Xblock service is called with the expected parameters. """ from unittest.mock import patch +from django.core.files import File from django.http import JsonResponse from django.urls import reverse +from mock import MagicMock from rest_framework import status from rest_framework.test import APITestCase from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -17,6 +19,8 @@ ASSET_KEY_STRING = "asset-v1:dede+aba+weagi+type@asset+block@_0e37192a-42c4-441e-a3e1-8e40ec304e2e.jpg" +mock_image = MagicMock(file=File) +mock_image.name = "test.jpg" class AssetsViewTestCase(AuthorizeStaffTestCase): @@ -146,7 +150,7 @@ def get_url_params(self): def get_test_data(self): return { - "file": ASSET_KEY_STRING, + "file": mock_image, } def assert_assets_handler_called(self, *, mock_handle_assets, response): @@ -159,7 +163,7 @@ def assert_assets_handler_called(self, *, mock_handle_assets, response): course_id = self.get_course_key_string() - assert passed_args.data.get("file") == ASSET_KEY_STRING + assert passed_args.data.get("file").name == mock_image.name assert passed_args.method == "POST" assert passed_args.path == self.get_url() diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_xblock.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_xblock.py index 688a6cc0cd64..64462a6ee218 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_xblock.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_xblock.py @@ -1,5 +1,5 @@ """ -Tests for the xblock view of the Studio Content API. This tests only the view itself, +Tests for the xblock view of the CMS API. This tests only the view itself, not the underlying Xblock service. It checks that the xblock_handler method of the Xblock service is called with the expected parameters. """ @@ -50,7 +50,7 @@ def send_request(self, _url, _data): return_value=JsonResponse( { "locator": TEST_LOCATOR, - "courseKey": AuthorizeStaffTestCase.get_course_key_string(), + "course_key": AuthorizeStaffTestCase.get_course_key_string(), } ), ) @@ -128,7 +128,6 @@ def test_xblock_handler_called_with_correct_arguments(self): assert response.status_code == status.HTTP_200_OK data = response.json() assert data["locator"] == TEST_LOCATOR - assert data["courseKey"] == self.get_course_key_string() class XBlockViewPostTest(XBlockViewTestCase, ModuleStoreTestCase, APITestCase): @@ -150,7 +149,6 @@ def get_test_data(self): return { "parent_locator": course_id, "category": "html", - "courseKey": course_id, } def assert_xblock_handler_called(self, *, mock_handle_xblock, response): @@ -161,9 +159,6 @@ def assert_xblock_handler_called(self, *, mock_handle_xblock, response): mock_handle_xblock.assert_called_once() passed_args = mock_handle_xblock.call_args[0][0] - course_id = self.get_course_key_string() - - assert passed_args.data.get("courseKey") == course_id assert passed_args.method == "POST" assert passed_args.path == self.get_url() @@ -187,7 +182,6 @@ def test_xblock_handler_called_with_correct_arguments(self): assert response.status_code == status.HTTP_200_OK data = response.json() assert data["locator"] == TEST_LOCATOR - assert data["courseKey"] == self.get_course_key_string() class XBlockViewPutTest(XBlockViewTestCase, ModuleStoreTestCase, APITestCase): @@ -196,10 +190,8 @@ class XBlockViewPutTest(XBlockViewTestCase, ModuleStoreTestCase, APITestCase): """ def get_test_data(self): - course_id = self.get_course_key_string() return { "category": "html", - "courseKey": course_id, "data": "

Updated block!

", "has_changes": True, "id": TEST_LOCATOR, @@ -216,9 +208,6 @@ def assert_xblock_handler_called(self, *, mock_handle_xblock, response): mock_handle_xblock.assert_called_once() passed_args = mock_handle_xblock.call_args[0][0] - course_id = self.get_course_key_string() - - assert passed_args.data.get("courseKey") == course_id assert passed_args.data.get("data") == "

Updated block!

" assert passed_args.data.get("id") == TEST_LOCATOR assert passed_args.method == "PUT" @@ -244,7 +233,6 @@ def test_xblock_handler_called_with_correct_arguments(self): assert response.status_code == status.HTTP_200_OK data = response.json() assert data["locator"] == TEST_LOCATOR - assert data["courseKey"] == self.get_course_key_string() class XBlockViewPatchTest(XBlockViewTestCase, ModuleStoreTestCase, APITestCase): @@ -253,10 +241,8 @@ class XBlockViewPatchTest(XBlockViewTestCase, ModuleStoreTestCase, APITestCase): """ def get_test_data(self): - course_id = self.get_course_key_string() return { "category": "html", - "courseKey": course_id, "data": "

Patched block!

", "has_changes": True, "id": TEST_LOCATOR, @@ -273,9 +259,6 @@ def assert_xblock_handler_called(self, *, mock_handle_xblock, response): mock_handle_xblock.assert_called_once() passed_args = mock_handle_xblock.call_args[0][0] - course_id = self.get_course_key_string() - - assert passed_args.data.get("courseKey") == course_id assert passed_args.data.get("data") == "

Patched block!

" assert passed_args.data.get("id") == TEST_LOCATOR assert passed_args.method == "PATCH" @@ -301,7 +284,6 @@ def test_xblock_handler_called_with_correct_arguments(self): assert response.status_code == status.HTTP_200_OK data = response.json() assert data["locator"] == TEST_LOCATOR - assert data["courseKey"] == self.get_course_key_string() class XBlockViewDeleteTest(XBlockViewTestCase, ModuleStoreTestCase, APITestCase): @@ -343,4 +325,3 @@ def test_xblock_handler_called_with_correct_arguments(self): assert response.status_code == status.HTTP_200_OK data = response.json() assert data["locator"] == TEST_LOCATOR - assert data["courseKey"] == self.get_course_key_string() diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/transcripts.py b/cms/djangoapps/contentstore/rest_api/v1/views/transcripts.py index af23a81fb867..f621929f9cc7 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/transcripts.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/transcripts.py @@ -1,5 +1,5 @@ """ -Public rest API endpoints for the Studio Content API video assets. +Public rest API endpoints for the CMS API video assets. """ import logging from rest_framework.generics import ( @@ -21,6 +21,11 @@ handle_transcript_download, ) import cms.djangoapps.contentstore.toggles as contentstore_toggles +from cms.djangoapps.contentstore.rest_api.v1.serializers import TranscriptSerializer +from rest_framework.parsers import (MultiPartParser, FormParser) +from openedx.core.lib.api.parsers import TypedFileUploadParser + +from .utils import validate_request_with_serializer log = logging.getLogger(__name__) toggles = contentstore_toggles @@ -29,11 +34,13 @@ @view_auth_classes() class TranscriptView(DeveloperErrorViewMixin, CreateAPIView, RetrieveAPIView, DestroyAPIView): """ - public rest API endpoints for the Studio Content API video transcripts. + public rest API endpoints for the CMS API video transcripts. course_key: required argument, needed to authorize course authors and identify the video. edx_video_id: optional query parameter, needed to identify the transcript. language_code: optional query parameter, needed to identify the transcript. """ + serializer_class = TranscriptSerializer + parser_classes = (MultiPartParser, FormParser, TypedFileUploadParser) def dispatch(self, request, *args, **kwargs): if not toggles.use_studio_content_api(): @@ -43,6 +50,7 @@ def dispatch(self, request, *args, **kwargs): @csrf_exempt @course_author_access_required @expect_json_in_class_view + @validate_request_with_serializer def create(self, request, course_key_string): # pylint: disable=arguments-differ return upload_transcript(request) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/utils.py b/cms/djangoapps/contentstore/rest_api/v1/views/utils.py new file mode 100644 index 000000000000..7175b76c7c26 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/utils.py @@ -0,0 +1,23 @@ +""" +Utilities for the REST API views. +""" +from functools import wraps +from django.http import HttpResponseBadRequest + + +def validate_request_with_serializer(view_func): + """ + A decorator to validate request data using the view's serializer. + + Usage: + @validate_request_with_serializer + def my_view_function(self, request, ...): + ... + """ + @wraps(view_func) + def _wrapped_view(instance, request, *args, **kwargs): + serializer = instance.serializer_class(data=request.data) + if not serializer.is_valid(): + return HttpResponseBadRequest(reason=serializer.errors) + return view_func(instance, request, *args, **kwargs) + return _wrapped_view diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/videos.py b/cms/djangoapps/contentstore/rest_api/v1/views/videos.py index 46282b2eb61d..710dbb5df204 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/videos.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/videos.py @@ -1,5 +1,5 @@ """ -Public rest API endpoints for the Studio Content API video assets. +Public rest API endpoints for the CMS API video assets. """ import logging from rest_framework.generics import ( @@ -7,10 +7,12 @@ RetrieveAPIView, DestroyAPIView ) +from rest_framework.parsers import (MultiPartParser, FormParser) from django.views.decorators.csrf import csrf_exempt from django.http import Http404 from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes +from openedx.core.lib.api.parsers import TypedFileUploadParser from common.djangoapps.util.json_request import expect_json_in_class_view from ....api import course_author_access_required @@ -19,10 +21,12 @@ handle_videos, get_video_encodings_download, handle_video_images, - enabled_video_features, - handle_generate_video_upload_link + enabled_video_features ) +from cms.djangoapps.contentstore.rest_api.v1.serializers import VideoUploadSerializer, VideoImageSerializer import cms.djangoapps.contentstore.toggles as contentstore_toggles +from .utils import validate_request_with_serializer + log = logging.getLogger(__name__) toggles = contentstore_toggles @@ -31,10 +35,11 @@ @view_auth_classes() class VideosView(DeveloperErrorViewMixin, CreateAPIView, RetrieveAPIView, DestroyAPIView): """ - public rest API endpoints for the Studio Content API video assets. + public rest API endpoints for the CMS API video assets. course_key: required argument, needed to authorize course authors and identify the video. video_id: required argument, needed to identify the video. """ + serializer_class = VideoUploadSerializer def dispatch(self, request, *args, **kwargs): # TODO: probably want to refactor this to a decorator. @@ -50,7 +55,9 @@ def dispatch(self, request, *args, **kwargs): @csrf_exempt @course_author_access_required @expect_json_in_class_view + @validate_request_with_serializer def create(self, request, course_key): # pylint: disable=arguments-differ + """Deprecated. Use the upload_link endpoint instead.""" return handle_videos(request, course_key.html_id()) @course_author_access_required @@ -70,6 +77,8 @@ class VideoImagesView(DeveloperErrorViewMixin, CreateAPIView): course_key: required argument, needed to authorize course authors and identify the video. video_id: required argument, needed to identify the video. """ + serializer_class = VideoImageSerializer + parser_classes = (MultiPartParser, FormParser, TypedFileUploadParser) def dispatch(self, request, *args, **kwargs): # TODO: probably want to refactor this to a decorator. @@ -85,6 +94,7 @@ def dispatch(self, request, *args, **kwargs): @csrf_exempt @course_author_access_required @expect_json_in_class_view + @validate_request_with_serializer def create(self, request, course_key, edx_video_id=None): # pylint: disable=arguments-differ return handle_video_images(request, course_key.html_id(), edx_video_id) @@ -140,6 +150,7 @@ class UploadLinkView(DeveloperErrorViewMixin, CreateAPIView): """ public rest API endpoint providing a list of enabled video features. """ + serializer_class = VideoUploadSerializer def dispatch(self, request, *args, **kwargs): # TODO: probably want to refactor this to a decorator. @@ -155,5 +166,6 @@ def dispatch(self, request, *args, **kwargs): @csrf_exempt @course_author_access_required @expect_json_in_class_view + @validate_request_with_serializer def create(self, request, course_key): # pylint: disable=arguments-differ - return handle_generate_video_upload_link(request, course_key.html_id()) + return handle_videos(request, course_key.html_id()) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py b/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py index 2da54811d4b3..0e5765d8a2e2 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py @@ -1,5 +1,5 @@ """ -Public rest API endpoints for the Studio Content API. +Public rest API endpoints for the CMS API. """ import logging from rest_framework.generics import RetrieveUpdateDestroyAPIView, CreateAPIView @@ -9,11 +9,14 @@ from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes from common.djangoapps.util.json_request import expect_json_in_class_view -from ....api import course_author_access_required - +from cms.djangoapps.contentstore.api import course_author_access_required from cms.djangoapps.contentstore.xblock_storage_handlers import view_handlers import cms.djangoapps.contentstore.toggles as contentstore_toggles +from cms.djangoapps.contentstore.rest_api.v1.serializers import XblockSerializer +from .utils import validate_request_with_serializer + + log = logging.getLogger(__name__) toggles = contentstore_toggles handle_xblock = view_handlers.handle_xblock @@ -22,11 +25,12 @@ @view_auth_classes() class XblockView(DeveloperErrorViewMixin, RetrieveUpdateDestroyAPIView, CreateAPIView): """ - Public rest API endpoints for the Studio Content API. + Public rest API endpoints for the CMS API. course_key: required argument, needed to authorize course authors. usage_key_string (optional): xblock identifier, for example in the form of "block-v1:+type@+block@" """ + serializer_class = XblockSerializer def dispatch(self, request, *args, **kwargs): # TODO: probably want to refactor this to a decorator. @@ -47,11 +51,13 @@ def retrieve(self, request, course_key, usage_key_string=None): @course_author_access_required @expect_json_in_class_view + @validate_request_with_serializer def update(self, request, course_key, usage_key_string=None): return handle_xblock(request, usage_key_string) @course_author_access_required @expect_json_in_class_view + @validate_request_with_serializer def partial_update(self, request, course_key, usage_key_string=None): return handle_xblock(request, usage_key_string) @@ -63,5 +69,6 @@ def destroy(self, request, course_key, usage_key_string=None): @csrf_exempt @course_author_access_required @expect_json_in_class_view + @validate_request_with_serializer def create(self, request, course_key, usage_key_string=None): return handle_xblock(request, usage_key_string) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 9162598645b9..1eb70347399c 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1406,11 +1406,16 @@ def test_course_overview_view_with_course(self): self.assertEqual(resp.status_code, 404) return + assets_url = reverse_course_url( + 'assets_handler', + course.location.course_key + ) self.assertContains( resp, - '
'.format( # lint-amnesty, pylint: disable=line-too-long + '
'.format( # lint-amnesty, pylint: disable=line-too-long locator=str(course.location), course_key=str(course.id), + assets_url=assets_url, ), status_code=200, html=True diff --git a/cms/djangoapps/contentstore/tests/test_video_utils.py b/cms/djangoapps/contentstore/tests/test_video_utils.py index 5ba7384dba2b..c81761a283f4 100644 --- a/cms/djangoapps/contentstore/tests/test_video_utils.py +++ b/cms/djangoapps/contentstore/tests/test_video_utils.py @@ -5,7 +5,7 @@ from datetime import datetime from unittest import TestCase -from unittest.mock import MagicMock, patch +from unittest import mock import ddt import pytz @@ -144,7 +144,7 @@ def mocked_youtube_thumbnail_response( return mocked_response @override_settings(AWS_ACCESS_KEY_ID='test_key_id', AWS_SECRET_ACCESS_KEY='test_secret') - @patch('requests.get') + @mock.patch('requests.get') @ddt.data( ( { @@ -228,7 +228,7 @@ def mocked_youtube_thumbnail_responses(resolutions): self.assertEqual(thumbnail_content_type, 'image/jpeg') @override_settings(AWS_ACCESS_KEY_ID='test_key_id', AWS_SECRET_ACCESS_KEY='test_secret') - @patch('requests.get') + @mock.patch('requests.get') def test_scrape_youtube_thumbnail(self, mocked_request): """ Test that youtube thumbnails are correctly scrapped. @@ -273,8 +273,8 @@ def test_scrape_youtube_thumbnail(self, mocked_request): ) ) @override_settings(AWS_ACCESS_KEY_ID='test_key_id', AWS_SECRET_ACCESS_KEY='test_secret') - @patch('cms.djangoapps.contentstore.video_utils.LOGGER') - @patch('requests.get') + @mock.patch('cms.djangoapps.contentstore.video_utils.LOGGER') + @mock.patch('requests.get') @ddt.unpack def test_scrape_youtube_thumbnail_logging( self, @@ -333,8 +333,8 @@ def test_scrape_youtube_thumbnail_logging( ) ), ) - @patch('cms.djangoapps.contentstore.video_utils.LOGGER') - @patch('cms.djangoapps.contentstore.video_utils.download_youtube_video_thumbnail') + @mock.patch('cms.djangoapps.contentstore.video_utils.LOGGER') + @mock.patch('cms.djangoapps.contentstore.video_utils.download_youtube_video_thumbnail') @ddt.unpack def test_no_video_thumbnail_downloaded( self, @@ -376,7 +376,16 @@ class S3Boto3TestCase(TestCase): def setUp(self): self.storage = S3Boto3Storage() - self.storage._connections.connection = MagicMock() # pylint: disable=protected-access + self.storage._connections.connection = mock.MagicMock() # pylint: disable=protected-access + + def order_dict(self, dictionary): + """ + sorting dict key:values for tests cases. + """ + sorted_key_values = sorted(dictionary.items()) + dictionary.clear() + dictionary.update(sorted_key_values) + return dictionary def test_video_backend(self): self.assertEqual( @@ -408,17 +417,18 @@ def test_storage_without_global_default_acl_setting(self): content = ContentFile('new content') storage = S3Boto3Storage(**{'bucket_name': 'test'}) - storage._connections.connection = MagicMock() # pylint: disable=protected-access + storage._connections.connection = mock.MagicMock() # pylint: disable=protected-access storage.save(name, content) storage.bucket.Object.assert_called_once_with(name) obj = storage.bucket.Object.return_value obj.upload_fileobj.assert_called_with( - content, - ExtraArgs={ + mock.ANY, + ExtraArgs=self.order_dict({ 'ContentType': 'text/plain', - } + }), + Config=storage.transfer_config # pylint: disable=protected-access ) @override_settings(AWS_DEFAULT_ACL='public-read') @@ -435,7 +445,7 @@ def test_storage_without_global_default_acl_setting_and_bucket_acls(self, defaul name = 'test_storage_save.txt' content = ContentFile('new content') storage = S3Boto3Storage(**{'bucket_name': 'test', 'default_acl': default_acl}) - storage._connections.connection = MagicMock() # pylint: disable=protected-access + storage._connections.connection = mock.MagicMock() # pylint: disable=protected-access storage.save(name, content) storage.bucket.Object.assert_called_once_with(name) @@ -451,8 +461,9 @@ def test_storage_without_global_default_acl_setting_and_bucket_acls(self, defaul del ExtraArgs['ACL'] obj.upload_fileobj.assert_called_with( - content, - ExtraArgs=ExtraArgs + mock.ANY, + ExtraArgs=self.order_dict(ExtraArgs), + Config=storage.transfer_config # pylint: disable=protected-access ) @ddt.data('public-read', 'private') @@ -465,15 +476,16 @@ def test_storage_passing_default_acl_as_none(self, input_acl): content = ContentFile('new content') storage = S3Boto3Storage(**{'bucket_name': 'test', 'default_acl': None}) - storage._connections.connection = MagicMock() # pylint: disable=protected-access + storage._connections.connection = mock.MagicMock() # pylint: disable=protected-access storage.save(name, content) storage.bucket.Object.assert_called_once_with(name) obj = storage.bucket.Object.return_value obj.upload_fileobj.assert_called_with( - content, + mock.ANY, + Config=storage.transfer_config, # pylint: disable=protected-access ExtraArgs={ 'ContentType': 'text/plain', - } + }, ) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 97b70e828ce2..1a4b709622e6 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -1390,9 +1390,63 @@ def get_help_urls(): return help_tokens +def get_response_format(request): + return request.GET.get('format') or request.POST.get('format') or 'html' + + +def request_response_format_is_json(request, response_format): + return response_format == 'json' or 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json') + + +def get_library_context(request, request_is_json=False): + """ + Utils is used to get context of course home library tab. + It is used for both DRF and django views. + """ + from cms.djangoapps.contentstore.views.course import ( + get_allowed_organizations, + get_allowed_organizations_for_libraries, + user_can_create_organizations, + _accessible_libraries_iter, + _get_course_creator_status, + _format_library_for_view, + ) + from cms.djangoapps.contentstore.views.library import ( + LIBRARIES_ENABLED, + ) + + libraries = _accessible_libraries_iter(request.user) if LIBRARIES_ENABLED else [] + data = { + 'libraries': [_format_library_for_view(lib, request) for lib in libraries], + } + + if not request_is_json: + return { + **data, + 'in_process_course_actions': [], + 'courses': [], + 'libraries_enabled': LIBRARIES_ENABLED, + 'show_new_library_button': LIBRARIES_ENABLED and request.user.is_active, + 'user': request.user, + 'request_course_creator_url': reverse('request_course_creator'), + 'course_creator_status': _get_course_creator_status(request.user), + 'allow_unicode_course_id': settings.FEATURES.get('ALLOW_UNICODE_COURSE_ID', False), + 'archived_courses': True, + 'allow_course_reruns': settings.FEATURES.get('ALLOW_COURSE_RERUNS', True), + 'rerun_creator_status': GlobalStaff().has_user(request.user), + 'split_studio_home': split_library_view_on_dashboard(), + 'active_tab': 'libraries', + 'allowed_organizations_for_libraries': get_allowed_organizations_for_libraries(request.user), + 'allowed_organizations': get_allowed_organizations(request.user), + 'can_create_organizations': user_can_create_organizations(request.user), + } + + return data + + def get_home_context(request): """ - Utils is used to get context of course grading. + Utils is used to get context of course home. It is used for both DRF and django views. """ @@ -1420,8 +1474,14 @@ def get_home_context(request): courses_iter, in_process_course_actions = get_courses_accessible_to_user(request, org) user = request.user libraries = [] + response_format = get_response_format(request) + if not split_library_view_on_dashboard() and LIBRARIES_ENABLED: - libraries = _accessible_libraries_iter(request.user) + accessible_libraries = _accessible_libraries_iter(user) + libraries = [_format_library_for_view(lib, request) for lib in accessible_libraries] + + if split_library_view_on_dashboard() and request_response_format_is_json(request, response_format): + libraries = get_library_context(request, True)['libraries'] def format_in_process_course_view(uca): """ @@ -1456,7 +1516,7 @@ def format_in_process_course_view(uca): 'libraries_enabled': LIBRARIES_ENABLED, 'redirect_to_library_authoring_mfe': should_redirect_to_library_authoring_mfe(), 'library_authoring_mfe_url': LIBRARY_AUTHORING_MICROFRONTEND_URL, - 'libraries': [_format_library_for_view(lib, request) for lib in libraries], + 'libraries': libraries, 'show_new_library_button': user_can_create_library(user) and not should_redirect_to_library_authoring_mfe(), 'user': user, 'request_course_creator_url': reverse('request_course_creator'), diff --git a/cms/djangoapps/contentstore/video_storage_handlers.py b/cms/djangoapps/contentstore/video_storage_handlers.py index d1a7c55ac6f5..6e83a5b9e3cc 100644 --- a/cms/djangoapps/contentstore/video_storage_handlers.py +++ b/cms/djangoapps/contentstore/video_storage_handlers.py @@ -729,7 +729,9 @@ def videos_post(course, request): """ if use_mock_video_uploads(): - return {'files': [{'file_name': 'video.mp4', 'upload_url': 'http://example.com/put_video'}]}, 200 + return {'files': [{ + 'file_name': 'video.mp4', 'upload_url': 'http://example.com/put_video', 'edx_video_id': '1234' + }]}, 200 error = None data = request.json diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index 148b259898cc..3c31637d4c78 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -336,7 +336,7 @@ def create_support_legend_dict(): template_id = "peer-assessment" elif category == 'problem': # Override generic "Problem" name to describe this blank template: - display_name = _("Blank Advanced Problem") + display_name = _("Blank Problem") templates_for_category.append( create_template_dict(display_name, category, support_level_without_template, template_id, 'advanced') ) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index d74a99a8e391..4a70e5028ce3 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -89,7 +89,6 @@ from ..tasks import rerun_course as rerun_course_task from ..toggles import ( default_enable_flexible_peer_openassessments, - split_library_view_on_dashboard, use_new_course_outline_page, use_new_home_page, use_new_updates_page, @@ -102,6 +101,7 @@ get_course_settings, get_course_grading, get_home_context, + get_library_context, get_lms_link_for_item, get_proctored_exam_settings_url, get_course_outline_url, @@ -121,7 +121,6 @@ update_course_discussions_settings, ) from .component import ADVANCED_COMPONENT_TYPES -from .library import LIBRARIES_ENABLED log = logging.getLogger(__name__) User = get_user_model() @@ -551,26 +550,7 @@ def library_listing(request): """ List all Libraries available to the logged in user """ - libraries = _accessible_libraries_iter(request.user) if LIBRARIES_ENABLED else [] - data = { - 'in_process_course_actions': [], - 'courses': [], - 'libraries_enabled': LIBRARIES_ENABLED, - 'libraries': [_format_library_for_view(lib, request) for lib in libraries], - 'show_new_library_button': LIBRARIES_ENABLED and request.user.is_active, - 'user': request.user, - 'request_course_creator_url': reverse('request_course_creator'), - 'course_creator_status': _get_course_creator_status(request.user), - 'allow_unicode_course_id': settings.FEATURES.get('ALLOW_UNICODE_COURSE_ID', False), - 'archived_courses': True, - 'allow_course_reruns': settings.FEATURES.get('ALLOW_COURSE_RERUNS', True), - 'rerun_creator_status': GlobalStaff().has_user(request.user), - 'split_studio_home': split_library_view_on_dashboard(), - 'active_tab': 'libraries', - 'allowed_organizations': get_allowed_organizations(request.user), - 'allowed_organizations_for_libraries': get_allowed_organizations_for_libraries(request.user), - 'can_create_organizations': user_can_create_organizations(request.user), - } + data = get_library_context(request) return render_to_response('index.html', data) diff --git a/cms/djangoapps/contentstore/views/tests/test_block.py b/cms/djangoapps/contentstore/views/tests/test_block.py index ce5771ef83e8..26b3f91a0bd7 100644 --- a/cms/djangoapps/contentstore/views/tests/test_block.py +++ b/cms/djangoapps/contentstore/views/tests/test_block.py @@ -2675,10 +2675,7 @@ def setUp(self): XBlockStudioConfiguration.objects.create( name="openassessment", enabled=True, support_level="us" ) - # Library Sourced Block and Library Content block has it's own category. - XBlockStudioConfiguration.objects.create( - name="library_sourced", enabled=True, support_level="fs" - ) + # Library Content block has its own category. XBlockStudioConfiguration.objects.create( name="library_content", enabled=True, support_level="fs" ) @@ -2779,7 +2776,7 @@ def test_basic_components_support_levels(self): self._verify_basic_component("video", "Video", "us") problem_templates = self.get_templates_of_type("problem") problem_no_boilerplate = self.get_template( - problem_templates, "Blank Advanced Problem" + problem_templates, "Blank Problem" ) self.assertIsNotNone(problem_no_boilerplate) self.assertEqual("us", problem_no_boilerplate["support_level"]) diff --git a/cms/djangoapps/contentstore/views/tests/test_container_page.py b/cms/djangoapps/contentstore/views/tests/test_container_page.py index d3bf988d2a45..b78c0cce8347 100644 --- a/cms/djangoapps/contentstore/views/tests/test_container_page.py +++ b/cms/djangoapps/contentstore/views/tests/test_container_page.py @@ -9,6 +9,7 @@ from django.http import Http404 from django.test.client import RequestFactory +from django.urls import reverse from pytz import UTC from urllib.parse import quote @@ -57,11 +58,16 @@ def setUp(self): self.store.publish(self.vertical.location, self.user.id) def test_container_html(self): + assets_url = reverse( + 'assets_handler', kwargs={'course_key_string': str(self.child_container.location.course_key)} + ) self._test_html_content( self.child_container, expected_section_tag=( '