Fix renaming a group removes any associated submissions (#7224)

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
 

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

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

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?

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) }

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