From efc04e3399af3cfc69603fb0d9902a372a6427ee Mon Sep 17 00:00:00 2001 From: Jeremy Bowman Date: Thu, 29 Oct 2020 11:17:23 -0400 Subject: [PATCH] Remove notifier_api app DEPR-111 (#25464) Part of the notifier service deprecation (DEPR-106). Also removed pdfminer from the package uninstall list, since we no longer install the package it conflicts with either. --- lms/djangoapps/discussion/README.rst | 2 +- .../discussion/notifier_api/README.rst | 1 - .../discussion/notifier_api/__init__.py | 0 .../discussion/notifier_api/serializers.py | 70 ----- .../discussion/notifier_api/tests.py | 259 ------------------ .../discussion/notifier_api/urls.py | 16 -- .../discussion/notifier_api/views.py | 55 ---- lms/urls.py | 3 - pavelib/prereqs.py | 1 - 9 files changed, 1 insertion(+), 406 deletions(-) delete mode 100644 lms/djangoapps/discussion/notifier_api/README.rst delete mode 100644 lms/djangoapps/discussion/notifier_api/__init__.py delete mode 100644 lms/djangoapps/discussion/notifier_api/serializers.py delete mode 100644 lms/djangoapps/discussion/notifier_api/tests.py delete mode 100644 lms/djangoapps/discussion/notifier_api/urls.py delete mode 100644 lms/djangoapps/discussion/notifier_api/views.py diff --git a/lms/djangoapps/discussion/README.rst b/lms/djangoapps/discussion/README.rst index 12f472c794..c15e9d7696 100644 --- a/lms/djangoapps/discussion/README.rst +++ b/lms/djangoapps/discussion/README.rst @@ -12,7 +12,7 @@ Discussions related functionality is scattered across a number of places and sho * ``openedx/core/djangoapps/django_comment_common`` * ``openedx/core/lib/xblock_builtin/xblock_discussion`` -Ideally, what we want in the long term is for all of this extracted into a new repository that holds the code for both the inline discussion XBlock as well as all the Django apps. Use of the notifier API should be replaced with edx-ace. +Ideally, what we want in the long term is for all of this extracted into a new repository that holds the code for both the inline discussion XBlock as well as all the Django apps. That being said, it's not clear what the path forward for this app is. Forum functionality is something that has not been actively worked on for a while, and it's been suggested that we should remove this app altogether in favor of having better integration with other more established forum software. This decision is usually complicated by the desire to have tight integration with courseware and access to data for analytics. diff --git a/lms/djangoapps/discussion/notifier_api/README.rst b/lms/djangoapps/discussion/notifier_api/README.rst deleted file mode 100644 index 80e9568232..0000000000 --- a/lms/djangoapps/discussion/notifier_api/README.rst +++ /dev/null @@ -1 +0,0 @@ -See ``lms/djangoapps/discussion/README.rst`` diff --git a/lms/djangoapps/discussion/notifier_api/__init__.py b/lms/djangoapps/discussion/notifier_api/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/lms/djangoapps/discussion/notifier_api/serializers.py b/lms/djangoapps/discussion/notifier_api/serializers.py deleted file mode 100644 index 4d1dbbb5be..0000000000 --- a/lms/djangoapps/discussion/notifier_api/serializers.py +++ /dev/null @@ -1,70 +0,0 @@ -import six -from django.contrib.auth.models import User -from django.http import Http404 -from rest_framework import serializers - -from lms.djangoapps.discussion.notification_prefs import NOTIFICATION_PREF_KEY -from openedx.core.djangoapps.course_groups.cohorts import is_course_cohorted -from openedx.core.djangoapps.lang_pref import LANGUAGE_KEY - - -class NotifierUserSerializer(serializers.ModelSerializer): - """ - A serializer containing all information about a user needed by the notifier - (namely the user's name, email address, notification and language - preferences, and course enrollment and cohort information). - - Because these pieces of information reside in different tables, this is - designed to work well with prefetch_related and select_related, which - require the use of all() instead of get() or filter(). The following fields - should be prefetched on the user objects being serialized: - * profile - * preferences - * courseenrollment_set - * course_groups - * roles__permissions - """ - name = serializers.SerializerMethodField() - preferences = serializers.SerializerMethodField() - course_info = serializers.SerializerMethodField() - - def get_name(self, user): - return user.profile.name - - def get_preferences(self, user): - return { - pref.key: pref.value - for pref - in user.preferences.all() - if pref.key in [LANGUAGE_KEY, NOTIFICATION_PREF_KEY] - } - - def get_course_info(self, user): - cohort_id_map = { - cohort.course_id: cohort.id - for cohort in user.course_groups.all() - } - see_all_cohorts_set = { - role.course_id - for role in user.roles.all() - for perm in role.permissions.all() if perm.name == "see_all_cohorts" - } - ret = {} - for enrollment in user.courseenrollment_set.all(): - if enrollment.is_active: - try: - ret[six.text_type(enrollment.course_id)] = { - "cohort_id": cohort_id_map.get(enrollment.course_id), - "see_all_cohorts": ( - enrollment.course_id in see_all_cohorts_set or - not is_course_cohorted(enrollment.course_id) - ), - } - except Http404: # is_course_cohorted raises this if course does not exist - pass - return ret - - class Meta(object): - model = User - fields = ("id", "email", "name", "preferences", "course_info") - read_only_fields = ("id", "email") diff --git a/lms/djangoapps/discussion/notifier_api/tests.py b/lms/djangoapps/discussion/notifier_api/tests.py deleted file mode 100644 index be1dca8d89..0000000000 --- a/lms/djangoapps/discussion/notifier_api/tests.py +++ /dev/null @@ -1,259 +0,0 @@ -# pylint: disable=missing-docstring - - -import itertools - -import ddt -import six -from six.moves import range -from django.conf import settings -from django.test.client import RequestFactory -from django.test.utils import override_settings -from opaque_keys.edx.locator import CourseLocator - -from lms.djangoapps.discussion.notification_prefs import NOTIFICATION_PREF_KEY -from lms.djangoapps.discussion.notifier_api.views import NotifierUsersViewSet -from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory -from openedx.core.djangoapps.django_comment_common.models import Permission, Role -from openedx.core.djangoapps.lang_pref import LANGUAGE_KEY -from openedx.core.djangoapps.user_api.models import UserPreference -from openedx.core.djangoapps.user_api.tests.factories import UserPreferenceFactory -from student.models import CourseEnrollment -from student.tests.factories import CourseEnrollmentFactory, UserFactory -from util.testing import UrlResetMixin -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -from xmodule.modulestore.tests.factories import CourseFactory - - -@ddt.ddt -@override_settings(EDX_API_KEY="test_api_key") -class NotifierUsersViewSetTest(UrlResetMixin, ModuleStoreTestCase): - def setUp(self): - super(NotifierUsersViewSetTest, self).setUp() - self.courses = [] - self.cohorts = [] - self.user = UserFactory() - self.notification_pref = UserPreferenceFactory( - user=self.user, - key=NOTIFICATION_PREF_KEY, - value="notification pref test value" - ) - - self.list_view = NotifierUsersViewSet.as_view({"get": "list"}) - self.detail_view = NotifierUsersViewSet.as_view({"get": "retrieve"}) - - def _set_up_course(self, is_course_cohorted, is_user_cohorted, is_moderator): - cohort_config = {"cohorted": True} if is_course_cohorted else {} - course = CourseFactory( - number=("TestCourse{}".format(len(self.courses))), - cohort_config=cohort_config - ) - self.courses.append(course) - CourseEnrollmentFactory(user=self.user, course_id=course.id) - if is_user_cohorted: - cohort = CohortFactory.create( - name="Test Cohort", - course_id=course.id, - users=[self.user] - ) - self.cohorts.append(cohort) - if is_moderator: - moderator_perm, _ = Permission.objects.get_or_create(name="see_all_cohorts") - moderator_role = Role.objects.create(name="Moderator", course_id=course.id) - moderator_role.permissions.add(moderator_perm) - self.user.roles.add(moderator_role) - - def _assert_basic_user_info_correct(self, user, result_user): - self.assertEqual(result_user["id"], user.id) - self.assertEqual(result_user["email"], user.email) - self.assertEqual(result_user["name"], user.profile.name) - - def test_without_api_key(self): - request = RequestFactory().get("dummy") - for view in [self.list_view, self.detail_view]: - response = view(request) - self.assertEqual(response.status_code, 403) - - # Detail view tests - - def _make_detail_request(self): - request = RequestFactory().get("dummy", HTTP_X_EDX_API_KEY=settings.EDX_API_KEY) - return self.detail_view( - request, - **{NotifierUsersViewSet.lookup_field: str(self.user.id)} - ) - - def _get_detail(self): - response = self._make_detail_request() - self.assertEqual(response.status_code, 200) - self.assertEqual( - set(response.data.keys()), - {"id", "email", "name", "preferences", "course_info"} - ) - return response.data - - def test_detail_invalid_user(self): - UserPreference.objects.all().delete() - response = self._make_detail_request() - self.assertEqual(response.status_code, 404) - - def test_basic_user_info(self): - result = self._get_detail() - self._assert_basic_user_info_correct(self.user, result) - - def test_course_info(self): - expected_course_info = {} - for is_course_cohorted, is_user_cohorted, is_moderator in ( - itertools.product([True, False], [True, False], [True, False]) - ): - self._set_up_course(is_course_cohorted, is_user_cohorted, is_moderator) - expected_course_info[six.text_type(self.courses[-1].id)] = { - "cohort_id": self.cohorts[-1].id if is_user_cohorted else None, - "see_all_cohorts": is_moderator or not is_course_cohorted - } - result = self._get_detail() - self.assertEqual(result["course_info"], expected_course_info) - - def test_course_info_unenrolled(self): - self._set_up_course(False, False, False) - course_id = self.courses[0].id - CourseEnrollment.unenroll(self.user, course_id) - result = self._get_detail() - self.assertNotIn(six.text_type(course_id), result["course_info"]) - - def test_course_info_no_enrollments(self): - result = self._get_detail() - self.assertEqual(result["course_info"], {}) - - def test_course_info_non_existent_course_enrollment(self): - CourseEnrollmentFactory( - user=self.user, - course_id=CourseLocator(org="dummy", course="dummy", run="non_existent") - ) - result = self._get_detail() - self.assertEqual(result["course_info"], {}) - - def test_preferences(self): - lang_pref = UserPreferenceFactory( - user=self.user, - key=LANGUAGE_KEY, - value="language pref test value" - ) - UserPreferenceFactory(user=self.user, key="non_included_key") - result = self._get_detail() - self.assertEqual( - result["preferences"], - { - NOTIFICATION_PREF_KEY: self.notification_pref.value, - LANGUAGE_KEY: lang_pref.value, - } - ) - - # List view tests - - def _make_list_request(self, page, page_size): - request = RequestFactory().get( - "dummy", - {"page": page, "page_size": page_size}, - HTTP_X_EDX_API_KEY=settings.EDX_API_KEY - ) - return self.list_view(request) - - def _get_list(self, page=1, page_size=''): - response = self._make_list_request(page, page_size) - self.assertEqual(response.status_code, 200) - self.assertEqual( - set(response.data.keys()), - {"count", "next", "previous", "results"} - ) - return response.data["results"] - - def test_no_users(self): - UserPreference.objects.all().delete() - results = self._get_list() - self.assertEqual(len(results), 0) - - def test_multiple_users(self): - other_user = UserFactory() - other_notification_pref = UserPreferenceFactory( - user=other_user, - key=NOTIFICATION_PREF_KEY, - value="other value" - ) - self._set_up_course(is_course_cohorted=True, is_user_cohorted=True, is_moderator=False) - self._set_up_course(is_course_cohorted=False, is_user_cohorted=False, is_moderator=False) - # Users have different sets of enrollments - CourseEnrollmentFactory(user=other_user, course_id=self.courses[0].id) - - result_map = {result["id"]: result for result in self._get_list()} - self.assertEqual(set(result_map.keys()), {self.user.id, other_user.id}) - for user in [self.user, other_user]: - self._assert_basic_user_info_correct(user, result_map[user.id]) - self.assertEqual( - result_map[self.user.id]["preferences"], - {NOTIFICATION_PREF_KEY: self.notification_pref.value} - ) - self.assertEqual( - result_map[other_user.id]["preferences"], - {NOTIFICATION_PREF_KEY: other_notification_pref.value} - ) - self.assertEqual( - result_map[self.user.id]["course_info"], - { - six.text_type(self.courses[0].id): { - "cohort_id": self.cohorts[0].id, - "see_all_cohorts": False, - }, - six.text_type(self.courses[1].id): { - "cohort_id": None, - "see_all_cohorts": True, - }, - } - ) - self.assertEqual( - result_map[other_user.id]["course_info"], - { - six.text_type(self.courses[0].id): { - "cohort_id": None, - "see_all_cohorts": False, - }, - } - ) - - @ddt.data( - 3, # Factor of num of results - 5, # Non-factor of num of results - 12, # Num of results - 15 # More than num of results - ) - def test_pagination(self, page_size): - num_users = 12 - users = [self.user] - while len(users) < num_users: - new_user = UserFactory() - users.append(new_user) - UserPreferenceFactory(user=new_user, key=NOTIFICATION_PREF_KEY) - - num_pages = (num_users - 1) // page_size + 1 - result_list = [] - for i in range(1, num_pages + 1): - result_list.extend(self._get_list(page=i, page_size=page_size)) - result_map = {result["id"]: result for result in result_list} - - self.assertEqual(len(result_list), num_users) - for user in users: - self._assert_basic_user_info_correct(user, result_map[user.id]) - self.assertEqual( - self._make_list_request(page=(num_pages + 1), page_size=page_size).status_code, - 404 - ) - - def test_db_access(self): - for _ in range(10): - new_user = UserFactory() - UserPreferenceFactory(user=new_user, key=NOTIFICATION_PREF_KEY) - - # The number of queries is one for the users plus one for each prefetch - # in NotifierUsersViewSet (roles__permissions does one for each table). - with self.assertNumQueries(6): - self._get_list() diff --git a/lms/djangoapps/discussion/notifier_api/urls.py b/lms/djangoapps/discussion/notifier_api/urls.py deleted file mode 100644 index 2d8aa6fe8e..0000000000 --- a/lms/djangoapps/discussion/notifier_api/urls.py +++ /dev/null @@ -1,16 +0,0 @@ -""" -URLs for the notifier api app -""" - - -from django.conf.urls import include, url -from rest_framework import routers - -from lms.djangoapps.discussion.notifier_api.views import NotifierUsersViewSet - -notifier_api_router = routers.DefaultRouter() -notifier_api_router.register(r'users', NotifierUsersViewSet, basename="notifier_users") - -urlpatterns = [ - url(r'^v1/', include(notifier_api_router.urls)), -] diff --git a/lms/djangoapps/discussion/notifier_api/views.py b/lms/djangoapps/discussion/notifier_api/views.py deleted file mode 100644 index de25b178ec..0000000000 --- a/lms/djangoapps/discussion/notifier_api/views.py +++ /dev/null @@ -1,55 +0,0 @@ -""" -Django views for the Notifier. -""" - - -from django.contrib.auth.models import User -from rest_framework import pagination -from rest_framework.response import Response -from rest_framework.viewsets import ReadOnlyModelViewSet - -from lms.djangoapps.discussion.notification_prefs import NOTIFICATION_PREF_KEY -from lms.djangoapps.discussion.notifier_api.serializers import NotifierUserSerializer -from openedx.core.lib.api.permissions import ApiKeyHeaderPermission - - -class NotifierPaginator(pagination.PageNumberPagination): - """ - Paginator for the notifier API. - """ - page_size = 10 - page_size_query_param = "page_size" - - def get_paginated_response(self, data): - """ - Construct a response with pagination information. - """ - return Response({ - 'next': self.get_next_link(), - 'previous': self.get_previous_link(), - 'count': self.page.paginator.count, - 'results': data - }) - - -class NotifierUsersViewSet(ReadOnlyModelViewSet): - """ - An endpoint that the notifier can use to retrieve users who have enabled - daily forum digests, including all information that the notifier needs about - such users. - """ - permission_classes = (ApiKeyHeaderPermission,) - serializer_class = NotifierUserSerializer - pagination_class = NotifierPaginator - - # See NotifierUserSerializer for notes about related tables - queryset = User.objects.filter( - preferences__key=NOTIFICATION_PREF_KEY - ).select_related( - "profile" - ).prefetch_related( - "preferences", - "courseenrollment_set", - "course_groups", - "roles__permissions" - ) diff --git a/lms/urls.py b/lms/urls.py index 5e99a65b31..bdca1ccd11 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -108,9 +108,6 @@ urlpatterns = [ url(r'^heartbeat', include('openedx.core.djangoapps.heartbeat.urls')), - url(r'^notifier_api/', include('lms.djangoapps.discussion.notifier_api.urls')), - url(r'^/api/notifier/', include('lms.djangoapps.discussion.notifier_api.urls')), - url(r'^i18n/', include('django.conf.urls.i18n')), # Enrollment API RESTful endpoints diff --git a/pavelib/prereqs.py b/pavelib/prereqs.py index f44da6eaa5..4d2e0b1903 100644 --- a/pavelib/prereqs.py +++ b/pavelib/prereqs.py @@ -197,7 +197,6 @@ PACKAGES_TO_UNINSTALL = [ "i18n-tools", # Because now it's called edx-i18n-tools "moto", # Because we no longer use it and it conflicts with recent jsondiff versions "python-saml", # Because python3-saml shares the same directory name - "pdfminer", # Replaced by pdfminer.six, which shares the same directory name "pytest-faulthandler", # Because it was bundled into pytest "djangorestframework-jwt", # Because now its called drf-jwt. ]