Closes #627

MarkUsProject · Feb 21, 2012 · d202ba9 · d202ba9
1 parent 968b2a1
commit d202ba9
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 36 deletions.
diff --git a/app/controllers/assignments_controller.rb b/app/controllers/assignments_controller.rb
@@ -48,7 +48,15 @@ def student_interface
     if @grouping.nil?
       if @assignment.group_max == 1
         begin
-          @student.create_group_for_working_alone_student(@assignment.id)
+          # fix for issue #627
+          # currently create_group_for_working_alone_student only returns false
+          # when saving a grouping throws an exception
+          if !@student.create_group_for_working_alone_student(@assignment.id)
+            # if create_group_for_working_alone_student returned false then the student
+            # must have an ( empty ) existing grouping that he is not a member of.
+            # we must delete this grouping for the transaction to succeed.
+            Grouping.find_by_group_id_and_assignment_id( Group.find_by_group_name(@student.user_name), @assignment.id).destroy
+          end
         rescue RuntimeError => @error
           render 'shared/generic_error', :layout => 'error'
           return
@@ -319,7 +327,15 @@ def creategroup
           raise I18n.t('create_group.fail.can_not_work_alone',
                         :group_min => @assignment.group_min)
         end
-        @student.create_group_for_working_alone_student(@assignment.id)
+        # fix for issue #627
+        # currently create_group_for_working_alone_student only returns false
+        # when saving a grouping throws an exception
+        if !@student.create_group_for_working_alone_student(@assignment.id)
+          # if create_group_for_working_alone_student returned false then the student
+          # must have an ( empty ) existing grouping that he is not a member of.
+          # we must delete this grouping for the transaction to succeed.
+          Grouping.find_by_group_id_and_assignment_id( Group.find_by_group_name(@student.user_name), @assignment.id).destroy
+        end
       else
         @student.create_autogenerated_name_group(@assignment.id)
       end

diff --git a/app/models/student.rb b/app/models/student.rb
@@ -131,20 +131,16 @@ def create_group_for_working_alone_student(aid)
       end
 
       @grouping.group = @group
-
       begin
-       if !@grouping.save
-         m_logger = MarkusLogger.instance
-         m_logger.log("Could not create a grouping for Student '#{self.user_name}'. The grouping was:  #{@grouping.inspect} - errors: #{@grouping.errors.inspect}", MarkusLogger::ERROR)
-         raise "Sorry!  For some reason, your grouping could not be created.  Please wait a few seconds, and hit refresh to try again.  If you come back to this page, you should inform the course instructor."
+        if !@grouping.save
+          m_logger = MarkusLogger.instance
+          m_logger.log("Could not create a grouping for Student '#{self.user_name}'. The grouping was:  #{@grouping.inspect} - errors: #{@grouping.errors.inspect}", MarkusLogger::ERROR)
+          raise "Sorry!  For some reason, your grouping could not be created.  Please wait a few seconds, and hit refresh to try again.  If you come back to this page, you should inform the course instructor."
         end
       # This exception will only be thrown when we try to save to a grouping that already exists
       rescue ActiveRecord::RecordNotUnique => e
-
-        # See Github issue #627
-        # If grouping.save fails then the @grouping.id will not be set properly, but we need it to set the membership bellow
-        # WARNING: a grouping can be retrieved uniquely only by its id or a combination of group_id and assignment_id
-        @grouping.id = Grouping.find_by_group_id_and_assignment_id(@grouping.group_id, aid).id
+        # transaction has failed, so quit it
+        return false
       end
 
       # We give students the tokens for the test framework
@@ -154,13 +150,9 @@ def create_group_for_working_alone_student(aid)
       @member = StudentMembership.new(:grouping_id => @grouping.id,
               :membership_status => StudentMembership::STATUSES[:inviter],
               :user_id => self.id)
+      @member.save
 
-      if !@member.save
-        m_logger = MarkusLogger.instance
-        m_logger.log("Could not create a membership for Student '#{self.user_name}'. The membership was:  #{@member.inspect} - errors: #{@member.errors.inspect}", MarkusLogger::ERROR)
-      end
-
-      # Destroy all the other memebrships for this assignment
+      # Destroy all the other memberships for this assignment
       self.destroy_all_pending_memberships(@assignment.id)
 
       # Update repo permissions if need be. This has to happen

diff --git a/test/unit/student_test.rb b/test/unit/student_test.rb
@@ -359,24 +359,6 @@ class StudentTest < ActiveSupport::TestCase
           @student.create_autogenerated_name_group(@assignment.id)
         end
       end
-
-      # Regression test for #627
-      should "allow the student to join an existing empty grouping when working alone" do
-        # create another assignment to make sure that the "create_group_for_working_alone_student"
-        # function correctly matches the grouping for @assignment
-        @assignment2 = Assignment.make(:group_name_autogenerated => false)
-        @student.create_group_for_working_alone_student(@assignment2.id)
-        @student.create_group_for_working_alone_student(@assignment.id)
-
-        # only delete the last created membership ( for @assignment, in this case)
-        @assignment.student_memberships.each do |sm|
-          sm.delete
-        end
-
-        assert_nothing_raised(ActiveRecord::RecordNotUnique) do
-          @student.create_group_for_working_alone_student(@assignment.id)
-        end
-      end
     end
 
     context "and a grouping" do