Merge pull request #538 from edx/db/course-team-admin-grants

Add error messaging to course team page

cms/djangoapps/contentstore/features/common.py

@@ -4,10 +4,11 @@
 from lettuce import world, step
 from nose.tools import assert_true
 
-from auth.authz import get_user_by_email
+from auth.authz import get_user_by_email, get_course_groupname_for_role
 
 from selenium.webdriver.common.keys import Keys
 import time
+from django.contrib.auth.models import Group
 
 from logging import getLogger
 logger = getLogger(__name__)
@@ -163,18 +164,19 @@ def log_into_studio(
 
 
 def create_a_course():
-    world.scenario_dict['COURSE'] = world.CourseFactory.create(org='MITx', course='999', display_name='Robot Super Course')
+    course = world.CourseFactory.create(org='MITx', course='999', display_name='Robot Super Course')
+    world.scenario_dict['COURSE'] = course
+
+    user = world.scenario_dict.get("USER")
+    if not user:
+        user = get_user_by_email('robot+studio@edx.org')
 
     # Add the user to the instructor group of the course
     # so they will have the permissions to see it in studio
-
-    course = world.GroupFactory.create(name='instructor_MITx/{}/{}'.format(world.scenario_dict['COURSE'].number,
-                                                                    world.scenario_dict['COURSE'].display_name.replace(" ", "_")))
-    if world.scenario_dict.get('USER') is None:
-        user = world.scenario_dict['USER']
-    else:
-        user = get_user_by_email('robot+studio@edx.org')
-    user.groups.add(course)
+    for role in ("staff", "instructor"):
+        groupname = get_course_groupname_for_role(course.location, role)
+        group, __ = Group.objects.get_or_create(name=groupname)
+        user.groups.add(group)
     user.save()
     world.browser.reload()
 

cms/djangoapps/contentstore/features/course-team.feature

@@ -57,3 +57,30 @@ Feature: Course Team
         Then "frank" should not be marked as an admin
         And he cannot add users
         And he cannot delete users
+
+    Scenario: Admins should be able to give course ownership to someone else
+        Given I have opened a new course in Studio
+        And the user "gina" exists
+        And I am viewing the course team settings
+        When I add "gina" to the course team
+        And I make "gina" a course team admin
+        And I remove admin rights from myself
+        And "gina" logs in
+        And she selects the new course
+        And she views the course team settings
+        And she deletes me from the course team
+        And I log in
+        Then I do not see the course on my page
+
+    Scenario: Admins should be able to remove their own admin rights
+        Given I have opened a new course in Studio
+        And the user "harry" exists as a course admin
+        And I am viewing the course team settings
+        Then I should be marked as an admin
+        And I can add users
+        And I can delete users
+        When I remove admin rights from myself
+        Then I should not be marked as an admin
+        And I cannot add users
+        And I cannot delete users
+        And I cannot make myself a course team admin

cms/djangoapps/contentstore/features/course-team.py

@@ -17,13 +17,20 @@ def view_grading_settings(_step, whom):
     world.css_click(link_css)
 
 
-@step(u'the user "([^"]*)" exists( as a course admin)?$')
-def create_other_user(_step, name, course_admin):
-    user = create_studio_user(uname=name, password=PASSWORD, email=(name + EMAIL_EXTENSION))
-    if course_admin:
+@step(u'the user "([^"]*)" exists( as a course (admin|staff member))?$')
+def create_other_user(_step, name, has_extra_perms, role_name):
+    email = name + EMAIL_EXTENSION
+    user = create_studio_user(uname=name, password=PASSWORD, email=email)
+    if has_extra_perms:
         location = world.scenario_dict["COURSE"].location
-        for role in ("staff", "instructor"):
-            group, __ = Group.objects.get_or_create(name=get_course_groupname_for_role(location, role))
+        if role_name == "admin":
+            # admins get staff privileges, as well
+            roles = ("staff", "instructor")
+        else:
+            roles = ("staff",)
+        for role in roles:
+            groupname = get_course_groupname_for_role(location, role)
+            group, __ = Group.objects.get_or_create(name=groupname)
             user.groups.add(group)
         user.save()
 
@@ -47,6 +54,17 @@ def delete_other_user(_step, name):
     to_delete_css = '.user-item .item-actions a.remove-user[data-id="{email}"]'.format(
         email="{0}{1}".format(name, EMAIL_EXTENSION))
     world.css_click(to_delete_css)
+    # confirm prompt
+    world.css_click(".wrapper-prompt-warning .action-primary")
+
+
+@step(u's?he deletes me from the course team')
+def other_delete_self(_step):
+    to_delete_css = '.user-item .item-actions a.remove-user[data-id="{email}"]'.format(
+        email="robot+studio@edx.org")
+    world.css_click(to_delete_css)
+    # confirm prompt
+    world.css_click(".wrapper-prompt-warning .action-primary")
 
 
 @step(u'I make "([^"]*)" a course team admin')
@@ -56,10 +74,14 @@ def make_course_team_admin(_step, name):
     world.css_click(admin_btn_css)
 
 
-@step(u'I remove admin rights from "([^"]*)"')
-def remove_course_team_admin(_step, name):
+@step(u'I remove admin rights from ("([^"]*)"|myself)')
+def remove_course_team_admin(_step, outer_capture, name):
+    if outer_capture == "myself":
+        email = world.scenario_dict["USER"].email
+    else:
+        email = name + EMAIL_EXTENSION
     admin_btn_css = '.user-item[data-email="{email}"] .user-actions .remove-admin-role'.format(
-        email=name+EMAIL_EXTENSION)
+        email=email)
     world.css_click(admin_btn_css)
 
 
@@ -68,8 +90,9 @@ def other_user_login(_step, name):
     log_into_studio(uname=name, password=PASSWORD, email=name + EMAIL_EXTENSION)
 
 
+@step(u'I( do not)? see the course on my page')
 @step(u's?he does( not)? see the course on (his|her) page')
-def see_course(_step, inverted, gender):
+def see_course(_step, inverted, gender='self'):
     class_css = 'span.class-name'
     all_courses = world.css_find(class_css, wait_time=1)
     all_names = [item.html for item in all_courses]
@@ -89,6 +112,12 @@ def marked_as_admin(_step, name, inverted):
         assert world.is_css_present(flag_css)
 
 
+@step(u'I should( not)? be marked as an admin')
+def self_marked_as_admin(_step, inverted):
+    return marked_as_admin(_step, "robot+studio", inverted)
+
+
+@step(u'I can(not)? delete users')
 @step(u's?he can(not)? delete users')
 def can_delete_users(_step, inverted):
     to_delete_css = 'a.remove-user'
@@ -98,6 +127,7 @@ def can_delete_users(_step, inverted):
         assert world.is_css_present(to_delete_css)
 
 
+@step(u'I can(not)? add users')
 @step(u's?he can(not)? add users')
 def can_add_users(_step, inverted):
     add_css = 'a.create-user-button'
@@ -105,3 +135,17 @@ def can_add_users(_step, inverted):
         assert world.is_css_not_present(add_css)
     else:
         assert world.is_css_present(add_css)
+
+
+@step(u'I can(not)? make ("([^"]*)"|myself) a course team admin')
+@step(u's?he can(not)? make ("([^"]*)"|me) a course team admin')
+def can_make_course_admin(_step, inverted, outer_capture, name):
+    if outer_capture == "myself":
+        email = world.scenario_dict["USER"].email
+    else:
+        email = name + EMAIL_EXTENSION
+    add_button_css = '.user-item[data-email="{email}"] .add-admin-role'.format(email=email)
+    if inverted:
+        assert world.is_css_not_present(add_button_css)
+    else:
+        assert world.is_css_present(add_button_css)

cms/djangoapps/contentstore/views/user.py

@@ -16,10 +16,10 @@ from xmodule.modulestore import Location
 from contentstore.utils import get_lms_link_for_item
 from util.json_request import JsonResponse
 from auth.authz import (
-    STAFF_ROLE_NAME, INSTRUCTOR_ROLE_NAME,
-    add_user_to_course_group, remove_user_from_course_group,
-    get_course_groupname_for_role)
-from course_creators.views import get_course_creator_status, add_user_with_status_unrequested, user_requested_access
+    STAFF_ROLE_NAME, INSTRUCTOR_ROLE_NAME, get_course_groupname_for_role)
+from course_creators.views import (
+    get_course_creator_status, add_user_with_status_unrequested,
+    user_requested_access)
 
 from .access import has_access
 
@@ -154,16 +154,17 @@ def course_team_user(request, org, course, name, email):
         return JsonResponse(msg, 400)
 
     # make sure that the role groups exist
-    staff_groupname = get_course_groupname_for_role(location, "staff")
-    staff_group, __ = Group.objects.get_or_create(name=staff_groupname)
-    inst_groupname = get_course_groupname_for_role(location, "instructor")
-    inst_group, __ = Group.objects.get_or_create(name=inst_groupname)
+    groups = {}
+    for role in roles:
+        groupname = get_course_groupname_for_role(location, role)
+        group, __ = Group.objects.get_or_create(name=groupname)
+        groups[role] = group
 
     if request.method == "DELETE":
         # remove all roles in this course from this user: but fail if the user
         # is the last instructor in the course team
-        instructors = set(inst_group.user_set.all())
-        staff = set(staff_group.user_set.all())
+        instructors = set(groups["instructor"].user_set.all())
+        staff = set(groups["staff"].user_set.all())
         if user in instructors and len(instructors) == 1:
             msg = {
                 "error": _("You may not remove the last instructor from a course")
@@ -171,9 +172,9 @@ def course_team_user(request, org, course, name, email):
             return JsonResponse(msg, 400)
 
         if user in instructors:
-            user.groups.remove(inst_group)
+            user.groups.remove(groups["instructor"])
         if user in staff:
-            user.groups.remove(staff_group)
+            user.groups.remove(groups["staff"])
         user.save()
         return JsonResponse()
 
@@ -198,19 +199,21 @@ def course_team_user(request, org, course, name, email):
                 "error": _("Only instructors may create other instructors")
             }
             return JsonResponse(msg, 400)
-        add_user_to_course_group(request.user, user, location, role)
+        user.groups.add(groups["instructor"])
+        user.save()
     elif role == "staff":
         # if we're trying to downgrade a user from "instructor" to "staff",
         # make sure we have at least one other instructor in the course team.
-        instructors = set(inst_group.user_set.all())
+        instructors = set(groups["instructor"].user_set.all())
         if user in instructors:
             if len(instructors) == 1:
                 msg = {
                     "error": _("You may not remove the last instructor from a course")
                 }
                 return JsonResponse(msg, 400)
-            remove_user_from_course_group(request.user, user, location, "instructor")
-        add_user_to_course_group(request.user, user, location, role)
+            user.groups.remove(groups["instructor"])
+        user.groups.add(groups["staff"])
+        user.save()
     return JsonResponse()
 
 

cms/envs/common.py

@@ -238,6 +238,7 @@ PIPELINE_JS = {
             rooted_glob(COMMON_ROOT / 'static/', 'coffee/src/**/*.js') +
             rooted_glob(PROJECT_ROOT / 'static/', 'coffee/src/**/*.js')
         ) + ['js/hesitate.js', 'js/base.js', 'js/views/feedback.js',
+             'js/models/course.js',
              'js/models/section.js', 'js/views/section.js',
              'js/models/metadata_model.js', 'js/views/metadata_editor_view.js',
              'js/models/textbook.js', 'js/views/textbook.js',

cms/static/coffee/spec/models/course_spec.coffee

+describe "CMS.Models.Course", ->
+    describe "basic", ->
+        beforeEach ->
+            @model = new CMS.Models.Course({
+                name: "Greek Hero"
+            })
+
+        it "should take a name argument", ->
+            expect(@model.get("name")).toEqual("Greek Hero")

cms/static/js/models/course.js

+CMS.Models.Course = Backbone.Model.extend({
+    defaults: {
+        "name": ""
+    },
+    validate: function(attrs, options) {
+        if (!attrs.name) {
+            return gettext("You must specify a name");
+        }
+    }
+});

cms/templates/base.html

@@ -58,6 +58,18 @@
     <script type="text/javascript" src="//www.youtube.com/player_api"></script>
 
     <script src="${static.url('js/views/feedback.js')}"></script>
+    % if context_course:
+    <script type="text/javascript">
+window.course = new CMS.Models.Course({
+  id: "${context_course.id}",
+  name: "${context_course.display_name_with_default | h}",
+  url_name: "${context_course.location.name | h}",
+  org: "${context_course.location.org | h}",
+  num: "${context_course.location.course | h}",
+  revision: "${context_course.location.revision | h}"
+});
+    </script>
+    % endif
 
     <!-- view -->
     <div class="wrapper wrapper-view">

cms/templates/manage_users.html

 <%! from django.utils.translation import ugettext as _ %>
 <%! from django.core.urlresolvers import reverse %>
 <%! from auth.authz import is_user_in_course_group_role %>
+<%! import json %>
 <%inherit file="base.html" />
 <%block name="title">${_("Course Team Settings")}</%block>
 <%block name="bodyclass">is-signedin course users team</%block>
@@ -161,18 +162,55 @@
 
 <%block name="jsextra">
 <script type="text/javascript">
+  var staffEmails = ${json.dumps([user.email for user in staff])};
   var tplUserURL = "${reverse('course_team_user', kwargs=dict(
       org=context_course.location.org,
       course=context_course.location.course,
       name=context_course.location.name,
       email="@@EMAIL@@",
-    ))}"
+    ))}";
+  var unknownErrorMessage = gettext("Unknown")
 
   $(document).ready(function() {
     var $createUserForm = $('#create-user-form');
     var $createUserFormWrapper = $createUserForm.closest('.wrapper-create-user');
     $createUserForm.bind('submit', function(e) {
       e.preventDefault();
+      var email = $('#user-email-input').val().trim();
+      if(!email) {
+        var msg = new CMS.Views.Prompt.Error({
+          title: gettext("A valid email address is required"),
+          message: gettext("You must enter a valid email address in order to add a new team member"),
+          actions: {
+            primary: {
+              text: gettext("Return and add email address"),
+              click: function(view) {
+                view.hide();
+                $("#user-email-input").focus();
+              }
+            }
+          }
+        })
+        msg.show();
+        return;
+      }
+      if(_.contains(staffEmails, email)) {
+        var msg = new CMS.Views.Prompt.Warning({
+          title: gettext("Already a course team member"),
+          message: _.template(gettext("{email} is already on the “{course}” team. If you're trying to add a new member, please double-check the email address you provided."), {email: email, course: course.escape('name')}, {interpolate: /\{(.+?)\}/g}),
+          actions: {
+            primary: {
+              text: gettext("Return to team listing"),
+              click: function(view) {
+                view.hide();
+                $("#user-email-input").focus();
+              }
+            }
+          }
+        })
+        msg.show();
+        return;
+      }
       var url = tplUserURL.replace("@@EMAIL@@", $('#user-email-input').val().trim())
       $.ajax({
         url: url,
@@ -189,9 +227,9 @@
         error: function(jqXHR, textStatus, errorThrown) {
           var message;
           try {
-            message = JSON.parse(jqXHR.responseText).error || "Unknown";
+            message = JSON.parse(jqXHR.responseText).error || unknownErrorMessage;
           } catch (e) {
-            message = "Unknown";
+            message = unknownErrorMessage
           }
           var prompt = new CMS.Views.Prompt.Error({
             title: gettext("Error adding user"),
@@ -233,38 +271,58 @@
     });
 
     $('.remove-user').click(function() {
-      var url = tplUserURL.replace("@@EMAIL@@", $(this).data('id'))
-      $.ajax({
-        url: url,
-        type: 'DELETE',
-        dataType: 'json',
-        contentType: 'application/json',
-        success: function(data) {
-          location.reload();
-        },
-        notifyOnError: false,
-        error: function(jqXHR, textStatus, errorThrown) {
-          var message;
-          try {
-            message = JSON.parse(jqXHR.responseText).error || "Unknown";
-          } catch (e) {
-            message = "Unknown";
-          }
-          var prompt = new CMS.Views.Prompt.Error({
-            title: gettext("Error removing user"),
-            message: message,
-            actions: {
-              primary: {
-                text: gettext("OK"),
-                click: function(view) {
-                  view.hide();
+      var email = $(this).data('id');
+      var msg = new CMS.Views.Prompt.Warning({
+        title: gettext("Are you sure?"),
+        message: _.template(gettext("Are you sure you want to delete {email} from the course team for “{course}”?"), {email: email, course: course.get('name')}, {interpolate: /\{(.+?)\}/g}),
+        actions: {
+          primary: {
+            text: gettext("Delete"),
+            click: function(view) {
+              view.hide();
+              var url = tplUserURL.replace("@@EMAIL@@", email)
+              $.ajax({
+                url: url,
+                type: 'DELETE',
+                dataType: 'json',
+                contentType: 'application/json',
+                success: function(data) {
+                  location.reload();
+                },
+                notifyOnError: false,
+                error: function(jqXHR, textStatus, errorThrown) {
+                  var message;
+                  try {
+                    message = JSON.parse(jqXHR.responseText).error || unknownErrorMessage;
+                  } catch (e) {
+                    message = unknownErrorMessage;
+                  }
+                  var prompt = new CMS.Views.Prompt.Error({
+                    title: gettext("Error removing user"),
+                    message: message,
+                    actions: {
+                      primary: {
+                        text: gettext("OK"),
+                        click: function(view) {
+                          view.hide();
+                        }
+                      }
+                    }
+                  })
+                  prompt.show();
                 }
-              }
+              });
             }
-          })
-          prompt.show();
+          },
+          secondary: {
+            text: gettext("Cancel"),
+            click: function(view) {
+              view.hide();
+            }
+          }
         }
       });
+      msg.show();
     });
 
     $(".toggle-admin-role").click(function(e) {
@@ -291,16 +349,16 @@
         error: function(jqXHR, textStatus, errorThrown) {
           var message;
           try {
-            message = JSON.parse(jqXHR.responseText).error || "Unknown";
+            message = JSON.parse(jqXHR.responseText).error || unknownErrorMessage;
           } catch (e) {
-            message = "Unknown";
+            message = unknownErrorMessage;
           }
           var prompt = new CMS.Views.Prompt.Error({
-            title: gettext("Error changing user"),
+            title: gettext("There was an error changing the user's role"),
             message: message,
             actions: {
               primary: {
-                text: gettext("OK"),
+                text: gettext("Try Again"),
                 click: function(view) {
                   view.hide();
                 }