allow users to manage group membership limits

fixes CNVS-11830
fixes CNVS-11831

test plan

 - add a course group
 - set the max members in the group creation dialog
 - verify that the limit is enforced

 - enforced it a course group
 - change the max members in the group edit dialog
 - verify that the new limit is enforced

 - edit the group set
 - change the max members limit for the group set
 - verify that the limit is changed for all of the groups in the set

Change-Id: I6f112d29d6c5ca662da1598e29db2f3081f30148
Reviewed-on: https://gerrit.instructure.com/33722
Reviewed-by: Joel Hough <joel@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>
QA-Review: Trevor deHaan <tdehaan@instructure.com>
Product-Review: Drew Bowman <dbowman@instructure.com>

app/coffeescripts/models/Group.coffee

@@ -51,8 +51,16 @@ define [
       else
         "/api/v1/groups/#{@id}"
 
+    theLimit: ->
+      max_membership = @get('max_membership')
+      max_membership or @collection?.category?.get('group_limit')
+
     isFull: ->
-      limit = @collection?.category?.get 'group_limit'
+      limit = @get('max_membership')
+      (!limit and @groupCategoryLimitMet()) or (limit and @get('members_count') >= limit)
+
+    groupCategoryLimitMet: ->
+      limit = @collection?.category?.get('group_limit')
       limit and @get('members_count') >= limit
 
     isLocked: ->

app/coffeescripts/models/GroupCategory.coffee

@@ -18,6 +18,11 @@ define [
       super
       if groups = @get('groups')
         @groups groups
+      @on 'change:group_limit', @updateGroups
+
+    updateGroups: ->
+      if @_groups
+        @_groups.fetch()
 
     groups: (models = null) ->
       @_groups = new GroupCollection models,

app/coffeescripts/views/groups/manage/GroupCategoryEditView.coffee

@@ -45,6 +45,10 @@ define [
       @$selfSignupControls.css opacity: if disabled then 0.5 else 1
       @$selfSignupControls.find(':input').prop 'disabled', disabled
 
+    validateFormData: (data, errors) ->
+      if !@$("[name=group_limit]")[0].validity.valid
+        {"group_limit": [{message: I18n.t('group_limit_number', 'Group limit must be a number') }]}
+
     toJSON: ->
       json = @model.present()
       _.extend {},

app/coffeescripts/views/groups/manage/GroupDetailView.coffee

@@ -27,10 +27,16 @@ define [
 
     summary: ->
       count = @model.usersCount()
-      if ENV.group_user_type is 'student'
-        I18n.t "student_count", "student", count: count
+      if @model.theLimit()
+        if ENV.group_user_type is 'student'
+          I18n.t "student_count_max", "%{count} / %{max} students", count: count, max: @model.theLimit()
+        else
+          I18n.t "user_count_max", "%{count} / %{max} users", count: count, max: @model.theLimit()
       else
-        I18n.t "user_count", "user", count: count
+        if ENV.group_user_type is 'student'
+          I18n.t "student_count", "student", count: count
+        else
+          I18n.t "user_count", "user", count: count
 
     editGroup: (e) =>
       e.preventDefault()

app/coffeescripts/views/groups/manage/GroupEditView.coffee

@@ -27,6 +27,10 @@ define [
     translations:
       too_long: I18n.t "name_too_long", "Name is too long"
 
+    validateFormData: (data, errors) ->
+      if !$("[name=max_membership]")[0].validity.valid
+        {"max_membership": [{message: I18n.t('max_membership_number', 'Max membership must be a number') }]}
+
     openAgain: ->
       super
       # reset the form contents

app/coffeescripts/views/groups/manage/GroupView.coffee

@@ -42,6 +42,7 @@ define [
       @users = @model.users()
       @model.on 'destroy', @remove, this
       @model.on 'change:members_count', @updateFullState, this
+      @model.on 'change:max_membership', @updateFullState, this
 
     afterRender: ->
       @$el.toggleClass 'group-expanded', @expanded

app/coffeescripts/views/groups/manage/UnassignedUsersView.coffee

@@ -35,6 +35,7 @@ define [
 
     attach: ->
       @collection.on 'reset', @render
+      @collection.on 'remove', @render
       @collection.on 'moved', @highlightUser
       @collection.on 'filterOut', _(=> @checkScroll()).debounce(50)
 

app/controllers/groups_controller.rb

@@ -139,7 +139,7 @@ class GroupsController < ApplicationController
   include Api::V1::Group
   include Api::V1::GroupCategory
 
-  SETTABLE_GROUP_ATTRIBUTES = %w(name description join_level is_public group_category avatar_attachment storage_quota_mb)
+  SETTABLE_GROUP_ATTRIBUTES = %w(name description join_level is_public group_category avatar_attachment storage_quota_mb max_membership)
 
   include TextHelper
 
@@ -192,7 +192,7 @@ class GroupsController < ApplicationController
   #  Only include groups that are in this type of context.
   #
   # @example_request
-  #     curl https://<canvas>/api/v1/users/self/groups?context_type=Account \ 
+  #     curl https://<canvas>/api/v1/users/self/groups?context_type=Account \
   #          -H 'Authorization: Bearer <token>'
   #
   # @returns [Group]
@@ -225,7 +225,7 @@ class GroupsController < ApplicationController
   # Returns the list of active groups in the given context that are visible to user.
   #
   # @example_request
-  #     curl https://<canvas>/api/v1/courses/1/groups \ 
+  #     curl https://<canvas>/api/v1/courses/1/groups \
   #          -H 'Authorization: Bearer <token>'
   #
   # @returns [Group]
@@ -300,7 +300,7 @@ class GroupsController < ApplicationController
   # the rights to see it.
   #
   # @example_request
-  #     curl https://<canvas>/api/v1/groups/<group_id> \ 
+  #     curl https://<canvas>/api/v1/groups/<group_id> \
   #          -H 'Authorization: Bearer <token>'
   #
   # @argument include[] [String, "permissions"]
@@ -392,11 +392,11 @@ class GroupsController < ApplicationController
   #   ignored if the caller does not have the manage_storage_quotas permission.
   #
   # @example_request
-  #     curl https://<canvas>/api/v1/groups \ 
-  #          -F 'name=Math Teachers' \ 
-  #          -F 'description=A place to gather resources for our classes.' \ 
-  #          -F 'is_public=true' \ 
-  #          -F 'join_level=parent_context_auto_join' \ 
+  #     curl https://<canvas>/api/v1/groups \
+  #          -F 'name=Math Teachers' \
+  #          -F 'description=A place to gather resources for our classes.' \
+  #          -F 'is_public=true' \
+  #          -F 'join_level=parent_context_auto_join' \
   #          -H 'Authorization: Bearer <token>'
   #
   # @returns Group
@@ -472,10 +472,10 @@ class GroupsController < ApplicationController
   #   ignored if the caller does not have the manage_storage_quotas permission.
   #
   # @example_request
-  #     curl https://<canvas>/api/v1/groups/<group_id> \ 
-  #          -X PUT \ 
-  #          -F 'name=Algebra Teachers' \ 
-  #          -F 'join_level=parent_context_request' \ 
+  #     curl https://<canvas>/api/v1/groups/<group_id> \
+  #          -X PUT \
+  #          -F 'name=Algebra Teachers' \
+  #          -F 'join_level=parent_context_request' \
   #          -H 'Authorization: Bearer <token>'
   #
   # @returns Group
@@ -513,8 +513,8 @@ class GroupsController < ApplicationController
   # Deletes a group and removes all members.
   #
   # @example_request
-  #     curl https://<canvas>/api/v1/groups/<group_id> \ 
-  #          -X DELETE \ 
+  #     curl https://<canvas>/api/v1/groups/<group_id> \
+  #          -X DELETE \
   #          -H 'Authorization: Bearer <token>'
   #
   # @returns Group
@@ -547,8 +547,8 @@ class GroupsController < ApplicationController
   #   An array of email addresses to be sent invitations.
   #
   # @example_request
-  #     curl https://<canvas>/api/v1/groups/<group_id>/invite \ 
-  #          -F 'invitees[]=leonard@example.com&invitees[]=sheldon@example.com' \ 
+  #     curl https://<canvas>/api/v1/groups/<group_id>/invite \
+  #          -F 'invitees[]=leonard@example.com&invitees[]=sheldon@example.com' \
   #          -H 'Authorization: Bearer <token>'
   def invite
     find_group

app/models/group.rb

@@ -76,6 +76,7 @@ class Group < ActiveRecord::Base
   before_validation :ensure_defaults
   before_save :maintain_category_attribute
   after_save :close_memberships_if_deleted
+  after_save :update_max_membership_from_group_category
 
   include StickySisFields
   are_sis_sticky :name
@@ -88,6 +89,11 @@ class Group < ActiveRecord::Base
     end
   end
 
+  validates_each :max_membership do |record, attr, value|
+    next if value.nil?
+    record.errors.add attr, t(:greater_than_1, "Must be greater than 1") unless value.to_i > 1
+  end
+
   alias_method :participating_users_association, :participating_users
 
   def participating_users(user_ids = nil)
@@ -102,13 +108,13 @@ class Group < ActiveRecord::Base
   alias_method_chain :wiki, :create
 
   def auto_accept?
-    self.group_category && 
+    self.group_category &&
     self.group_category.allows_multiple_memberships? &&
     self.join_level == 'parent_context_auto_join'
   end
 
   def allow_join_request?
-    self.group_category && 
+    self.group_category &&
     self.group_category.allows_multiple_memberships? &&
     ['parent_context_auto_join', 'parent_context_request'].include?(self.join_level)
   end
@@ -120,9 +126,25 @@ class Group < ActiveRecord::Base
   end
 
   def full?
+    !student_organized? && ((!max_membership && group_category_limit_met?) || (max_membership && participating_users.size >= max_membership))
+  end
+
+  def group_category_limit_met?
     group_category && group_category.group_limit && participating_users.size >= group_category.group_limit
   end
 
+  def student_organized?
+    group_category && group_category.student_organized?
+  end
+  private :group_category_limit_met?
+
+  def update_max_membership_from_group_category
+    if group_category && group_category.group_limit && (!max_membership || max_membership == 0)
+      self.max_membership = group_category.group_limit
+      self.save
+    end
+  end
+
   def free_association?(user)
     auto_accept? || allow_join_request? || allow_self_signup?(user)
   end
@@ -387,11 +409,11 @@ class Group < ActiveRecord::Base
     can :create_collaborations and
     can :create_conferences and
     can :manage_calendar and
-    can :manage_content and 
+    can :manage_content and
     can :manage_files and
     can :manage_wiki and
     can :post_to_forum and
-    can :read and 
+    can :read and
     can :read_roster and
     can :send_messages and
     can :send_messages_all and
@@ -437,7 +459,7 @@ class Group < ActiveRecord::Base
     given { |user, session| self.context && self.context.grants_right?(user, session, :view_group_pages) }
     can :read and can :read_roster
 
-    # Participate means the user is connected to the group somehow and can be  
+    # Participate means the user is connected to the group somehow and can be
     given { |user| user && can_participate?(user) }
     can :participate
 
@@ -504,11 +526,11 @@ class Group < ActiveRecord::Base
   def storage_quota_mb
     quota / 1.megabyte
   end
-  
+
   def storage_quota_mb=(val)
     self.storage_quota = val.try(:to_i).try(:megabytes)
   end
-  
+
   TAB_HOME, TAB_PAGES, TAB_PEOPLE, TAB_DISCUSSIONS, TAB_FILES,
     TAB_CONFERENCES, TAB_ANNOUNCEMENTS, TAB_PROFILE, TAB_SETTINGS, TAB_COLLABORATIONS = *1..20
   def tabs_available(user=nil, opts={})
@@ -527,7 +549,7 @@ class Group < ActiveRecord::Base
     available_tabs << { :id => TAB_CONFERENCES, :label => t('#tabs.conferences', "Conferences"), :css_class => 'conferences', :href => :group_conferences_path } if user && self.grants_right?(user, nil, :read)
     available_tabs << { :id => TAB_COLLABORATIONS, :label => t('#tabs.collaborations', "Collaborations"), :css_class => 'collaborations', :href => :group_collaborations_path } if user && self.grants_right?(user, nil, :read)
     if root_account.try(:canvas_network_enabled?) && user && grants_right?(user, nil, :manage)
-      available_tabs << { :id => TAB_SETTINGS, :label => t('#tabs.settings', 'Settings'), :css_class => 'settings', :href => :edit_group_path } 
+      available_tabs << { :id => TAB_SETTINGS, :label => t('#tabs.settings', 'Settings'), :css_class => 'settings', :href => :edit_group_path }
     end
     available_tabs
   end

app/models/group_category.rb

@@ -31,6 +31,7 @@ class GroupCategory < ActiveRecord::Base
   EXPORTABLE_ASSOCIATIONS = [:context, :groups, :assignments]
 
   after_save :auto_create_groups
+  after_update :update_groups_max_membership
 
   validates_each :name do |record, attr, value|
     next unless record.name_changed? || value.blank?
@@ -406,4 +407,9 @@ class GroupCategory < ActiveRecord::Base
     current_progress.reload
   end
 
+  def update_groups_max_membership
+    if group_limit_changed?
+      groups.update_all(:max_membership => group_limit)
+    end
+  end
 end

app/views/jst/groups/manage/groupCategoryEdit.handlebars

@@ -14,7 +14,6 @@
       </label>
       {{> [groups/manage/selfSignupHelp]}}
     </div>
-    <input name="group_limit" type="hidden" value="">
     <div class="self-signup-controls">
       <div class="control-group">
         <label class="checkbox" for="restrict_self_signup">
@@ -31,6 +30,13 @@
         <span class="hint-text">
           ({{#t "leave_group_limit_blank"}}Leave blank for no limit{{/t}})
         </span>
+        <div>
+          <strong>
+            <em>
+              {{#t "group_limit_override"}}Changing this will override any individually set group limits{{/t}}
+            </em>
+          </strong>
+        </div>
       </div>
     </div>
   </fieldset>

app/views/jst/groups/manage/groupEdit.handlebars

@@ -15,6 +15,15 @@
     </select>
   </div>
 </div>
+{{else}}
+<div class="control-group">
+  <label class="control-label" for="group_max_membership">{{#t "group_max_membership"}}Limit groups to{{/t}}</label>
+  <div class="controls">
+    <input type="number" id="group_max_membership" name='max_membership' value="{{max_membership}}" class="input-mini">
+    <span>{{#t "group_max_membership_members"}}members{{/t}}</span>
+    <span class="hint-text">{{#t "group_max_membership_help"}}(Leave blank to use group set max){{/t}}</span>
+  </div>
+</div>
 {{/ifEqual}}
 
 <div class="form-controls">
@@ -23,4 +32,4 @@
   <button class="btn btn-primary"
           data-text-while-loading='{{#t "saving"}}Saving...{{/t}}'
           type="submit">{{#t "save"}}Save{{/t}}</button>
-</div>
+</div>

lib/api/v1/group.rb

@@ -21,7 +21,7 @@ module Api::V1::Group
   include Api::V1::Context
 
   API_GROUP_JSON_OPTS = {
-    :only => %w(id name description is_public join_level group_category_id),
+    :only => %w(id name description is_public join_level group_category_id max_membership),
     :methods => %w(members_count storage_quota_mb),
   }
 

spec/apis/v1/groups_api_spec.rb

@@ -28,6 +28,7 @@ describe "Groups API", type: :request do
       'is_public' => group.is_public,
       'join_level' => group.join_level,
       'members_count' => group.members_count,
+      'max_membership' => group.max_membership,
       'avatar_url' => group.avatar_attachment && "http://www.example.com/images/thumbnails/#{group.avatar_attachment.id}/#{group.avatar_attachment.uuid}",
       'context_type' => group.context_type,
       "#{group.context_type.downcase}_id" => group.context_id,

spec/models/group_category_spec.rb

@@ -266,6 +266,20 @@ describe GroupCategory do
     end
   end
 
+  describe "max_membership_change" do
+    it "should update groups if the group limit changed" do
+      course_with_teacher(:active_all => true)
+      category = group_category
+      category.group_limit = 2
+      category.save
+      group = category.groups.create(:context => @course)
+      group.max_membership.should == 2
+      category.group_limit = 4
+      category.save
+      group.reload.max_membership.should == 4
+    end
+  end
+
   describe "group_for" do
     before :each do
       course_with_teacher(:active_all => true)

spec/models/group_spec.rb

@@ -19,7 +19,7 @@
 require File.expand_path(File.dirname(__FILE__) + '/../spec_helper.rb')
 
 describe Group do
-  
+
   before do
     course_model
     group_model(:context => @course)
@@ -30,11 +30,11 @@ describe Group do
       group_model
     end
   end
-  
+
   it "should have a wiki" do
     @group.wiki.should_not be_nil
   end
-  
+
   it "should be private by default" do
     @group.is_public.should be_false
   end
@@ -54,7 +54,7 @@ describe Group do
     @group.save.should be_false
     @group.reload.is_public.should be_true
   end
-  
+
   context "#peer_groups" do
     it "should find all peer groups" do
       context = course_model
@@ -70,7 +70,7 @@ describe Group do
       group1.peer_groups.should_not be_include(group1)
       group1.peer_groups.should_not be_include(group4)
     end
-    
+
     it "should not find peer groups for student organized groups" do
       context = course_model
       group_category = GroupCategory.student_organized_for(context)
@@ -79,19 +79,19 @@ describe Group do
       group1.peer_groups.should be_empty
     end
   end
-  
+
   context "atom" do
     it "should have an atom name as it's own name" do
       group_model(:name => 'some unique name')
       @group.to_atom.title.should eql('some unique name')
     end
-    
+
     it "should have a link to itself" do
       link = @group.to_atom.links.first.to_s
       link.should eql("/groups/#{@group.id}")
     end
   end
-  
+
   context "add_user" do
     it "should be able to add a person to the group" do
       user_model
@@ -99,7 +99,7 @@ describe Group do
       @group.add_user(@user)
       @group.users.should be_include(@user)
     end
-    
+
     it "shouldn't be able to add a person to the group twice" do
       user_model
       pseudonym_model(:user_id => @user.id)
@@ -111,7 +111,7 @@ describe Group do
       @group.users.should be_include(@user)
       @group.users.count.should == 1
     end
-    
+
     it "should remove that user from peer groups" do
       context = course_model
       group_category = context.group_categories.create!(:name => "worldCup")
@@ -121,7 +121,7 @@ describe Group do
       pseudonym_model(:user_id => @user.id)
       group1.add_user(@user)
       group1.users.should be_include(@user)
-      
+
       group2.add_user(@user)
       group2.users.should be_include(@user)
       group1.reload
@@ -320,6 +320,22 @@ describe Group do
       @group.should be_full
     end
 
+    it "returns true when max_membership has been met" do
+      @group.group_category = @course.group_categories.build(:name => 'foo')
+      @group.group_category.group_limit = 0
+      @group.max_membership = 1
+      @group.add_user user_model, 'accepted'
+      @group.should be_full
+    end
+
+    it "returns false when max_membership has not been met" do
+      @group.group_category = @course.group_categories.build(:name => 'foo')
+      @group.group_category.group_limit = 0
+      @group.max_membership = 2
+      @group.add_user user_model, 'accepted'
+      @group.should_not be_full
+    end
+
     it "returns false when category group_limit has not been met" do
       # no category
       @group.should_not be_full
@@ -593,4 +609,19 @@ describe Group do
       end
     end
   end
+
+  describe "#update_max_membership_from_group_category" do
+    it "should set max_membership if there is a group category" do
+      @group.group_category = @course.group_categories.build(:name => 'foo')
+      @group.group_category.group_limit = 1
+      @group.update_max_membership_from_group_category
+      @group.max_membership.should == 1
+    end
+
+    it "should do nothing if there is no group category" do
+      @group.max_membership.should be_nil
+      @group.update_max_membership_from_group_category
+      @group.max_membership.should be_nil
+    end
+  end
 end