From 39e25b8a29e7296be99e8813a096f8d45111bf38 Mon Sep 17 00:00:00 2001 From: Andrew Cain Date: Tue, 22 Mar 2022 13:54:47 +1100 Subject: [PATCH 1/2] test: enhance testing of tutorial enrolments check issues with shifting between tutorials with restricted groups --- test/models/tutorial_enrolment_model_test.rb | 196 +++++++++++++++++++ 1 file changed, 196 insertions(+) diff --git a/test/models/tutorial_enrolment_model_test.rb b/test/models/tutorial_enrolment_model_test.rb index 461fde862..482b0cfa3 100644 --- a/test/models/tutorial_enrolment_model_test.rb +++ b/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) From c4019ade01b5139aa2345d2d00f72161c262210f Mon Sep 17 00:00:00 2001 From: Andrew Cain Date: Tue, 22 Mar 2022 13:57:02 +1100 Subject: [PATCH 2/2] fix: ensure validation works correctly for tutorial enrolments with restricted groups Ensure that destroying the enrolment checks if this breaks groups. Ensure that changing enrolments correctly checks groups. Ensure this works with multiple groups in the same tutorial --- app/models/project.rb | 14 +++-- app/models/tutorial_enrolment.rb | 89 +++++++++++++++++++++++++++----- 2 files changed, 86 insertions(+), 17 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 4453d427b..17bd7b928 100644 --- a/app/models/project.rb +++ b/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 diff --git a/app/models/tutorial_enrolment.rb b/app/models/tutorial_enrolment.rb index 353bcd8ad..9ec6777fa 100644 --- a/app/models/tutorial_enrolment.rb +++ b/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