Merge pull request #13939 from edx/aj/fix-discussion-user-roles

Fix discussion user roles

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):
 

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):
     """

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

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,

lms/templates/discussion/_user_profile.html

@@ -3,8 +3,7 @@
 <div class="user-profile">
   <div class="sidebar-username"><a class="learner-profile-link" href="${learner_profile_page_url}">${django_user.username | h}</a></div>
   <div class="sidebar-user-roles">
-    <% 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)}
   </div>
   <div class="sidebar-threads-count">${ungettext('%s discussion started', '%s discussions started', profiled_user['threads_count']) % span(profiled_user['threads_count']) | h}</div>
   <div class="sidebar-comments-count">${ungettext('%s comment', '%s comments', profiled_user['comments_count']) % span(profiled_user['comments_count']) | h}</div>