From a5cc1bcf96d963e97bd4132525f5e74924f72139 Mon Sep 17 00:00:00 2001 From: Joe Tanner Date: Thu, 2 May 2013 09:18:22 -0600 Subject: [PATCH] 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 Tested-by: Jenkins QA-Review: Cam Theriault Product-Review: Joe Tanner --- .../group_categories_controller.rb | 1 + app/controllers/groups_controller.rb | 5 ++++ app/models/group.rb | 4 ++++ app/models/group_category.rb | 1 + app/models/group_membership.rb | 8 +++++++ app/views/groups/_add_group_category.html.erb | 4 ++++ app/views/groups/_category.html.erb | 4 ++++ .../groups/_edit_group_category.html.erb | 11 ++++++++- app/views/groups/_group.html.erb | 8 +++++-- ...15057_add_group_limit_to_group_category.rb | 11 +++++++++ public/javascripts/manage_groups.js | 23 +++++++++++++++++-- spec/models/group_membership_spec.rb | 15 ++++++++++++ spec/models/group_spec.rb | 19 +++++++++++++++ .../admin/account_admin_manage_groups_spec.rb | 1 + spec/selenium/groups_spec.rb | 20 ++++++++++++++++ spec/selenium/helpers/manage_groups_common.rb | 4 ++++ spec/selenium/manage_groups_spec.rb | 8 +++++++ 17 files changed, 142 insertions(+), 5 deletions(-) create mode 100644 db/migrate/20130430215057_add_group_limit_to_group_category.rb diff --git a/app/controllers/group_categories_controller.rb b/app/controllers/group_categories_controller.rb index afe4b73c122..14e474c0075 100644 --- a/app/controllers/group_categories_controller.rb +++ b/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 diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 8790177d881..36b4c3ac668 100644 --- a/app/controllers/groups_controller.rb +++ b/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' diff --git a/app/models/group.rb b/app/models/group.rb index d3b596735bb..324e390bee0 100644 --- a/app/models/group.rb +++ b/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 diff --git a/app/models/group_category.rb b/app/models/group_category.rb index 902aa9f36f7..b6fe9749626 100644 --- a/app/models/group_category.rb +++ b/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) diff --git a/app/models/group_membership.rb b/app/models/group_membership.rb index d9e85249e13..d68cb036d3a 100644 --- a/app/models/group_membership.rb +++ b/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 diff --git a/app/views/groups/_add_group_category.html.erb b/app/views/groups/_add_group_category.html.erb index 037e56fd794..796c83d49bb 100644 --- a/app/views/groups/_add_group_category.html.erb +++ b/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 => ''.html_safe, :wrapper => '' %>
+ <%= t :group_structure_group_limit, "*Limit groups to* %{group_limit} members", + :group_limit => ''.html_safe, + :wrapper => '' %> + <%= t :group_limit_blank, '(leave blank for no limit)' %> diff --git a/app/views/groups/_category.html.erb b/app/views/groups/_category.html.erb index f72cf41aa68..0bda653b644 100644 --- a/app/views/groups/_category.html.erb +++ b/app/views/groups/_category.html.erb @@ -6,6 +6,7 @@
<%= category && category.has_heterogenous_group? %> + <%= category && category.group_limit %>
"> @@ -33,6 +34,9 @@ <%= 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?")) %> +

+ <%= t 'group_limit_blurb', 'Groups are limited to *%{count}* members', :count => (category && category.group_limit), :wrapper => '\1' %> +

diff --git a/app/views/groups/_edit_group_category.html.erb b/app/views/groups/_edit_group_category.html.erb index e2860bce748..80e81e6ffb9 100644 --- a/app/views/groups/_edit_group_category.html.erb +++ b/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?")) %> -
<%= 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") %>
+
+ <%= check_box_tag 'category[restrict_self_signup]', "1", false, :disabled => true, :id => 'category_restrict_self_signup' %> + +
+
diff --git a/app/views/groups/_group.html.erb b/app/views/groups/_group.html.erb index a83d4d0f416..1b0ebcceae0 100644 --- a/app/views/groups/_group.html.erb +++ b/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 %> - <%= link_to(management_text, management_url) %> + <%= disabled ? content_tag(:span, management_text, :class => 'muted') : link_to(management_text, management_url) %> <% end %>
diff --git a/db/migrate/20130430215057_add_group_limit_to_group_category.rb b/db/migrate/20130430215057_add_group_limit_to_group_category.rb new file mode 100644 index 00000000000..8912ec28320 --- /dev/null +++ b/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 diff --git a/public/javascripts/manage_groups.js b/public/javascripts/manage_groups.js index 7e0fdfdea1b..b52dc8c401e 100644 --- a/public/javascripts/manage_groups.js +++ b/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); } diff --git a/spec/models/group_membership_spec.rb b/spec/models/group_membership_spec.rb index 9e4b6d667a7..b72f2797416 100644 --- a/spec/models/group_membership_spec.rb +++ b/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 } diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 1ed2ab06557..85bae7139c3 100644 --- a/spec/models/group_spec.rb +++ b/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 diff --git a/spec/selenium/admin/account_admin_manage_groups_spec.rb b/spec/selenium/admin/account_admin_manage_groups_spec.rb index 6f029480fa2..c610fca8e4c 100644 --- a/spec/selenium/admin/account_admin_manage_groups_spec.rb +++ b/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" diff --git a/spec/selenium/groups_spec.rb b/spec/selenium/groups_spec.rb index d244cc8e095..033501ed935 100644 --- a/spec/selenium/groups_spec.rb +++ b/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") diff --git a/spec/selenium/helpers/manage_groups_common.rb b/spec/selenium/helpers/manage_groups_common.rb index b001c733bd7..e3423242d20 100644 --- a/spec/selenium/helpers/manage_groups_common.rb +++ b/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] diff --git a/spec/selenium/manage_groups_spec.rb b/spec/selenium/manage_groups_spec.rb index 069dd57f053..edfa21163a8 100644 --- a/spec/selenium/manage_groups_spec.rb +++ b/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")