Merge pull request #4937 from edx/dan-f/improve-course-group-coverage

Improve test coverage for course groups

common/djangoapps/course_groups/cohorts.py

@@ -4,6 +4,7 @@ forums, and to the cohort admin views.
 """
 
 from django.http import Http404
+
 import logging
 import random
 
@@ -86,14 +87,14 @@ def is_commentable_cohorted(course_key, commentable_id):
 
 def get_cohorted_commentables(course_key):
     """
-    Given a course_key return a list of strings representing cohorted commentables
+    Given a course_key return a set of strings representing cohorted commentables.
     """
 
     course = courses.get_course_by_id(course_key)
 
     if not course.is_cohorted:
         # this is the easy case :)
-        ans = []
+        ans = set()
     else:
         ans = course.cohorted_discussions
 
@@ -213,8 +214,13 @@ def add_cohort(course_key, name):
                                       name=name).exists():
         raise ValueError("Can't create two cohorts with the same name")
 
+    try:
+        course = courses.get_course_by_id(course_key)
+    except Http404:
+        raise ValueError("Invalid course_key")
+
     return CourseUserGroup.objects.create(
-        course_id=course_key,
+        course_id=course.id,
         group_type=CourseUserGroup.COHORT,
         name=name
     )
@@ -240,7 +246,6 @@ def add_user_to_cohort(cohort, username_or_email):
 
     Raises:
         User.DoesNotExist if can't find user.
-
         ValueError if user already present in this cohort.
     """
     user = get_user_by_username_or_email(username_or_email)

common/djangoapps/course_groups/views.py

@@ -3,7 +3,8 @@ from django.views.decorators.http import require_POST
 from django.contrib.auth.models import User
 from django.core.paginator import Paginator, EmptyPage, PageNotAnInteger
 from django.core.urlresolvers import reverse
-from django.http import HttpResponse
+from django.http import Http404, HttpResponse, HttpResponseBadRequest
+from django.utils.translation import ugettext as _
 import json
 import logging
 import re
@@ -13,7 +14,7 @@ from courseware.courses import get_course_with_access
 from edxmako.shortcuts import render_to_response
 
 from . import cohorts
-
+from .models import CourseUserGroup
 
 log = logging.getLogger(__name__)
 
@@ -115,17 +116,18 @@ def users_in_cohort(request, course_key_string, cohort_id):
     cohort = cohorts.get_cohort_by_id(course_key, int(cohort_id))
 
     paginator = Paginator(cohort.users.all(), 100)
-    page = request.GET.get('page')
     try:
-        users = paginator.page(page)
-    except PageNotAnInteger:
-        # return the first page
-        page = 1
+        page = int(request.GET.get('page'))
+    except (TypeError, ValueError):
+        return HttpResponseBadRequest(_('Requested page must be numeric'))
+    else:
+        if page < 0:
+            return HttpResponseBadRequest(_('Requested page must be greater than zero'))
+
+    try:
         users = paginator.page(page)
     except EmptyPage:
-        # Page is out of range.  Return last page
-        page = paginator.num_pages
-        contacts = paginator.page(page)
+        users = []  # When page > number of pages, return a blank page
 
     user_info = [{'username': u.username,
                   'email': u.email,
@@ -154,12 +156,20 @@ def add_users_to_cohort(request, course_key_string, cohort_id):
                   'previous_cohort': ...}, ...],
      'present': [str1, str2, ...],    # already there
      'unknown': [str1, str2, ...]}
+
+     Raises Http404 if the cohort cannot be found for the given course.
     """
     # this is a string when we get it here
     course_key = SlashSeparatedCourseKey.from_deprecated_string(course_key_string)
     get_course_with_access(request.user, 'staff', course_key)
 
-    cohort = cohorts.get_cohort_by_id(course_key, cohort_id)
+    try:
+        cohort = cohorts.get_cohort_by_id(course_key, cohort_id)
+    except CourseUserGroup.DoesNotExist:
+        raise Http404("Cohort (ID {cohort_id}) not found for {course_key_string}".format(
+            cohort_id=cohort_id,
+            course_key_string=course_key_string
+        ))
 
     users = request.POST.get('users', '')
     added = []

common/static/js/course_groups/cohorts.js

@@ -220,7 +220,7 @@ var CohortManager = (function ($) {
                 });
             } else if (state == state_detail) {
                 detail_header.text("Members of " + cohort_title);
-                $.ajax(detail_url).done(show_users).fail(function() {
+                $.ajax(detail_url, {data: {page: 1}}).done(show_users).fail(function() {
                     log_error("Error trying to load users in cohort");
                 });
             }