E1920 Fix Code Climate issues: team.rb

  - 20 out of 34 issues are resolved in the team.rb file
  - 12 issues are exempt based on the requirements specification
  - 1 issue is reported as False Positive for Rails 4 and can be ignored
    * Mass assignment is not restricted using attr_accessible
    * Confirmed with Zachariah Mathews
  - Total ignored 13 Code Climate issues that still remain in the file
  - Resolving issues in the team.rb required additional changes to be
    made in the following files:
    * spec/models/team_spec.rb

app/models/team.rb

@@ -2,7 +2,7 @@ class Team < ActiveRecord::Base
   has_many :teams_users, dependent: :destroy
   has_many :users, through: :teams_users
   has_many :join_team_requests, dependent: :destroy
-  has_one :team_node, foreign_key: :node_object_id, dependent: :destroy
+  has_one :team_node, foreign_key: :node_object_id, dependent: :destroy, inverse_of: 'node_object'
   has_many :signed_up_teams, dependent: :destroy
   has_many :bids, dependent: :destroy
   has_paper_trail
@@ -32,7 +32,7 @@ def delete
 
   # Get the node type of the tree structure
   def node_type
-    "TeamNode"
+    'TeamNode'
   end
 
   # Get the names of the users
@@ -97,11 +97,11 @@ def self.check_for_existing(parent, name, team_type)
   # Start by adding single members to teams that are one member too small.
   # Add two-member teams to teams that two members too small. etc.
   def self.randomize_all_by_parent(parent, team_type, min_team_size)
-    participants = Participant.where(parent_id: parent.id, type: parent.class.to_s + "Participant")
+    participants = Participant.where(parent_id: parent.id, type: parent.class.to_s + 'Participant')
     participants = participants.sort { rand(-1..1) }
     users = participants.map {|p| User.find(p.user_id) }.to_a
     # find teams still need team members and users who are not in any team
-    teams = Team.where(parent_id: parent.id, type: parent.class.to_s + "Team").to_a
+    teams = Team.where(parent_id: parent.id, type: parent.class.to_s + 'Team').to_a
     teams_num = teams.size
     i = 0
     teams_num.times do
@@ -126,7 +126,7 @@ def self.randomize_all_by_parent(parent, team_type, min_team_size)
   def self.create_team_from_single_users(min_team_size, parent, team_type, users)
     num_of_teams = users.length.fdiv(min_team_size).ceil
     next_team_member_index = 0
-    for i in (1..num_of_teams).to_a
+    (1..num_of_teams).to_a.each do |i|
       team = Object.const_get(team_type + 'Team').create(name: 'Team_' + i.to_s, parent_id: parent.id)
       TeamNode.create(parent_id: parent.id, node_object_id: team.id)
       min_team_size.times do
@@ -153,7 +153,7 @@ def self.assign_single_users_to_teams(min_team_size, parent, teams, users)
   end
 
   # Generate the team name
-  def self.generate_team_name(team_name_prefix = '')
+  def self.generate_team_name
     counter = 1
     loop do
       team_name = "Team_#{counter}"
@@ -164,36 +164,29 @@ def self.generate_team_name(team_name_prefix = '')
 
   # Extract team members from the csv and push to DB,  changed to hash by E1776
   # todo check if the starting_index is necessary
-  def import_team_members(starting_index = 0, row_hash)
-    starting_index
+  def import_team_members(row_hash, starting_index = 0)
     index = 0
     row_hash[:teammembers].each do |teammember|
       next if index < starting_index # not sure this will work, hash is not ordered like array
       user = User.find_by(name: teammember.to_s)
-      if user.nil?
-        raise ImportError, "The user '#{teammember.to_s}' was not found. <a href='/users/new'>Create</a> this user?"
-      else
-        add_member(user) if TeamsUser.find_by(team_id: id, user_id: user.id).nil?
-      end
+      raise ImportError, "The user '#{teammember}' was not found. <a href='/users/new'>Create</a> this user?" if user.nil?
+      add_member(user) if TeamsUser.find_by(team_id: id, user_id: user.id).nil?
       index += 1
     end
   end
 
   #  changed to hash by E1776
   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].empty? && (options[:has_teamname] == "true_first" || options[:has_teamname] == "true_last"))
-    if options[:has_teamname] == "true_first" || options[:has_teamname] == "true_last"
+    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'))
+    if options[:has_teamname] == 'true_first' || options[:has_teamname] == 'true_last'
       name = row_hash[:teamname].to_s
-      team = where(["name =? && parent_id =?", name, id]).first
+      team = find_by(name: name, parent_id: id)
       team_exists = !team.nil?
-      name = handle_duplicate(team, name, id, options[:handle_dups], teamtype)
+      name = handle_duplicate(team, name, options[:handle_dups], teamtype)
     else
-      if teamtype.is_a?(CourseTeam)
-        name = self.generate_team_name(Course.find(id).name)
-      elsif teamtype.is_a?(AssignmentTeam)
-        name = self.generate_team_name(Assignment.find(id).name)
-      end
+      name = self.generate_team_name if teamtype.is_a?(CourseTeam)
+      name = self.generate_team_name if teamtype.is_a?(AssignmentTeam)
     end
     if name
       team = Object.const_get(teamtype.to_s).create_team_and_node(id)
@@ -202,40 +195,33 @@ def self.import(row_hash, id, options, teamtype)
     end
 
     # 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"
+    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, id, handle_dups, teamtype)
+  def self.handle_duplicate(team, name, handle_dups, teamtype)
     return name if team.nil? # no duplicate
-    return nil if handle_dups == "ignore" # ignore: do not create the new team
-    if handle_dups == "rename" # rename: rename new team
-      if teamtype.is_a?(CourseTeam)
-        return self.generate_team_name(Course.find(id).name)
-      elsif  teamtype.is_a?(AssignmentTeam)
-        return self.generate_team_name(Assignment.find(id).name)
-      end
+    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)
     end
-    if handle_dups == "replace" # replace: delete old team
+    if handle_dups == 'replace' # replace: delete old team
       team.delete
       return name
-    else # handle_dups = "insert"
-      return nil
     end
+    # handle_dups = ignore: do not create the new team
+    # handle_dups = insert: do not insert
+    nil
   end
 
   # Export the teams to csv
   def self.export(csv, parent_id, options, teamtype)
-    if teamtype.is_a?(CourseTeam)
-      teams = CourseTeam.where(parent_id: parent_id)
-    elsif teamtype.is_a?(AssignmentTeam)
-      teams = AssignmentTeam.where(parent_id: parent_id)
-    end
+    teams = CourseTeam.where(parent_id: parent_id) if teamtype.is_a?(CourseTeam)
+    teams = AssignmentTeam.where(parent_id: parent_id) if teamtype.is_a?(AssignmentTeam)
     teams.each do |team|
       output = []
       output.push(team.name)
-      if options[:team_name] == "false"
+      if options[:team_name] == 'false'
         team_members = TeamsUser.where(team_id: team.id)
         team_members.each do |user|
           output.push(user.name)
@@ -248,8 +234,8 @@ def self.export(csv, parent_id, options, teamtype)
 
   # Create the team with corresponding tree node
   def self.create_team_and_node(id)
-    parent = parent_model id # current_task will be either a course object or an assignment object. # current_task will be either a course object or an assignment object.
-    team_name = Team.generate_team_name(parent.name)
+    # current_task will be either a course object or an assignment object.
+    team_name = Team.generate_team_name
     team = self.create(name: team_name, parent_id: id)
     # new teamnode will have current_task.id as parent_id and team_id as node_object_id.
     TeamNode.create(parent_id: id, node_object_id: team.id)

spec/models/team_spec.rb

@@ -159,16 +159,16 @@
       allow(Team).to receive(:find_by).with(name: 'Team_1').and_return(team)
 
       allow(Team).to receive(:find_by).with(name: 'Team_2').and_return(nil)
-      expect(Team.generate_team_name('no name')).to eq('Team_2')
+      expect(Team.generate_team_name).to eq('Team_2')
     end
   end
 
   describe '.import_team_members' do
     context 'when cannot find a user by name' do
       it 'raises an ImportError' do
         allow(User).to receive(:find_by).with(name: 'no name').and_return(nil)
-        expect { team.import_team_members(0, {teammembers: ['no name']}) }.to raise_error(ImportError,
-                                                                           "The user 'no name' was not found. <a href='/users/new'>Create</a> this user?")
+        expect { team.import_team_members({teammembers: ['no name']}) }.to raise_error(ImportError,
+                                                                                       "The user 'no name' was not found. <a href='/users/new'>Create</a> this user?")
       end
     end
 
@@ -177,7 +177,7 @@
         allow(User).to receive(:find_by).with(name: 'no name').and_return(user)
         allow(TeamsUser).to receive(:find_by).with(team_id: 1, user_id: 1).and_return(nil)
         allow_any_instance_of(Team).to receive(:add_member).with(user).and_return(true)
-        expect(team.import_team_members(0, {teammembers: ['no name']})).to eq(['no name'])
+        expect(team.import_team_members({teammembers: ['no name']})).to eq(['no name'])
       end
     end
   end
@@ -219,30 +219,30 @@
   describe '.handle_duplicate' do
     context 'when parameterized team is nil' do
       it 'returns team name' do
-        expect(Team.handle_duplicate(nil, 'no name', 1, 'replace', CourseTeam.new)).to eq('no name')
+        expect(Team.handle_duplicate(nil, 'no name', 'replace', CourseTeam.new)).to eq('no name')
       end
     end
 
     context 'when parameterized team is not nil' do
       context 'when handle_dups option is ignore' do
         it 'does not create the new team and returns nil' do
-          expect(Team.handle_duplicate(team, 'no name', 1, 'ignore', CourseTeam.new)).to be nil
+          expect(Team.handle_duplicate(team, 'no name', 'ignore', CourseTeam.new)).to be nil
         end
       end
 
       context 'when handle_dups option is rename' do
         it 'returns new team name' do
           allow(Course).to receive(:find).with(1).and_return(double('Course', name: 'no course'))
-          allow(Team).to receive(:generate_team_name).with('no course').and_return('new team name')
-          expect(Team.handle_duplicate(team, 'no name', 1, 'rename', CourseTeam.new)).to eq('new team name')
+          allow(Team).to receive(:generate_team_name).and_return('new team name')
+          expect(Team.handle_duplicate(team, 'no name', 'rename', CourseTeam.new)).to eq('new team name')
         end
       end
 
       context 'when handle_dups option is replace' do
         it 'deletes the old team' do
           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)
-          expect(Team.handle_duplicate(team, 'no name', 1, 'replace', CourseTeam.new)).to eq('no name')
+          expect(Team.handle_duplicate(team, 'no name', 'replace', CourseTeam.new)).to eq('no name')
         end
       end
     end