Address Dave's comments on https://github.com/MITx/mitx/pull/1350

common/djangoapps/course_groups/cohorts.py

@@ -8,6 +8,7 @@ from django.http import Http404
 import logging
 
 from courseware import courses
+from student.models import get_user_by_username_or_email
 from .models import CourseUserGroup
 
 log = logging.getLogger(__name__)
@@ -178,10 +179,7 @@ def add_user_to_cohort(cohort, username_or_email):
 
         CohortConflict if user already in another cohort.
     """
-    if '@' in username_or_email:
-        user = User.objects.get(email=username_or_email)
-    else:
-        user = User.objects.get(username=username_or_email)
+    user = get_user_by_username_or_email(username_or_email)
 
     # If user in any cohorts in this course already, complain
     course_cohorts = CourseUserGroup.objects.filter(

common/djangoapps/course_groups/views.py

 from django_future.csrf import ensure_csrf_cookie
 from django.contrib.auth.decorators import login_required
+from django.views.decorators.http import require_POST
 from django.contrib.auth.models import User
 from django.core.context_processors import csrf
 from django.core.paginator import Paginator, EmptyPage, PageNotAnInteger
@@ -8,10 +9,10 @@ from django.http import HttpResponse, HttpResponseForbidden, Http404
 from django.shortcuts import redirect
 import json
 import logging
+import re
 
 from courseware.courses import get_course_with_access
 from mitxmako.shortcuts import render_to_response, render_to_string
-from string_util import split_by_comma_and_whitespace
 
 from .models import CourseUserGroup
 from . import cohorts
@@ -21,13 +22,20 @@ import track.views
 
 log = logging.getLogger(__name__)
 
-def JsonHttpReponse(data):
+def json_http_response(data):
     """
-    Return an HttpResponse with the data json-serialized and the right content type
-    header.  Named to look like a class.
+    Return an HttpResponse with the data json-serialized and the right content
+    type header.
     """
     return HttpResponse(json.dumps(data), content_type="application/json")
 
+def split_by_comma_and_whitespace(s):
+    """
+    Split a string both by commas and whitespice.  Returns a list.
+    """
+    return re.split(r'[\s|,|]+', s)
+
+
 @ensure_csrf_cookie
 def list_cohorts(request, course_id):
     """
@@ -41,11 +49,12 @@ def list_cohorts(request, course_id):
     all_cohorts = [{'name': c.name, 'id': c.id}
                for c in cohorts.get_course_cohorts(course_id)]
 
-    return JsonHttpReponse({'success': True,
+    return json_http_response({'success': True,
                             'cohorts': all_cohorts})
 
 
 @ensure_csrf_cookie
+@require_POST
 def add_cohort(request, course_id):
     """
     Return json of dict:
@@ -60,22 +69,18 @@ def add_cohort(request, course_id):
     """
     get_course_with_access(request.user, course_id, 'staff')
 
-
-    if request.method != "POST":
-        raise Http404("Must POST to add cohorts")
-
     name = request.POST.get("name")
     if not name:
-        return JsonHttpReponse({'success': False,
+        return json_http_response({'success': False,
                                 'msg': "No name specified"})
 
     try:
         cohort = cohorts.add_cohort(course_id, name)
     except ValueError as err:
-        return JsonHttpReponse({'success': False,
+        return json_http_response({'success': False,
                                 'msg': str(err)})
 
-    return JsonHttpReponse({'success': 'True',
+    return json_http_response({'success': 'True',
                             'cohort': {
                                 'id': cohort.id,
                                 'name': cohort.name
@@ -98,6 +103,8 @@ def users_in_cohort(request, course_id, cohort_id):
     """
     get_course_with_access(request.user, course_id, 'staff')
 
+    # this will error if called with a non-int cohort_id.  That's ok--it
+    # shoudn't happen for valid clients.
     cohort = cohorts.get_cohort_by_id(course_id, int(cohort_id))
 
     paginator = Paginator(cohort.users.all(), 100)
@@ -118,13 +125,14 @@ def users_in_cohort(request, course_id, cohort_id):
                   'name': '{0} {1}'.format(u.first_name, u.last_name)}
                   for u in users]
 
-    return JsonHttpReponse({'success': True,
+    return json_http_response({'success': True,
                             'page': page,
                             'num_pages': paginator.num_pages,
                             'users': user_info})
 
 
 @ensure_csrf_cookie
+@require_POST
 def add_users_to_cohort(request, course_id, cohort_id):
     """
     Return json dict of:
@@ -140,9 +148,6 @@ def add_users_to_cohort(request, course_id, cohort_id):
     """
     get_course_with_access(request.user, course_id, 'staff')
 
-    if request.method != "POST":
-        raise Http404("Must POST to add users to cohorts")
-
     cohort = cohorts.get_cohort_by_id(course_id, cohort_id)
 
     users = request.POST.get('users', '')
@@ -166,13 +171,14 @@ def add_users_to_cohort(request, course_id, cohort_id):
                               'msg': str(err)})
 
 
-    return JsonHttpReponse({'success': True,
+    return json_http_response({'success': True,
                             'added': added,
                             'present': present,
                             'conflict': conflict,
                             'unknown': unknown})
 
 @ensure_csrf_cookie
+@require_POST
 def remove_user_from_cohort(request, course_id, cohort_id):
     """
     Expects 'username': username in POST data.
@@ -185,22 +191,19 @@ def remove_user_from_cohort(request, course_id, cohort_id):
     """
     get_course_with_access(request.user, course_id, 'staff')
 
-    if request.method != "POST":
-        raise Http404("Must POST to add users to cohorts")
-
     username = request.POST.get('username')
     if username is None:
-        return JsonHttpReponse({'success': False,
+        return json_http_response({'success': False,
                                 'msg': 'No username specified'})
 
     cohort = cohorts.get_cohort_by_id(course_id, cohort_id)
     try:
         user = User.objects.get(username=username)
         cohort.users.remove(user)
-        return JsonHttpReponse({'success': True})
+        return json_http_response({'success': True})
     except User.DoesNotExist:
         log.debug('no user')
-        return JsonHttpReponse({'success': False,
+        return json_http_response({'success': False,
                                 'msg': "No user '{0}'".format(username)})
 
 

common/djangoapps/student/models.py

@@ -613,7 +613,20 @@ class CourseEnrollmentAllowed(models.Model):
 
 #cache_relation(User.profile)
 
-#### Helper methods for use from python manage.py shell.
+#### Helper methods for use from python manage.py shell and other classes.
+
+def get_user_by_username_or_email(username_or_email):
+    """
+    Return a User object, looking up by email if username_or_email contains a
+    '@', otherwise by username.
+
+    Raises:
+        User.DoesNotExist is lookup fails.
+    """
+    if '@' in username_or_email:
+        return User.objects.get(email=username_or_email)
+    else:
+        return User.objects.get(username=username_or_email)
 
 
 def get_user(email):

common/lib/string_util.py

-import itertools
-
-def split_by_comma_and_whitespace(s):
-    """
-    Split a string both by on commas and whitespice.
-    """
-    # Note: split() with no args removes empty strings from output
-    lists = [x.split() for x in s.split(',')]
-    # return all of them
-    return itertools.chain(*lists)
-

lms/djangoapps/django_comment_client/base/views.py

@@ -28,6 +28,8 @@ from django_comment_client.utils import JsonResponse, JsonError, extract, get_co
 from django_comment_client.permissions import check_permissions_by_view, cached_has_permission
 from django_comment_client.models import Role
 
+log = logging.getLogger(__name__)
+
 def permitted(fn):
     @functools.wraps(fn)
     def wrapper(request, *args, **kwargs):
@@ -59,17 +61,12 @@ def ajax_content_response(request, course_id, content, template_name):
         'annotated_content_info': annotated_content_info,
     })
 
-    
-    
-def is_moderator(user, course_id):
-  cached_has_permission(user, "see_all_cohorts", course_id)    
-    
+
 @require_POST
 @login_required
 @permitted
 def create_thread(request, course_id, commentable_id):
-    print "\n\n\n\n\n*******************"
-    print commentable_id
+    log.debug("Creating new thread in %r, id %r", course_id, commentable_id)
     course = get_course_with_access(request.user, course_id, 'load')
     post = request.POST
 
@@ -91,29 +88,23 @@ def create_thread(request, course_id, commentable_id):
         'course_id'          : course_id,
         'user_id'            : request.user.id,
     })
-    
-    
-    #now cohort the thread if the commentable is cohorted
-    #if the group id came in from the form, set it there, otherwise, 
-    #see if the user and the commentable are cohorted
-    if is_commentable_cohorted(course_id,commentable_id):
-      if 'group_id' in post: #if a group id was submitted in the form
-        posted_group_id = post['group_id']
-      else:
-        post_group_id = None
-    
-      user_group_id = get_cohort_id(request.user, course_id)
-
-      if is_moderator(request.user,course_id):
-        if post_group_id is None:
-          group_id = user_group_id
+
+
+    # Cohort the thread if the commentable is cohorted.
+    if is_commentable_cohorted(course_id, commentable_id):
+        user_group_id = get_cohort_id(request.user, course_id)
+        # TODO (vshnayder): once we have more than just cohorts, we'll want to
+        # change this to a single get_group_for_user_and_commentable function
+        # that can do different things depending on the commentable_id
+        if cached_has_permission(request.user, "see_all_cohorts", course_id):
+            # admins can optionally choose what group to post as
+            group_id = post.get('group_id', user_group_id)
         else:
-          group_id = post_group_id
-      else:
-          group_id = user_group_id
-      
-      thread.update_attributes(**{'group_id' :group_id})
-    
+            # regular users always post with their own id.
+            group_id = user_group_id
+
+        thread.update_attributes(group_id=group_id)
+
     thread.save()
     if post.get('auto_subscribe', 'false').lower() == 'true':
         user = cc.User.from_django_user(request.user)

lms/djangoapps/django_comment_client/forum/views.py

@@ -17,7 +17,8 @@ from courseware.access import has_access
 from urllib import urlencode
 from operator import methodcaller
 from django_comment_client.permissions import check_permissions_by_view
-from django_comment_client.utils import merge_dict, extract, strip_none, strip_blank, get_courseware_context
+from django_comment_client.utils import (merge_dict, extract, strip_none,
+                                         strip_blank, get_courseware_context)
 
 import django_comment_client.utils as utils
 import comment_client as cc
@@ -58,16 +59,17 @@ def get_threads(request, course_id, discussion_id=None, per_page=THREADS_PER_PAG
         user.default_sort_key = request.GET.get('sort_key')
         user.save()
 
-        
+
     #if the course-user is cohorted, then add the group id
-    group_id = get_cohort_id(user,course_id);
+    group_id = get_cohort_id(user,course_id)
     if group_id:
-      default_query_params["group_id"] = group_id; 
-      print("\n\n\n\n\n****************GROUP ID IS ")
-      print group_id
-        
+        default_query_params["group_id"] = group_id
+
     query_params = merge_dict(default_query_params,
-                              strip_none(extract(request.GET, ['page', 'sort_key', 'sort_order', 'text', 'tags', 'commentable_ids'])))
+                              strip_none(extract(request.GET,
+                                                 ['page', 'sort_key',
+                                                  'sort_order', 'text',
+                                                  'tags', 'commentable_ids'])))
 
     threads, page, num_pages = cc.Thread.search(query_params)
 
@@ -226,7 +228,7 @@ def single_thread(request, course_id, discussion_id, thread_id):
         #    course_id,
         #)
 
-        
+
         annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info)
 
         context = {