Merge pull request #366 from macite/fix/tutorial-group-validations

Fix/tutorial group validations

app/models/project.rb

@@ -104,14 +104,22 @@ def specific_permission_hash(role, perm_hash, _other)
   end
 
   def enrol_in(tutorial)
+    tutorial_enrolment = matching_enrolment(tutorial)
+    return tutorial_enrolment if tutorial_enrolment.present? && tutorial_enrolment.tutorial_id == tutorial.id
+
     # Check if multiple enrolments changing to a single enrolment - due to no stream.
     # No need to delete if only 1, as that would be updated as well.
     if tutorial_enrolments.count > 1 && tutorial.tutorial_stream.nil?
-      # So remove current enrolments
-      tutorial_enrolments.delete_all()
+      begin
+        # So remove current enrolments
+        tutorial_enrolments.destroy_all()
+        # and there is no longer an associated tutorial enrolment
+        tutorial_enrolment = nil
+      rescue ActiveRecord::RecordNotDestroyed => e
+        raise ActiveRecord::RecordNotDestroyed.new("Unable to change tutorial due to group enrolment in current tutorials.", e.record)
+      end
     end
 
-    tutorial_enrolment = matching_enrolment(tutorial)
     if tutorial_enrolment.nil?
       tutorial_enrolment = TutorialEnrolment.new
       tutorial_enrolment.tutorial = tutorial

app/models/tutorial_enrolment.rb

@@ -29,7 +29,10 @@ class TutorialEnrolment < ApplicationRecord
   validate :ensure_cannot_enrol_in_tutorial_with_no_stream_when_enrolled_in_stream
 
   # Ensure in the same tutorial as a group if needed
-  validate :must_be_in_group_tutorials
+  validate :validate_tutorial_change, if: :will_save_change_to_tutorial_id?
+
+  # Ensure we check if we can leave the tutorial... and remove groups if needed
+  before_destroy :remove_from_groups_on_destroy
 
   def unit_must_be_same
     if project.unit.present? and tutorial.unit.present? and not project.unit.eql? tutorial.unit
@@ -80,22 +83,80 @@ def ensure_only_one_tutorial_per_stream
     end
   end
 
-  #
-  # Check to see if the student has a valid tutorial
-  #
-  def must_be_in_group_tutorials
-    project.groups.each do |g|
-      next unless g.limit_members_to_tutorial?
-      next if tutorial_id == g.tutorial_id
-
-      if g.group_set.allow_students_to_manage_groups
-        # leave group
-        g.remove_member(project)
+  def action_on_student_leave_tutorial(for_tutorial_id = nil)
+    # If there are no groups then you can change the tutorial
+    return :none_can_leave if project.groups.count == 0
+
+    result = :none_can_leave
+
+    # Now get the group
+    project.groups.where(tutorial_id: for_tutorial_id || tutorial_id).each do |grp|
+
+      # You can move if the tutorial allows it
+      next unless grp.limit_members_to_tutorial?
+
+      # Remove from the group if we can... otherwise this is an error!
+      if grp.group_set.allow_students_to_manage_groups
+        result = :leave_after_remove_from_group
       else
-        errors.add(:groups, "require #{project.student.name} to be in tutorial #{g.tutorial.abbreviation}")
-        break
+        # We have a group that cannot be left - the student cannot remove themselves and they must be in this tutorial
+        return :leave_denied
       end
     end
+
+    result
   end
 
+  private
+    # You can change the tutorial unless you are in a group that must be in this tutorial
+    def validate_tutorial_change
+      # If there is no change of tutorial id then you can change the tutorial
+      return unless tutorial_id_change_to_be_saved
+
+      # Get ids from change
+      id_from = tutorial_id_change_to_be_saved[0]
+      id_to = tutorial_id_change_to_be_saved[1]
+
+      # If no real change... no problem
+      return if id_from == id_to
+
+      # What action needs to occur when the student leaves this tutorial?
+      action = action_on_student_leave_tutorial(id_from)
+
+      return if action == :none_can_leave
+
+      if action == :leave_denied
+        abbr = Tutorial.find(id_from).abbreviation
+        errors.add(:groups, "require #{project.student.name} to be in tutorial #{abbr}")
+      else # leave after remove from group
+        project.groups.where(tutorial_id: id_from).each do |grp|
+          # Skip groups that can be in other tutorials
+          next unless grp.limit_members_to_tutorial?
+
+          # Remove from the group if we can... otherwise this is an error!
+          if grp.group_set.allow_students_to_manage_groups
+            grp.remove_member(project)
+          else
+            errors.add(:groups, "require #{project.student.name} to be in tutorial #{grp.tutorial.abbreviation}")
+          end
+        end
+      end
+    end
+
+    # Check group removal on delete
+    def remove_from_groups_on_destroy
+      project.groups.where(tutorial_id: tutorial_id).each do |grp|
+        # Skip groups that can be in other tutorials
+        next unless grp.limit_members_to_tutorial?
+
+        # Remove from the group if we can... otherwise this is an error!
+        if grp.group_set.allow_students_to_manage_groups
+          grp.remove_member(project)
+        else
+          errors.add(:groups, "require #{project.student.name} to be in tutorial #{grp.tutorial.abbreviation}")
+          throw :abort
+        end
+      end
+    end
+
 end

test/models/tutorial_enrolment_model_test.rb

@@ -13,6 +13,202 @@ def test_default_create
     assert_equal tutorial_enrolment.project.campus, tutorial_enrolment.tutorial.campus
   end
 
+  # Check that changing to a tutorial without a stream works as intended when there is a limited group
+  def test_group_change_on_cloud_tutorial_switch
+    # Create a unit with 2 streams, and a tutorial in each stream
+    unit = FactoryBot.create(:unit, with_students: false, group_sets: 1, stream_count: 2, tutorials: 0)
+
+    # Create the stream tutorials - factory will only create tutorials without streams
+    t1 = Tutorial.create unit: unit, tutorial_stream: unit.tutorial_streams.first, abbreviation: 'T01'
+    t2 = Tutorial.create unit: unit, tutorial_stream: unit.tutorial_streams.last, abbreviation: 'T02'
+    t3 = Tutorial.create unit: unit, tutorial_stream: nil, abbreviation: 'T03'
+
+    # Now make the group fixed, inflexible group
+    grp_set = unit.group_sets.first
+    grp_set.update(keep_groups_in_same_class: true, allow_students_to_manage_groups: false)
+
+    # Make sure the first group is in a stream 1 tutorial
+    grp = Group.create!({group_set: unit.group_sets.first, name: 'test-group', tutorial: t1})
+
+    assert grp_set.keep_groups_in_same_class
+    refute grp_set.allow_students_to_manage_groups
+    assert grp.limit_members_to_tutorial?
+
+    # Enrol a student - enrol in 2 tutorials to trigger destory of enrolments on cloud change
+    # cloud change with 1 tutorial is the same as between tutorials in the same stream
+    project = FactoryBot.create(:project, unit: unit)
+    tutorial_enrolment = project.enrol_in(t1) # the group's tutorial
+    other_enrolment = project.enrol_in(t2)
+
+    # Now enrol in the group
+    grp.add_member project
+
+    # Now you cannot leave...
+    assert_equal :leave_denied, tutorial_enrolment.action_on_student_leave_tutorial
+
+    # So this should fail!
+    assert_raises(ActiveRecord::RecordNotDestroyed, "Unable to change tutorial due to group enrolment in current tutorials.") { other_enrolment = project.enrol_in(t3) }
+
+    # If we make this more flexible...
+    grp_set.update(allow_students_to_manage_groups: true)
+
+    # This should succeed
+    other_enrolment = project.enrol_in(t3)
+
+    # But they are no longer in the group
+    refute grp.has_user(project.student)
+
+    unit.destroy
+  end
+
+  # Check the actions that should occur on changing a tutorial
+  def test_action_leave_tutorial
+    # Create a unit with 2 streams, and a tutorial in each stream
+    unit = FactoryBot.create(:unit, with_students: false, group_sets: 1, stream_count: 1, tutorials: 0)
+
+    # Create the stream tutorials - factory will only create tutorials without streams
+    t1 = Tutorial.create unit: unit, tutorial_stream: unit.tutorial_streams.first, abbreviation: 'T01'
+
+    grp_set = unit.group_sets.first
+
+    # Make sure the first group is in a stream 1 tutorial
+    grp = Group.create!({group_set: unit.group_sets.first, name: 'test-change-streams', tutorial: t1})
+
+    # Check this is currently a flexible group
+    refute grp_set.keep_groups_in_same_class
+    assert grp_set.allow_students_to_manage_groups
+    refute grp.limit_members_to_tutorial?
+
+    # Enrol a student
+    project = FactoryBot.create(:project, unit: unit)
+    tutorial_enrolment = project.enrol_in(t1)
+
+    # Now enrol in the group
+    grp.add_member project
+
+    # This is a flexible group... so leaving has no effect
+    assert_equal :none_can_leave, tutorial_enrolment.action_on_student_leave_tutorial
+
+    # Now make the group fixed
+    grp_set.update(keep_groups_in_same_class: true)
+    grp.reload
+
+    assert grp_set.keep_groups_in_same_class
+    assert grp_set.allow_students_to_manage_groups
+    assert grp.limit_members_to_tutorial?
+
+    # Now you can leave if you remove from the group
+    assert_equal :leave_after_remove_from_group, tutorial_enrolment.action_on_student_leave_tutorial
+
+    # Now make the group more inflexible
+    grp_set.update(allow_students_to_manage_groups: false)
+    assert grp_set.keep_groups_in_same_class
+    refute grp_set.allow_students_to_manage_groups
+    assert grp.limit_members_to_tutorial?
+
+    # Now you cannot leave...
+    assert_equal :leave_denied, tutorial_enrolment.action_on_student_leave_tutorial
+
+    grp.remove_member project
+    assert_equal :none_can_leave, tutorial_enrolment.action_on_student_leave_tutorial
+
+    unit.destroy
+  end
+
+  # Check that changing tutorials in other streams does not break group enrolment in other streams
+  def test_change_tutorial_does_not_break_group_in_other_stream
+    # Create a unit with 2 streams, and a tutorial in each stream
+    unit = FactoryBot.create(:unit, with_students: false, group_sets: 1, stream_count: 3, tutorials: 0)
+
+    # Create the stream tutorials - factory will only create tutorials without streams
+    t1 = Tutorial.create unit: unit, tutorial_stream: unit.tutorial_streams.second, abbreviation: 'T01'
+    t2 = Tutorial.create unit: unit, tutorial_stream: unit.tutorial_streams.first, abbreviation: 'T02'
+    t3 = Tutorial.create unit: unit, tutorial_stream: unit.tutorial_streams.second, abbreviation: 'T03'
+    t4 = Tutorial.create unit: unit, tutorial_stream: unit.tutorial_streams.first, abbreviation: 'T04'
+    t5 = Tutorial.create unit: unit, tutorial_stream: unit.tutorial_streams.last, abbreviation: 'T05'
+    t6 = Tutorial.create unit: unit, tutorial_stream: unit.tutorial_streams.last, abbreviation: 'T06'
+
+    unit.group_sets.first.update(keep_groups_in_same_class: true)
+
+    assert unit.group_sets.first.keep_groups_in_same_class
+
+    # Make sure the first group is in a stream 1 tutorial
+    grp = Group.create!({group_set: unit.group_sets.first, name: 'test-change-streams', tutorial: t1})
+    assert grp.limit_members_to_tutorial?
+
+    # Enrol a student
+    project = FactoryBot.create(:project, unit: unit)
+    tutorial_enrolment = project.enrol_in(t1)
+
+    # Make sure this has all worked
+    assert_equal tutorial_enrolment.project, project
+    assert_equal tutorial_enrolment.tutorial, t1
+    assert tutorial_enrolment.valid?
+
+    # Now enrol in the group
+    grp.add_member project
+
+    project.reload
+    grp.reload
+
+    # Ensure things are still valid
+    assert grp.valid?
+    assert_equal project.group_for_groupset(unit.group_sets.first), grp
+
+    # Enrol in other tutorial and check still in group
+    other_enrolment = project.enrol_in(t2)
+
+    assert tutorial_enrolment.valid?
+    assert other_enrolment.valid?
+    assert_equal t2, other_enrolment.tutorial
+    assert_equal grp, project.group_for_groupset(unit.group_sets.first)
+    assert project.group_membership_for_groupset(unit.group_sets.first).active
+
+    # Now change that enrolment...
+    other_enrolment = project.enrol_in(t4)
+
+    assert tutorial_enrolment.valid?
+    assert other_enrolment.valid?
+    assert_equal t4, other_enrolment.tutorial
+    assert_equal grp, project.group_for_groupset(unit.group_sets.first)
+    assert project.group_membership_for_groupset(unit.group_sets.first).active
+
+    # Enrol in other tutorial and check still in group
+    other_enrolment = project.enrol_in(t5)
+
+    assert tutorial_enrolment.valid?
+    assert other_enrolment.valid?
+    assert_equal t5, other_enrolment.tutorial
+    assert_equal grp, project.group_for_groupset(unit.group_sets.first)
+    assert project.group_membership_for_groupset(unit.group_sets.first).active
+
+    # Now change that enrolment...
+    other_enrolment = project.enrol_in(t6)
+
+    assert tutorial_enrolment.valid?
+    assert other_enrolment.valid?
+    assert_equal t6, other_enrolment.tutorial
+    assert_equal grp, project.group_for_groupset(unit.group_sets.first)
+    assert project.group_membership_for_groupset(unit.group_sets.first).active
+
+    # Check that updating with no change does not remoe group membership
+    updated_enrolment = project.enrol_in(t1)
+
+    project.reload
+    grp.reload
+
+    assert_equal project.group_for_groupset(unit.group_sets.first), grp
+    assert_equal updated_enrolment, tutorial_enrolment
+
+    # Check that moving to another tutorial does remove from group
+    updated_enrolment = project.enrol_in(t3)
+
+    assert_nil project.group_for_groupset(unit.group_sets.first)
+    assert updated_enrolment.valid?
+
+    unit.destroy
+  end
+
   def test_specific_create
     unit = FactoryBot.create(:unit, with_students: false)
     campus = FactoryBot.create(:campus)