From 66934ce220fc91a2ed14fa7082f17e5209aed06e Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Fri, 25 Jan 2013 13:33:19 -0500 Subject: [PATCH] Address Dave's comments on https://github.com/MITx/mitx/pull/1350 --- common/djangoapps/course_groups/cohorts.py | 6 +-- common/djangoapps/course_groups/views.py | 49 ++++++++++--------- common/djangoapps/student/models.py | 15 +++++- common/lib/string_util.py | 11 ----- .../django_comment_client/base/views.py | 47 +++++++----------- .../django_comment_client/forum/views.py | 20 ++++---- 6 files changed, 72 insertions(+), 76 deletions(-) delete mode 100644 common/lib/string_util.py diff --git a/common/djangoapps/course_groups/cohorts.py b/common/djangoapps/course_groups/cohorts.py index 1c1208b8f7..3fe333f9c8 100644 --- a/common/djangoapps/course_groups/cohorts.py +++ b/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( diff --git a/common/djangoapps/course_groups/views.py b/common/djangoapps/course_groups/views.py index d64622dcb0..75d7f31626 100644 --- a/common/djangoapps/course_groups/views.py +++ b/common/djangoapps/course_groups/views.py @@ -1,5 +1,6 @@ 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)}) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 611b6effbe..ab7f435f68 100644 --- a/common/djangoapps/student/models.py +++ b/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): diff --git a/common/lib/string_util.py b/common/lib/string_util.py deleted file mode 100644 index 0db385f2d6..0000000000 --- a/common/lib/string_util.py +++ /dev/null @@ -1,11 +0,0 @@ -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) - diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index eee6c6d09d..777c7bafce 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/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) diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index 93752f6c33..6a182665a8 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/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 = {