diff --git a/Changelog.md b/Changelog.md index aa3adc9881..5208ffe8cd 100644 --- a/Changelog.md +++ b/Changelog.md @@ -7,6 +7,7 @@ ### ✨ New features and improvements - Enable test results downloads through the API (#7754) - Provide suggestions for partial student matching scans (#7760) +- Allow inactive students to join groups (#7757) ### 🐛 Bug fixes diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 76b3e2ae35..78ce50cb87 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -306,6 +306,7 @@ def upload group_rows << row.compact_blank end + if result[:invalid_lines].empty? @current_job = CreateGroupsJob.perform_later assignment, group_rows session[:job_id] = @current_job.job_id @@ -489,8 +490,8 @@ def invite_member if errors.blank? to_invite.each do |i| i = i.strip - invited_user = current_course.students.joins(:user).where(hidden: false).find_by('users.user_name': i) - if invited_user.receives_invite_emails? + invited_user = current_course.students.joins(:user).find_by('users.user_name': i) + if invited_user&.receives_invite_emails? NotificationMailer.with(inviter: current_role, invited: invited_user, grouping: @grouping).grouping_invite_email.deliver_later @@ -699,9 +700,6 @@ def add_member(student, grouping, assignment) end @bad_user_names = [] - if student.hidden - raise I18n.t('groups.invite_member.errors.not_found', user_name: student.user_name) - end if student.has_accepted_grouping_for?(assignment.id) raise I18n.t('groups.invite_member.errors.already_grouped', user_name: student.user_name) end diff --git a/app/models/grouping.rb b/app/models/grouping.rb index 5040f41451..b907b23c53 100644 --- a/app/models/grouping.rb +++ b/app/models/grouping.rb @@ -288,7 +288,7 @@ def invite(members, all_errors = [] members.each do |m| m = m.strip - user = course.students.joins(:user).where(hidden: false).find_by('users.user_name': m) + user = course.students.joins(:user).find_by('users.user_name': m) begin if user.nil? raise I18n.t('groups.invite_member.errors.not_found', user_name: m) @@ -305,7 +305,7 @@ def invite(members, # Add a new member to base def add_member(role, set_membership_status = StudentMembership::STATUSES[:accepted]) - if role.has_accepted_grouping_for?(self.assessment_id) || role.hidden + if role.has_accepted_grouping_for?(self.assessment_id) nil else member = StudentMembership.new(role: role, membership_status: @@ -329,6 +329,8 @@ def add_member(role, set_membership_status = StudentMembership::STATUSES[:accept def can_invite?(role) if self.inviter == role raise I18n.t('groups.invite_member.errors.inviting_self') + elsif role.hidden + raise I18n.t('groups.invite_member.errors.inactive_student', user_name: role.user_name) elsif !extension.nil? raise I18n.t('groups.invite_member.errors.extension_exists') elsif self.student_membership_number >= self.assignment.group_max diff --git a/config/locales/views/groups/en.yml b/config/locales/views/groups/en.yml index 1dfb51543a..f307e9eb0c 100644 --- a/config/locales/views/groups/en.yml +++ b/config/locales/views/groups/en.yml @@ -42,6 +42,7 @@ en: extension_exists: You cannot modify a group with an extension to the due date. Please contact your instructor if you wish to make changes. group_max_reached: 'Could not invite %{user_name}: group maximum has been reached.' inactive_grader: Cannot assign inactive grader(s) %{user_names}. + inactive_student: 'Could not invite %{user_name}: this student is inactive.' inviting_self: You cannot invite yourself to your own group. need_to_create_group: You must create a group before you can invite members. not_found: No student '%{user_name}' could be found. diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 3f47e9eee5..ce27a90f89 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -532,7 +532,8 @@ # Setup for Git Repository allow(Settings.repository).to receive(:type).and_return('git') - @assignment = create(:assignment) + # Create assignment with group_max of 3 to accommodate test files + @assignment = create(:assignment, assignment_properties_attributes: { group_max: 3 }) # Create students corresponding to the file_good @student_user_names = %w[c8shosta c5bennet] @@ -570,6 +571,67 @@ expect(flash[:error]).not_to be_blank expect(response).to redirect_to(action: 'index') end + + it 'accepts groups within the maximum group size' do + @assignment.update!(assignment_properties_attributes: { group_max: 2 }) + # Students c8shosta and c5bennet are already created in the outer before block + expect do + post_as instructor, :upload, params: { + course_id: course.id, + assignment_id: @assignment.id, + upload_file: fixture_file_upload('groups/form_good.csv', 'text/csv') + } + end.to have_enqueued_job(CreateGroupsJob) + + expect(response).to have_http_status(:found) + expect(flash[:error]).to be_blank + expect(response).to redirect_to(action: 'index') + end + + it 'accepts single-member groups' do + @assignment.update!(assignment_properties_attributes: { group_max: 1 }) + create(:student, user: create(:end_user, user_name: 'solo_student')) + + csv_content = "group1,solo_student\n" + file = Tempfile.new(['test_upload', '.csv']) + file.write(csv_content) + file.rewind + + expect do + post_as instructor, :upload, params: { course_id: course.id, + assignment_id: @assignment.id, + upload_file: fixture_file_upload(file.path, 'text/csv') } + end.to have_enqueued_job(CreateGroupsJob) + + file.close + file.unlink + + expect(flash[:error]).to be_blank + end + + context 'when uploading groups with inactive students' do + before do + @assignment.update!(assignment_properties_attributes: { group_max: 2 }) + # Create an active student + @active_student = create(:student, user: create(:end_user, user_name: 'active_student')) + # Create an inactive student (hidden: true) + @inactive_student = create(:student, hidden: true, user: create(:end_user, user_name: 'inactive_student')) + end + + it 'allows importing groups with inactive students (ISSUE-7743)' do + expect do + post_as instructor, :upload, params: { + course_id: course.id, + assignment_id: @assignment.id, + upload_file: fixture_file_upload('groups/form_with_inactive_students.csv', 'text/csv') + } + end.to have_enqueued_job(CreateGroupsJob) + + expect(response).to have_http_status(:found) + expect(flash[:error]).to be_blank + expect(response).to redirect_to(action: 'index') + end + end end describe '#create_groups_when_students_work_alone' do diff --git a/spec/fixtures/files/groups/form_group_size_exceeded.csv b/spec/fixtures/files/groups/form_group_size_exceeded.csv new file mode 100644 index 0000000000..9c3dec65d4 --- /dev/null +++ b/spec/fixtures/files/groups/form_group_size_exceeded.csv @@ -0,0 +1 @@ +group1,student1,student2,student3 diff --git a/spec/fixtures/files/groups/form_multiple_oversized.csv b/spec/fixtures/files/groups/form_multiple_oversized.csv new file mode 100644 index 0000000000..22af17ea03 --- /dev/null +++ b/spec/fixtures/files/groups/form_multiple_oversized.csv @@ -0,0 +1,5 @@ +group1,student1,student2,student3 +group2,student4,student5 +group3,student6,student7,student8 +group4,student9,student10,student11 +group5,student12,student13,student14 diff --git a/spec/fixtures/files/groups/form_with_inactive_students.csv b/spec/fixtures/files/groups/form_with_inactive_students.csv new file mode 100644 index 0000000000..6d223a0760 --- /dev/null +++ b/spec/fixtures/files/groups/form_with_inactive_students.csv @@ -0,0 +1 @@ +group1,active_student,inactive_student diff --git a/spec/models/grouping_spec.rb b/spec/models/grouping_spec.rb index c186548234..feddf8f5ad 100644 --- a/spec/models/grouping_spec.rb +++ b/spec/models/grouping_spec.rb @@ -45,15 +45,25 @@ context 'hidden students' do let(:hidden) { create(:student, hidden: true) } - it 'cannot be invited' do - grouping.invite(hidden.user.user_name) - expect(grouping.memberships.count).to eq(0) + it 'can be invited by instructors' do + grouping.invite(hidden.user.user_name, StudentMembership::STATUSES[:accepted], invoked_by_instructor: true) + expect(grouping.memberships.count).to eq(1) + expect(grouping.memberships.first.role).to eq(hidden) end - it 'cannot be added' do - grouping.add_member(hidden) + it 'cannot be invited by students' do + errors = grouping.invite(hidden.user.user_name) + expect(errors).not_to be_empty + expect(errors.first).to include('inactive') expect(grouping.memberships.count).to eq(0) end + + it 'can be added' do + result = grouping.add_member(hidden) + expect(result).not_to be_nil + expect(grouping.memberships.count).to eq(1) + expect(grouping.memberships.first.role).to eq(hidden) + end end it 'displays Empty Group since no students in the group' do