From 663feedca71f9334b7db7dad54cd3173f5a30e4a Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 15 Feb 2024 16:03:55 -0500 Subject: [PATCH 1/4] feat: Upgrade Python dependency ora2 (#34244) Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master` Co-authored-by: jansenk Co-authored-by: Jansen Kantor --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 05b032acad0e..0a74f59aa7c9 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -791,7 +791,7 @@ optimizely-sdk==4.1.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/bundled.in -ora2==6.0.33 +ora2==6.0.34 # via -r requirements/edx/bundled.in packaging==23.2 # via diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index c7d3690ac890..1901fcbd7e43 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1323,7 +1323,7 @@ optimizely-sdk==4.1.1 # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt -ora2==6.0.33 +ora2==6.0.34 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 68a8b04ff879..7e288539d4ea 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -931,7 +931,7 @@ optimizely-sdk==4.1.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt -ora2==6.0.33 +ora2==6.0.34 # via -r requirements/edx/base.txt packaging==23.2 # via diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 42ff33ec6afc..47b9be3f82fd 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -989,7 +989,7 @@ optimizely-sdk==4.1.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt -ora2==6.0.33 +ora2==6.0.34 # via -r requirements/edx/base.txt packaging==23.2 # via From 95b3e88ba576886bb29a18a0e64b2fca0ea9642b Mon Sep 17 00:00:00 2001 From: Michael Roytman Date: Fri, 16 Feb 2024 08:48:29 -0500 Subject: [PATCH 2/4] temp: add supplemental logging to debug IDV issues (#34248) This commit adds some supplemental, more verbose logging to the results_callback view in the verify_student Djangoapp. This endpoint is called by identity verification (IDV) providers to POST an IDV review to edX. We are experiencing issues with receiving IDV reviews from our IDV provider, and these logs will help us diagnose whether there is an issue in edX's systems. These logs will be removed after our investigation is complete. --- lms/djangoapps/verify_student/views.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/lms/djangoapps/verify_student/views.py b/lms/djangoapps/verify_student/views.py index 99cf7004992d..06e287d7314b 100644 --- a/lms/djangoapps/verify_student/views.py +++ b/lms/djangoapps/verify_student/views.py @@ -1072,6 +1072,18 @@ def results_callback(request): # lint-amnesty, pylint: disable=too-many-stateme reason = body_dict.get("Reason", "") error_code = body_dict.get("MessageType", "") + # TODO: These logs must be removed once the investigation in COSMO-184 is complete. + # COSMO-196 was created to track the cleanup of these logs. + log.info( + "[COSMO-184] Software Secure review received for receipt_id={receipt_id}, " + "with result={result} and reason={reason}." + .format( + receipt_id=receipt_id, + result=result, + reason=reason, + ) + ) + try: attempt = SoftwareSecurePhotoVerification.objects.get(receipt_id=receipt_id) except SoftwareSecurePhotoVerification.DoesNotExist: @@ -1099,6 +1111,10 @@ def results_callback(request): # lint-amnesty, pylint: disable=too-many-stateme SoftwareSecurePhotoVerification.objects.filter(pk=previous_verification.pk ).update(expiry_email_date=None) log.debug(f'Approving verification for {receipt_id}') + + # TODO: These logs must be removed once the investigation in COSMO-184 is complete. + # COSMO-196 was created to track the cleanup of these logs. + log.info("[COSMO-184] Approved verification for receipt_id={receipt_id}.".format(receipt_id=receipt_id)) attempt.approve() expiration_datetime = attempt.expiration_datetime.date() @@ -1121,6 +1137,11 @@ def results_callback(request): # lint-amnesty, pylint: disable=too-many-stateme elif result == "FAIL": log.debug("Denying verification for %s", receipt_id) + + # TODO: These logs must be removed once the investigation in COSMO-184 is complete. + # COSMO-196 was created to track the cleanup of these logs. + log.info("[COSMO-184] Denied verification for receipt_id={receipt_id}.".format(receipt_id=receipt_id)) + attempt.deny(json.dumps(reason), error_code=error_code) reverify_url = f'{settings.ACCOUNT_MICROFRONTEND_URL}/id-verification' verification_status_email_vars['reasons'] = reason From 4d1d82dd354328bdf5849c1def1d189de5e977bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 16 Feb 2024 14:56:30 -0300 Subject: [PATCH 3/4] feat: export all course tags as csv (#34091) --- .../core/djangoapps/content_tagging/api.py | 45 ++++- .../rest_api/v1/objecttag_export_helpers.py | 88 +++++++++ .../v1/tests/test_objecttag_export_helpers.py | 177 ++++++++++++++++++ .../rest_api/v1/tests/test_views.py | 94 ++++++++-- .../content_tagging/rest_api/v1/urls.py | 16 +- .../content_tagging/rest_api/v1/views.py | 97 +++++++++- .../content_tagging/tests/test_api.py | 130 ++++++++++++- .../core/djangoapps/content_tagging/types.py | 9 +- 8 files changed, 630 insertions(+), 26 deletions(-) create mode 100644 openedx/core/djangoapps/content_tagging/rest_api/v1/objecttag_export_helpers.py create mode 100644 openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_objecttag_export_helpers.py diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 0a9d2b1886e1..629b380f97d2 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -3,12 +3,16 @@ """ from __future__ import annotations +from itertools import groupby + import openedx_tagging.core.tagging.api as oel_tagging -from django.db.models import QuerySet, Exists, OuterRef -from openedx_tagging.core.tagging.models import Taxonomy +from django.db.models import Exists, OuterRef, Q, QuerySet +from opaque_keys.edx.keys import CourseKey, LearningContextKey +from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy from organizations.models import Organization from .models import TaxonomyOrg +from .types import ObjectTagByObjectIdDict, TaxonomyDict def create_taxonomy( @@ -126,6 +130,43 @@ def get_unassigned_taxonomies(enabled=True) -> QuerySet: ) +def get_all_object_tags( + content_key: LearningContextKey, +) -> tuple[ObjectTagByObjectIdDict, TaxonomyDict]: + """ + Returns a tuple with a dictionary of grouped object tags for all blocks and a dictionary of taxonomies. + """ + # ToDo: Add support for other content types (like LibraryContent and LibraryBlock) + if isinstance(content_key, CourseKey): + course_key_str = str(content_key) + # We use a block_id_prefix (i.e. the modified course id) to get the tags for the children of the Content + # (course) in a single db query. + block_id_prefix = course_key_str.replace("course-v1:", "block-v1:", 1) + else: + raise NotImplementedError(f"Invalid content_key: {type(content_key)} -> {content_key}") + + # There is no API method in oel_tagging.api that does this yet, + # so for now we have to build the ORM query directly. + all_object_tags = list(ObjectTag.objects.filter( + Q(object_id__startswith=block_id_prefix) | Q(object_id=course_key_str), + Q(tag__isnull=False, tag__taxonomy__isnull=False), + ).select_related("tag__taxonomy")) + + grouped_object_tags: ObjectTagByObjectIdDict = {} + taxonomies: TaxonomyDict = {} + + for object_id, block_tags in groupby(all_object_tags, lambda x: x.object_id): + grouped_object_tags[object_id] = {} + for taxonomy_id, taxonomy_tags in groupby(block_tags, lambda x: x.tag.taxonomy_id): + object_tags_list = list(taxonomy_tags) + grouped_object_tags[object_id][taxonomy_id] = object_tags_list + + if taxonomy_id not in taxonomies: + taxonomies[taxonomy_id] = object_tags_list[0].tag.taxonomy + + return grouped_object_tags, taxonomies + + # Expose the oel_tagging APIs get_taxonomy = oel_tagging.get_taxonomy diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/objecttag_export_helpers.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/objecttag_export_helpers.py new file mode 100644 index 000000000000..14103642d8ec --- /dev/null +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/objecttag_export_helpers.py @@ -0,0 +1,88 @@ +""" +This module contains helper functions to build a object tree with object tags. +""" + +from __future__ import annotations + +from typing import Iterator + +from attrs import define +from opaque_keys.edx.keys import CourseKey, LearningContextKey + +from xmodule.modulestore.django import modulestore + +from ...types import ObjectTagByObjectIdDict, ObjectTagByTaxonomyIdDict + + +@define +class TaggedContent: + """ + A tagged content, with its tags and children. + """ + display_name: str + block_id: str + category: str + object_tags: ObjectTagByTaxonomyIdDict + children: list[TaggedContent] | None + + +def iterate_with_level( + tagged_content: TaggedContent, level: int = 0 +) -> Iterator[tuple[TaggedContent, int]]: + """ + Iterator that yields the tagged content and the level of the block + """ + yield tagged_content, level + if tagged_content.children: + for child in tagged_content.children: + yield from iterate_with_level(child, level + 1) + + +def build_object_tree_with_objecttags( + content_key: LearningContextKey, + object_tag_cache: ObjectTagByObjectIdDict, +) -> TaggedContent: + """ + Returns the object with the tags associated with it. + """ + store = modulestore() + + if isinstance(content_key, CourseKey): + course = store.get_course(content_key) + if course is None: + raise ValueError(f"Course not found: {content_key}") + else: + raise NotImplementedError(f"Invalid content_key: {type(content_key)} -> {content_key}") + + display_name = course.display_name_with_default + course_id = str(course.id) + + tagged_course = TaggedContent( + display_name=display_name, + block_id=course_id, + category=course.category, + object_tags=object_tag_cache.get(str(content_key), {}), + children=None, + ) + + blocks = [(tagged_course, course)] + + while blocks: + tagged_block, xblock = blocks.pop() + tagged_block.children = [] + + if xblock.has_children: + for child_id in xblock.children: + child_block = store.get_item(child_id) + tagged_child = TaggedContent( + display_name=child_block.display_name_with_default, + block_id=str(child_id), + category=child_block.category, + object_tags=object_tag_cache.get(str(child_id), {}), + children=None, + ) + tagged_block.children.append(tagged_child) + + blocks.append((tagged_child, child_block)) + + return tagged_course diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_objecttag_export_helpers.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_objecttag_export_helpers.py new file mode 100644 index 000000000000..28d75f0fdfb6 --- /dev/null +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_objecttag_export_helpers.py @@ -0,0 +1,177 @@ +""" +Test the objecttag_export_helpers module +""" +from unittest.mock import patch + +from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, ModuleStoreTestCase +from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory + +from .... import api +from ....tests.test_api import TestGetAllObjectTagsMixin +from ..objecttag_export_helpers import TaggedContent, build_object_tree_with_objecttags, iterate_with_level + + +class TaggedCourseMixin(TestGetAllObjectTagsMixin, ModuleStoreTestCase): # type: ignore[misc] + """ + Mixin with a course structure and taxonomies + """ + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + CREATE_USER = False + + def setUp(self): + super().setUp() + + # Patch modulestore + self.patcher = patch("openedx.core.djangoapps.content_tagging.tasks.modulestore", return_value=self.store) + self.addCleanup(self.patcher.stop) + self.patcher.start() + + # Create course + self.course = CourseFactory.create( + org=self.orgA.short_name, + number="test_course", + run="test_run", + display_name="Test Course", + ) + self.expected_tagged_xblock = TaggedContent( + display_name="Test Course", + block_id="course-v1:orgA+test_course+test_run", + category="course", + children=[], + object_tags={ + self.taxonomy_1.id: list(self.course_tags), + }, + ) + + # Create XBlocks + self.sequential = BlockFactory.create( + parent=self.course, + category="sequential", + display_name="test sequential", + ) + # Tag blocks + tagged_sequential = TaggedContent( + display_name="test sequential", + block_id="block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential", + category="sequential", + children=[], + object_tags={ + self.taxonomy_1.id: list(self.sequential_tags1), + self.taxonomy_2.id: list(self.sequential_tags2), + }, + ) + + assert self.expected_tagged_xblock.children is not None # type guard + self.expected_tagged_xblock.children.append(tagged_sequential) + + # Untagged blocks + sequential2 = BlockFactory.create( + parent=self.course, + category="sequential", + display_name="untagged sequential", + ) + untagged_sequential = TaggedContent( + display_name="untagged sequential", + block_id="block-v1:orgA+test_course+test_run+type@sequential+block@untagged_sequential", + category="sequential", + children=[], + object_tags={}, + ) + assert self.expected_tagged_xblock.children is not None # type guard + self.expected_tagged_xblock.children.append(untagged_sequential) + BlockFactory.create( + parent=sequential2, + category="vertical", + display_name="untagged vertical", + ) + untagged_vertical = TaggedContent( + display_name="untagged vertical", + block_id="block-v1:orgA+test_course+test_run+type@vertical+block@untagged_vertical", + category="vertical", + children=[], + object_tags={}, + ) + assert untagged_sequential.children is not None # type guard + untagged_sequential.children.append(untagged_vertical) + # /Untagged blocks + + vertical = BlockFactory.create( + parent=self.sequential, + category="vertical", + display_name="test vertical1", + ) + tagged_vertical = TaggedContent( + display_name="test vertical1", + block_id="block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1", + category="vertical", + children=[], + object_tags={ + self.taxonomy_2.id: list(self.vertical1_tags), + }, + ) + assert tagged_sequential.children is not None # type guard + tagged_sequential.children.append(tagged_vertical) + + vertical2 = BlockFactory.create( + parent=self.sequential, + category="vertical", + display_name="test vertical2", + ) + untagged_vertical2 = TaggedContent( + display_name="test vertical2", + block_id="block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical2", + category="vertical", + children=[], + object_tags={}, + ) + assert tagged_sequential.children is not None # type guard + tagged_sequential.children.append(untagged_vertical2) + + html = BlockFactory.create( + parent=vertical2, + category="html", + display_name="test html", + ) + tagged_text = TaggedContent( + display_name="test html", + block_id="block-v1:orgA+test_course+test_run+type@html+block@test_html", + category="html", + children=[], + object_tags={ + self.taxonomy_2.id: list(self.html_tags), + }, + ) + assert untagged_vertical2.children is not None # type guard + untagged_vertical2.children.append(tagged_text) + + self.all_object_tags, _ = api.get_all_object_tags(self.course.id) + self.expected_tagged_content_list = [ + (self.expected_tagged_xblock, 0), + (tagged_sequential, 1), + (tagged_vertical, 2), + (untagged_vertical2, 2), + (tagged_text, 3), + (untagged_sequential, 1), + (untagged_vertical, 2), + ] + + +class TestContentTagChildrenExport(TaggedCourseMixin): # type: ignore[misc] + """ + Test helper functions for exporting tagged content + """ + def test_build_object_tree(self) -> None: + """ + Test if we can export a course + """ + with self.assertNumQueries(3): + tagged_xblock = build_object_tree_with_objecttags(self.course.id, self.all_object_tags) + + assert tagged_xblock == self.expected_tagged_xblock + + def test_iterate_with_level(self) -> None: + """ + Test if we can iterate over the tagged content in the correct order + """ + tagged_content_list = list(iterate_with_level(self.expected_tagged_xblock)) + assert tagged_content_list == self.expected_tagged_content_list diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index a9307fc07dbf..9b87f35e0483 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -4,11 +4,12 @@ from __future__ import annotations -from urllib.parse import parse_qs, urlparse +import abc import json +from io import BytesIO from unittest.mock import MagicMock +from urllib.parse import parse_qs, urlparse -import abc import ddt from django.contrib.auth import get_user_model from django.core.files.uploadedfile import SimpleUploadedFile @@ -27,24 +28,24 @@ OrgContentCreatorRole, OrgInstructorRole, OrgLibraryUserRole, - OrgStaffRole, -) -from openedx.core.djangoapps.content_libraries.api import ( - AccessLevel, - create_library, - set_library_user_permissions, + OrgStaffRole ) +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.content_libraries.api import AccessLevel, create_library, set_library_user_permissions from openedx.core.djangoapps.content_tagging import api as tagging_api from openedx.core.djangoapps.content_tagging.models import TaxonomyOrg from openedx.core.djangolib.testing.utils import skip_unless_cms from openedx.core.lib import blockstore_api +from .test_objecttag_export_helpers import TaggedCourseMixin + User = get_user_model() TAXONOMY_ORG_LIST_URL = "/api/content_tagging/v1/taxonomies/" TAXONOMY_ORG_DETAIL_URL = "/api/content_tagging/v1/taxonomies/{pk}/" TAXONOMY_ORG_UPDATE_ORG_URL = "/api/content_tagging/v1/taxonomies/{pk}/orgs/" OBJECT_TAG_UPDATE_URL = "/api/content_tagging/v1/object_tags/{object_id}/?taxonomy={taxonomy_id}" +OBJECT_TAGS_EXPORT_URL = "/api/content_tagging/v1/object_tags/{object_id}/export/" OBJECT_TAGS_URL = "/api/content_tagging/v1/object_tags/{object_id}/" TAXONOMY_TEMPLATE_URL = "/api/content_tagging/v1/taxonomies/import/{filename}" TAXONOMY_CREATE_IMPORT_URL = "/api/content_tagging/v1/taxonomies/import/" @@ -1782,6 +1783,77 @@ def test_object_tags_query_count(self): assert response.data[object_id]["taxonomies"][0]["tags"] == expected_tags +@skip_unless_cms +@ddt.ddt +class TestContentObjectChildrenExportView(TaggedCourseMixin, APITestCase): # type: ignore[misc] + """ + Tests exporting course children with tags + """ + def setUp(self): + super().setUp() + self.user = UserFactory.create() + self.staff = UserFactory.create( + username="staff", + email="staff@example.com", + is_staff=True, + ) + + self.staffA = UserFactory.create( + username="staffA", + email="userA@example.com", + ) + update_org_role(self.staff, OrgStaffRole, self.staffA, [self.orgA.short_name]) + + @ddt.data( + "staff", + "staffA", + ) + def test_export_course(self, user_attr) -> None: + url = OBJECT_TAGS_EXPORT_URL.format(object_id=str(self.course.id)) + + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + response = self.client.get(url) + assert response.status_code == status.HTTP_200_OK + assert response.headers['Content-Type'] == 'text/csv' + + expected_csv = ( + '"Name","Type","ID","1-taxonomy-1","2-taxonomy-2"\r\n' + '"Test Course","course","course-v1:orgA+test_course+test_run","Tag 1.1",""\r\n' + '" test sequential","sequential","block-v1:orgA+test_course+test_run+type@sequential+block@test_' + 'sequential","Tag 1.1, Tag 1.2","Tag 2.1"\r\n' + '" test vertical1","vertical","block-v1:orgA+test_course+test_run+type@vertical+block@test_' + 'vertical1","","Tag 2.2"\r\n' + '" test vertical2","vertical","block-v1:orgA+test_course+test_run+type@vertical+block@test_' + 'vertical2","",""\r\n' + '" test html","html","block-v1:orgA+test_course+test_run+type@html+block@test_html","","Tag 2.1"\r\n' + '" untagged sequential","sequential","block-v1:orgA+test_course+test_run+type@sequential+block@untagged_' + 'sequential","",""\r\n' + '" untagged vertical","vertical","block-v1:orgA+test_course+test_run+type@vertical+block@untagged_' + 'vertical","",""\r\n' + ) + + zip_content = BytesIO(b"".join(response.streaming_content)).getvalue() # type: ignore[attr-defined] + assert zip_content == expected_csv.encode() + + def test_export_course_anoymous_forbidden(self) -> None: + url = OBJECT_TAGS_EXPORT_URL.format(object_id=str(self.course.id)) + response = self.client.get(url) + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_export_course_user_forbidden(self) -> None: + url = OBJECT_TAGS_EXPORT_URL.format(object_id=str(self.course.id)) + self.client.force_authenticate(user=self.user) + response = self.client.get(url) + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_export_course_invalid_id(self) -> None: + url = OBJECT_TAGS_EXPORT_URL.format(object_id="invalid") + self.client.force_authenticate(user=self.staff) + response = self.client.get(url) + assert response.status_code == status.HTTP_400_BAD_REQUEST + + @skip_unless_cms @ddt.ddt class TestDownloadTemplateView(APITestCase): @@ -1793,7 +1865,7 @@ class TestDownloadTemplateView(APITestCase): ("template.json", "application/json"), ) @ddt.unpack - def test_download(self, filename, content_type): + def test_download(self, filename, content_type) -> None: url = TAXONOMY_TEMPLATE_URL.format(filename=filename) response = self.client.get(url) assert response.status_code == status.HTTP_200_OK @@ -1801,12 +1873,12 @@ def test_download(self, filename, content_type): assert response.headers['Content-Disposition'] == f'attachment; filename="{filename}"' assert int(response.headers['Content-Length']) > 0 - def test_download_not_found(self): + def test_download_not_found(self) -> None: url = TAXONOMY_TEMPLATE_URL.format(filename="template.txt") response = self.client.get(url) assert response.status_code == status.HTTP_404_NOT_FOUND - def test_download_method_not_allowed(self): + def test_download_method_not_allowed(self) -> None: url = TAXONOMY_TEMPLATE_URL.format(filename="template.txt") response = self.client.post(url) assert response.status_code == status.HTTP_405_METHOD_NOT_ALLOWED diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py index ad7fd8005c71..50fec093c1fb 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py @@ -2,15 +2,11 @@ Taxonomies API v1 URLs. """ -from rest_framework.routers import DefaultRouter +from django.urls.conf import include, path +from openedx_tagging.core.tagging.rest_api.v1 import views as oel_tagging_views +from openedx_tagging.core.tagging.rest_api.v1 import views_import as oel_tagging_views_import from openedx_tagging.core.tagging.rest_api.v1.views import ObjectTagCountsView - -from django.urls.conf import path, include - -from openedx_tagging.core.tagging.rest_api.v1 import ( - views as oel_tagging_views, - views_import as oel_tagging_views_import, -) +from rest_framework.routers import DefaultRouter from . import views @@ -30,5 +26,9 @@ oel_tagging_views_import.TemplateView.as_view(), name="taxonomy-import-template", ), + path( + "object_tags//export/", + views.ObjectTagExportView.as_view(), + ), path('', include(router.urls)) ] diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py index 151bc09f5d76..ca8343312bfd 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -1,6 +1,14 @@ """ Tagging Org API Views """ +from __future__ import annotations + +import csv +from typing import Iterator + +from django.http import StreamingHttpResponse +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey from openedx_tagging.core.tagging import rules as oel_tagging_rules from openedx_tagging.core.tagging.rest_api.v1.views import ObjectTagView, TaxonomyView from rest_framework import status @@ -8,18 +16,21 @@ from rest_framework.exceptions import PermissionDenied, ValidationError from rest_framework.request import Request from rest_framework.response import Response +from rest_framework.views import APIView from ...api import ( create_taxonomy, - get_taxonomy, + get_all_object_tags, get_taxonomies, get_taxonomies_for_org, + get_taxonomy, get_unassigned_taxonomies, - set_taxonomy_orgs, + set_taxonomy_orgs ) from ...rules import get_admin_orgs -from .serializers import TaxonomyOrgListQueryParamsSerializer, TaxonomyOrgSerializer, TaxonomyUpdateOrgBodySerializer from .filters import ObjectTagTaxonomyOrgFilterBackend, UserOrgFilterBackend +from .objecttag_export_helpers import build_object_tree_with_objecttags, iterate_with_level +from .serializers import TaxonomyOrgListQueryParamsSerializer, TaxonomyOrgSerializer, TaxonomyUpdateOrgBodySerializer class TaxonomyOrgView(TaxonomyView): @@ -135,3 +146,83 @@ class ObjectTagOrgView(ObjectTagView): Refer to ObjectTagView docstring for usage details. """ filter_backends = [ObjectTagTaxonomyOrgFilterBackend] + + +class ObjectTagExportView(APIView): + """" + View to export a CSV with all children and tags for a given course/context. + """ + def get(self, request: Request, **kwargs) -> StreamingHttpResponse: + """ + Export a CSV with all children and tags for a given course/context. + """ + + class Echo(object): + """ + Class that implements just the write method of the file-like interface, + used for the streaming response. + """ + def write(self, value): + return value + + def _generate_csv_rows() -> Iterator[str]: + """ + Receives the blocks, tags and taxonomies and returns a CSV string + """ + + header = {"name": "Name", "type": "Type", "id": "ID"} + + # Prepare the header for the taxonomies + for taxonomy_id, taxonomy in taxonomies.items(): + header[f"taxonomy_{taxonomy_id}"] = taxonomy.export_id + + csv_writer = csv.DictWriter(pseudo_buffer, fieldnames=header.keys(), quoting=csv.QUOTE_NONNUMERIC) + yield csv_writer.writerow(header) + + # Iterate over the blocks and yield the rows + for item, level in iterate_with_level(tagged_content): + block_data = { + "name": level * " " + item.display_name, + "type": item.category, + "id": item.block_id, + } + + # Add the tags for each taxonomy + for taxonomy_id in taxonomies: + if taxonomy_id in item.object_tags: + block_data[f"taxonomy_{taxonomy_id}"] = ", ".join([ + object_tag.value + for object_tag in item.object_tags[taxonomy_id] + ]) + + yield csv_writer.writerow(block_data) + + object_id: str = kwargs.get('context_id', None) + + try: + content_key = CourseKey.from_string(object_id) + except InvalidKeyError as e: + raise ValidationError("context_id is not a valid course key.") from e + + # Check if the user has permission to view object tags for this object_id + try: + if not self.request.user.has_perm( + "oel_tagging.view_objecttag", + # The obj arg expects a model, but we are passing an object + oel_tagging_rules.ObjectTagPermissionItem(taxonomy=None, object_id=object_id), # type: ignore[arg-type] + ): + raise PermissionDenied( + "You do not have permission to view object tags for this object_id." + ) + except ValueError as e: + raise ValidationError from e + + all_object_tags, taxonomies = get_all_object_tags(content_key) + tagged_content = build_object_tree_with_objecttags(content_key, all_object_tags) + pseudo_buffer = Echo() + + return StreamingHttpResponse( + streaming_content=_generate_csv_rows(), + content_type="text/csv", + headers={'Content-Disposition': f'attachment; filename="{object_id}_tags.csv"'}, + ) diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index 807b7d8e1dc6..37d40ad61c24 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -1,7 +1,8 @@ """Tests for the Tagging models""" import ddt from django.test.testcases import TestCase -from openedx_tagging.core.tagging.models import Tag +from opaque_keys.edx.keys import CourseKey +from openedx_tagging.core.tagging.models import ObjectTag, Tag from organizations.models import Organization from .. import api @@ -231,3 +232,130 @@ def test_get_tags(self): assert result[0]["_id"] == self.tag_all_orgs.id assert result[0]["parent_value"] is None assert result[0]["depth"] == 0 + + +class TestGetAllObjectTagsMixin: + """ + Set up data to test get_all_object_tags functions + """ + + def setUp(self): + super().setUp() + + self.orgA = Organization.objects.create(name="Organization A", short_name="orgA") + self.taxonomy_1 = api.create_taxonomy(name="Taxonomy 1") + api.set_taxonomy_orgs(self.taxonomy_1, all_orgs=True) + Tag.objects.create( + taxonomy=self.taxonomy_1, + value="Tag 1.1", + ) + Tag.objects.create( + taxonomy=self.taxonomy_1, + value="Tag 1.2", + ) + + self.taxonomy_2 = api.create_taxonomy(name="Taxonomy 2") + api.set_taxonomy_orgs(self.taxonomy_2, all_orgs=True) + + Tag.objects.create( + taxonomy=self.taxonomy_2, + value="Tag 2.1", + ) + Tag.objects.create( + taxonomy=self.taxonomy_2, + value="Tag 2.2", + ) + + api.tag_object( + object_id="course-v1:orgA+test_course+test_run", + taxonomy=self.taxonomy_1, + tags=['Tag 1.1'], + ) + self.course_tags = api.get_object_tags("course-v1:orgA+test_course+test_run") + + # Tag blocks + api.tag_object( + object_id="block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential", + taxonomy=self.taxonomy_1, + tags=['Tag 1.1', 'Tag 1.2'], + ) + self.sequential_tags1 = api.get_object_tags( + "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential", + taxonomy_id=self.taxonomy_1.id, + + ) + api.tag_object( + object_id="block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential", + taxonomy=self.taxonomy_2, + tags=['Tag 2.1'], + ) + self.sequential_tags2 = api.get_object_tags( + "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential", + taxonomy_id=self.taxonomy_2.id, + ) + api.tag_object( + object_id="block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1", + taxonomy=self.taxonomy_2, + tags=['Tag 2.2'], + ) + self.vertical1_tags = api.get_object_tags( + "block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1" + ) + api.tag_object( + object_id="block-v1:orgA+test_course+test_run+type@html+block@test_html", + taxonomy=self.taxonomy_2, + tags=['Tag 2.1'], + ) + self.html_tags = api.get_object_tags("block-v1:orgA+test_course+test_run+type@html+block@test_html") + + # Create "deleted" object tags, which will be omitted from the results. + for object_id in ( + "course-v1:orgA+test_course+test_run", + "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential", + "block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1", + "block-v1:orgA+test_course+test_run+type@html+block@test_html", + ): + ObjectTag.objects.create( + object_id=str(object_id), + taxonomy=None, + tag=None, + _value="deleted tag", + _name="deleted taxonomy", + ) + + self.expected_objecttags = { + "course-v1:orgA+test_course+test_run": { + self.taxonomy_1.id: list(self.course_tags), + }, + "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential": { + self.taxonomy_1.id: list(self.sequential_tags1), + self.taxonomy_2.id: list(self.sequential_tags2), + }, + "block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1": { + self.taxonomy_2.id: list(self.vertical1_tags), + }, + "block-v1:orgA+test_course+test_run+type@html+block@test_html": { + self.taxonomy_2.id: list(self.html_tags), + }, + } + + +class TestGetAllObjectTags(TestGetAllObjectTagsMixin, TestCase): + """ + Test get_all_object_tags api function + """ + + def test_get_all_object_tags(self): + """ + Test the get_all_object_tags function + """ + with self.assertNumQueries(1): + object_tags, taxonomies = api.get_all_object_tags( + CourseKey.from_string("course-v1:orgA+test_course+test_run") + ) + + assert object_tags == self.expected_objecttags + assert taxonomies == { + self.taxonomy_1.id: self.taxonomy_1, + self.taxonomy_2.id: self.taxonomy_2, + } diff --git a/openedx/core/djangoapps/content_tagging/types.py b/openedx/core/djangoapps/content_tagging/types.py index 44b3fd3e8a59..685df7b3afb3 100644 --- a/openedx/core/djangoapps/content_tagging/types.py +++ b/openedx/core/djangoapps/content_tagging/types.py @@ -1,9 +1,16 @@ """ Types used by content tagging API and implementation """ -from typing import Union +from __future__ import annotations + +from typing import Dict, List, Union from opaque_keys.edx.keys import CourseKey, UsageKey from opaque_keys.edx.locator import LibraryLocatorV2 +from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy ContentKey = Union[LibraryLocatorV2, CourseKey, UsageKey] + +ObjectTagByTaxonomyIdDict = Dict[int, List[ObjectTag]] +ObjectTagByObjectIdDict = Dict[str, ObjectTagByTaxonomyIdDict] +TaxonomyDict = Dict[int, Taxonomy] From 6353bb2e8ecfc081164e3d403e28c58fac310e76 Mon Sep 17 00:00:00 2001 From: Chris Pappas Date: Fri, 16 Feb 2024 13:12:40 -0500 Subject: [PATCH 4/4] feat: make FA form error messaging more descript (#34247) * feat: make FA form error messaging more descript * chore: quality * fix: split up tests * fix: make test more specific * chore: fix comment typo * chore: fix comment typo --- .../views/financial_assistance_form_view.js | 15 ++++++++++++++- .../financial_assistance_form_view_spec.js | 13 +++++++++---- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/lms/static/js/financial-assistance/views/financial_assistance_form_view.js b/lms/static/js/financial-assistance/views/financial_assistance_form_view.js index 3ff0b2b9ff2f..63dfed8aee6c 100644 --- a/lms/static/js/financial-assistance/views/financial_assistance_form_view.js +++ b/lms/static/js/financial-assistance/views/financial_assistance_form_view.js @@ -28,7 +28,8 @@ return FormView.extend({ el: '.financial-assistance-wrapper', events: { - 'click .js-submit-form': 'submitForm' + 'click .js-submit-form': 'submitForm', + 'ajaxError': 'handleAjaxError' }, tpl: formViewTpl, fieldTpl: formFieldTpl, @@ -101,6 +102,12 @@ if (error.status === 0) { msg = gettext('An error has occurred. Check your Internet connection and try again.'); + } else if (error.status === 403) { + txt = [ + 'You must confirm your email to complete registration before applying for financial assistance.', + 'If you continue to have issues please contact support.' + ], + msg = gettext(txt.join(' ')); } this.errors = [HtmlUtils.joinHtml( @@ -155,6 +162,12 @@ }); } } + }, + + // this.model.save() makes an ajax call, which, when it errors, + // should have an error message displayed on the banner on the page + handleAjaxError: function (event, request, settings, thrownError) { + this.saveError(request); } }); } diff --git a/lms/static/js/spec/financial-assistance/financial_assistance_form_view_spec.js b/lms/static/js/spec/financial-assistance/financial_assistance_form_view_spec.js index 738dd272ef80..a65a7ab8de11 100644 --- a/lms/static/js/spec/financial-assistance/financial_assistance_form_view_spec.js +++ b/lms/static/js/spec/financial-assistance/financial_assistance_form_view_spec.js @@ -98,11 +98,11 @@ define([ expect(view.$('.js-success-message').length).toEqual(1); }; - failedSubmission = function() { + failedSubmission = function(statusCode) { expect(view.$('.js-success-message').length).toEqual(0); expect(view.$formFeedback.find('.' + view.formErrorsJsHook).length).toEqual(0); validSubmission(); - view.model.trigger('error', {status: 500}); + view.model.trigger('error', {status: statusCode}); expect(view.$('.js-success-message').length).toEqual(0); expect(view.$formFeedback.find('.' + view.formErrorsJsHook).length).toEqual(1); }; @@ -166,7 +166,12 @@ define([ }); it('should submit the form and show an error message if content is valid and API returns error', function() { - failedSubmission(); + failedSubmission(500); + }); + + it('should submit the form and show an error message if content is valid and API returns 403 error', function() { + failedSubmission(403); + expect(view.$('.message-copy').text()).toContain('You must confirm your email'); }); it('should allow form resubmission after a front end validation failure', function() { @@ -176,7 +181,7 @@ define([ }); it('should allow form resubmission after an API error is returned', function() { - failedSubmission(); + failedSubmission(500); successfulSubmission(); });