From 96cec7fa82c1c55e31e0196d3a025111a4fe134a Mon Sep 17 00:00:00 2001 From: Nikolay G Titov Date: Sun, 31 Mar 2019 13:08:42 -0400 Subject: [PATCH] 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 | 4 +- app/models/team.rb | 20 +-- spec/models/student_task_spec.rb | 15 ++- spec/models/team_spec.rb | 129 ++++++++++++++++++- spec/models/teams_user_spec.rb | 128 ++++++++++++++++++ spec/models/vm_question_response_row_spec.rb | 27 ++++ spec/models/vm_question_response_spec.rb | 14 ++ 7 files changed, 313 insertions(+), 24 deletions(-) create mode 100644 spec/models/teams_user_spec.rb diff --git a/app/controllers/import_file_controller.rb b/app/controllers/import_file_controller.rb index 516a8456ea6..fde159e1ddc 100644 --- a/app/controllers/import_file_controller.rb +++ b/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] diff --git a/app/models/team.rb b/app/models/team.rb index 71c57e150ae..e7e97e708af 100644 --- a/app/models/team.rb +++ b/app/models/team.rb @@ -174,26 +174,26 @@ 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 @@ -201,8 +201,7 @@ def self.import(row_hash, id, options, teamtype) 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) diff --git a/spec/models/student_task_spec.rb b/spec/models/student_task_spec.rb index df31f78888a..a2310db986d 100644 --- a/spec/models/student_task_spec.rb +++ b/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 diff --git a/spec/models/team_spec.rb b/spec/models/team_spec.rb index f37291c99e0..6b9bf863dbb 100644 --- a/spec/models/team_spec.rb +++ b/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 diff --git a/spec/models/teams_user_spec.rb b/spec/models/teams_user_spec.rb new file mode 100644 index 00000000000..98550277143 --- /dev/null +++ b/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 diff --git a/spec/models/vm_question_response_row_spec.rb b/spec/models/vm_question_response_row_spec.rb index 242bf0153b2..03c75305de6 100644 --- a/spec/models/vm_question_response_row_spec.rb +++ b/spec/models/vm_question_response_row_spec.rb @@ -1,5 +1,11 @@ describe VmQuestionResponseRow do let(:row){ VmQuestionResponseRow.new(text: 'Question for testing average score', id: 1, weight: 1, max_score: 5, seq: 1)} + let(:question) { Question.new } + let(:scored_question) { ScoredQuestion.new } + before(:each) do + allow(Question).to receive(:find).with(1).and_return(question) + allow(Question).to receive(:find).with(2).and_return(scored_question) + end describe "#average_score_for_row" do it 'returns correct average score for all not nil scores' do @@ -76,4 +82,25 @@ expect(row.average_score_for_row).to eq(2.5) end end + + describe "#question_max_score" do + it 'returns N/A since the question type is not specified and question is not scored question' do + expect(row.question_max_score).to eq('N/A') + end + + it 'returns N/A since the question type is MultipleChoiceCheckbox and question is not scored question' do + question.type = 'MultipleChoiceCheckbox' + expect(row.question_max_score).to eq('N/A') + end + + it 'returns 1 by specifying the type to Checkbox and question is not scored question' do + question.type = 'Checkbox' + expect(row.question_max_score).to eq(1) + end + + it 'returns correct by specifying the type to Checkbox and question is not scored question' do + row.instance_variable_set(:@question_id, 2) + expect(row.question_max_score).to eq(5) + end + end end diff --git a/spec/models/vm_question_response_spec.rb b/spec/models/vm_question_response_spec.rb index 5b601d65074..77be60455fa 100644 --- a/spec/models/vm_question_response_spec.rb +++ b/spec/models/vm_question_response_spec.rb @@ -3,6 +3,7 @@ let(:author_feedback_questionnaire) { AuthorFeedbackQuestionnaire.new } let(:teammate_review_questionnaire) { TeammateReviewQuestionnaire.new } let(:metareview_questionnaire) { MetareviewQuestionnaire.new } + let(:bookmarkrating_questionnaire) { BookmarkRatingQuestionnaire.new } let(:assignment) { build(:assignment) } let(:question) { build(:question, id: 2, questionnaire: review_questionnaire, weight: 2, type: 'good') } let(:questions) { [question] } @@ -109,6 +110,19 @@ expect(response.list_of_reviewers.first).to eq(participant) end end + + context 'when intitialized with a bookmark review type (unhandled)' do + it 'does not add reviews' do + response = VmQuestionResponse.new(bookmarkrating_questionnaire, assignment, 1) + allow(participant).to receive(:bookmark_reviews).and_return(reviews) + allow(BookmarkRatingResponseMap).to receive(:find_by).with(id: 1).and_return(double('BookmarkRatingResponseMap', reviewer_id: 1)) + response.add_reviews(participant, team, false) + expect(response.list_of_reviews).to be_empty + expect(response.list_of_reviewers).to be_empty + expect(response.list_of_reviews.first).to be_nil + expect(response.list_of_reviewers.first).to be_nil + end + end end describe '#display_team_members' do