From 3d265127594eba5960c648765e975301e4a3278b Mon Sep 17 00:00:00 2001 From: Erica Nwoga <66533300+chimsara@users.noreply.github.com> Date: Wed, 4 Oct 2023 10:36:18 -0400 Subject: [PATCH 1/3] feat: add python apis to agreements models (#32967) * feat: added python apis to agreements models * feat: add python apis to agreements app * feat: updated pyton apis and rest api * feat: fixed python apis and added tests * test: Added error handling and updated test functions * style: fixed test formatting * style: fixed test formatting * style: fixed test formatting * feat: Fixed error handling for python APIs * feat: Fixed error handling for python APIs * fix: edited python api for agreements app * style: quality revisions for python apis --- openedx/core/djangoapps/agreements/api.py | 172 ++++++++++++++++++ openedx/core/djangoapps/agreements/data.py | 23 +++ .../djangoapps/agreements/tests/test_api.py | 88 +++++++++ 3 files changed, 283 insertions(+) create mode 100644 openedx/core/djangoapps/agreements/data.py diff --git a/openedx/core/djangoapps/agreements/api.py b/openedx/core/djangoapps/agreements/api.py index 7b95607de6..2489cefb85 100644 --- a/openedx/core/djangoapps/agreements/api.py +++ b/openedx/core/djangoapps/agreements/api.py @@ -9,6 +9,11 @@ from django.core.exceptions import ObjectDoesNotExist from opaque_keys.edx.keys import CourseKey from openedx.core.djangoapps.agreements.models import IntegritySignature +from openedx.core.djangoapps.agreements.models import LTIPIITool +from openedx.core.djangoapps.agreements.models import LTIPIISignature + +from .data import LTIToolsReceivingPIIData +from .data import LTIPIISignatureData log = logging.getLogger(__name__) User = get_user_model() @@ -68,3 +73,170 @@ def get_integrity_signatures_for_course(course_id): """ course_key = CourseKey.from_string(course_id) return IntegritySignature.objects.filter(course_key=course_key) + + +def create_lti_pii_signature(username, course_id, lti_tools): + """ + Creates an lti pii tool signature. If the signature already exist, do not create a new one. + + Arguments: + * course_key (str) + * lti_tools (dict) + * lti_tools_hash (int) + Returns: + * An LTIPIISignature, or None if a signature already exists. + """ + course_key = CourseKey.from_string(course_id) + lti_tools_hash = hash(str(lti_tools)) + + # if user and course exists, update, otherwise create a new signature + try: + user = User.objects.get(username=username) + LTIPIISignature.objects.get(user=user, course_key=course_key) + except User.DoesNotExist: + return None + except LTIPIISignature.DoesNotExist: + signature = LTIPIISignature.objects.create( + user=user, + course_key=course_key, + lti_tools=lti_tools, + lti_tools_hash=lti_tools_hash) + else: + signature = LTIPIISignature.objects.update( + user=user, + course_key=course_key, + lti_tools=lti_tools, + lti_tools_hash=lti_tools_hash) + + return signature + + +def get_lti_pii_signature(username, course_id): + """ + Get the lti pii signature of a user in a course. + + Arguments: + * username (str) + * course_id (str) + + Returns: + * An LTIPIISignature object, or None if one does not exist for the + user + course combination. + """ + course_key = CourseKey.from_string(course_id) + try: + user = User.objects.get(username=username) + signature = LTIPIISignature.objects.get(user=user, course_key=course_key) + except (User.DoesNotExist, LTIPIISignature.DoesNotExist): + return None + else: + return LTIPIISignatureData(user=signature.user, course_id=str(signature.course_key), + lti_tools=signature.lti_tools, lti_tools_hash=signature.lti_tools_hash) + + +def get_pii_receiving_lti_tools(course_id): + """ + Get a course's LTI tools that share PII. + + Arguments: + * course_id (str) + + Returns: + * A List of LTI tools sharing PII. + """ + + course_key = CourseKey.from_string(course_id) + try: + course_ltipiitools = LTIPIITool.objects.get(course_key=course_key).lti_tools + except LTIPIITool.DoesNotExist: + return None + + return LTIToolsReceivingPIIData(lii_tools_receiving_pii=course_ltipiitools) + + +def user_lti_pii_signature_needed(username, course_id): + """ + Determines if a user needs to acknowledge the LTI PII Agreement. + + Arguments: + * username (str) + + Returns: + * True if the user needs to sign a new acknowledgement. + * False if the acknowledgements are up to date. + """ + course_has_lti_pii_tools = _course_has_lti_pii_tools(course_id) + signature_exists = _user_lti_pii_signature_exists(username, course_id) + signature_out_of_date = _user_signature_out_of_date(username, course_id) + + return ((course_has_lti_pii_tools and (not signature_exists)) or + (course_has_lti_pii_tools and signature_exists and signature_out_of_date)) + + +def _course_has_lti_pii_tools(course_id): + """ + Determines if a specifc course has lti tools sharing pii. + + Arguments: + * course_id (str) + + Returns: + * True if the course does have a list. + * False if the course does not. + """ + course_key = CourseKey.from_string(course_id) + try: + course_lti_pii_tools = LTIPIITool.objects.get(course_key=course_key) + except LTIPIITool.DoesNotExist: + # no entry in the database + return False + else: + # returns True if there are entries, and False if the list is empty + return bool(course_lti_pii_tools.lti_tools) + + +def _user_lti_pii_signature_exists(username, course_id): + """ + Determines if a user's lti pii signature exists for a specfic course + + Arguments: + * username (str) + * course_id (str) + + Returns: + * True if user has a signature for the given course. + * False if the user does not have a signature for the given course. + """ + course_key = CourseKey.from_string(course_id) + + try: + user = User.objects.get(username=username) + LTIPIISignature.objects.get(user=user, course_key=course_key) + except (User.DoesNotExist, LTIPIISignature.DoesNotExist): + return False + else: + return True # signature exist + + +def _user_signature_out_of_date(username, course_id): + """ + Determines if a user's existing lti pii signature is out-of-date for a given course. + + Arguments: + * username (str) + * course_id (str) + + Returns: + * True if signature is out-of-date and needs a new signature. + * False if the user has an up-to-date signature. + """ + course_key = CourseKey.from_string(course_id) + + try: + user = User.objects.get(username=username) + user_lti_pii_signature_hash = LTIPIISignature.objects.get(course_key=course_key, user=user).lti_tools_hash + course_lti_pii_tools_hash = LTIPIITool.objects.get(course_key=course_key).lti_tools_hash + except (User.DoesNotExist, LTIPIISignature.DoesNotExist, LTIPIITool.DoesNotExist): + return False + else: + return user_lti_pii_signature_hash != course_lti_pii_tools_hash diff --git a/openedx/core/djangoapps/agreements/data.py b/openedx/core/djangoapps/agreements/data.py new file mode 100644 index 0000000000..9d843c73cb --- /dev/null +++ b/openedx/core/djangoapps/agreements/data.py @@ -0,0 +1,23 @@ +""" +Public data structures for this app. +""" +import attr + + +@attr.s(frozen=True, auto_attribs=True) +class LTIToolsReceivingPIIData: + """ + Class that stores data about the list of LTI tools sharing PII + """ + lii_tools_receiving_pii: {} + + +@attr.s(frozen=True, auto_attribs=True) +class LTIPIISignatureData: + """ + Class that stores an lti pii signature + """ + user: str + course_id: str + lti_tools: str + lti_tools_hash: str diff --git a/openedx/core/djangoapps/agreements/tests/test_api.py b/openedx/core/djangoapps/agreements/tests/test_api.py index 45b48f7b4c..c660657899 100644 --- a/openedx/core/djangoapps/agreements/tests/test_api.py +++ b/openedx/core/djangoapps/agreements/tests/test_api.py @@ -10,10 +10,17 @@ from openedx.core.djangoapps.agreements.api import ( create_integrity_signature, get_integrity_signature, get_integrity_signatures_for_course, + get_pii_receiving_lti_tools, + create_lti_pii_signature, + get_lti_pii_signature ) from openedx.core.djangolib.testing.utils import skip_unless_lms from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.tests.factories import CourseFactory # lint-amnesty, pylint: disable=wrong-import-order +from ..models import ( + LTIPIITool, +) +from opaque_keys.edx.keys import CourseKey LOGGER_NAME = "openedx.core.djangoapps.agreements.api" @@ -98,3 +105,84 @@ class TestIntegritySignatureApi(SharedModuleStoreTestCase): """ self.assertEqual(signature.user, self.user) self.assertEqual(signature.course_key, self.course.id) + + +@skip_unless_lms +class TestLTIPIISignatureApi(SharedModuleStoreTestCase): + """ + Tests for the lti pii signature API + """ + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.user = UserFactory() + cls.course = CourseFactory() + cls.course_id = str(cls.course.id) + cls.lti_tools = {"first_lti_tool": "This is the first tool", + "second_lti_tool": "This is the second tool", } + cls.lti_tools_2 = {"first_lti_tool": "This is the first lti tool", + "second_lti_tool": "This is the second tool", + "third_lti_tool": "This is the third tool", } + LTIPIITool.objects.create( + course_key=CourseKey.from_string(cls.course_id), + lti_tools=cls.lti_tools, + lti_tools_hash=11111, + ) + + def test_create_lti_pii_signature(self): + """ + Test to check if an lti pii signature is created from a course and its lti tools. + """ + signature = create_lti_pii_signature(self.user.username, self.course_id, self.lti_tools) + self._assert_lti_pii_signature(signature) + + def test_create_multiple_lti_pii_signature(self): + """ + Test that lti pii signatures are either created or updated + """ + + create_lti_pii_signature(self.user.username, self.course_id, self.lti_tools) # first signature + s1 = get_lti_pii_signature(self.user.username, self.course_id) # retrieve the database entry + create_lti_pii_signature(self.user.username, self.course_id, self.lti_tools_2) # signature with updated tools + s2 = get_lti_pii_signature(self.user.username, self.course_id) # retrieve the updated database entry + self.assertNotEqual(s1, s2) # the signatue retrieved from the database should be the updated version + + def _assert_lti_pii_signature(self, signature): + """ + Helper function to assert the returned lti pii signature has the correct + user and course key + """ + self.assertEqual(signature.user, self.user) + self.assertEqual(signature.course_key, self.course.id) + + +@skip_unless_lms +class TestLTIPIIToolsApi(SharedModuleStoreTestCase): + """ + Tests for the lti pii tool sharing API. To make sure the list of LTI tools can be retreived from the Model. + """ + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory() + cls.course_id = str(cls.course.id) + cls.lti_tools = {"first_lti_tool": "This is the first tool", + "second_lti_tool": "This is the second tool", } + LTIPIITool.objects.create( + course_key=CourseKey.from_string(cls.course_id), + lti_tools=cls.lti_tools, + lti_tools_hash=11111, + ) + + def test_get_pii_receiving_lti_tools(self): + """ + Test to check if a course's lti pii tools can be retrieved. + """ + data = get_pii_receiving_lti_tools(self.course_id) + self._assert_ltitools(data.lii_tools_receiving_pii) + + def _assert_ltitools(self, lti_list): + """ + Helper function to assert the returned list has the correct tools + """ + self.assertEqual(self.lti_tools, lti_list) From 5bc0a575b86f198a6fb59ae704aec9ee5567e0b3 Mon Sep 17 00:00:00 2001 From: Marcos Date: Thu, 28 Sep 2023 15:45:28 -0300 Subject: [PATCH 2/3] feat: Added Waffle Flags for Courseware Search feature --- .gitignore | 1 + lms/djangoapps/courseware/tests/test_views.py | 44 ++++++++++++++++++- lms/djangoapps/courseware/toggles.py | 20 +++++++++ lms/djangoapps/courseware/views/views.py | 18 ++++++-- lms/urls.py | 10 +++++ 5 files changed, 89 insertions(+), 4 deletions(-) diff --git a/.gitignore b/.gitignore index d6f1202bf4..1866979edc 100644 --- a/.gitignore +++ b/.gitignore @@ -136,6 +136,7 @@ build \#*\# .env/ openedx/core/djangoapps/django_comment_common/comment_client/python +openedx/core/djangoapps/cache_toolbox/__pycache__ autodeploy.properties .ws_migrations_complete dist diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index c7d7ce7b17..e71f3f07ec 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -30,6 +30,7 @@ from opaque_keys.edx.keys import CourseKey, UsageKey from pytz import UTC from openedx.core.djangoapps.waffle_utils.models import WaffleFlagCourseOverrideModel from rest_framework import status +from rest_framework.test import APIClient from web_fragments.fragment import Fragment from xblock.core import XBlock from xblock.fields import Scope, String @@ -76,7 +77,10 @@ from lms.djangoapps.courseware.block_render import get_block, handle_xblock_call from lms.djangoapps.courseware.tests.factories import StudentModuleFactory from lms.djangoapps.courseware.tests.helpers import MasqueradeMixin, get_expiration_banner_text, set_preview_mode from lms.djangoapps.courseware.testutils import RenderXBlockTestMixin -from lms.djangoapps.courseware.toggles import COURSEWARE_OPTIMIZED_RENDER_XBLOCK +from lms.djangoapps.courseware.toggles import ( + COURSEWARE_MICROFRONTEND_SEARCH_ENABLED, + COURSEWARE_OPTIMIZED_RENDER_XBLOCK, +) from lms.djangoapps.courseware.user_state_client import DjangoXBlockUserStateClient from lms.djangoapps.courseware.views.views import ( BasePublicVideoXBlockView, @@ -3683,3 +3687,41 @@ class TestPublicVideoXBlockEmbedView(TestBasePublicVideoXBlock): assert template == 'public_video_share_embed.html' assert context['fragment'] == fragment assert context['course'] == self.course + + +class TestCoursewareMFESearchAPI(SharedModuleStoreTestCase): + """ + Tests the endpoint to fetch the Courseware Search waffle flag enabled status. + """ + + def setUp(self): + super().setUp() + + self.course = CourseFactory.create() + + self.client = APIClient() + self.apiUrl = reverse('courseware_search_enabled_view', kwargs={'course_id': str(self.course.id)}) + + @override_waffle_flag(COURSEWARE_MICROFRONTEND_SEARCH_ENABLED, active=True) + def test_courseware_mfe_search_enabled(self): + """ + Getter to check if user is allowed to use Courseware Search. + """ + + response = self.client.get(self.apiUrl, content_type='application/json') + body = json.loads(response.content.decode('utf-8')) + + self.assertEqual(response.status_code, 200) + self.assertEqual(body, {'enabled': True}) + + @override_waffle_flag(COURSEWARE_MICROFRONTEND_SEARCH_ENABLED, active=False) + def test_is_mfe_search_disabled(self): + """ + Getter to check if user is allowed to use Courseware Search. + """ + + response = self.client.get(self.apiUrl, content_type='application/json') + body = json.loads(response.content.decode('utf-8')) + + self.assertEqual(response.status_code, 200) + self.assertEqual(body, {'enabled': False}) diff --git a/lms/djangoapps/courseware/toggles.py b/lms/djangoapps/courseware/toggles.py index 3e13755123..78cecac7f5 100644 --- a/lms/djangoapps/courseware/toggles.py +++ b/lms/djangoapps/courseware/toggles.py @@ -55,6 +55,19 @@ COURSEWARE_MICROFRONTEND_PROGRESS_MILESTONES_STREAK_CELEBRATION = CourseWaffleFl f'{WAFFLE_FLAG_NAMESPACE}.mfe_progress_milestones_streak_celebration', __name__ ) +# .. toggle_name: courseware.mfe_courseware_search +# .. toggle_implementation: WaffleFlag +# .. toggle_default: False +# .. toggle_description: Enables Courseware Search on Learning MFE +# .. toggle_use_cases: temporary +# .. toggle_creation_date: 2023-09-28 +# .. toggle_target_removal_date: None +# .. toggle_tickets: KBK-20 +# .. toggle_warning: None. +COURSEWARE_MICROFRONTEND_SEARCH_ENABLED = CourseWaffleFlag( + f'{WAFFLE_FLAG_NAMESPACE}.mfe_courseware_search', __name__ +) + # .. toggle_name: courseware.mfe_progress_milestones_streak_discount_enabled # .. toggle_implementation: CourseWaffleFlag # .. toggle_default: False @@ -153,3 +166,10 @@ def course_is_invitation_only(courselike) -> bool: def learning_assistant_is_active(course_key): return COURSEWARE_LEARNING_ASSISTANT.is_enabled(course_key) + + +def courseware_mfe_search_is_enabled(course_key=None): + """ + Return whether the courseware.mfe_courseware_search flag is on. + """ + return COURSEWARE_MICROFRONTEND_SEARCH_ENABLED.is_enabled(course_key) diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 085e9e5a19..fdc90a46a5 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -18,8 +18,8 @@ from django.contrib.auth.models import AnonymousUser, User # lint-amnesty, pyli from django.core.exceptions import PermissionDenied from django.db import transaction from django.db.models import Q, prefetch_related_objects -from django.http import Http404, HttpResponse, HttpResponseBadRequest, HttpResponseForbidden from django.shortcuts import redirect +from django.http import JsonResponse, Http404, HttpResponse, HttpResponseBadRequest, HttpResponseForbidden from django.template.context_processors import csrf from django.urls import reverse from django.utils.decorators import method_decorator @@ -38,8 +38,8 @@ from markupsafe import escape from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey from openedx_filters.learning.filters import CourseAboutRenderStarted -from pytz import UTC from requests.exceptions import ConnectionError, Timeout # pylint: disable=redefined-builtin +from pytz import UTC from rest_framework import status from rest_framework.decorators import api_view, throttle_classes from rest_framework.response import Response @@ -87,7 +87,7 @@ from lms.djangoapps.courseware.masquerade import is_masquerading_as_specific_stu from lms.djangoapps.courseware.model_data import FieldDataCache from lms.djangoapps.courseware.models import BaseStudentModuleHistory, StudentModule from lms.djangoapps.courseware.permissions import MASQUERADE_AS_STUDENT, VIEW_COURSE_HOME, VIEW_COURSEWARE -from lms.djangoapps.courseware.toggles import course_is_invitation_only +from lms.djangoapps.courseware.toggles import course_is_invitation_only, courseware_mfe_search_is_enabled from lms.djangoapps.courseware.user_state_client import DjangoXBlockUserStateClient from lms.djangoapps.courseware.utils import ( _use_new_financial_assistance_flow, @@ -2275,3 +2275,15 @@ def get_learner_username(learner_identifier): learner = User.objects.filter(Q(username=learner_identifier) | Q(email=learner_identifier)).first() if learner: return learner.username + + +@api_view(['GET']) +def courseware_mfe_search_enabled(request, course_id=None): + """ + Simple GET endpoint to expose whether the course may use Courseware Search. + """ + + course_key = CourseKey.from_string(course_id) if course_id else None + + payload = {"enabled": courseware_mfe_search_is_enabled(course_key)} + return JsonResponse(payload) diff --git a/lms/urls.py b/lms/urls.py index dc673054df..8b314f9f1e 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -752,6 +752,16 @@ urlpatterns += [ ), ] +urlpatterns += [ + re_path( + r'^courses/{}/courseware-search/enabled/$'.format( + settings.COURSE_ID_PATTERN, + ), + courseware_views.courseware_mfe_search_enabled, + name='courseware_search_enabled_view', + ), +] + urlpatterns += [ re_path( r'^courses/{}/lti_tab/(?P[^/]+)/$'.format( From c1163717dfc4f4a65db8f081e2e02eaa4d155721 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Wed, 4 Oct 2023 13:34:16 -0400 Subject: [PATCH 3/3] build: log `npm run postinstall` steps to STDOUT so they are visible in CI (#33142) Without this change, `npm run postinstall` (aka scripts/copy-node-modules.sh) uses `set -x`, which echo steps to STDERR. By default, it seems that GitHub Actions doesn't show STDERR. Having the steps visible in CI was very useful while debugging some Tutor build improvements, so I figured it would be good to upstream the change. --- scripts/copy-node-modules.sh | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/scripts/copy-node-modules.sh b/scripts/copy-node-modules.sh index db997da957..f49514b6de 100755 --- a/scripts/copy-node-modules.sh +++ b/scripts/copy-node-modules.sh @@ -26,28 +26,33 @@ log ( ) { echo -e "${COL_LOG}$* $COL_OFF" } +# Print a command (prefixed with '+') and then run it, to aid in debugging. +# This is just like `set -x`, except that `set -x` prints to STDERR, which is hidden +# by GitHub Actions logs. This functions prints to STDOUT, which is visible. +log_and_run ( ) { + log "+$*" + "$@" # Joins arguments to form a command (quoting as necessary) and runs the command. +} + log "=====================================================================================" log "Copying required assets from node_modules..." log "-------------------------------------------------------------------------------" -# Start echoing all commands back to user for ease of debugging. -set -x - log "Ensuring vendor directories exist..." -mkdir -p "$vendor_js" -mkdir -p "$vendor_css" +log_and_run mkdir -p "$vendor_js" +log_and_run mkdir -p "$vendor_css" log "Copying studio-frontend JS & CSS from node_modules into vendor directores..." while read -r -d $'\0' src_file ; do if [[ "$src_file" = *.css ]] || [[ "$src_file" = *.css.map ]] ; then - cp --force "$src_file" "$vendor_css" + log_and_run cp --force "$src_file" "$vendor_css" else - cp --force "$src_file" "$vendor_js" + log_and_run cp --force "$src_file" "$vendor_js" fi done < <(find "$node_modules/@edx/studio-frontend/dist" -type f -print0) log "Copying certain JS modules from node_modules into vendor directory..." -cp --force \ +log_and_run cp --force \ "$node_modules/backbone.paginator/lib/backbone.paginator.js" \ "$node_modules/backbone/backbone.js" \ "$node_modules/bootstrap/dist/js/bootstrap.bundle.js" \ @@ -66,8 +71,8 @@ cp --force \ log "Copying certain JS developer modules into vendor directory..." if [[ "${NODE_ENV:-production}" = development ]] ; then - cp --force "$node_modules/sinon/pkg/sinon.js" "$vendor_js" - cp --force "$node_modules/squirejs/src/Squire.js" "$vendor_js" + log_and_run cp --force "$node_modules/sinon/pkg/sinon.js" "$vendor_js" + log_and_run cp --force "$node_modules/squirejs/src/Squire.js" "$vendor_js" else # TODO: https://github.com/openedx/edx-platform/issues/31768 # In the old implementation of this scipt (pavelib/assets.py), these two @@ -77,13 +82,10 @@ else # However, in the future, it would be good to only copy them for dev # builds. Furthermore, these libraries should not be `npm install`ed # into prod builds in the first place. - cp --force "$node_modules/sinon/pkg/sinon.js" "$vendor_js" || true # "|| true" means "tolerate errors"; in this case, - cp --force "$node_modules/squirejs/src/Squire.js" "$vendor_js" || true # that's "tolerate if these files don't exist." + log_and_run cp --force "$node_modules/sinon/pkg/sinon.js" "$vendor_js" || true # "|| true" means "tolerate errors"; in this case, + log_and_run cp --force "$node_modules/squirejs/src/Squire.js" "$vendor_js" || true # that's "tolerate if these files don't exist." fi -# Done echoing. -set +x - log "-------------------------------------------------------------------------------" log " Done copying required assets from node_modules." log "====================================================================================="