limit self-assigned group size, closes #CNVS-5407

test plan:
- go to the groups page of a course or account
  /courses/*/groups or /accounts/*/groups
- edit a group category by clicking on the pencil icon or
  click 'make a new set of groups'
- check the 'Allow self sign-up' checkbox
- a textbox should appear to limit group members
- enter a number (2 or 3) for the group limit and save
- add at least on group
- masquerade as students in the course and join a group until it's full
- once the group is full students should not be able to join the group
- also, when editing a group category you should be able to remove the group limit

Change-Id: I0d035320a171b03a7cd2e4d81ecb0648c5aacad0
Reviewed-on: https://gerrit.instructure.com/20309
Reviewed-by: Jon Jensen <jon@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>
QA-Review: Cam Theriault <cam@instructure.com>
Product-Review: Joe Tanner <joe@instructure.com>

app/controllers/group_categories_controller.rb

@@ -260,6 +260,7 @@ class GroupCategoriesController < ApplicationController
     end
     @group_category.name = name
     @group_category.configure_self_signup(enable_self_signup, restrict_self_signup)
+    @group_category.group_limit = args[:group_limit]
     @group_category.save
   end
 

app/controllers/groups_controller.rb

@@ -245,6 +245,11 @@ class GroupsController < ApplicationController
         @current_conferences = @group.web_conferences.select{|c| c.active? && c.users.include?(@current_user) } rescue []
         @stream_items = @current_user.try(:cached_recent_stream_items, { :contexts => @context }) || []
         if params[:join] && @group.grants_right?(@current_user, :join)
+          if @group.full?
+            flash[:error] = t('errors.group_full', 'The group is full.')
+            redirect_to course_groups_url(@group.context)
+            return
+          end
           @group.request_user(@current_user)
           if !@group.grants_right?(@current_user, session, :read)
             render :action => 'membership_pending'

app/models/group.rb

@@ -99,6 +99,10 @@ class Group < ActiveRecord::Base
       (self.group_category.restricted_self_signup? && self.has_common_section_with_user?(user)))
   end
 
+  def full?
+    group_category && group_category.group_limit && participating_users.size >= group_category.group_limit
+  end
+
   def free_association?(user)
     auto_accept? || allow_join_request? || allow_self_signup?(user)
   end

app/models/group_category.rb

@@ -22,6 +22,7 @@ class GroupCategory < ActiveRecord::Base
   has_many :groups, :dependent => :destroy
   has_many :assignments, :dependent => :nullify
   validates_length_of :name, :maximum => maximum_string_length, :allow_nil => true, :allow_blank => true
+  validates_numericality_of :group_limit, :greater_than => 1, :allow_nil => true
 
   scope :active, where(:deleted_at => nil)
 

app/models/group_membership.rb

@@ -30,6 +30,7 @@ class GroupMembership < ActiveRecord::Base
   before_save :capture_old_group_id
 
   before_validation :verify_section_homogeneity_if_necessary
+  validate :validate_within_group_limit
 
   after_save :ensure_mutually_exclusive_membership
   after_save :touch_groups
@@ -113,6 +114,13 @@ class GroupMembership < ActiveRecord::Base
     end
   end
   protected :verify_section_homogeneity_if_necessary
+
+  def validate_within_group_limit
+    if new_record? && group.full?
+      errors.add(:group_id, t('errors.group_full', 'The group is full.'))
+    end
+  end
+  protected :validate_within_group_limit
   
   attr_accessor :old_group_id
   def capture_old_group_id

app/views/groups/_add_group_category.html.erb

@@ -60,6 +60,10 @@ require([
             <%= t :group_structure_self_signup, "*Create* %{number_of_groups} *groups now*",
               :number_of_groups => '<input type="text" name="category[create_group_count]" id="category_create_group_count" value="0" style="width: 25px;"/>'.html_safe,
               :wrapper => '<label for="category_create_group_count">\1</label>' %><br/>
+            <%= t :group_structure_group_limit, "*Limit groups to* %{group_limit} members",
+              :group_limit => '<input type="number" name="category[group_limit]" id="category_group_limit" class="input-mini">'.html_safe,
+              :wrapper => '<label for="category_group_limit">\1</label>' %>
+            <small class="muted"><%= t :group_limit_blank, '(leave blank for no limit)' %></small>
           </div>
         </td>
       </tr>

app/views/groups/_category.html.erb

@@ -6,6 +6,7 @@
     <div style="display: none;">
       <span class="self_signup"><%= category && category.self_signup %></span>
       <span class="heterogenous"><%= category && category.has_heterogenous_group? %></span>
+      <span class="group_limit"><%= category && category.group_limit %></span>
     </div>
     <div class="links">
       <a href="#" class="add_group_link no-hover" title="<%= t 'buttons.add_group', "Add Another Group" %>"><i class="icon-add standalone-icon"></i></a>
@@ -33,6 +34,9 @@
         <span class="restricted_self_signup_text" style="<%= hidden unless category && category.restricted_self_signup? %>"><%= t 'restricted_self_signup_blurb', "All students in a group are required to be in the same section." %></span>
         <%= link_to(image_tag('help.png'), '#', :class => 'self_signup_help_link no-hover',
                     :title => t(:self_signup_help_tooltip, "What Are Self Sign-Up Groups?")) %>
+        <p class="group_limit_blurb" style="<%= hidden unless category && category.group_limit %>">
+          <%= t 'group_limit_blurb', 'Groups are limited to *%{count}* members', :count => (category && category.group_limit), :wrapper => '<span class="group_limit_text">\1</span>' %>
+        </p>
       </div>
     </div>
   </div>

app/views/groups/_edit_group_category.html.erb

@@ -6,7 +6,16 @@
     <%= link_to(image_tag('help.png'), '#', :class => 'self_signup_help_link no-hover',
                 :title => t(:self_signup_help_tooltip, "What Are Self Sign-Up Groups?")) %>
   </div>
-  <div style="margin: 0.5em 0;"><%= check_box_tag 'category[restrict_self_signup]', "1", false, :disabled => true, :id => 'category_restrict_self_signup' %><%= label_tag :restrict_self_signup, t(:restricted_self_signup, "Require group members to be in the same section") %></div>
+  <div style="margin: 0.5em 0;">
+    <%= check_box_tag 'category[restrict_self_signup]', "1", false, :disabled => true, :id => 'category_restrict_self_signup' %>
+    <label for="category_restrict_self_signup"><%= t :restricted_self_signup, "Require group members to be in the same section" %></label>
+  </div>
+  <div id="group_structure_self_signup_subcontainer" style="display:none;">
+    <%= t :group_structure_group_limit, "*Limit groups to* %{group_limit} members",
+      :group_limit => '<input type="number" name="category[group_limit]" id="category_group_limit" class="input-mini">'.html_safe,
+      :wrapper => '<label for="category_group_limit">\1</label>' %>
+    <small class="muted"><%= t :group_limit_blank, '(leave blank for no limit)' %></small>
+  </div>
   <div class="form-actions">
     <button type="button" class="btn cancel_button"><%= t '#buttons.cancel', 'Cancel' %></button>
     <button type="submit" class="btn btn-primary submit_button"><%= t '#buttons.update', 'Update' %></button>

app/views/groups/_group.html.erb

@@ -17,12 +17,16 @@
          # a group should always be present if in_group is false, and for the
          # group to be being displayed with in_group false, it must be free
          # associative
+         disabled = false
          management_url = context_url @context, :context_group_url, group.id, :join => 1
          management_text =
            if group.auto_accept?
              t 'actions.join_group', 'join this group'
            elsif group.allow_self_signup?(@current_user)
-             if in_categories.include?(group.group_category)
+             if group.full?
+               disabled = true
+               t 'group_full', 'group full'
+             elsif in_categories.include?(group.group_category)
                t 'actions.switch_group', 'switch to this group'
              else
                t 'actions.join_group', 'join this group'
@@ -32,7 +36,7 @@
            end
        end %>
     <% if management_url %>
-      <span class="management"><%= link_to(management_text, management_url) %></span>
+      <span class="management"><%= disabled ? content_tag(:span, management_text, :class => 'muted') : link_to(management_text, management_url) %></span>
     <% end %>
   </div>
   <div class="group_info">

db/migrate/20130430215057_add_group_limit_to_group_category.rb

@@ -0,0 +1,11 @@
+class AddGroupLimitToGroupCategory < ActiveRecord::Migration
+  tag :predeploy
+
+  def self.up
+    add_column :group_categories, :group_limit, :integer
+  end
+
+  def self.down
+    remove_column :group_categories, :group_limit
+  end
+end

public/javascripts/manage_groups.js

@@ -203,6 +203,8 @@ define([
           // attempted group membership claims the user was unacceptable for
           // some reason (probably section).
           message = data.errors.user_id[0].message;
+        } else if (data.errors && data.errors.group_id) {
+          message = data.errors.group_id[0].message;
         } else {
           message = I18n.t('errors.unknown', 'An unexpected error occurred.');
         }
@@ -315,6 +317,8 @@ define([
       $category.find('.self_signup_text').showIf(category.self_signup);
       $category.find('.restricted_self_signup_text').showIf(category.self_signup === 'restricted');
       $category.find('.assign_students_link').showIf(category.self_signup !== 'restricted');
+      $category.find('.group_limit_blurb').showIf(category.group_limit);
+      $category.find('.group_limit, .group_limit_text').text(category.group_limit || '');
     },
 
     addGroupToSidebar: function(group) {
@@ -452,6 +456,12 @@ define([
           if(found) {
             return I18n.t('errors.category_in_use', "\"%{category_name}\" is already in use", {category_name: val});
           }
+        },
+        'group_limit': function(val, data) {
+          if (parseInt(val) <= 1) {
+            return I18n.t('errors.group_limit', 'Group limit must be blank or greater than 1')
+          }
+          return false;
         }
       },
       beforeSubmit: function(data) {
@@ -483,14 +493,16 @@ define([
       var $form = $("#edit_category_form").clone(true);
 
       // fill out form given the current category values
-      var data = $category.getTemplateData({textValues: ['category_name', 'self_signup', 'heterogenous']});
+      var data = $category.getTemplateData({textValues: ['category_name', 'self_signup', 'heterogenous', 'group_limit']});
       var form_data = {
         name: data.category_name,
         enable_self_signup: data.self_signup !== null && data.self_signup !== '',
-        restrict_self_signup: data.self_signup == 'restricted'
+        restrict_self_signup: data.self_signup == 'restricted',
+        group_limit: data.group_limit
       };
       $form.fillFormData(form_data, { object_name: 'category' });
       $form.find("#category_restrict_self_signup").prop('disabled', !form_data.enable_self_signup || data.heterogenous == 'true');
+      $form.find("#group_structure_self_signup_subcontainer").showIf( $form.find('#category_enable_self_signup').is(':checked') );
 
       $category.addClass('editing');
       $category.prepend($form.show());
@@ -616,6 +628,12 @@ define([
           if(found) {
             return I18n.t('errors.category_in_use', "\"%{category_name}\" is already in use", {category_name: val});
           }
+        },
+        'category[group_limit]': function(val, data) {
+          if (parseInt(val) <= 1) {
+            return I18n.t('errors.group_limit', 'Group limit must be blank or greater than 1')
+          }
+          return false;
         }
       },
       beforeSubmit: function(data) {
@@ -662,6 +680,7 @@ define([
       var heterogenous = $(this).parents('.group_category').find('.heterogenous').text() == 'true';
       var disable_restrict = !self_signup || heterogenous;
       $("#edit_category_form #category_restrict_self_signup").prop('disabled', disable_restrict);
+      $("#edit_category_form #group_structure_self_signup_subcontainer").showIf(self_signup);
       if (disable_restrict) {
         $("#edit_category_form #category_restrict_self_signup").prop('checked', false);
       }

spec/models/group_membership_spec.rb

@@ -48,6 +48,21 @@ describe GroupMembership do
     gm2.reload.should be_deleted
   end
 
+  it "should not be valid if the group is full" do
+    course
+    category = @course.group_categories.build(:name => "category 1")
+    category.group_limit = 2
+    category.save!
+    group = category.groups.create!(:context => @course)
+    # when the group is full
+    group.group_memberships.create!(:user => user_model, :workflow_state => 'accepted')
+    group.group_memberships.create!(:user => user_model, :workflow_state => 'accepted')
+    # expect
+    membership = group.group_memberships.build(:user => user_model, :workflow_state => 'accepted')
+    membership.should_not be_valid
+    membership.errors[:group_id].should == "The group is full."
+  end
+
   context "section homogeneity" do
     # can't use 'course' because it is defined in spec_helper, so use 'course1'
     let(:course1) { course_with_teacher(:active_all => true); @course }

spec/models/group_spec.rb

@@ -312,6 +312,25 @@ describe Group do
     end
   end
 
+  context "#full?" do
+    it "returns true when category group_limit has been met" do
+      @group.group_category = @course.group_categories.build(:name => 'foo')
+      @group.group_category.group_limit = 1
+      @group.add_user user_model, 'accepted'
+      @group.should be_full
+    end
+
+    it "returns false when category group_limit has not been met" do
+      # no category
+      @group.should_not be_full
+      # not full
+      @group.group_category = @course.group_categories.build(:name => 'foo')
+      @group.group_category.group_limit = 2
+      @group.add_user user_model, 'accepted'
+      @group.should_not be_full
+    end
+  end
+
   context "has_member?" do
     it "should be true for accepted memberships, regardless of moderator flag" do
       @user1 = user_model

spec/selenium/admin/account_admin_manage_groups_spec.rb

@@ -234,6 +234,7 @@ describe "account admin manage groups" do
       form.find_element(:id, "category_enable_self_signup").click
       wait_for_ajaximations
       is_checked('#category_enable_self_signup').should be_true
+      f('#group_structure_self_signup_subcontainer').should be_displayed
       submit_form(form)
       wait_for_ajaximations
       driver.find_element(:css, "#category_#{@courses_group_category.id} .self_signup_text").should include_text "Self sign-up is enabled"

spec/selenium/groups_spec.rb

@@ -38,6 +38,26 @@ describe "groups" do
     @student.group_memberships.first.should be_accepted
   end
 
+  it "should not allow students to join self-signup groups that are full" do
+    course_with_student_logged_in(:active_all => true)
+    category1 = @course.group_categories.create!(:name => "category 1")
+    category1.configure_self_signup(true, false)
+    category1.group_limit = 2
+    category1.save!
+    g1 = @course.groups.create!(:name => "some group", :group_category => category1)
+
+    g1.add_user user_model
+    g1.add_user user_model
+
+    get "/courses/#{@course.id}/groups"
+
+    group_div = f("#group_#{g1.id}")
+    f(".name", group_div).text.should == "some group"
+
+    f(".management a", group_div).should be_blank
+    f(".management", group_div).text.should == 'group full'
+  end
+
   it "should not show student organized, invite only groups" do
     course_with_student_logged_in(:active_all => true)
     g1 = @course.groups.create!(:name => "my group")

spec/selenium/helpers/manage_groups_common.rb

@@ -8,6 +8,10 @@ require File.expand_path(File.dirname(__FILE__) + '/../common')
     enable_self_signup = form.find_element(:css, "#category_enable_self_signup")
     enable_self_signup.click unless !!enable_self_signup.attribute('checked') == !!opts[:enable_self_signup]
 
+    if opts[:enable_self_signup] && opts[:group_limit]
+      replace_content f('#category_group_limit', form), opts[:group_limit]
+    end
+
     restrict_self_signup = form.find_element(:css, "#category_restrict_self_signup")
     restrict_self_signup.click unless !!restrict_self_signup.attribute('checked') == !!opts[:restrict_self_signup]
     if opts[:group_count]

spec/selenium/manage_groups_spec.rb

@@ -137,6 +137,14 @@ describe "manage groups" do
     new_category.groups.size.should == 2
   end
 
+  it "should honor group_limit when adding a self signup category" do
+    @course.enroll_student(user_model(:name => "John Doe"))
+    get "/courses/#{@course.id}/groups"
+    # submit new category form
+    new_category = add_category(@course, 'New Category', :enable_self_signup => true, :group_limit => '2')
+    new_category.group_limit.should == 2
+  end
+
   it "should preserve group to category association when editing a group" do
     groups_student_enrollment 3
     group_category = @course.group_categories.create(:name => "Existing Category")