From 1df895ecfeeb75e9e58e8bcf5c6bddf022bb96e6 Mon Sep 17 00:00:00 2001 From: ch-iv <108201575+ch-iv@users.noreply.github.com> Date: Thu, 3 Oct 2024 11:25:44 -0400 Subject: [PATCH] Fix renaming a group removes any associated submissions (#7224) --- Changelog.md | 1 + app/controllers/groups_controller.rb | 6 +- app/models/grouping.rb | 13 ++++ config/locales/views/groups/en.yml | 1 + spec/controllers/groups_controller_spec.rb | 70 +++++++++++++++++++++- spec/models/grouping_spec.rb | 8 +++ 6 files changed, 96 insertions(+), 3 deletions(-) diff --git a/Changelog.md b/Changelog.md index 9b3645e3ba..da42888226 100644 --- a/Changelog.md +++ b/Changelog.md @@ -9,6 +9,7 @@ ### 🐛 Bug fixes - Fix incorrect calculation of token penalties when submissions are on time (#7216) +- Fix bug where renaming a group to an existing group in a different assignment resulted in incorrect repository mapping (#7224) ### 🔧 Internal changes diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index b35b0b3079..434980f8c1 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -58,9 +58,9 @@ def rename_group @group = @grouping.group # Checking if a group with this name already exists - if (@groups = current_course.groups.where(group_name: params[:new_groupname]).first) + if (@existing_group = current_course.groups.where(group_name: params[:new_groupname]).first) existing = true - groupexist_id = @groups.id + groupexist_id = @existing_group.id end if existing @@ -74,6 +74,8 @@ def rename_group if Grouping.exists?(assessment_id: @assignment.id, group_id: groupexist_id) flash_now(:error, I18n.t('groups.group_name_already_in_use')) + elsif @grouping.has_submitted_files? || @grouping.has_non_empty_submission? + flash_now(:error, I18n.t('groups.group_name_already_in_use_diff_assignment')) else @grouping.update_attribute(:group_id, groupexist_id) end diff --git a/app/models/grouping.rb b/app/models/grouping.rb index 8e13ad89f1..0457cbd10a 100644 --- a/app/models/grouping.rb +++ b/app/models/grouping.rb @@ -754,6 +754,19 @@ def get_random_incomplete(current_role) results.where.not('groupings.id': self.id).order('RANDOM()').first&.grouping end + # Checks if a grouping uploaded any files + def has_submitted_files? + access_repo do |repo| + revision = repo.get_revision_by_timestamp(Time.current) + + files = revision.tree_at_path(assignment.repository_folder, with_attrs: false).select do |_, obj| + obj.is_a?(Repository::RevisionFile) && Repository.get_class.internal_file_names.exclude?(obj.name) + end + + return files.length > 0 + end + end + private # Takes in a collection of results specified by +results+, and filters them using +filter_data+. Assumes diff --git a/config/locales/views/groups/en.yml b/config/locales/views/groups/en.yml index e09222b70d..ede7fcc849 100644 --- a/config/locales/views/groups/en.yml +++ b/config/locales/views/groups/en.yml @@ -27,6 +27,7 @@ en: empty: Empty Group grace_day_over_limit: You have assigned one or more students who has less grace credits than the amount already used by group %{group} for this assignment. group_name_already_in_use: This name is already in use for this assignment. + group_name_already_in_use_diff_assignment: The group can't be renamed, because it would result in the loss of submission data. Try a name that is not in use for another assignment. help: Manage student groups. You can create new groups and manually add and remove students to groups. invalid_group_warning: 'Your group is currently invalid. You probably haven''t met the group size minimum. You may not be able to submit anything, and your work may not be graded, until you have met this minimum. ' invalidate_confirm: This will prevent this group from submitting, even with the required number of students. Are you sure? diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 9c26718b2e..2029a142da 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -1,6 +1,7 @@ describe GroupsController do # TODO: add 'role is from a different course' shared tests to each route test below let(:grouping) { create(:grouping) } + let(:group) { grouping.group } let(:assignment) { grouping.assignment } let(:course) { assignment.course } @@ -135,7 +136,74 @@ end end - describe '#rename_group' + describe '#rename_group' do + # rubocop:disable RSpec/LetSetup + let!(:another_assignment) { create(:assignment) } + let!(:placeholder_group) do + create(:group, assignments: [another_assignment], group_name: 'placeholder_group') + end + # rubocop:enable RSpec/LetSetup + + context 'default grouping' do + it 'should rename a group' do + # The id param expects grouping id, not group id. + expect do + post_as instructor, :rename_group, params: { + course_id: course.id, + assignment_id: assignment.id, + id: grouping.id, + new_groupname: 'renamed' + } + end.to change { group.reload.group_name }.from('group1').to('renamed') + end + end + + context 'grouping with submitted files' do + let!(:another_grouping) { create(:grouping, assignment: assignment) } + + before do + @file = create(:assignment_file, assignment: assignment) + another_grouping.group.access_repo do |repo| + txn = repo.get_transaction('markus') + assignment_folder = File.join(assignment.repository_folder, File::SEPARATOR) + begin + txn.add(File.join(assignment_folder, 'Shapes.java'), 'shapes content', 'text/plain') + repo.commit(txn) + end + end + end + + it 'flashes an error' do + post_as instructor, :rename_group, params: { + course_id: course.id, + assignment_id: assignment.id, + id: another_grouping.id, + new_groupname: 'placeholder_group' + } + + expect(flash[:error].map do |f| + extract_text f + end).to eq [I18n.t('groups.group_name_already_in_use_diff_assignment')] + end + end + + context 'grouping with a submission but no files' do + let!(:another_grouping) { create(:grouping_with_inviter_and_submission, assignment: assignment) } + + it 'flashes an error' do + post_as instructor, :rename_group, params: { + course_id: course.id, + assignment_id: assignment.id, + id: another_grouping.id, + new_groupname: 'placeholder_group' + } + + expect(flash[:error].map do |f| + extract_text f + end).to eq [I18n.t('groups.group_name_already_in_use_diff_assignment')] + end + end + end describe '#valid_grouping' do let(:unapproved_grouping) { create(:grouping_with_inviter, instructor_approved: false) } diff --git a/spec/models/grouping_spec.rb b/spec/models/grouping_spec.rb index 90ba87c3fb..e9b420978c 100644 --- a/spec/models/grouping_spec.rb +++ b/spec/models/grouping_spec.rb @@ -38,6 +38,10 @@ expect(grouping.has_submission?).to be false end + it 'does not have submitted files' do + expect(grouping.has_submitted_files?).to be false + end + context 'hidden students' do let(:hidden) { create(:student, hidden: true) } @@ -669,6 +673,10 @@ def expect_updated_criteria_coverage_count_eq(expected_count) expect(missing_files.length).to eq(0) end end + + it 'has submitted files' do + expect(@grouping.has_submitted_files?).to be true + end end describe '#has_submission?' do