From 92b99c7dc300a2aaea8a96095f1944c49bb84551 Mon Sep 17 00:00:00 2001 From: Awais Jibran Date: Fri, 4 Nov 2016 23:36:01 +0500 Subject: [PATCH] Fix discussion user roles --- .../test/acceptance/pages/lms/discussion.py | 4 ++ .../tests/discussion/test_discussion.py | 63 ++++++++++++++++--- lms/djangoapps/discussion/tests/test_views.py | 16 +++++ lms/djangoapps/discussion/views.py | 13 +++- lms/templates/discussion/_user_profile.html | 3 +- 5 files changed, 86 insertions(+), 13 deletions(-) diff --git a/common/test/acceptance/pages/lms/discussion.py b/common/test/acceptance/pages/lms/discussion.py index 6536602d82..705e9c881a 100644 --- a/common/test/acceptance/pages/lms/discussion.py +++ b/common/test/acceptance/pages/lms/discussion.py @@ -665,6 +665,10 @@ class DiscussionUserProfilePage(CoursePage): self.wait_for_page() self.q(css='.learner-profile-link').first.click() + def get_user_roles(self): + """Get user roles""" + return self.q(css='.sidebar-user-roles').text[0] + class DiscussionTabHomePage(CoursePage, DiscussionPageMixin): diff --git a/common/test/acceptance/tests/discussion/test_discussion.py b/common/test/acceptance/tests/discussion/test_discussion.py index 4899c937ae..226fa15b08 100644 --- a/common/test/acceptance/tests/discussion/test_discussion.py +++ b/common/test/acceptance/tests/discussion/test_discussion.py @@ -1145,21 +1145,29 @@ class DiscussionUserProfileTest(UniqueCourseTest): def setUp(self): super(DiscussionUserProfileTest, self).setUp() - CourseFixture(**self.course_info).install() + self.setup_course() # The following line creates a user enrolled in our course, whose # threads will be viewed, but not the one who will view the page. # It isn't necessary to log them in, but using the AutoAuthPage # saves a lot of code. - self.profiled_user_id = AutoAuthPage( - self.browser, - username=self.PROFILED_USERNAME, - course_id=self.course_id - ).visit().get_user_id() + self.profiled_user_id = self.setup_user(username=self.PROFILED_USERNAME) # now create a second user who will view the profile. - self.user_id = AutoAuthPage( - self.browser, - course_id=self.course_id - ).visit().get_user_id() + self.user_id = self.setup_user() + + def setup_course(self): + """ + Set up the for the course discussion user-profile tests. + """ + return CourseFixture(**self.course_info).install() + + def setup_user(self, roles=None, **user_info): + """ + Helper method to create and authenticate a user. + """ + roles_str = '' + if roles: + roles_str = ','.join(roles) + return AutoAuthPage(self.browser, course_id=self.course_id, roles=roles_str, **user_info).visit().get_user_id() def check_pages(self, num_threads): # set up the stub server to return the desired amount of thread results @@ -1262,6 +1270,41 @@ class DiscussionUserProfileTest(UniqueCourseTest): learner_profile_page.wait_for_page() self.assertTrue(learner_profile_page.field_is_visible('username')) + def test_learner_profile_roles(self): + """ + Test that on the learner profile page user roles are correctly listed according to the course. + """ + # Setup a learner with roles in a Course-A. + expected_student_roles = ['Administrator', 'Community TA', 'Moderator', 'Student'] + self.profiled_user_id = self.setup_user( + roles=expected_student_roles, + username=self.PROFILED_USERNAME + ) + + # Visit the page and verify the roles are listed correctly. + page = self.check_pages(1) + student_roles = page.get_user_roles() + self.assertEqual(student_roles, ', '.join(expected_student_roles)) + + # Save the course_id of Course-A before setting up a new course. + old_course_id = self.course_id + + # Setup Course-B and set user do not have additional roles and test roles are displayed correctly. + self.course_info['number'] = self.unique_id + self.setup_course() + new_course_id = self.course_id + + # Set the user to have no extra role in the Course-B and verify the existing + # user is updated. + profiled_student_user_id = self.setup_user(roles=None, username=self.PROFILED_USERNAME) + self.assertEqual(self.profiled_user_id, profiled_student_user_id) + self.assertNotEqual(old_course_id, new_course_id) + + # Visit the user profile in course discussion page of Course-B. Make sure the + # roles are listed correctly. + page = self.check_pages(1) + self.assertEqual(page.get_user_roles(), u'Student') + class DiscussionSearchAlertTest(UniqueCourseTest): """ diff --git a/lms/djangoapps/discussion/tests/test_views.py b/lms/djangoapps/discussion/tests/test_views.py index 00dafece4a..20b14a2530 100644 --- a/lms/djangoapps/discussion/tests/test_views.py +++ b/lms/djangoapps/discussion/tests/test_views.py @@ -1118,6 +1118,7 @@ class UserProfileTestCase(ForumsEnableMixin, UrlResetMixin, ModuleStoreTestCase) self.student = UserFactory.create() self.profiled_user = UserFactory.create() CourseEnrollmentFactory.create(user=self.student, course_id=self.course.id) + CourseEnrollmentFactory.create(user=self.profiled_user, course_id=self.course.id) def get_response(self, mock_request, params, **headers): mock_request.side_effect = make_mock_request_impl( @@ -1189,6 +1190,21 @@ class UserProfileTestCase(ForumsEnableMixin, UrlResetMixin, ModuleStoreTestCase) def test_ajax_p2(self, mock_request): self.check_ajax(mock_request, page="2") + def test_404_non_enrolled_user(self, __): + """ + Test that when student try to visit un-enrolled students' discussion profile, + the system raises Http404. + """ + unenrolled_user = UserFactory.create() + request = RequestFactory().get("dummy_url") + request.user = self.student + with self.assertRaises(Http404): + views.user_profile( + request, + self.course.id.to_deprecated_string(), + unenrolled_user.id + ) + def test_404_profiled_user(self, mock_request): request = RequestFactory().get("dummy_url") request.user = self.student diff --git a/lms/djangoapps/discussion/views.py b/lms/djangoapps/discussion/views.py index b1f1c81a51..c32959169c 100644 --- a/lms/djangoapps/discussion/views.py +++ b/lms/djangoapps/discussion/views.py @@ -21,6 +21,7 @@ from openedx.core.djangoapps.course_groups.cohorts import ( get_course_cohorts, ) from courseware.access import has_access +from student.models import CourseEnrollment from xmodule.modulestore.django import modulestore from django_comment_common.utils import ThreadContext @@ -409,7 +410,13 @@ def user_profile(request, course_key, user_id): #TODO: Allow sorting? course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=True) + try: + # If user is not enrolled in the course, do not proceed. + django_user = User.objects.get(id=user_id) + if not CourseEnrollment.is_enrolled(django_user, course.id): + raise Http404 + query_params = { 'page': request.GET.get('page', 1), 'per_page': THREADS_PER_PAGE, # more than threads_per_page to show more activities @@ -443,11 +450,15 @@ def user_profile(request, course_key, user_id): 'annotated_content_info': annotated_content_info, }) else: - django_user = User.objects.get(id=user_id) + user_roles = django_user.roles.filter( + course_id=course.id + ).order_by("name").values_list("name", flat=True).distinct() + context = { 'course': course, 'user': request.user, 'django_user': django_user, + 'django_user_roles': user_roles, 'profiled_user': profiled_user.to_dict(), 'threads': threads, 'user_info': user_info, diff --git a/lms/templates/discussion/_user_profile.html b/lms/templates/discussion/_user_profile.html index ea732716f0..e18b85cac6 100644 --- a/lms/templates/discussion/_user_profile.html +++ b/lms/templates/discussion/_user_profile.html @@ -3,8 +3,7 @@
${django_user.username | h}
- <% role_names = django_user.roles.order_by("name").values_list("name", flat=True).distinct() %> - ${", ".join(_(role_name) for role_name in role_names)} + ${", ".join(_(role_name) for role_name in django_user_roles)}
${ungettext('%s discussion started', '%s discussions started', profiled_user['threads_count']) % span(profiled_user['threads_count']) | h}
${ungettext('%s comment', '%s comments', profiled_user['comments_count']) % span(profiled_user['comments_count']) | h}