E1920 Fix Code Climate Issues: Development of additional Rspec Tests

  - Additional Rspec tests are added into existing files:
    * student_task_spec.rb: + 1 Rspec Test
    * team_spec.rb: + 14 Rspec Tests
    * vm_question_response_row_spec.rb: + 4 Rspec Tests
    * vm_question_response_spec.rb: + 1 Rspec Test
  - New Rspec tests are developed per existing Model:
    * teams_user_spec.rb + 14 Rspec Tests
  - Fixed bugs found during implementation of Rspec Tests and Refactor:
    * import_file_controller.rb: teamtype argument must be instance of
      either Assignment or Course Team, to allow current implementation
      to work properly. Only class type would result in false execution
      of Object.const_get(teamtype.class.to_s).create_team_and_node(id)
    * teams.rb: Mostly refactoring and improvements of the code quality.
      Adding comments.

app/controllers/import_file_controller.rb

@@ -93,9 +93,9 @@ def import_from_hash(session, params)
       begin
         @header_integrated_body.each do |row_hash|
           if params[:model] == "AssignmentTeam"
-            teamtype = AssignmentTeam
+            teamtype = AssignmentTeam.new
           else
-            teamtype = CourseTeam
+            teamtype = CourseTeam.new
           end
           options = eval(params[:options])
           options[:has_teamname] = params[:has_teamname]

app/models/team.rb

@@ -174,35 +174,34 @@ def import_team_members(row_hash, starting_index = 0)
     end
   end
 
-  #  changed to hash by E1776
+  # Creates or retrieves the team before importing members into it
   def self.import(row_hash, id, options, teamtype)
     raise ArgumentError, 'Not enough fields on this line.' if row_hash.empty? ||
-        (row_hash[:teammembers].length < 2 && (options[:has_teamname] == 'true_first' || options[:has_teamname] == 'true_last'))
+        (row_hash[:teammembers].size < 2 && (options[:has_teamname] == 'true_first' || options[:has_teamname] == 'true_last'))
     if options[:has_teamname] == 'true_first' || options[:has_teamname] == 'true_last'
       name = row_hash[:teamname].to_s
       team = find_by(name: name, parent_id: id)
       team_exists = !team.nil?
       name = handle_duplicate(team, name, options[:handle_dups], teamtype)
     else
-      name = self.generate_team_name if teamtype.is_a?(CourseTeam)
-      name = self.generate_team_name if teamtype.is_a?(AssignmentTeam)
+      name = self.generate_team_name if teamtype.is_a?(CourseTeam) || teamtype.is_a?(AssignmentTeam)
     end
     if name
-      team = Object.const_get(teamtype.to_s).create_team_and_node(id)
+      team = Object.const_get(teamtype.class.to_s).create_team_and_node(id)
       team.name = name
       team.save
     end
-
-    # insert team members into team unless team was pre-existing & we ignore duplicate teams
+    # Return if team is not retrieved or is not created
+    return if team.nil?
+    # Otherwise, insert team members into team unless team was pre-existing & we ignore duplicate teams
     team.import_team_members(row_hash) unless team_exists && options[:handle_dups] == 'ignore'
   end
 
   # Handle existence of the duplicate team
   def self.handle_duplicate(team, name, handle_dups, teamtype)
     return name if team.nil? # no duplicate
     if handle_dups == 'rename' # rename: rename new team
-      return self.generate_team_name if teamtype.is_a?(CourseTeam)
-      return self.generate_team_name if teamtype.is_a?(AssignmentTeam)
+      return self.generate_team_name if teamtype.is_a?(CourseTeam) || teamtype.is_a?(AssignmentTeam)
     end
     if handle_dups == 'replace' # replace: delete old team
       team.delete
@@ -217,6 +216,9 @@ def self.handle_duplicate(team, name, handle_dups, teamtype)
   def self.export(csv, parent_id, options, teamtype)
     teams = CourseTeam.where(parent_id: parent_id) if teamtype.is_a?(CourseTeam)
     teams = AssignmentTeam.where(parent_id: parent_id) if teamtype.is_a?(AssignmentTeam)
+    # Return if teams are not retrieved
+    return csv if teams.nil?
+    # Otherwise, iterate through each team and export its name and each team memeber of a team into csv
     teams.each do |team|
       output = []
       output.push(team.name)

spec/models/student_task_spec.rb

@@ -1,13 +1,14 @@
 describe StudentTask do
   # Write your mocked object here!
-  let(:participant) { build(:participant) }
-  let(:user) { build(:student) }
   let(:assignment) { build(:assignment) }
+  let(:participant) { build(:participant, user_id: 1) }
   let(:student_task) do
-    StudentTask.new(
-      user: user,
-      participant: participant,
-      assignment: assignment
-    )
+    StudentTask.new(assignment: assignment, participant: participant, stage_deadline: Time.zone.now.to_s)
+  end
+
+  describe '#from_participant' do
+    it 'creates new student task using student task as participant' do
+      expect(StudentTask.from_participant(student_task).stage_deadline.to_s).to eq(student_task.stage_deadline.to_s)
+    end
   end
 end

spec/models/team_spec.rb

@@ -7,6 +7,8 @@
   let(:user2) { build(:student, id: 2) }
   let(:user3) { build(:student, id: 3) }
   let(:team) { build(:assignment_team, id: 1, name: 'no team', users: [user]) }
+  let(:team2) { build(:assignment_team, id: 2, name: 'assignment team', users: []) }
+  let(:team3) { build(:course_team, id: 3, name: 'course team', users: [user2]) }
   let(:team_user) { build(:team_user, id: 1, user: user) }
   before(:each) do
     allow(TeamsUser).to receive(:where).with(team_id: 1).and_return([team_user])
@@ -183,13 +185,101 @@
   end
 
   describe '.import' do
-    context 'when row is empty and has_column_names option is not true' do
+    context 'when row is empty and no options are specified' do
       it 'raises an ArgumentError' do
-        expect { Team.import({}, 1, {has_column_names: 'false'}, AssignmentTeam.new) }
+        expect { Team.import({}, 1, {}, AssignmentTeam.new) }
+          .to raise_error(ArgumentError, 'Not enough fields on this line.')
+        end
+    end
+
+    context 'when row contains only one team member and has_teamname option is true_first or true_last' do
+      it 'raises an ArgumentError' do
+        expect { Team.import({teammembers: [user.name]}, 1, {has_teamname: 'true_first'}, AssignmentTeam.new) }
+          .to raise_error(ArgumentError, 'Not enough fields on this line.')
+      end
+
+      it 'raises an ArgumentError' do
+        expect { Team.import({teammembers: [user.name]}, 1, {has_teamname: 'true_last'}, AssignmentTeam.new) }
           .to raise_error(ArgumentError, 'Not enough fields on this line.')
       end
     end
 
+    context 'when row contains only one team member, no options, and team type is AssignmentTeam' do
+      it 'generates new name, saves new team with new name in DB and imports team members' do
+        allow(Team).to receive(:find_by).with(name: 'Team_1').and_return(nil)
+        allow(AssignmentTeam).to receive(:create_team_and_node).with(1).and_return(team)
+        allow(team).to receive(:import_team_members).with(teammembers: [user.name]).and_return(true)
+        expect(team).to receive(:import_team_members).with(teammembers: [user.name])
+        expect(Team.import({teammembers: [user.name]}, 1, {}, AssignmentTeam.new)).to be true
+      end
+    end
+
+    context 'when row contains only one team member, no options, and team type is Team' do
+      it 'name is not generated, returns without creating new team and importing team members' do
+        expect(Team).to_not receive(:create_team_and_node).with(1)
+        expect(team).to_not receive(:import_team_members).with(teammembers: [user.name])
+        Team.import({teammembers: [user.name]}, 1, {}, Team.new)
+      end
+    end
+
+    context 'when row contains team name and multiple team members and multiple options are given' do
+      it 'handles duplicated teams by renaming it and imports team members into renamed team' do
+        allow(Team).to receive(:find_by).with(name: team.name, parent_id: 1).and_return(team)
+        allow(Team).to receive(:find_by).with(name: 'Team_1').and_return(nil)
+        allow(AssignmentTeam).to receive(:create_team_and_node).with(1).and_return(team)
+        allow(team).to receive(:import_team_members).with(teamname: team.name, teammembers: [user.name, user2.name]).and_return(true)
+        expect(team).to receive(:import_team_members).with(teamname: team.name, teammembers: [user.name, user2.name])
+        # Generate with options has_teamname: 'true_first', handle_dups: 'rename'
+        expect(Team.import({teamname: team.name, teammembers: [user.name, user2.name]}, 1,
+                           {has_teamname: 'true_first', handle_dups: 'rename'}, AssignmentTeam.new)).to be true
+      end
+
+      it 'handles duplicated teams by replacing old team and imports team members into new team' do
+        allow(Team).to receive(:find_by).with(name: team.name, parent_id: 1).and_return(team)
+        allow(TeamsUser).to receive_message_chain(:where, :find_each).with(team_id: 1).with(no_args).and_yield(team_user)
+        allow(team_user).to receive(:destroy).and_return(team_user)
+        allow(AssignmentTeam).to receive(:create_team_and_node).with(1).and_return(team2)
+        allow(team2).to receive(:import_team_members).with(teamname: team.name, teammembers: [user.name, user2.name]).and_return(true)
+        expect(team2).to receive(:import_team_members).with(teamname: team.name, teammembers: [user.name, user2.name])
+        # Generate with options has_teamname: 'true_last', handle_dups: 'replace'
+        expect(Team.import({teamname: team.name, teammembers: [user.name, user2.name]}, 1,
+                           {has_teamname: 'true_last', handle_dups: 'replace'}, AssignmentTeam.new)).to be true
+      end
+
+      it 'does not create new team, but imports team members into existing team' do
+        allow(Team).to receive(:find_by).with(name: team.name, parent_id: 1).and_return(team)
+        allow(User).to receive(:find_by).with(name: user.name).and_return(user)
+        allow(User).to receive(:find_by).with(name: user2.name).and_return(user2)
+        allow(TeamsUser).to receive(:find_by).with(team_id: 1, user_id: 1).and_return(team_user)
+        allow(TeamsUser).to receive(:find_by).with(team_id: 1, user_id: 2).and_return(team_user)
+        expect(AssignmentTeam).to_not receive(:create_team_and_node).with(1)
+        allow(team).to receive(:import_team_members).with(teamname: team.name, teammembers: [user.name, user2.name]).and_return(true)
+        expect(team).to receive(:import_team_members).with(teamname: team.name, teammembers: [user.name, user2.name])
+        # Generate with options has_teamname: 'true_first'
+        expect(Team.import({teamname: team.name, teammembers: [user.name, user2.name]}, 1,
+                           {has_teamname: 'true_first'}, AssignmentTeam.new)).to be true
+      end
+
+      it 'has no duplicate, creates new team and imports team members ' do
+        allow(Team).to receive(:find_by).with(name: '', parent_id: 1).and_return(nil)
+        allow(AssignmentTeam).to receive(:create_team_and_node).with(1).and_return(team)
+        allow(team).to receive(:import_team_members).with(teammembers: [user.name, user2.name]).and_return(true)
+        expect(team).to receive(:import_team_members).with(teammembers: [user.name, user2.name])
+        # Generate with options has_teamname: 'true_last'
+        expect(Team.import({teammembers: [user.name, user2.name]}, 1,
+                           {has_teamname: 'true_last'}, AssignmentTeam.new)).to be true
+      end
+
+      it 'ignores duplicates, does not creates new team, and does not imports team members ' do
+        allow(Team).to receive(:find_by).with(name: team.name, parent_id: 1).and_return(team)
+        expect(AssignmentTeam).to_not receive(:create_team_and_node).with(1)
+        expect(team).to_not receive(:import_team_members).with(teammembers: [user.name, user2.name])
+        # Generate with options has_teamname: 'true_last', handle_dups: 'replace'
+        Team.import({teamname: team.name, teammembers: [user.name, user2.name]}, 1,
+                    {has_teamname: 'true_last', handle_dups: 'ignore'}, AssignmentTeam.new)
+      end
+    end
+
     # E1776 (Fall 2017)
     #
     # The tests below are no longer reflective of the current import that uses row_hash ==> {teammembers: ['name', 'name'], teamname: 'teamname'}.
@@ -249,10 +339,37 @@
   end
 
   describe '.export' do
-    it 'exports teams to csv' do
-      allow(AssignmentTeam).to receive(:where).with(parent_id: 1).and_return([team])
-      allow(TeamsUser).to receive(:where).with(team_id: 1).and_return([team_user])
-      expect(Team.export([], 1, {team_name: 'false'}, AssignmentTeam.new)).to eq([["no team", "no name"]])
+    context 'when exporting single assignment and course teams to csv' do
+      it 'exports single assignments team to csv' do
+        allow(AssignmentTeam).to receive(:where).with(parent_id: 1).and_return([team])
+        allow(TeamsUser).to receive(:where).with(team_id: 1).and_return([team_user])
+        expect(Team.export([], 1, {team_name: 'false'}, AssignmentTeam.new)).to eq([[team.name, user.name]])
+      end
+      it 'exports single course team to csv' do
+        allow(CourseTeam).to receive(:where).with(parent_id: 1).and_return([team3])
+        allow(TeamsUser).to receive(:where).with(team_id: 3).and_return([team_user])
+        expect(Team.export([], 1, {team_name: 'false'}, CourseTeam.new)).to eq([[team3.name, user.name]])
+      end
+      it 'exports only single assignment team name to csv' do
+        allow(AssignmentTeam).to receive(:where).with(parent_id: 1).and_return([team])
+        allow(TeamsUser).to receive(:where).with(team_id: 1).and_return([team_user])
+        expect(Team.export([], 1, {}, AssignmentTeam.new)).to eq([[team.name]])
+      end
+    end
+
+    context 'when exporting multiple assignment teams to csv' do
+      it 'exports single assignments team to csv' do
+        allow(AssignmentTeam).to receive(:where).with(parent_id: 1).and_return([team, team2])
+        allow(TeamsUser).to receive(:where).with(team_id: 1).and_return([team_user])
+        allow(TeamsUser).to receive(:where).with(team_id: 2).and_return([])
+        expect(Team.export([], 1, {team_name: 'false'}, AssignmentTeam.new)).to eq([[team.name, user.name], [team2.name]])
+      end
+    end
+
+    context 'when exporting unhandled teams to csv' do
+      it 'does not export unhandled teams to csv' do
+        expect(Team.export([], 1, {team_name: 'false'}, Team.new)).to be_empty
+      end
     end
   end
 end

spec/models/teams_user_spec.rb

@@ -0,0 +1,128 @@
+describe TeamsUser do
+  # Write your mocked object here!
+  let(:participant) { build(:participant, user_id: 1) }
+  let(:user) { build(:student, id: 1, name: 'user name', fullname: 'user name', participants: [participant], parent_id: 1) }
+  let(:team) { build(:assignment_team, id: 1, name: 'assignment team', users: [], parent_id: 1) }
+  let(:team_user) { build(:team_user, id: 1, user: user) }
+
+  describe '#name' do
+    it 'determines user name based on the ip address' do
+      teams_user = TeamsUser.new
+      teams_user.user = user
+      expect(teams_user.name(nil)).to eq(user.name)
+    end
+  end
+
+  describe '#delete' do
+    it 'destroys the self team user object' do
+      allow(TeamUserNode).to receive(:find_by).with(node_object_id: 1).and_return(TeamUserNode.new)
+      expect(TeamUserNode).to receive(:find_by).with(node_object_id: 1)
+      expect(team_user.destroy)
+      team_user.delete
+    end
+  end
+
+
+  describe '#remove_team' do
+    context 'when team user is found' do
+      it 'removes entry in the TeamUsers table for the given user and given team id' do
+        allow(TeamsUser).to receive(:find_by).with(user_id: user.id, team_id: team.id).and_return(team_user)
+        expect(team_user.destroy)
+        TeamsUser.remove_team(user.id, team.id)
+      end
+    end
+
+    context 'when team user is not found' do
+      it 'does not remove entry in the TeamUsers table for the given user and given team id' do
+        allow(TeamsUser).to receive(:find_by).with(user_id: user.id, team_id: team.id).and_return(nil)
+        expect(TeamsUser).to_not receive(:destroy)
+        TeamsUser.remove_team(user.id, team.id)
+      end
+    end
+  end
+
+  describe '#first_by_team_id' do
+    context 'when team users are found' do
+      it 'returns team user' do
+        allow(TeamsUser).to receive(:find_by).with(team_id: team.id).and_return(team_user)
+        expect(TeamsUser.first_by_team_id(team.id)).to eq(team_user)
+      end
+    end
+
+    context 'when no team users are found' do
+      it 'returns team user' do
+        allow(TeamsUser).to receive(:find_by).with(team_id: 2).and_return(nil)
+        expect(TeamsUser.first_by_team_id(2)).to eq(nil)
+      end
+    end
+  end
+
+  describe '#team_empty?' do
+    context 'when team users are found' do
+      it 'returns false' do
+        allow(TeamsUser).to receive(:where).with('team_id = ?', team.id).and_return([team_user])
+        expect(TeamsUser.team_empty?(team.id)).to be false
+      end
+    end
+
+    context 'when no team users are found' do
+      it 'returns true' do
+        allow(TeamsUser).to receive(:where).with('team_id = ?', 2).and_return([])
+        expect(TeamsUser.team_empty?(2)).to be true
+      end
+    end
+  end
+
+  describe '#add_member_to_invited_team' do
+    context 'when invited user, team user and assignment team are all found' do
+      it 'returns true' do
+        allow(TeamsUser).to receive(:where).with(team_id: team.id).and_return([team_user])
+        allow(TeamsUser).to receive(:where).with(['user_id = ?', team.id]).and_return([team_user])
+        allow(AssignmentTeam).to receive(:find_by).with(id: team.id, parent_id: team.parent_id).and_return(team)
+        allow(User).to receive(:find).with(user.id).and_return(user)
+        allow(TeamsUser).to receive(:create).with(user_id: user.id, team_id: team.id).and_return(team_user)
+        allow(TeamNode).to receive(:find_by).with(node_object_id: 1).and_return(double('TeamNode', id: 1))
+        expect(TeamsUser.add_member_to_invited_team(team.id, user.id, team.id)).to be true
+      end
+    end
+
+    context 'when team user or assignment team is not found' do
+      it 'returns nil when team user is not found' do
+        allow(TeamsUser).to receive(:where).with(['user_id = ?', team.id]).and_return([])
+        expect(TeamsUser.add_member_to_invited_team(team.id, user.id, team.id)).to eq(nil)
+      end
+
+      it 'returns nil when assignment team is not found' do
+        allow(TeamsUser).to receive(:where).with(['user_id = ?', team.id]).and_return([team_user])
+        allow(AssignmentTeam).to receive(:find_by).with(id: team.id, parent_id: team.parent_id).and_return(nil)
+        allow(User).to receive(:find).with(user.id).and_return(user)
+        expect(TeamsUser.add_member_to_invited_team(team.id, user.id, team.id)).to eq(nil)
+      end
+    end
+  end
+
+  describe '#team_id' do
+    context 'when teams user and team are found' do
+      it 'returns team user id' do
+        allow(TeamsUser).to receive(:where).with(user_id: user.id).and_return([team_user])
+        allow(Team).to receive(:find).with(team.id).and_return(team)
+        expect(TeamsUser.team_id(team.id, user.id)).to eq(team_user.id)
+      end
+    end
+
+    context 'when teams user is not found' do
+      it 'nil' do
+        allow(TeamsUser).to receive(:where).with(user_id: user.id).and_return([])
+        expect(TeamsUser.team_id(team.id, user.id)).to eq(nil)
+      end
+    end
+
+    context 'when teams parent id varies from assignment id' do
+      it 'nil' do
+        allow(TeamsUser).to receive(:where).with(user_id: user.id).and_return([team_user])
+        allow(Team).to receive(:find).with(team.id).and_return(Team.new)
+        expect(TeamsUser.team_id(team.id, user.id)).to eq(nil)
+      end
+    end
+  end
+end