From 1d34e3c63bea0418b598130204eeddcc0a8151b0 Mon Sep 17 00:00:00 2001 From: KrishaDarji Date: Tue, 17 Mar 2026 16:13:33 -0400 Subject: [PATCH 01/14] changed role to duty for mentor class --- app/models/mentored_team.rb | 51 ++++++++++++++++++++++--------------- db/schema.rb | 2 +- 2 files changed, 32 insertions(+), 21 deletions(-) diff --git a/app/models/mentored_team.rb b/app/models/mentored_team.rb index 77cf25627..eeb701990 100644 --- a/app/models/mentored_team.rb +++ b/app/models/mentored_team.rb @@ -2,33 +2,44 @@ class MentoredTeam < AssignmentTeam - # Custom validation to ensure the team type is 'MentoredTeam' - validate :type_must_be_mentored_team - - # adds members to the team who are not mentors - def add_member(user) - return false if user == mentor - super(user) + # Returns the participant on this team whose duty name is 'Mentor' + def mentor + participants.joins(:duty).find_by(duties: { name: 'Mentor' }) end - # Assigning a user as mentor of the team - # Validates if user has the role and permission to be a mentor - def assign_mentor(user) - return false unless user.role&.name&.downcase&.include?('mentor') - self.mentor = user - save + # Assigns a participant as mentor by setting their duty to the 'Mentor' duty. + # The participant must already be a member of this team. + def assign_mentor(participant) + mentor_duty = Duty.find_by(name: 'Mentor') + return false unless mentor_duty + return false unless participants.exists?(id: participant.id) + + participant.update(duty_id: mentor_duty.id) end - # Unassigns mentor from team + # Clears the mentor duty from the current mentor participant. def remove_mentor - self.mentor = nil - save + mentor&.update(duty_id: nil) + end + + # Prevents adding a participant who already holds the mentor duty on this team. + def add_member(participant_or_user) + participant = resolve_participant(participant_or_user) + return { success: false, error: 'Participant is already the mentor of this team' } if mentor_participant?(participant) + + super(participant_or_user) end private - # Check if the team type is 'MentoredTeam' - def type_must_be_mentored_team - errors.add(:type, 'must be MentoredTeam') unless type == 'MentoredTeam' + def resolve_participant(participant_or_user) + return participant_or_user if participant_or_user.is_a?(Participant) + + AssignmentParticipant.find_by(user_id: participant_or_user.id, parent_id: parent_id) + end + + def mentor_participant?(participant) + return false unless participant + mentor&.id == participant.id end -end +end diff --git a/db/schema.rb b/db/schema.rb index cddbe12c6..6a198491c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -456,9 +456,9 @@ add_foreign_key "assignments_duties", "duties" add_foreign_key "courses", "institutions" add_foreign_key "courses", "users", column: "instructor_id" + add_foreign_key "duties", "users", column: "instructor_id" add_foreign_key "invitations", "participants", column: "from_id" add_foreign_key "invitations", "participants", column: "to_id" - add_foreign_key "duties", "users", column: "instructor_id" add_foreign_key "items", "questionnaires" add_foreign_key "participants", "duties" add_foreign_key "participants", "join_team_requests" From 6d2a59c91be4368a26e253203bd82b5596485279 Mon Sep 17 00:00:00 2001 From: KrishaDarji Date: Mon, 23 Mar 2026 21:29:06 -0400 Subject: [PATCH 02/14] Completed requirement - no user should appear on multiple course teams within the same course or multiple assignment teams within the same assignment --- app/models/teams_participant.rb | 2 +- ...d_unique_index_on_participant_id_to_teams_participants.rb | 5 +++++ db/schema.rb | 3 ++- 3 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20260314000000_add_unique_index_on_participant_id_to_teams_participants.rb diff --git a/app/models/teams_participant.rb b/app/models/teams_participant.rb index 701d4eb50..afcf193ac 100644 --- a/app/models/teams_participant.rb +++ b/app/models/teams_participant.rb @@ -5,7 +5,7 @@ class TeamsParticipant < ApplicationRecord belongs_to :team belongs_to :user - validates :participant_id, uniqueness: { scope: :team_id } + validates :participant_id, uniqueness: true validates :user_id, presence: true end diff --git a/db/migrate/20260314000000_add_unique_index_on_participant_id_to_teams_participants.rb b/db/migrate/20260314000000_add_unique_index_on_participant_id_to_teams_participants.rb new file mode 100644 index 000000000..c80337ea0 --- /dev/null +++ b/db/migrate/20260314000000_add_unique_index_on_participant_id_to_teams_participants.rb @@ -0,0 +1,5 @@ +class AddUniqueIndexOnParticipantIdToTeamsParticipants < ActiveRecord::Migration[8.0] + def change + add_index :teams_participants, :participant_id, unique: true, name: 'index_teams_participants_on_participant_id_unique' + end +end diff --git a/db/schema.rb b/db/schema.rb index 6a198491c..e9103fc77 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2026_03_13_064334) do +ActiveRecord::Schema[8.0].define(version: 2026_03_14_000000) do create_table "account_requests", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.string "username" t.string "full_name" @@ -408,6 +408,7 @@ t.bigint "participant_id", null: false t.integer "user_id", null: false t.index ["participant_id"], name: "index_teams_participants_on_participant_id" + t.index ["participant_id"], name: "index_teams_participants_on_participant_id_unique", unique: true t.index ["team_id"], name: "index_teams_participants_on_team_id" t.index ["user_id"], name: "index_teams_participants_on_user_id" end From 98015ae1a80a60a935d5d2ae949ebdf20ff5c1c3 Mon Sep 17 00:00:00 2001 From: AtharvaW195 Date: Mon, 23 Mar 2026 22:16:17 -0400 Subject: [PATCH 03/14] Added the Team Limits Configuration --- .../join_team_requests_controller.rb | 66 ++++++++++--------- app/models/teams_participant.rb | 17 ++++- 2 files changed, 51 insertions(+), 32 deletions(-) diff --git a/app/controllers/join_team_requests_controller.rb b/app/controllers/join_team_requests_controller.rb index 8aaf4b916..fc910b6c2 100644 --- a/app/controllers/join_team_requests_controller.rb +++ b/app/controllers/join_team_requests_controller.rb @@ -166,45 +166,51 @@ def destroy # Accept a join team request and add the participant to the team def accept team = @join_team_request.team - if team.full? - return render json: { error: 'Team is full' }, status: :unprocessable_entity - end + participant = @join_team_request.participant - participant = @join_team_request.participant + ActiveRecord::Base.transaction do + team.with_lock do + # Re-check while holding the lock (race-safe) + if team.full? + tp = TeamsParticipant.new(team: team, participant: participant, user_id: participant.user_id) + tp.errors.add(:base, 'Team is full') + raise ActiveRecord::RecordInvalid, tp + end - # Use a transaction to ensure both removal and addition happen atomically - ActiveRecord::Base.transaction do - # Find and remove participant from their old team (if any) + # Remove participant from their old team (if any) old_team_participant = TeamsParticipant.find_by(participant_id: participant.id) - if old_team_participant - old_team = old_team_participant.team + if old_team_participant && old_team_participant.team_id != team.id old_team_participant.destroy! - - # If the old team is now empty, optionally clean up (but keep the team for now) - Rails.logger.info "Removed participant #{participant.id} from old team #{old_team&.id}" end - # Add participant to the new team - TeamsParticipant.create!( - participant_id: participant.id, - team_id: team.id, - user_id: participant.user_id - ) + # Add participant to the new team (uses Team#add_member capacity check + MentoredTeam override) + result = team.add_member(participant) + unless result[:success] + tp = TeamsParticipant.new(team: team, participant: participant, user_id: participant.user_id) + tp.errors.add(:base, result[:error].presence || 'Unable to add participant to team') + raise ActiveRecord::RecordInvalid, tp + end + end - # Update the request status - @join_team_request.update!(reply_status: ACCEPTED) + # Update the request status (only after successful add) + @join_team_request.update!(reply_status: ACCEPTED) - render json: { - message: 'Join team request accepted successfully', - join_team_request: JoinTeamRequestSerializer.new(@join_team_request).as_json - }, status: :ok - end - rescue ActiveRecord::RecordInvalid => e - render json: { error: e.message }, status: :unprocessable_entity - rescue StandardError => e - render json: { error: e.message }, status: :unprocessable_entity + render json: { + message: 'Join team request accepted successfully', + join_team_request: JoinTeamRequestSerializer.new(@join_team_request).as_json + }, status: :ok end - +rescue ActiveRecord::RecordInvalid => e + msg = + if e.record&.errors&.any? + e.record.errors.full_messages.join(', ') + else + e.message + end + render json: { error: msg }, status: :unprocessable_entity +rescue StandardError => e + render json: { error: e.message }, status: :unprocessable_entity +end # PATCH /join_team_requests/1/decline # Decline a join team request def decline diff --git a/app/models/teams_participant.rb b/app/models/teams_participant.rb index afcf193ac..cf1f6d093 100644 --- a/app/models/teams_participant.rb +++ b/app/models/teams_participant.rb @@ -1,11 +1,24 @@ # frozen_string_literal: true class TeamsParticipant < ApplicationRecord - belongs_to :participant + belongs_to :participant belongs_to :team belongs_to :user validates :participant_id, uniqueness: true validates :user_id, presence: true -end + validate :team_not_full, on: :create + + private + + def team_not_full + return unless team + max = team.max_size + return if max.blank? + + if team.participants.count >= max + errors.add(:base, "Team is at full capacity (max #{max}).") + end + end +end \ No newline at end of file From e288b9a0b5d5142fc27b5d57b4ec656c7e3e5c46 Mon Sep 17 00:00:00 2001 From: Saladin Al-Bataineh Date: Thu, 26 Mar 2026 16:06:32 -0400 Subject: [PATCH 04/14] Fix CourseTeam membership validation to check course participation - Fixed validate_membership in CourseTeam to check CourseParticipant instead of assignment participants - Removed invalid course.max_team_size call in Team#max_size since Course model does not have a max_team_size column - All CourseTeam specs passing (9/9) --- app/models/course_team.rb | 4 ++-- app/models/team.rb | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/app/models/course_team.rb b/app/models/course_team.rb index 1230f0175..f41910d39 100644 --- a/app/models/course_team.rb +++ b/app/models/course_team.rb @@ -30,8 +30,8 @@ def copy_to_assignment_team(assignment) protected def validate_membership(user) - # Check if user is enrolled in any assignment in the course - course.assignments.any? { |assignment| assignment.participants.exists?(user: user) } + # Verify user is enrolled in a course (E2610) + CourseParticipant.exists?(user_id: user.id, parent_id: course.id) end private diff --git a/app/models/team.rb b/app/models/team.rb index e2c39fbaa..217b99e33 100644 --- a/app/models/team.rb +++ b/app/models/team.rb @@ -35,8 +35,6 @@ def team_size def max_size if is_a?(AssignmentTeam) && assignment&.max_team_size assignment.max_team_size - elsif is_a?(CourseTeam) && course&.max_team_size - course.max_team_size else nil end From 47f7348d41622f693aec8c1b9c010a2a75c108d4 Mon Sep 17 00:00:00 2001 From: KrishaDarji Date: Sat, 28 Mar 2026 18:55:47 -0400 Subject: [PATCH 05/14] Tests for duty based access and team capacity check --- app/models/assignment_team.rb | 10 +- app/models/course_team.rb | 10 +- app/models/mentored_team.rb | 52 +++-- app/models/teams_participant.rb | 18 +- spec/models/mentored_team_spec.rb | 264 +++++++++------------ spec/models/team_association_spec.rb | 92 ++++++++ spec/models/team_capacity_spec.rb | 64 +++++ spec/models/team_conversion_spec.rb | 80 +++++++ spec/models/team_edge_cases_spec.rb | 87 +++++++ spec/models/teams_participant_spec.rb | 322 +++++--------------------- 10 files changed, 536 insertions(+), 463 deletions(-) create mode 100644 spec/models/team_association_spec.rb create mode 100644 spec/models/team_capacity_spec.rb create mode 100644 spec/models/team_conversion_spec.rb create mode 100644 spec/models/team_edge_cases_spec.rb diff --git a/app/models/assignment_team.rb b/app/models/assignment_team.rb index 2bf8e6815..41543f7ca 100644 --- a/app/models/assignment_team.rb +++ b/app/models/assignment_team.rb @@ -78,13 +78,13 @@ def path # - Copies team members from the assignment team to the course team def copy_to_course_team(course) course_team = CourseTeam.new( - name: "#{name} (Course)", # Appends "(Course)" to the team name - max_team_size: max_team_size, # Preserves original max team size - course: course # Associates new team with the given course + name: "#{name} (Course)", + parent_id: course.id ) if course_team.save - team_members.each do |member| - course_team.add_member(member.user) # Copies each member to the new course team + participants.each do |participant| + c_participant = CourseParticipant.find_by(user_id: participant.user_id, parent_id: course.id) + course_team.add_member(c_participant) if c_participant end end course_team # Returns the newly created course team object diff --git a/app/models/course_team.rb b/app/models/course_team.rb index f41910d39..62a6552e1 100644 --- a/app/models/course_team.rb +++ b/app/models/course_team.rb @@ -15,13 +15,13 @@ def add_member(user) # - Copies team members from the assignment team to the course team def copy_to_assignment_team(assignment) assignment_team = AssignmentTeam.new( - name: "#{name} (Assignment)", # Appends "(Assignment)" to the team name - max_team_size: max_team_size, # Preserves original max team size - assignment: assignment # Associates the course team with an assignment + name: "#{name} (Assignment)", + parent_id: assignment.id ) if assignment_team.save - team_members.each do |member| - assignment_team.add_member(member.user) # Copies each member to the new assignment team + participants.each do |participant| + a_participant = AssignmentParticipant.find_by(user_id: participant.user_id, parent_id: assignment.id) + assignment_team.add_member(a_participant) if a_participant end end assignment_team # Returns the newly created assignment team object diff --git a/app/models/mentored_team.rb b/app/models/mentored_team.rb index eeb701990..4602e3fe5 100644 --- a/app/models/mentored_team.rb +++ b/app/models/mentored_team.rb @@ -2,44 +2,50 @@ class MentoredTeam < AssignmentTeam - # Returns the participant on this team whose duty name is 'Mentor' - def mentor - participants.joins(:duty).find_by(duties: { name: 'Mentor' }) + # adds members to the team who are not mentors + def add_member(user) + return false if user == mentor + super(user) end - # Assigns a participant as mentor by setting their duty to the 'Mentor' duty. - # The participant must already be a member of this team. - def assign_mentor(participant) + # Assigning a participant as mentor of the team + # Validates if participant has the duty of mentor + def assign_mentor(user) mentor_duty = Duty.find_by(name: 'Mentor') return false unless mentor_duty - return false unless participants.exists?(id: participant.id) + + participant = AssignmentParticipant.find_by(user_id: user.id, parent_id: parent_id) + return false unless participant participant.update(duty_id: mentor_duty.id) end - # Clears the mentor duty from the current mentor participant. + # Unassigns mentor from team def remove_mentor - mentor&.update(duty_id: nil) - end - - # Prevents adding a participant who already holds the mentor duty on this team. - def add_member(participant_or_user) - participant = resolve_participant(participant_or_user) - return { success: false, error: 'Participant is already the mentor of this team' } if mentor_participant?(participant) + mentor_duty = Duty.find_by(name: 'Mentor') + return unless mentor_duty - super(participant_or_user) + mentor_participant = AssignmentParticipant + .joins('INNER JOIN teams_participants ON teams_participants.participant_id = participants.id') + .where('teams_participants.team_id = ? AND participants.duty_id = ?', id, mentor_duty.id) + .first + mentor_participant&.update(duty_id: nil) end private - def resolve_participant(participant_or_user) - return participant_or_user if participant_or_user.is_a?(Participant) - - AssignmentParticipant.find_by(user_id: participant_or_user.id, parent_id: parent_id) + # Check if the team type is 'MentoredTeam' + def type_must_be_mentored_team + errors.add(:type, 'must be MentoredTeam') unless type == 'MentoredTeam' end - def mentor_participant?(participant) - return false unless participant - mentor&.id == participant.id + def mentor + mentor_duty = Duty.find_by(name: 'Mentor') + return nil unless mentor_duty + + AssignmentParticipant + .joins('INNER JOIN teams_participants ON teams_participants.participant_id = participants.id') + .where('teams_participants.team_id = ? AND participants.duty_id = ?', id, mentor_duty.id) + .first&.user end end diff --git a/app/models/teams_participant.rb b/app/models/teams_participant.rb index cf1f6d093..a933069e9 100644 --- a/app/models/teams_participant.rb +++ b/app/models/teams_participant.rb @@ -1,24 +1,10 @@ # frozen_string_literal: true class TeamsParticipant < ApplicationRecord - belongs_to :participant + belongs_to :participant belongs_to :team belongs_to :user validates :participant_id, uniqueness: true validates :user_id, presence: true - - validate :team_not_full, on: :create - - private - - def team_not_full - return unless team - max = team.max_size - return if max.blank? - - if team.participants.count >= max - errors.add(:base, "Team is at full capacity (max #{max}).") - end - end -end \ No newline at end of file +end diff --git a/spec/models/mentored_team_spec.rb b/spec/models/mentored_team_spec.rb index 5d95b50c3..482a7d077 100644 --- a/spec/models/mentored_team_spec.rb +++ b/spec/models/mentored_team_spec.rb @@ -1,152 +1,112 @@ -# # frozen_string_literal: true - -# require 'rails_helper' - -# RSpec.describe MentoredTeam, type: :model do - -# include RolesHelper -# # -------------------------------------------------------------------------- -# # Global Setup -# # -------------------------------------------------------------------------- -# # Create the full roles hierarchy once, to be shared by all examples. -# let!(:roles) { create_roles_hierarchy } - -# # ------------------------------------------------------------------------ -# # Helper: DRY-up creation of student users with a predictable pattern. -# # ------------------------------------------------------------------------ -# def create_student(suffix) -# User.create!( -# name: suffix, -# email: "#{suffix}@example.com", -# full_name: suffix.split('_').map(&:capitalize).join(' '), -# password_digest: "password", -# role_id: roles[:student].id, -# institution_id: institution.id -# ) -# end - -# # ------------------------------------------------------------------------ -# # Shared Data Setup: Build core domain objects used across tests. -# # ------------------------------------------------------------------------ -# let(:institution) do -# # All users belong to the same institution to satisfy foreign key constraints. -# Institution.create!(name: "NC State") -# end - -# let(:instructor) do -# # The instructor will own assignments and courses in subsequent tests. -# User.create!( -# name: "instructor", -# full_name: "Instructor User", -# email: "instructor@example.com", -# password_digest: "password", -# role_id: roles[:instructor].id, -# institution_id: institution.id -# ) -# end - -# let(:team_owner) do -# User.create!( -# name: "team_owner", -# full_name: "Team Owner", -# email: "team_owner@example.com", -# password_digest: "password", -# role_id: roles[:student].id, -# institution_id: institution.id -# ) -# end - -# let!(:assignment) { Assignment.create!(name: "Assignment 1", instructor_id: instructor.id, max_team_size: 3) } -# let!(:course) { Course.create!(name: "Course 1", instructor_id: instructor.id, institution_id: institution.id, directory_path: "/course1") } - -# let(:mentor_role) { create(:role, :mentor) } - -# let(:mentor) do -# User.create!( -# name: "mentor_user", -# full_name: "Mentor User", -# email: "mentor@example.com", -# password_digest: "password", -# role_id: mentor_role.id, -# institution_id: institution.id -# ) -# end - -# let(:mentored_team) do -# MentoredTeam.create!( -# parent_id: mentor.id, -# assignment: assignment, -# name: 'team 3', -# ) -# end - -# let(:user) do -# User.create!( -# name: "student_user", -# full_name: "Student User", -# email: "student@example.com", -# password_digest: "password", -# role_id: roles[:student].id, -# institution_id: institution.id -# ) -# end - -# let!(:team) { create(:mentored_team, assignment: assignment) } - - -# describe 'validations' do -# it { should validate_presence_of(:mentor) } -# it { should validate_presence_of(:type) } - -# it 'requires type to be MentoredTeam' do -# team.type = 'AssignmentTeam' -# expect(team).not_to be_valid -# expect(team.errors[:type]).to include('must be MentoredTeam') -# end - -# it 'requires mentor to have mentor role' do -# non_mentor = create(:user) -# team.mentor = non_mentor -# expect(team).not_to be_valid -# expect(team.errors[:mentor]).to include('must have mentor role') -# end -# end - -# describe 'associations' do -# it { should belong_to(:mentor).class_name('User') } -# it { should belong_to(:assignment) } -# it { should belong_to(:user).optional } -# it { should have_many(:teams_participants).dependent(:destroy) } -# it { should have_many(:users).through(:teams_participants) } -# end - -# describe 'team management' do -# let(:enrolled_user) { create(:user) } - -# before do -# @participant = create(:assignment_participant, user: enrolled_user, assignment: assignment) -# end - -# it 'can add enrolled user' do -# expect(team.add_member(enrolled_user)).to be_truthy -# expect(team.has_member?(enrolled_user)).to be_truthy -# end - -# it 'cannot add mentor as member' do -# expect(team.add_member(team.mentor)).to be_falsey -# expect(team.has_member?(team.mentor)).to be_falsey -# end - -# it 'can assign new mentor' do -# new_mentor = create(:user, role: mentor_role) -# expect(team.assign_mentor(new_mentor)).to be_truthy -# expect(team.mentor).to eq(new_mentor) -# end - -# it 'cannot assign non-mentor as mentor' do -# non_mentor = create(:user) -# expect(team.assign_mentor(non_mentor)).to be_falsey -# expect(team.mentor).not_to eq(non_mentor) -# end -# end -# end +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe MentoredTeam, type: :model do + include RolesHelper + + before(:all) { @roles = create_roles_hierarchy } + + let(:institution) { Institution.create!(name: 'NC State') } + let(:instructor) { User.create!(name: 'inst_m', full_name: 'Instructor', email: 'inst_m@test.com', password_digest: 'x', role_id: @roles[:instructor].id, institution_id: institution.id) } + let(:assignment) { Assignment.create!(name: 'Mentored Assign', instructor_id: instructor.id, max_team_size: 5) } + let(:team) { MentoredTeam.create!(name: 'Mentored Team 1', parent_id: assignment.id) } + let(:mentor_duty) { Duty.create!(name: 'Mentor', instructor_id: instructor.id) } + + def make_user(suffix) + User.create!(name: suffix, full_name: suffix, email: "#{suffix}@test.com", password_digest: 'x', role_id: @roles[:student].id, institution_id: institution.id) + end + + def make_participant(suffix) + user = make_user(suffix) + AssignmentParticipant.create!(user: user, parent_id: assignment.id, handle: user.name) + end + + # ── Subclass-specific behavior ────────────────────────────────────────────── + describe '#assign_mentor' do + it 'sets the mentor duty on the participant' do + mentor_duty + participant = make_participant('mentor1') + team.add_member(participant) + + result = team.assign_mentor(participant.user) + expect(result).to be true + expect(participant.reload.duty_id).to eq(mentor_duty.id) + end + + it 'returns false when Mentor duty does not exist' do + participant = make_participant('mentor2') + team.add_member(participant) + + result = team.assign_mentor(participant.user) + expect(result).to be false + end + + it 'returns false when user is not a participant in the assignment' do + mentor_duty + outsider = make_user('outsider_m') + result = team.assign_mentor(outsider) + expect(result).to be false + end + + it 'identifies mentor by duty not by role' do + mentor_duty + participant = make_participant('duty_mentor') + team.add_member(participant) + team.assign_mentor(participant.user) + + expect(team.send(:mentor)).to eq(participant.user) + expect(participant.reload.duty).to eq(mentor_duty) + end + end + + describe '#remove_mentor' do + it 'clears the duty from the mentor participant' do + mentor_duty + participant = make_participant('remove_mentor') + team.add_member(participant) + team.assign_mentor(participant.user) + + team.remove_mentor + expect(participant.reload.duty_id).to be_nil + end + + it 'does nothing when no mentor is assigned' do + mentor_duty + expect { team.remove_mentor }.not_to raise_error + end + end + + describe '#add_member' do + it 'blocks adding the mentor as a regular member' do + mentor_duty + participant = make_participant('blocked_mentor') + team.add_member(participant) + team.assign_mentor(participant.user) + + result = team.add_member(participant.user) + expect(result).to be false + end + + it 'allows adding a non-mentor participant' do + participant = make_participant('regular_member') + result = team.add_member(participant) + expect(result[:success]).to be true + end + + it 'inherits capacity limit from AssignmentTeam' do + small_assignment = Assignment.create!(name: 'Small Assign', instructor_id: instructor.id, max_team_size: 1) + small_team = MentoredTeam.create!(name: 'Small Team', parent_id: small_assignment.id) + + user1 = make_user('cap1') + user2 = make_user('cap2') + p1 = AssignmentParticipant.create!(user: user1, parent_id: small_assignment.id, handle: user1.name) + p2 = AssignmentParticipant.create!(user: user2, parent_id: small_assignment.id, handle: user2.name) + + small_team.add_member(p1) + result = small_team.add_member(p2) + expect(result[:success]).to be false + expect(result[:error]).to match(/capacity/) + end + end +end diff --git a/spec/models/team_association_spec.rb b/spec/models/team_association_spec.rb new file mode 100644 index 000000000..05993ec32 --- /dev/null +++ b/spec/models/team_association_spec.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Association integrity', type: :model do + include RolesHelper + + before(:all) { @roles = create_roles_hierarchy } + + let(:institution) { Institution.create!(name: 'NC State') } + let(:instructor) { User.create!(name: 'inst_a', full_name: 'Instructor', email: 'inst_a@test.com', password_digest: 'x', role_id: @roles[:instructor].id, institution_id: institution.id) } + let(:assignment) { Assignment.create!(name: 'Assoc Assign', instructor_id: instructor.id, max_team_size: 3) } + let(:course) { Course.create!(name: 'Assoc Course', instructor_id: instructor.id, institution_id: institution.id, directory_path: '/assoc') } + + def make_user(suffix) + User.create!(name: suffix, full_name: suffix, email: "#{suffix}@test.com", password_digest: 'x', role_id: @roles[:student].id, institution_id: institution.id) + end + + # ── Team is abstract ──────────────────────────────────────────────────────── + describe 'Team as abstract superclass' do + it 'is invalid with type set directly to Team' do + team = Team.new(parent_id: assignment.id, name: 'Raw Team', type: 'Team') + expect(team).not_to be_valid + end + + it 'is valid as MentoredTeam subclass' do + team = MentoredTeam.new(parent_id: assignment.id, name: 'MT') + expect(team).to be_valid + end + end + + # ── AssignmentTeam associations ───────────────────────────────────────────── + describe 'AssignmentTeam' do + let(:team) { AssignmentTeam.create!(name: 'Assoc AT', parent_id: assignment.id) } + + it 'is linked to the correct assignment via parent_id' do + expect(team.assignment).to eq(assignment) + end + + it 'membership records are scoped to this team' do + user = make_user('at_user') + participant = AssignmentParticipant.create!(user: user, parent_id: assignment.id, handle: user.name) + team.add_member(participant) + + expect(TeamsParticipant.where(team_id: team.id).count).to eq(1) + end + + it 'participants association returns only members of this team' do + user1 = make_user('at_u1') + user2 = make_user('at_u2') + p1 = AssignmentParticipant.create!(user: user1, parent_id: assignment.id, handle: user1.name) + p2 = AssignmentParticipant.create!(user: user2, parent_id: assignment.id, handle: user2.name) + + team2 = AssignmentTeam.create!(name: 'Assoc AT2', parent_id: assignment.id) + team.add_member(p1) + team2.add_member(p2) + + expect(team.participants).to include(p1) + expect(team.participants).not_to include(p2) + end + end + + # ── CourseTeam associations ───────────────────────────────────────────────── + describe 'CourseTeam' do + let(:team) { CourseTeam.create!(name: 'Assoc CT', parent_id: course.id) } + + it 'is linked to the correct course via parent_id' do + expect(team.course).to eq(course) + end + + it 'membership records are scoped to this team' do + user = make_user('ct_user') + participant = CourseParticipant.create!(user: user, parent_id: course.id, handle: user.name) + team.add_member(participant) + + expect(TeamsParticipant.where(team_id: team.id).count).to eq(1) + end + end + + # ── MentoredTeam associations ─────────────────────────────────────────────── + describe 'MentoredTeam' do + let(:team) { MentoredTeam.create!(name: 'Assoc MT', parent_id: assignment.id) } + + it 'is a subclass of AssignmentTeam' do + expect(team).to be_a(AssignmentTeam) + end + + it 'is linked to the correct assignment' do + expect(team.assignment).to eq(assignment) + end + end +end diff --git a/spec/models/team_capacity_spec.rb b/spec/models/team_capacity_spec.rb new file mode 100644 index 000000000..2f78ad3cc --- /dev/null +++ b/spec/models/team_capacity_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Capacity and size limits', type: :model do + include RolesHelper + + before(:all) { @roles = create_roles_hierarchy } + + let(:institution) { Institution.create!(name: 'NC State') } + let(:instructor) { User.create!(name: 'inst_cap', full_name: 'Instructor', email: 'inst_cap@test.com', password_digest: 'x', role_id: @roles[:instructor].id, institution_id: institution.id) } + let(:assignment) { Assignment.create!(name: 'Cap Assign', instructor_id: instructor.id, max_team_size: 2) } + let(:team) { AssignmentTeam.create!(name: 'Cap Team', parent_id: assignment.id) } + + def make_participant(suffix, assign = assignment) + user = User.create!(name: suffix, full_name: suffix, email: "#{suffix}@test.com", password_digest: 'x', role_id: @roles[:student].id, institution_id: institution.id) + AssignmentParticipant.create!(user: user, parent_id: assign.id, handle: user.name) + end + + describe 'AssignmentTeam capacity' do + it 'returns false for full? when under capacity' do + expect(team.full?).to be false + end + + it 'returns true for full? when at max_team_size' do + 2.times { |i| team.add_member(make_participant("cap#{i}")) } + team.reload + expect(team.full?).to be true + end + + it 'rejects member when team is full' do + 2.times { |i| team.add_member(make_participant("full#{i}")) } + extra = make_participant('extra') + result = team.add_member(extra) + expect(result[:success]).to be false + expect(result[:error]).to match(/capacity/) + end + + it 'returns correct team_size' do + p1 = make_participant('sz1') + team.add_member(p1) + team.reload + expect(team.team_size).to eq(1) + end + + it 'returns max_size equal to assignment max_team_size' do + expect(team.max_size).to eq(2) + end + end + + describe 'CourseTeam has no capacity limit' do + let(:course) { Course.create!(name: 'Cap Course', instructor_id: instructor.id, institution_id: institution.id, directory_path: '/cap') } + let(:course_team) { CourseTeam.create!(name: 'Cap Course Team', parent_id: course.id) } + + it 'always returns false for full?' do + 5.times do |i| + user = User.create!(name: "ccu#{i}", full_name: "ccu#{i}", email: "ccu#{i}@test.com", password_digest: 'x', role_id: @roles[:student].id, institution_id: institution.id) + p = CourseParticipant.create!(user: user, parent_id: course.id, handle: user.name) + course_team.add_member(p) + end + expect(course_team.full?).to be false + end + end +end diff --git a/spec/models/team_conversion_spec.rb b/spec/models/team_conversion_spec.rb new file mode 100644 index 000000000..74c691e69 --- /dev/null +++ b/spec/models/team_conversion_spec.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Subclass conversion behavior', type: :model do + include RolesHelper + + before(:all) { @roles = create_roles_hierarchy } + + let(:institution) { Institution.create!(name: 'NC State') } + let(:instructor) { User.create!(name: 'inst_c', full_name: 'Instructor', email: 'inst_c@test.com', password_digest: 'x', role_id: @roles[:instructor].id, institution_id: institution.id) } + let(:course) { Course.create!(name: 'Course Conv', instructor_id: instructor.id, institution_id: institution.id, directory_path: '/conv') } + let(:assignment) { Assignment.create!(name: 'Assign Conv', instructor_id: instructor.id, max_team_size: 5) } + + def make_user(suffix) + User.create!(name: suffix, full_name: suffix, email: "#{suffix}@test.com", password_digest: 'x', role_id: @roles[:student].id, institution_id: institution.id) + end + + # ── CourseTeam → AssignmentTeam ───────────────────────────────────────────── + describe 'CourseTeam#copy_to_assignment_team' do + let(:course_team) { CourseTeam.create!(name: 'Course Team Conv', parent_id: course.id) } + + it 'creates a new AssignmentTeam' do + result = course_team.copy_to_assignment_team(assignment) + expect(result).to be_a(AssignmentTeam) + end + + it 'appends (Assignment) to the name' do + result = course_team.copy_to_assignment_team(assignment) + expect(result.name).to include('(Assignment)') + end + + it 'associates the new team with the given assignment' do + result = course_team.copy_to_assignment_team(assignment) + expect(result.parent_id).to eq(assignment.id) + end + + it 'copies members to the new assignment team' do + user = make_user('conv_user1') + participant = CourseParticipant.create!(user: user, parent_id: course.id, handle: user.name) + course_team.add_member(participant) + + a_participant = AssignmentParticipant.create!(user: user, parent_id: assignment.id, handle: user.name) + result = course_team.copy_to_assignment_team(assignment) + + expect(result.users).to include(user) + end + end + + # ── AssignmentTeam → CourseTeam ───────────────────────────────────────────── + describe 'AssignmentTeam#copy_to_course_team' do + let(:assignment_team) { AssignmentTeam.create!(name: 'Assign Team Conv', parent_id: assignment.id) } + + it 'creates a new CourseTeam' do + result = assignment_team.copy_to_course_team(course) + expect(result).to be_a(CourseTeam) + end + + it 'appends (Course) to the name' do + result = assignment_team.copy_to_course_team(course) + expect(result.name).to include('(Course)') + end + + it 'associates the new team with the given course' do + result = assignment_team.copy_to_course_team(course) + expect(result.parent_id).to eq(course.id) + end + + it 'copies members to the new course team' do + user = make_user('conv_user2') + a_participant = AssignmentParticipant.create!(user: user, parent_id: assignment.id, handle: user.name) + assignment_team.add_member(a_participant) + + c_participant = CourseParticipant.create!(user: user, parent_id: course.id, handle: user.name) + result = assignment_team.copy_to_course_team(course) + + expect(result.users).to include(user) + end + end +end diff --git a/spec/models/team_edge_cases_spec.rb b/spec/models/team_edge_cases_spec.rb new file mode 100644 index 000000000..eed99c597 --- /dev/null +++ b/spec/models/team_edge_cases_spec.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Team edge cases', type: :model do + include RolesHelper + + before(:all) { @roles = create_roles_hierarchy } + + let(:institution) { Institution.create!(name: 'NC State') } + let(:instructor) { User.create!(name: 'inst_e', full_name: 'Instructor', email: 'inst_e@test.com', password_digest: 'x', role_id: @roles[:instructor].id, institution_id: institution.id) } + let(:assignment) { Assignment.create!(name: 'Edge Assign', instructor_id: instructor.id, max_team_size: 3) } + let(:team) { AssignmentTeam.create!(name: 'Edge Team', parent_id: assignment.id) } + + def make_participant(suffix, assign = assignment) + user = User.create!(name: suffix, full_name: suffix, email: "#{suffix}@test.com", password_digest: 'x', role_id: @roles[:student].id, institution_id: institution.id) + AssignmentParticipant.create!(user: user, parent_id: assign.id, handle: user.name) + end + + # ── already-enrolled member ───────────────────────────────────────────────── + describe 'adding already-enrolled member' do + it 'rejects adding the same participant twice to the same team' do + participant = make_participant('dup1') + team.add_member(participant) + result = team.add_member(participant) + expect(result[:success]).to be false + expect(result[:error]).to match(/already on the team/) + end + + it 'rejects adding same user to two teams in the same assignment' do + team2 = AssignmentTeam.create!(name: 'Edge Team 2', parent_id: assignment.id) + participant = make_participant('dup2') + team.add_member(participant) + result = team2.add_member(participant) + expect(result[:success]).to be false + end + end + + # ── zero members ──────────────────────────────────────────────────────────── + describe 'team with zero members' do + it 'reports team_size of 0' do + expect(team.team_size).to eq(0) + end + + it 'is not full when empty' do + expect(team.full?).to be false + end + + it 'destroys team when last member is removed' do + participant = make_participant('last_one') + team.add_member(participant) + team.remove_member(participant) + expect(Team.exists?(team.id)).to be false + end + + it 'does not raise error calling remove_mentor on empty MentoredTeam' do + mentored = MentoredTeam.create!(name: 'Empty Mentored', parent_id: assignment.id) + Duty.create!(name: 'Mentor', instructor_id: instructor.id) + expect { mentored.remove_mentor }.not_to raise_error + end + end + + # ── no participants in assignment ─────────────────────────────────────────── + describe 'assignment with no participants' do + let(:empty_assignment) { Assignment.create!(name: 'Empty Assign', instructor_id: instructor.id, max_team_size: 3) } + let(:empty_team) { AssignmentTeam.create!(name: 'Empty Team', parent_id: empty_assignment.id) } + + it 'allows creating a team for an assignment with no participants' do + expect(empty_team).to be_persisted + end + + it 'rejects adding a user who is not a participant in the assignment' do + user = User.create!(name: 'no_part', full_name: 'no_part', email: 'no_part@test.com', password_digest: 'x', role_id: @roles[:student].id, institution_id: institution.id) + result = empty_team.add_member(user) + expect(result[:success]).to be false + expect(result[:error]).to match(/not a participant/) + end + + it 'rejects a participant from a different assignment' do + other_assign = Assignment.create!(name: 'Other Assign', instructor_id: instructor.id, max_team_size: 3) + user = User.create!(name: 'wrong_assign', full_name: 'wrong_assign', email: 'wrong_assign@test.com', password_digest: 'x', role_id: @roles[:student].id, institution_id: institution.id) + participant = AssignmentParticipant.create!(user: user, parent_id: other_assign.id, handle: user.name) + result = empty_team.add_member(user) + expect(result[:success]).to be false + end + end +end diff --git a/spec/models/teams_participant_spec.rb b/spec/models/teams_participant_spec.rb index c9a711e4d..1670d2cbe 100644 --- a/spec/models/teams_participant_spec.rb +++ b/spec/models/teams_participant_spec.rb @@ -1,298 +1,96 @@ +# frozen_string_literal: true + require 'rails_helper' RSpec.describe TeamsParticipant, type: :model do include RolesHelper - # -------------------------------------------------------------------------- - # Global Setup - # -------------------------------------------------------------------------- - before(:all) do - @roles = create_roles_hierarchy - end - - # ------------------------------------------------------------------------ - # Helper: DRY-up creation of student users - # ------------------------------------------------------------------------ - def create_student(suffix) - User.create!( - name: suffix, - email: "#{suffix}@example.com", - full_name: suffix.split('_').map(&:capitalize).join(' '), - password_digest: "password", - role_id: @roles[:student].id, - institution_id: institution.id - ) - end - - # ------------------------------------------------------------------------ - # Shared Data Setup - # ------------------------------------------------------------------------ - let(:institution) do - Institution.create!(name: "NC State") - end - - let(:instructor) do - User.create!( - name: "instructor", - full_name: "Instructor User", - email: "instructor@example.com", - password_digest: "password", - role_id: @roles[:instructor].id, - institution_id: institution.id - ) - end - - let(:student_user) { create_student("student1") } - let(:another_student) { create_student("student2") } - - let(:assignment) { Assignment.create!(name: "Test Assignment", instructor_id: instructor.id, max_team_size: 3) } - - let(:team) do - AssignmentTeam.create!( - parent_id: assignment.id, - name: 'Test Team' - ) - end - - let(:another_team) do - AssignmentTeam.create!( - parent_id: assignment.id, - name: 'Another Team' - ) - end - - let(:participant) do - AssignmentParticipant.create!( - user_id: student_user.id, - parent_id: assignment.id, - handle: 'student1_handle' - ) - end + before(:all) { @roles = create_roles_hierarchy } - let(:another_participant) do - AssignmentParticipant.create!( - user_id: another_student.id, - parent_id: assignment.id, - handle: 'student2_handle' - ) - end + let(:institution) { Institution.create!(name: 'NC State') } + let(:instructor) { User.create!(name: 'inst', full_name: 'Instructor', email: 'inst@test.com', password_digest: 'x', role_id: @roles[:instructor].id, institution_id: institution.id) } + let(:assignment) { Assignment.create!(name: 'Assign 1', instructor_id: instructor.id, max_team_size: 3) } + let(:course) { Course.create!(name: 'Course 1', instructor_id: instructor.id, institution_id: institution.id, directory_path: '/c1') } - # -------------------------------------------------------------------------- - # Association Tests - # -------------------------------------------------------------------------- - describe 'associations' do - it { should belong_to(:participant) } - it { should belong_to(:team) } - it { should belong_to(:user) } + def make_user(suffix) + User.create!(name: suffix, full_name: suffix, email: "#{suffix}@test.com", password_digest: 'x', role_id: @roles[:student].id, institution_id: institution.id) end - # -------------------------------------------------------------------------- - # Validation Tests - # -------------------------------------------------------------------------- + # ── 1. Model Validations ──────────────────────────────────────────────────── describe 'validations' do - it 'is valid with valid attributes' do - teams_participant = TeamsParticipant.new( - participant_id: participant.id, - team_id: team.id, - user_id: student_user.id - ) - expect(teams_participant).to be_valid - end - - it 'requires user_id' do - teams_participant = TeamsParticipant.new( - participant_id: participant.id, - team_id: team.id - ) - expect(teams_participant).not_to be_valid - expect(teams_participant.errors[:user_id]).to include("can't be blank") - end - - it 'enforces uniqueness of participant_id scoped to team_id' do - TeamsParticipant.create!( - participant_id: participant.id, - team_id: team.id, - user_id: student_user.id - ) + let(:user) { make_user('v_user') } + let(:participant) { AssignmentParticipant.create!(user: user, parent_id: assignment.id, handle: user.name) } + let(:team) { AssignmentTeam.create!(name: 'Team V', parent_id: assignment.id) } - duplicate = TeamsParticipant.new( - participant_id: participant.id, - team_id: team.id, - user_id: student_user.id - ) - - expect(duplicate).not_to be_valid - expect(duplicate.errors[:participant_id]).to include("has already been taken") - end - - it 'allows same participant in different teams' do - # Note: This tests the model validation only - business logic may prevent this - TeamsParticipant.create!( - participant_id: participant.id, - team_id: team.id, - user_id: student_user.id - ) - - different_team_membership = TeamsParticipant.new( - participant_id: participant.id, - team_id: another_team.id, - user_id: student_user.id - ) - - # The model allows this, but business logic in controllers should prevent it - expect(different_team_membership).to be_valid + it 'is valid with all required fields' do + tp = TeamsParticipant.new(team: team, participant: participant, user: user) + expect(tp).to be_valid end - end - # -------------------------------------------------------------------------- - # Creation and Destruction Tests - # -------------------------------------------------------------------------- - describe 'creation' do - it 'creates a teams_participant record successfully' do - expect { - TeamsParticipant.create!( - participant_id: participant.id, - team_id: team.id, - user_id: student_user.id - ) - }.to change(TeamsParticipant, :count).by(1) + it 'is invalid without user_id' do + tp = TeamsParticipant.new(team: team, participant: participant) + expect(tp).not_to be_valid + expect(tp.errors[:user_id]).to be_present end - it 'associates participant with the correct team' do - teams_participant = TeamsParticipant.create!( - participant_id: participant.id, - team_id: team.id, - user_id: student_user.id - ) - - expect(teams_participant.team).to eq(team) - expect(teams_participant.participant).to eq(participant) - expect(teams_participant.user).to eq(student_user) + it 'is invalid without participant_id' do + tp = TeamsParticipant.new(team: team, user: user) + expect(tp).not_to be_valid end - end - describe 'destruction' do - it 'removes the teams_participant record' do - teams_participant = TeamsParticipant.create!( - participant_id: participant.id, - team_id: team.id, - user_id: student_user.id - ) - - expect { - teams_participant.destroy - }.to change(TeamsParticipant, :count).by(-1) + it 'is invalid without team_id' do + tp = TeamsParticipant.new(participant: participant, user: user) + expect(tp).not_to be_valid end - it 'does not destroy the associated team or participant' do - teams_participant = TeamsParticipant.create!( - participant_id: participant.id, - team_id: team.id, - user_id: student_user.id - ) - - team_id = team.id - participant_id = participant.id + # ── uniqueness constraint (our Step 2 change) ── + it 'prevents same participant from joining two different teams' do + team2 = AssignmentTeam.create!(name: 'Team V2', parent_id: assignment.id) + TeamsParticipant.create!(team: team, participant: participant, user: user) - teams_participant.destroy - - expect(Team.find_by(id: team_id)).not_to be_nil - expect(Participant.find_by(id: participant_id)).not_to be_nil + tp2 = TeamsParticipant.new(team: team2, participant: participant, user: user) + expect(tp2).not_to be_valid + expect(tp2.errors[:participant_id]).to be_present end - end - - # -------------------------------------------------------------------------- - # Team Membership Transfer Tests (for join team requests) - # -------------------------------------------------------------------------- - describe 'team membership transfer' do - it 'allows removing participant from old team and adding to new team' do - # Create initial membership - old_membership = TeamsParticipant.create!( - participant_id: participant.id, - team_id: team.id, - user_id: student_user.id - ) - - # Transfer to new team (simulating accept join team request) - old_membership.destroy - - new_membership = TeamsParticipant.create!( - participant_id: participant.id, - team_id: another_team.id, - user_id: student_user.id - ) - expect(new_membership).to be_persisted - expect(TeamsParticipant.find_by(participant_id: participant.id, team_id: team.id)).to be_nil - expect(TeamsParticipant.find_by(participant_id: participant.id, team_id: another_team.id)).not_to be_nil + it 'allows same participant on the same team only once' do + TeamsParticipant.create!(team: team, participant: participant, user: user) + tp2 = TeamsParticipant.new(team: team, participant: participant, user: user) + expect(tp2).not_to be_valid end - it 'updates team participant count correctly after transfer' do - # Add participant to first team - TeamsParticipant.create!( - participant_id: participant.id, - team_id: team.id, - user_id: student_user.id - ) + it 'allows different participants on the same team' do + user2 = make_user('v_user2') + participant2 = AssignmentParticipant.create!(user: user2, parent_id: assignment.id, handle: user2.name) + TeamsParticipant.create!(team: team, participant: participant, user: user) - # Add another participant to second team - TeamsParticipant.create!( - participant_id: another_participant.id, - team_id: another_team.id, - user_id: another_student.id - ) - - expect(team.participants.count).to eq(1) - expect(another_team.participants.count).to eq(1) - - # Transfer first participant to second team - TeamsParticipant.find_by(participant_id: participant.id, team_id: team.id).destroy - TeamsParticipant.create!( - participant_id: participant.id, - team_id: another_team.id, - user_id: student_user.id - ) - - team.reload - another_team.reload - - expect(team.participants.count).to eq(0) - expect(another_team.participants.count).to eq(2) + tp2 = TeamsParticipant.new(team: team, participant: participant2, user: user2) + expect(tp2).to be_valid end end - # -------------------------------------------------------------------------- - # Query Tests - # -------------------------------------------------------------------------- - describe 'querying' do - before do - TeamsParticipant.create!( - participant_id: participant.id, - team_id: team.id, - user_id: student_user.id - ) - TeamsParticipant.create!( - participant_id: another_participant.id, - team_id: another_team.id, - user_id: another_student.id - ) - end + # ── 2. Enrollment-based membership rules ─────────────────────────────────── + describe 'enrollment-based membership via Team#add_member' do + let(:team) { AssignmentTeam.create!(name: 'Enroll Team', parent_id: assignment.id) } - it 'finds teams_participant by participant_id' do - result = TeamsParticipant.find_by(participant_id: participant.id) - expect(result).not_to be_nil - expect(result.team_id).to eq(team.id) + it 'rejects a user not enrolled in the assignment' do + outsider = make_user('outsider') + result = team.add_member(outsider) + expect(result[:success]).to be false + expect(result[:error]).to match(/not a participant/) end - it 'finds teams_participant by team_id' do - result = TeamsParticipant.where(team_id: team.id) - expect(result.count).to eq(1) - expect(result.first.participant_id).to eq(participant.id) + it 'accepts a user enrolled in the assignment' do + user = make_user('enrolled') + participant = AssignmentParticipant.create!(user: user, parent_id: assignment.id, handle: user.name) + result = team.add_member(participant) + expect(result[:success]).to be true end - it 'finds all participants for a team through association' do - expect(team.participants).to include(participant) - expect(another_team.participants).to include(another_participant) + it 'rejects a user not enrolled in the assignment when passed as user object' do + outsider = make_user('not_enrolled') + result = team.add_member(outsider) + expect(result[:success]).to be false end end end From 112ea5708dc5532774be911e3a272042a57dffe3 Mon Sep 17 00:00:00 2001 From: Saladin Al-Bataineh Date: Sun, 29 Mar 2026 02:22:26 -0400 Subject: [PATCH 06/14] Add enrollment validation tests for AssignmentTeam and MentoredTeam - Added 'can add enrolled user' success case to assignment_team_spec - Added 'does not add unenrolled user' rejection case to mentored_team_spec - MentoredTeam tests verify duty-based mentor logic (not role-based) - All 29 examples passing across CourseTeam, AssignmentTeam, MentoredTeam --- spec/models/assignment_team_spec.rb | 6 ++++++ spec/models/mentored_team_spec.rb | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/spec/models/assignment_team_spec.rb b/spec/models/assignment_team_spec.rb index 50e03616f..900fd6c0a 100644 --- a/spec/models/assignment_team_spec.rb +++ b/spec/models/assignment_team_spec.rb @@ -128,5 +128,11 @@ def create_student(suffix) expect(result[:success]).to be false expect(result[:error]).to eq("#{unenrolled_user.name} is not a participant in this assignment") end + + it 'can add enrolled user' do + result = assignment_team.add_member(enrolled_user) + expect(result[:success]).to be true + expect(assignment_team.has_member?(enrolled_user)).to be true + end end end diff --git a/spec/models/mentored_team_spec.rb b/spec/models/mentored_team_spec.rb index 482a7d077..6e14847ef 100644 --- a/spec/models/mentored_team_spec.rb +++ b/spec/models/mentored_team_spec.rb @@ -88,6 +88,13 @@ def make_participant(suffix) expect(result).to be false end + it 'does not add unenrolled user' do + unenrolled = make_user('unenrolled_m') + result = team.add_member(unenrolled) + expect(result[:success]).to be false + expect(result[:error]).to eq("#{unenrolled.name} is not a participant in this assignment") + end + it 'allows adding a non-mentor participant' do participant = make_participant('regular_member') result = team.add_member(participant) From 7a948bec8f5bf1e0f22ffc7ed3feaadebbbe5d52 Mon Sep 17 00:00:00 2001 From: AtharvaW195 Date: Sun, 29 Mar 2026 16:04:32 -0400 Subject: [PATCH 07/14] Added the Team Limits Tests and Controller authorization Test --- app/models/team.rb | 13 +++---- app/models/teams_participant.rb | 13 +++++++ spec/models/teams_participant_spec.rb | 35 +++++++++++++++++++ .../v1/join_team_requests_controller_spec.rb | 13 ++++++- 4 files changed, 64 insertions(+), 10 deletions(-) diff --git a/app/models/team.rb b/app/models/team.rb index 217b99e33..71555e0c7 100644 --- a/app/models/team.rb +++ b/app/models/team.rb @@ -41,16 +41,11 @@ def max_size end def full? - current_size = participants.count + max = max_size + return false if max.blank? - # assignment teams use the column max_team_size - if is_a?(AssignmentTeam) && assignment&.max_team_size - return current_size >= assignment.max_team_size - end - - # course teams never fill up by default - false - end + participants.count >= max +end # Checks if the given participant is already on any team for the associated assignment or course. def participant_on_team?(participant) diff --git a/app/models/teams_participant.rb b/app/models/teams_participant.rb index a933069e9..572f9a35b 100644 --- a/app/models/teams_participant.rb +++ b/app/models/teams_participant.rb @@ -7,4 +7,17 @@ class TeamsParticipant < ApplicationRecord validates :participant_id, uniqueness: true validates :user_id, presence: true + + validate :team_not_full, on: :create + private + def team_not_full + return unless team + + max = team.max_size + return if max.blank? + + if team.participants.count >= max + errors.add(:base, "Team is at full capacity (max #{max}).") + end + end end diff --git a/spec/models/teams_participant_spec.rb b/spec/models/teams_participant_spec.rb index 1670d2cbe..aa590b5c5 100644 --- a/spec/models/teams_participant_spec.rb +++ b/spec/models/teams_participant_spec.rb @@ -68,6 +68,41 @@ def make_user(suffix) expect(tp2).to be_valid end end + # Verifies TeamsParticipant (join table) cannot bypass team capacity; creation must fail once AssignmentTeam reaches assignment.max_team_size. + describe 'capacity validation (team_not_full)' do + it 'is invalid when team is already at capacity' do + assignment.update!(max_team_size: 1) + team = AssignmentTeam.create!(name: 'Cap Team', parent_id: assignment.id) + + u1 = make_user('cap_u1') + p1 = AssignmentParticipant.create!(user: u1, parent_id: assignment.id, handle: u1.name) + TeamsParticipant.create!(team: team, participant: p1, user: u1) + + u2 = make_user('cap_u2') + p2 = AssignmentParticipant.create!(user: u2, parent_id: assignment.id, handle: u2.name) + tp2 = TeamsParticipant.new(team: team, participant: p2, user: u2) + + expect(tp2).not_to be_valid + expect(tp2.errors.full_messages.join(', ')).to match(/full capacity/i) + end + + it 'raises when creating beyond capacity' do + assignment.update!(max_team_size: 1) + team = AssignmentTeam.create!(name: 'Cap Team2', parent_id: assignment.id) + + u1 = make_user('cap2_u1') + p1 = AssignmentParticipant.create!(user: u1, parent_id: assignment.id, handle: u1.name) + TeamsParticipant.create!(team: team, participant: p1, user: u1) + + u2 = make_user('cap2_u2') + p2 = AssignmentParticipant.create!(user: u2, parent_id: assignment.id, handle: u2.name) + + expect { + TeamsParticipant.create!(team: team, participant: p2, user: u2) + }.to raise_error(ActiveRecord::RecordInvalid) + end + end + # ── 2. Enrollment-based membership rules ─────────────────────────────────── describe 'enrollment-based membership via Team#add_member' do diff --git a/spec/requests/api/v1/join_team_requests_controller_spec.rb b/spec/requests/api/v1/join_team_requests_controller_spec.rb index 6ab04a3ff..34880975e 100644 --- a/spec/requests/api/v1/join_team_requests_controller_spec.rb +++ b/spec/requests/api/v1/join_team_requests_controller_spec.rb @@ -309,7 +309,18 @@ expect(response).to have_http_status(:unprocessable_entity) expect(JSON.parse(response.body)['error']).to eq('Team is full') end - end + + # Ensures accepting a join request on a full team returns 422 and does not mutate state + it 'does not change reply_status or add participant when team is full' do + assignment.update!(max_team_size: 1) + + patch "/join_team_requests/#{join_team_request.id}/accept", headers: team_member_headers + + expect(response).to have_http_status(:unprocessable_entity) + expect(join_team_request.reload.reply_status).to eq('PENDING') + expect(team1.participants.reload).not_to include(participant2) + end + end context 'when filtering join team requests' do let(:student_token) { JsonWebToken.encode({id: student1.id}) } From 08cabb2d562136cfb5eedd50a7d30374adb0ecb0 Mon Sep 17 00:00:00 2001 From: AtharvaW195 Date: Sun, 29 Mar 2026 20:45:36 -0400 Subject: [PATCH 08/14] Enforce team capacity and tighten TeamsController auth --- app/controllers/teams_controller.rb | 26 ++++++++++- app/models/team.rb | 6 +-- spec/requests/api/v1/teams_controller_spec.rb | 46 +++++++++++++++++++ 3 files changed, 74 insertions(+), 4 deletions(-) diff --git a/app/controllers/teams_controller.rb b/app/controllers/teams_controller.rb index a24cd23f2..cd347a59b 100644 --- a/app/controllers/teams_controller.rb +++ b/app/controllers/teams_controller.rb @@ -1,10 +1,28 @@ class TeamsController < ApplicationController # Set the @team instance variable before executing actions except index and create - before_action :set_team, except: [:index, :create] + prepend_before_action :set_team, except: [:index, :create] # Validate team type only during team creation before_action :validate_team_type, only: [:create] + # Authorization policy for this controller (used by Authorization concern). + # - Teaching staff (TA and above) can access all actions. + # - Students can view only teams they belong to, and cannot manage membership. + def action_allowed? + return true if current_user_has_ta_privileges? + + case params[:action] + when 'index' + false + when 'show', 'members' + current_user_member_of_team?(@team) + when 'create', 'add_member', 'remove_member' + false + else + false + end + end + # GET /teams # Fetches all teams and renders them using TeamSerializer def index @@ -92,6 +110,12 @@ def set_team render json: { error: 'Team not found' }, status: :not_found end + def current_user_member_of_team?(team) + return false unless team && current_user + + team.participants.exists?(user_id: current_user.id) + end + # Whitelists the parameters allowed for team creation/updation def team_params params.require(:team).permit(:name, :type, :assignment_id) diff --git a/app/models/team.rb b/app/models/team.rb index 71555e0c7..e52fce7f9 100644 --- a/app/models/team.rb +++ b/app/models/team.rb @@ -42,10 +42,10 @@ def max_size def full? max = max_size - return false if max.blank? + return false if max.blank? - participants.count >= max -end + participants.count >= max + end # Checks if the given participant is already on any team for the associated assignment or course. def participant_on_team?(participant) diff --git a/spec/requests/api/v1/teams_controller_spec.rb b/spec/requests/api/v1/teams_controller_spec.rb index 9c4e660d9..ab3a4d0d2 100644 --- a/spec/requests/api/v1/teams_controller_spec.rb +++ b/spec/requests/api/v1/teams_controller_spec.rb @@ -126,6 +126,52 @@ let(:token) { JsonWebToken.encode(id: instructor.id) } let(:auth_headers) { { Authorization: "Bearer #{token}" } } + # Verifies TeamsController enforces role-based authorization with correct HTTP status codes + # (students receive 403 on restricted endpoints rather than succeeding or returning 500). + describe 'Authorization / HTTP status codes' do + let(:student_token) { JsonWebToken.encode(id: other_user.id) } + let(:student_headers) { { Authorization: "Bearer #{student_token}" } } + + it 'returns 403 for student on GET /teams' do + team_with_course + get '/teams', headers: student_headers + expect(response).to have_http_status(:forbidden) + expect(json_response).to have_key('error') + end + + it 'returns 403 for student on GET /teams/:id when not a member' do + get "/teams/#{team_with_course.id}", headers: student_headers + expect(response).to have_http_status(:forbidden) + expect(json_response).to have_key('error') + end + + it 'returns 403 for student on POST /teams' do + post '/teams', params: { team: { name: 'X', type: 'CourseTeam', assignment_id: assignment.id } }, headers: student_headers + expect(response).to have_http_status(:forbidden) + expect(json_response).to have_key('error') + end + + it 'returns 403 for student on POST /teams/:id/members' do + new_user = create(:user) + create(:course_participant, user: new_user, parent_id: course.id) + + post "/teams/#{team_with_course.id}/members", + params: { team_participant: { user_id: new_user.id } }, + headers: student_headers + + expect(response).to have_http_status(:forbidden) + expect(json_response).to have_key('error') + end + + it 'returns 403 for student on DELETE /teams/:id/members/:user_id' do + teams_participant_course + + delete "/teams/#{team_with_course.id}/members/#{other_user.id}", headers: student_headers + expect(response).to have_http_status(:forbidden) + expect(json_response).to have_key('error') + end + end + describe 'GET /teams' do it 'returns all teams' do team_with_course From 27ec482fc491adc5b4e0d7b3bea2fee39856d42d Mon Sep 17 00:00:00 2001 From: KrishaDarji Date: Mon, 30 Mar 2026 14:30:50 -0400 Subject: [PATCH 09/14] Fix MentoredTeam duty-based mentor, enforce unique team membership, and clean up teams controller --- app/controllers/teams_controller.rb | 10 ++-------- app/models/mentored_team.rb | 4 ++++ app/models/team.rb | 2 ++ app/models/teams_participant.rb | 8 +++----- spec/models/team_association_spec.rb | 3 +-- spec/models/team_conversion_spec.rb | 16 +++++++++------- 6 files changed, 21 insertions(+), 22 deletions(-) diff --git a/app/controllers/teams_controller.rb b/app/controllers/teams_controller.rb index cd347a59b..d469ede10 100644 --- a/app/controllers/teams_controller.rb +++ b/app/controllers/teams_controller.rb @@ -1,6 +1,6 @@ class TeamsController < ApplicationController # Set the @team instance variable before executing actions except index and create - prepend_before_action :set_team, except: [:index, :create] + before_action :set_team, except: [:index, :create] # Validate team type only during team creation before_action :validate_team_type, only: [:create] @@ -15,7 +15,7 @@ def action_allowed? when 'index' false when 'show', 'members' - current_user_member_of_team?(@team) + @team&.has_member?(current_user) when 'create', 'add_member', 'remove_member' false else @@ -110,12 +110,6 @@ def set_team render json: { error: 'Team not found' }, status: :not_found end - def current_user_member_of_team?(team) - return false unless team && current_user - - team.participants.exists?(user_id: current_user.id) - end - # Whitelists the parameters allowed for team creation/updation def team_params params.require(:team).permit(:name, :type, :assignment_id) diff --git a/app/models/mentored_team.rb b/app/models/mentored_team.rb index 4602e3fe5..f052726b5 100644 --- a/app/models/mentored_team.rb +++ b/app/models/mentored_team.rb @@ -25,6 +25,8 @@ def remove_mentor mentor_duty = Duty.find_by(name: 'Mentor') return unless mentor_duty + # Use raw SQL join because AssignmentParticipant has no has_many :teams_participants association. + # duty_id lives on participants table; team scoping is done via teams_participants.team_id. mentor_participant = AssignmentParticipant .joins('INNER JOIN teams_participants ON teams_participants.participant_id = participants.id') .where('teams_participants.team_id = ? AND participants.duty_id = ?', id, mentor_duty.id) @@ -43,6 +45,8 @@ def mentor mentor_duty = Duty.find_by(name: 'Mentor') return nil unless mentor_duty + # Use raw SQL join because AssignmentParticipant has no has_many :teams_participants association. + # duty_id lives on participants table; team scoping is done via teams_participants.team_id. AssignmentParticipant .joins('INNER JOIN teams_participants ON teams_participants.participant_id = participants.id') .where('teams_participants.team_id = ? AND participants.duty_id = ?', id, mentor_duty.id) diff --git a/app/models/team.rb b/app/models/team.rb index e52fce7f9..74459a3f5 100644 --- a/app/models/team.rb +++ b/app/models/team.rb @@ -35,6 +35,8 @@ def team_size def max_size if is_a?(AssignmentTeam) && assignment&.max_team_size assignment.max_team_size + elsif is_a?(CourseTeam) && course&.respond_to?(:max_team_size) && course&.max_team_size + course.max_team_size else nil end diff --git a/app/models/teams_participant.rb b/app/models/teams_participant.rb index 572f9a35b..e26b06595 100644 --- a/app/models/teams_participant.rb +++ b/app/models/teams_participant.rb @@ -9,15 +9,13 @@ class TeamsParticipant < ApplicationRecord validates :user_id, presence: true validate :team_not_full, on: :create + private + def team_not_full return unless team - max = team.max_size return if max.blank? - - if team.participants.count >= max - errors.add(:base, "Team is at full capacity (max #{max}).") - end + errors.add(:base, 'Team is at full capacity') if team.participants.count >= max end end diff --git a/spec/models/team_association_spec.rb b/spec/models/team_association_spec.rb index 05993ec32..92c9580e4 100644 --- a/spec/models/team_association_spec.rb +++ b/spec/models/team_association_spec.rb @@ -71,8 +71,7 @@ def make_user(suffix) it 'membership records are scoped to this team' do user = make_user('ct_user') participant = CourseParticipant.create!(user: user, parent_id: course.id, handle: user.name) - team.add_member(participant) - + TeamsParticipant.create!(team: team, participant: participant, user: user) expect(TeamsParticipant.where(team_id: team.id).count).to eq(1) end end diff --git a/spec/models/team_conversion_spec.rb b/spec/models/team_conversion_spec.rb index 74c691e69..23718d524 100644 --- a/spec/models/team_conversion_spec.rb +++ b/spec/models/team_conversion_spec.rb @@ -37,12 +37,13 @@ def make_user(suffix) it 'copies members to the new assignment team' do user = make_user('conv_user1') - participant = CourseParticipant.create!(user: user, parent_id: course.id, handle: user.name) - course_team.add_member(participant) - + # Create assignment participant first so copy_to_assignment_team can find them a_participant = AssignmentParticipant.create!(user: user, parent_id: assignment.id, handle: user.name) - result = course_team.copy_to_assignment_team(assignment) + # Add to course team via course participant + c_participant = CourseParticipant.create!(user: user, parent_id: course.id, handle: user.name) + course_team.add_member(c_participant) + result = course_team.copy_to_assignment_team(assignment) expect(result.users).to include(user) end end @@ -68,12 +69,13 @@ def make_user(suffix) it 'copies members to the new course team' do user = make_user('conv_user2') + # Create course participant first so copy_to_course_team can find them + c_participant = CourseParticipant.create!(user: user, parent_id: course.id, handle: user.name) + # Add to assignment team via assignment participant a_participant = AssignmentParticipant.create!(user: user, parent_id: assignment.id, handle: user.name) assignment_team.add_member(a_participant) - c_participant = CourseParticipant.create!(user: user, parent_id: course.id, handle: user.name) - result = assignment_team.copy_to_course_team(course) - + result = assignment_team.copy_to_course_team(course) expect(result.users).to include(user) end end From ef5c3d880692eacd4fa100f29c05b185fe869f18 Mon Sep 17 00:00:00 2001 From: KrishaDarji Date: Mon, 30 Mar 2026 15:26:28 -0400 Subject: [PATCH 10/14] restore CourseTeam max_size check in Team#max_size --- app/models/teams_participant.rb | 2 +- ...unique_index_on_participant_id_to_teams_participants.rb | 3 ++- spec/models/teams_participant_spec.rb | 7 +++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/models/teams_participant.rb b/app/models/teams_participant.rb index e26b06595..d18d51a03 100644 --- a/app/models/teams_participant.rb +++ b/app/models/teams_participant.rb @@ -5,7 +5,7 @@ class TeamsParticipant < ApplicationRecord belongs_to :team belongs_to :user - validates :participant_id, uniqueness: true + validates :participant_id, uniqueness: { scope: :team_id } validates :user_id, presence: true validate :team_not_full, on: :create diff --git a/db/migrate/20260314000000_add_unique_index_on_participant_id_to_teams_participants.rb b/db/migrate/20260314000000_add_unique_index_on_participant_id_to_teams_participants.rb index c80337ea0..ee2b4a3c8 100644 --- a/db/migrate/20260314000000_add_unique_index_on_participant_id_to_teams_participants.rb +++ b/db/migrate/20260314000000_add_unique_index_on_participant_id_to_teams_participants.rb @@ -1,5 +1,6 @@ class AddUniqueIndexOnParticipantIdToTeamsParticipants < ActiveRecord::Migration[8.0] def change - add_index :teams_participants, :participant_id, unique: true, name: 'index_teams_participants_on_participant_id_unique' + # Uniqueness is enforced at model level via participant_on_team? in Team#add_member + # A DB-level unique index on participant_id alone would prevent participants from moving between teams end end diff --git a/spec/models/teams_participant_spec.rb b/spec/models/teams_participant_spec.rb index aa590b5c5..2fb7f4794 100644 --- a/spec/models/teams_participant_spec.rb +++ b/spec/models/teams_participant_spec.rb @@ -44,13 +44,12 @@ def make_user(suffix) end # ── uniqueness constraint (our Step 2 change) ── - it 'prevents same participant from joining two different teams' do + it 'prevents same participant from joining two different teams via add_member' do team2 = AssignmentTeam.create!(name: 'Team V2', parent_id: assignment.id) TeamsParticipant.create!(team: team, participant: participant, user: user) - tp2 = TeamsParticipant.new(team: team2, participant: participant, user: user) - expect(tp2).not_to be_valid - expect(tp2.errors[:participant_id]).to be_present + result = team2.add_member(participant) + expect(result[:success]).to be false end it 'allows same participant on the same team only once' do From d468b9ab808997e4dd94d481297fe08bd92f83b1 Mon Sep 17 00:00:00 2001 From: AtharvaW195 Date: Mon, 30 Mar 2026 15:46:42 -0400 Subject: [PATCH 11/14] Added Requested changes for join_team controller --- .../join_team_requests_controller.rb | 38 ++++++++----------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/app/controllers/join_team_requests_controller.rb b/app/controllers/join_team_requests_controller.rb index fc910b6c2..c8e6f7b5b 100644 --- a/app/controllers/join_team_requests_controller.rb +++ b/app/controllers/join_team_requests_controller.rb @@ -166,15 +166,15 @@ def destroy # Accept a join team request and add the participant to the team def accept team = @join_team_request.team - participant = @join_team_request.participant + participant = @join_team_request.participant + + error_message = nil - ActiveRecord::Base.transaction do team.with_lock do # Re-check while holding the lock (race-safe) if team.full? - tp = TeamsParticipant.new(team: team, participant: participant, user_id: participant.user_id) - tp.errors.add(:base, 'Team is full') - raise ActiveRecord::RecordInvalid, tp + error_message = 'Team is full' + raise ActiveRecord::Rollback end # Remove participant from their old team (if any) @@ -186,31 +186,25 @@ def accept # Add participant to the new team (uses Team#add_member capacity check + MentoredTeam override) result = team.add_member(participant) unless result[:success] - tp = TeamsParticipant.new(team: team, participant: participant, user_id: participant.user_id) - tp.errors.add(:base, result[:error].presence || 'Unable to add participant to team') - raise ActiveRecord::RecordInvalid, tp + error_message = result[:error].presence || 'Unable to add participant to team' + raise ActiveRecord::Rollback end + + # Update the request status (only after successful add) + @join_team_request.update!(reply_status: ACCEPTED) end - # Update the request status (only after successful add) - @join_team_request.update!(reply_status: ACCEPTED) + return render json: { error: error_message }, status: :unprocessable_entity if error_message render json: { message: 'Join team request accepted successfully', join_team_request: JoinTeamRequestSerializer.new(@join_team_request).as_json }, status: :ok + rescue ActiveRecord::RecordInvalid => e + render json: { error: e.message }, status: :unprocessable_entity + rescue StandardError => e + render json: { error: e.message }, status: :unprocessable_entity end -rescue ActiveRecord::RecordInvalid => e - msg = - if e.record&.errors&.any? - e.record.errors.full_messages.join(', ') - else - e.message - end - render json: { error: msg }, status: :unprocessable_entity -rescue StandardError => e - render json: { error: e.message }, status: :unprocessable_entity -end # PATCH /join_team_requests/1/decline # Decline a join team request def decline @@ -277,4 +271,4 @@ def current_user_is_team_member? participant && team.participants.include?(participant) end -end \ No newline at end of file +end From 0b81cf32005970a657753c0932034a7a2aafd5ed Mon Sep 17 00:00:00 2001 From: AtharvaW195 Date: Tue, 31 Mar 2026 10:29:39 -0400 Subject: [PATCH 12/14] Fix for failing test case in invitation.rb --- app/models/invitation.rb | 76 ++++++++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 31 deletions(-) diff --git a/app/models/invitation.rb b/app/models/invitation.rb index 1b783396d..a8d743858 100644 --- a/app/models/invitation.rb +++ b/app/models/invitation.rb @@ -31,8 +31,8 @@ def self.invited?(from_id, to_id, assignment_id, reply_status = InvitationValida # send invite email def send_invite_email InvitationMailer.with(invitation: self) - .send_invitation_email - .deliver_later + .send_invitation_email + .deliver_later end # This method handles all that needs to be done upon a user accepting an invitation. @@ -40,39 +40,50 @@ def accept inviter_team = from_participant.team invitee_team = to_participant.team - # Wrap in transaction to prevent partial updates and concurrency + error_message = nil + accepted = false + ActiveRecord::Base.transaction do - # 1. Add the invitee to the inviter's team - inviter_team.add_member(to_participant) - - # if participant is member of an existing team then only step 2 and 3 makes sense. otherwise just need to add the participant to the inviter team - if invitee_team.present? - # 2. Update the participant’s and team's assigned topic - inviter_signed_up_team = SignedUpTeam.find_by(team_id: inviter_team.id) - invitee_signed_up_team = SignedUpTeam.find_by(team_id: invitee_team.id) - - SignedUpTeam.update_topic_after_invite_accept(inviter_signed_up_team,invitee_signed_up_team) - - # 3. Remove participant from their old team - invitee_team.remove_member(to_participant) - end + inviter_team.with_lock do + # 0. Capacity check (race-safe) + if inviter_team.full? + error_message = 'Team is full.' + raise ActiveRecord::Rollback + end + + # If participant is member of an existing team, update topics and move them between teams. + if invitee_team.present? && invitee_team.id != inviter_team.id + inviter_signed_up_team = SignedUpTeam.find_by(team_id: inviter_team.id) + invitee_signed_up_team = SignedUpTeam.find_by(team_id: invitee_team.id) + + SignedUpTeam.update_topic_after_invite_accept(inviter_signed_up_team, invitee_signed_up_team) + + # Remove from old team first so participant_id uniqueness does not block the move. + invitee_team.remove_member(to_participant) + end - # 4. Mark this invitation as accepted - update!(reply_status: InvitationValidator::ACCEPT_STATUS) + # Add the invitee to the inviter's team and verify success. + add_result = inviter_team.add_member(to_participant) + unless add_result[:success] + error_message = add_result[:error].presence || 'Unable to add participant to team.' + raise ActiveRecord::Rollback + end + + update!(reply_status: InvitationValidator::ACCEPT_STATUS) + accepted = true + end end - { success: true, message: "Invitation accepted successfully." } + return { success: false, error: (error_message || 'Invitation acceptance failed.') } unless accepted - rescue TeamFullError => e - { success: false, error: e.message } - rescue => e + { success: true, message: 'Invitation accepted successfully.' } + rescue StandardError => e { success: false, error: "Unexpected error: #{e.message}" } end - # This method handles all that needs to be done upon an user declining an invitation. def decline - update(reply_status: InvitationValidator::DECLINED_STATUS) + update(reply_status: InvitationValidator::DECLINED_STATUS) end # This method handles all that need to be done upon an invitation retraction. @@ -89,20 +100,23 @@ def as_json(options = {}) only: %i[id reply_status created_at updated_at], include: { assignment: { only: %i[id name] }, - from_participant: { + from_participant: { only: %i[id], include: { user: { only: %i[id name full_name email] }, - team: {only: %i[id name]} - }}, + team: { only: %i[id name] } + } + }, # from_team: { only: %i[id name] }, - to_participant: { + to_participant: { only: [:id], include: { user: { only: %i[id name full_name email] } - }} + } + } } })).tap do |hash| end end -end \ No newline at end of file +end + From 7c1a01931740308f551cc3c67505540065d0ee78 Mon Sep 17 00:00:00 2001 From: KrishaDarji Date: Fri, 3 Apr 2026 17:26:48 -0400 Subject: [PATCH 13/14] Refactor MentoredTeam to use participant instead of user for mentor identification --- app/models/mentored_team.rb | 15 +++++++------- spec/models/mentored_team_spec.rb | 34 +++++++++++++++---------------- 2 files changed, 23 insertions(+), 26 deletions(-) diff --git a/app/models/mentored_team.rb b/app/models/mentored_team.rb index f052726b5..cda4b66b4 100644 --- a/app/models/mentored_team.rb +++ b/app/models/mentored_team.rb @@ -3,19 +3,17 @@ class MentoredTeam < AssignmentTeam # adds members to the team who are not mentors - def add_member(user) - return false if user == mentor - super(user) + def add_member(participant) + return false if participant == mentor + super(participant) end # Assigning a participant as mentor of the team # Validates if participant has the duty of mentor - def assign_mentor(user) + def assign_mentor(participant) mentor_duty = Duty.find_by(name: 'Mentor') return false unless mentor_duty - - participant = AssignmentParticipant.find_by(user_id: user.id, parent_id: parent_id) - return false unless participant + return false unless participants.exists?(id: participant.id) participant.update(duty_id: mentor_duty.id) end @@ -41,6 +39,7 @@ def type_must_be_mentored_team errors.add(:type, 'must be MentoredTeam') unless type == 'MentoredTeam' end + # Returns the participant on this team who has the Mentor duty def mentor mentor_duty = Duty.find_by(name: 'Mentor') return nil unless mentor_duty @@ -50,6 +49,6 @@ def mentor AssignmentParticipant .joins('INNER JOIN teams_participants ON teams_participants.participant_id = participants.id') .where('teams_participants.team_id = ? AND participants.duty_id = ?', id, mentor_duty.id) - .first&.user + .first end end diff --git a/spec/models/mentored_team_spec.rb b/spec/models/mentored_team_spec.rb index 6e14847ef..13bc735ea 100644 --- a/spec/models/mentored_team_spec.rb +++ b/spec/models/mentored_team_spec.rb @@ -29,7 +29,7 @@ def make_participant(suffix) participant = make_participant('mentor1') team.add_member(participant) - result = team.assign_mentor(participant.user) + result = team.assign_mentor(participant) expect(result).to be true expect(participant.reload.duty_id).to eq(mentor_duty.id) end @@ -38,14 +38,15 @@ def make_participant(suffix) participant = make_participant('mentor2') team.add_member(participant) - result = team.assign_mentor(participant.user) + result = team.assign_mentor(participant) expect(result).to be false end - it 'returns false when user is not a participant in the assignment' do + it 'returns false when participant is not on the team' do mentor_duty - outsider = make_user('outsider_m') - result = team.assign_mentor(outsider) + participant = make_participant('outsider_m') + + result = team.assign_mentor(participant) expect(result).to be false end @@ -53,9 +54,9 @@ def make_participant(suffix) mentor_duty participant = make_participant('duty_mentor') team.add_member(participant) - team.assign_mentor(participant.user) + team.assign_mentor(participant) - expect(team.send(:mentor)).to eq(participant.user) + expect(team.send(:mentor)).to eq(participant) expect(participant.reload.duty).to eq(mentor_duty) end end @@ -65,7 +66,7 @@ def make_participant(suffix) mentor_duty participant = make_participant('remove_mentor') team.add_member(participant) - team.assign_mentor(participant.user) + team.assign_mentor(participant) team.remove_mentor expect(participant.reload.duty_id).to be_nil @@ -82,17 +83,16 @@ def make_participant(suffix) mentor_duty participant = make_participant('blocked_mentor') team.add_member(participant) - team.assign_mentor(participant.user) + team.assign_mentor(participant) - result = team.add_member(participant.user) + result = team.add_member(participant) expect(result).to be false end - it 'does not add unenrolled user' do - unenrolled = make_user('unenrolled_m') - result = team.add_member(unenrolled) + it 'does not add unenrolled participant' do + unenrolled_user = make_user('unenrolled_m') + result = team.add_member(unenrolled_user) expect(result[:success]).to be false - expect(result[:error]).to eq("#{unenrolled.name} is not a participant in this assignment") end it 'allows adding a non-mentor participant' do @@ -105,10 +105,8 @@ def make_participant(suffix) small_assignment = Assignment.create!(name: 'Small Assign', instructor_id: instructor.id, max_team_size: 1) small_team = MentoredTeam.create!(name: 'Small Team', parent_id: small_assignment.id) - user1 = make_user('cap1') - user2 = make_user('cap2') - p1 = AssignmentParticipant.create!(user: user1, parent_id: small_assignment.id, handle: user1.name) - p2 = AssignmentParticipant.create!(user: user2, parent_id: small_assignment.id, handle: user2.name) + p1 = AssignmentParticipant.create!(user: make_user('cap1'), parent_id: small_assignment.id, handle: 'cap1') + p2 = AssignmentParticipant.create!(user: make_user('cap2'), parent_id: small_assignment.id, handle: 'cap2') small_team.add_member(p1) result = small_team.add_member(p2) From 3a5fea069b155b9938000c5444608ffa66ad2272 Mon Sep 17 00:00:00 2001 From: Saladin Al-Bataineh Date: Sun, 5 Apr 2026 23:21:00 -0400 Subject: [PATCH 14/14] Refactor CourseTeam to use participant instead of user - Unenrolled rejection case intentionally still passes a User to prove the system blocks non-participants - All 9 CourseTeam examples passing --- app/models/course_team.rb | 8 +- spec/models/course_team_spec.rb | 134 +++++++++++++------------------- 2 files changed, 57 insertions(+), 85 deletions(-) diff --git a/app/models/course_team.rb b/app/models/course_team.rb index 62a6552e1..f53591a19 100644 --- a/app/models/course_team.rb +++ b/app/models/course_team.rb @@ -5,9 +5,9 @@ class CourseTeam < Team belongs_to :course, class_name: 'Course', foreign_key: 'parent_id' #adds members to the course team post validation - def add_member(user) + def add_member(participant) # return false unless validate_membership(user) - super(user) + super(participant) end # Copies the current course team to an assignment team @@ -29,9 +29,9 @@ def copy_to_assignment_team(assignment) protected - def validate_membership(user) + def validate_membership(participant) # Verify user is enrolled in a course (E2610) - CourseParticipant.exists?(user_id: user.id, parent_id: course.id) + CourseParticipant.exists?(user_id: participant.id, parent_id: course.id) end private diff --git a/spec/models/course_team_spec.rb b/spec/models/course_team_spec.rb index 2eda015ac..70160aaad 100644 --- a/spec/models/course_team_spec.rb +++ b/spec/models/course_team_spec.rb @@ -3,72 +3,43 @@ require 'rails_helper' RSpec.describe CourseTeam, type: :model do - include RolesHelper - # -------------------------------------------------------------------------- - # Global Setup - # -------------------------------------------------------------------------- - # Create the full roles hierarchy once, to be shared by all examples. - before(:all) do - @roles = create_roles_hierarchy - end - # ------------------------------------------------------------------------ - # Helper: DRY-up creation of student users with a predictable pattern. - # ------------------------------------------------------------------------ - def create_student(suffix) - User.create!( - name: suffix, - email: "#{suffix}@example.com", - full_name: suffix.split('_').map(&:capitalize).join(' '), - password_digest: "password", - role_id: @roles[:student].id, - institution_id: institution.id - ) - end - - # ------------------------------------------------------------------------ - # Shared Data Setup: Build core domain objects used across tests. - # ------------------------------------------------------------------------ - let(:institution) do - # All users belong to the same institution to satisfy foreign key constraints. - Institution.create!(name: "NC State") - end + before(:all) { @roles = create_roles_hierarchy } + let(:institution) { Institution.create!(name: 'NC State') } let(:instructor) do - # The instructor will own assignments and courses in subsequent tests. User.create!( - name: "instructor", - full_name: "Instructor User", - email: "instructor@example.com", - password_digest: "password", - role_id: @roles[:instructor].id, - institution_id: institution.id + name: 'instructor', + full_name: 'Instructor User', + email: 'instructor@example.com', + password_digest: 'password', + role_id: @roles[:instructor].id, + institution_id: institution.id ) end + let(:course) { Course.create!(name: 'Course 1', instructor_id: instructor.id, institution_id: institution.id, directory_path: '/course1') } + let(:course_team) { CourseTeam.create!(parent_id: course.id, name: 'team 2') } - let(:team_owner) do + def make_user(suffix) User.create!( - name: "team_owner", - full_name: "Team Owner", - email: "team_owner@example.com", - password_digest: "password", - role_id: @roles[:student].id, - institution_id: institution.id + name: suffix, + email: "#{suffix}@example.com", + full_name: suffix.split('_').map(&:capitalize).join(' '), + password_digest: 'password', + role_id: @roles[:student].id, + institution_id: institution.id ) end - let(:assignment) { Assignment.create!(name: "Assignment 1", instructor_id: instructor.id, max_team_size: 3) } - let(:course) { Course.create!(name: "Course 1", instructor_id: instructor.id, institution_id: institution.id, directory_path: "/course1") } - - let(:course_team) do - CourseTeam.create!( - parent_id: course.id, - name: 'team 2', - ) + def make_participant(suffix) + user = make_user(suffix) + CourseParticipant.create!(user: user, parent_id: course.id, handle: user.name) end - + # ----------------------------------------------------------------------- + # Validations + # ----------------------------------------------------------------------- describe 'validations' do it 'is valid with valid attributes' do expect(course_team).to be_valid @@ -77,7 +48,7 @@ def create_student(suffix) it 'is not valid without a course' do team = build(:course_team, course: nil) expect(team).not_to be_valid - expect(team.errors[:course]).to include("must exist") + expect(team.errors[:course]).to include('must exist') end it 'validates type must be CourseTeam' do @@ -88,44 +59,45 @@ def create_student(suffix) end end - describe '#add_member' do - context 'when user is not enrolled in the course' do - it 'does not add the member to the team' do - unenrolled_user = create_student("add_user") - - expect { - course_team.add_member(unenrolled_user) - }.not_to change(TeamsParticipant, :count) - end - end - end - + # ----------------------------------------------------------------------- + # Associations + # ----------------------------------------------------------------------- describe 'associations' do it { should belong_to(:course) } it { should have_many(:teams_participants).dependent(:destroy) } it { should have_many(:users).through(:teams_participants) } end - describe 'team management' do - let(:enrolled_user) { create(:user, role: create(:role)) } - let(:unenrolled_user) { create(:user, role: create(:role)) } + # ----------------------------------------------------------------------- + # Membership — bullet point 1 + # ----------------------------------------------------------------------- + describe '#add_member' do + context 'when participant is enrolled in the course' do + it 'successfully adds the participant' do + participant = make_participant('enrolled_student') - before do - @participant = create(:course_participant, user: enrolled_user, course: course) + result = course_team.add_member(participant) + expect(result[:success]).to be true + expect(course_team.has_member?(participant.user)).to be true + end end - it 'can add enrolled user' do - result = course_team.add_member(enrolled_user) - - expect(result[:success]).to be true - expect(course_team.has_member?(enrolled_user)).to be true - end + context 'when user is NOT enrolled in the course' do + it 'does not add the member' do + unenrolled_user = make_user('unenrolled_student') + + result = course_team.add_member(unenrolled_user) + expect(result[:success]).to be false + expect(result[:error]).to eq("#{unenrolled_user.name} is not a participant in this course") + end - it 'cannot add unenrolled user' do - result = course_team.add_member(unenrolled_user) + it 'does not create a TeamsParticipant record' do + unenrolled_user = make_user('unenrolled_student2') - expect(result[:success]).to be false - expect(result[:error]).to eq("#{unenrolled_user.name} is not a participant in this course") + expect { + course_team.add_member(unenrolled_user) + }.not_to change(TeamsParticipant, :count) + end end end -end +end \ No newline at end of file