Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Fall 2020 - E2083. Revision planning tool E2016

2020-11-24T15:14:16Z

Dyang23: /* Files Modified */

This page provides a description of the Expertiza based OSS project.

__TOC__
==Introduction==
Rounds of peer reviews may be implemented between submissions for assignments on Expertiza. In order to better track the implementation of reviewer's suggestions, a Revision Planning Tool should be implemented.

===Problem Statement===
In the first round of Expertiza reviews, we ask reviewers to give authors some guidance on how to improve their work. Then in the second round, reviewers rate how well authors have followed their suggestions. We could carry the interaction one step further if we asked authors to make up a revision plan based on the first-round reviews. That is, authors would say what they were planning to do to improve their work. Then second-round reviewers would assess how well they did it. In essence, this means that authors would be adding criteria to the second-round rubric that applied only to their submission. We are interested in having this implemented and used in a class so that we can study its effect.

===Previous Implementations===
Revision planning has been implemented twice before, once in [https://expertiza.csc.ncsu.edu/index.php/E1875_Revision_Planning_Tool E1875] and once in [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2020_E2016_Revision_planning_tool E2016]. While the functionality worked and effectively minimized changes to the code, they also had the following problems:
*Hardcoded “round” numbers in many places of the code.
*Documentation does not reflect the new changes they made.
*Revision planning responses and responses to the other items are not distinguished in heatgrid view.
*The idea of adding a <code>team_id</code> field to each question is intuitive. However, they failed to come up with a clean implementation of this idea. Specifically, they had passed some trailing parameters several methods down before reaching the place that needs them.

===Rationale===
To implement revision planning, [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2020_E2016_Revision_planning_tool E2016] added <code>is_revision_planning_enabled</code> (Boolean) to <code>Assignment</code> and <code>team_id</code> to <code>Question</code>. Adding a Boolean to enable revision planning for an assignment makes sense, but having certain questions belong to a team is not a clean implementation. Instead, we will create a new questionnaire that will belong to each team and keep track of the set of questions. We will use this new questionnaire to separate the revision plan responses and responses to other items in heat-grid view by creating a different table for each.

===Files Modified===
This is a list of files modified and added. The list will be updated as the implementation progresses.
 
''Changed'': 
*app/controllers/advice_controller.rb
*app/controllers/grades_controller.rb
*app/controllers/questionnaires_controller.rb
*app/controllers/questions_controller.rb
*app/controllers/response_controller.rb
*app/controllers/student_task_controller.rb
*app/helpers/summary_helper.rb
*app/helpers/grades_helper.rb
*app/models/questionnaire.rb
*app/models/response.rb
*app/models/student_task.rb
*app/models/vm_question_response.rb
*app/models/on_the_fly_calc.rb
*app/views/advice/edit_advice.html.erb
*app/views/assignments/edit/_general.html.erb
*app/views/grades/view_team.html.erb
*app/views/questionnaires/_questionnaire.html.erb
*app/views/response/response.html.erb
*app/views/student_task/view.html.erb
*config/routes.rb
*db/schema.rb
*spec/controllers/response_controller_spec.rb
*spec/models/response_spec.rb
*spec/models/review_response_map_spec.rb
*spec/features/assignment_creation_general_tab_spec.rb

''Added'': 
*app/controllers/revision_plan_questionnaires_controller.rb
*app/models/revision_plan_questionnaire.rb
*app/models/revision_plan_team_map.rb
*app/views/questionnaires/_questions.html.erb
*app/views/revision_plan_questionnaires/edit.html.erb
*db/migrate/20201106020642_create_revision_plan_team_maps.rb /
*db/migrate/20201105023711_add_is_revison_planning_enabled_to_assignments.rb
*spec/controllers/revision_plan_questionnaires_controller_spec.rb
*spec/factories/revision_plan_factory.rb

==Design==
===Database Design===
Items in green are additions.
[[File:E2083_Proposed_Design.png|1000px|center]]
 
Changes:
*In the assignment table we have added <code>is_revision_planning_enabled?</code> column to indicate whether the assignment accepts a revision plan along with review rubric.
*<code>RevisionPlanTeamMap</code> maps a questionnaire to an assignment team and round. This will map to a questionnaire of type revision plan that will be created by the revewee.
*<code>RevisionPlanQuestionnaire</code> extends <code>Questionnaire</code> using single table inheritance.

===User Interface===
====Enable Revision Planning====
In order to enable '''Revision Planning''', the setting must be enabled when creating or editing an assignment under the '''General''' tab. The wireframe below demonstrates creating an assignment, and editing the assignment functions similarly. '''Revision plan?''' should be checked to enable this option.

=====Initial Wireframe=====
[[File:Enable_Revision_Planning_Wireframe.png|700px|thumb|center|Wireframe of Enabling Assignment's Revision Planning]]

=====Final UI=====
[[File:Create_New_Assignment.png|700px|thumb|center|Create New Assignment page in the UI, with added Revision Planning checkbox]]

=====Implementation=====

* Added a Boolean field <code>is_revision_planning_enabled</code> to <code>Assignment</code> that saves whether revision planning is enabled.
''db/migrate/20201105023711_add_is_revison_planning_enabled_to_assignments.rb''

class AddIsRevisonPlanningEnabledToAssignments < ActiveRecord::Migration
def change
add_column :assignments, :is_revision_planning_enabled, :boolean, default: false
end
end

* Add checkbox to enable revision planning in edit assignment view.
''app/views/assignments/edit/_general.html.erb''

<tr>
<td style='padding:5px' id='assignment_revison_planning_field'>
<input name="assignment_form[assignment][is_revision_planning_enabled]" type="hidden" value="false"/>
<%= check_box_tag('assignment_form[assignment][is_revision_planning_enabled]', 'true', @assignment_form.assignment.is_revision_planning_enabled) %>
<%= label_tag('assignment_form[assignment][is_revision_planning_enabled]', 'Revision Planning?') %>
</td>
</tr>

====Assignment Overview Page (contains the Link to the revision planning page)====
The '''Revision Planning''' link is available to students during every submission period (except the first round submission) and not available during every review period. As shown in the wireframe, by clicking '''Revision Planning''' students would be redirected to the ‘Revision planning page’.

=====Initial Wireframe=====
[[File:Link_to_the_revision_planning.jpeg|600px|thumb|center|Wireframe of Assignment Overview Page]]

=====Final UI=====
[[File:Revision_Planning_Link.png|600px|thumb|center|Submit or Review Work page demonstrating added Revision Planning link]]

=====Implementation=====
* Show link to revision planning page when revision planning is enabled for an assignment, round 1 has passed and current stage is submission.

In order to check if a team is allowed to submit a revision plan <code>@can_submit_revision_plan</code> field is added to <code>StudentTaskController</code>'s view action. If a revision plan questionnaire (<code>@revision_plan_questionnaire_id</code>) exists for the current round then clicking the link performs <code>RevisionPlanQuestionnairesController</code>'s edit action else new action is performed.

''app/views/student_task/view.html.erb''

<% if @team && @assignment.is_revision_planning_enabled %>
<li>
<% if @can_submit_revision_plan %>
<% if @revision_plan_questionnaire_id %>
<%= link_to "Revision Planning", controller: 'revision_plan_questionnaires', action: 'edit', id: @revision_plan_questionnaire_id %>
<% else %>
<%= link_to "Revision Planning", controller: 'revision_plan_questionnaires', action: 'new', team_id: @team %>
<% end %>
(Add new questions to next-round rubric)
<% else %>
Revision Planning (You are not allowed to submit your revison plan right now)
<% end %>
</li>
<%end%>

''app/controllers/student_task_controller.rb''

def view
student_task = StudentTask.from_participant_id params[:id]
...
...
# Revision plan feature
@can_submit_revision_plan = student_task.can_submit_revision_plan?
@revision_plan_questionnaire_id = student_task.revision_plan_questionnaire_id
end

''app/models/student_task.rb''

def current_round
assignment.number_of_current_round(@topic.try(:id))
end
def can_submit_revision_plan?
current_stage == 'submission' && current_round > 1
end
def revision_plan_questionnaire_id
RevisionPlanTeamMap.find_by(team: @participant.team, used_in_round: current_round).try(:questionnaire_id)
end

====Editing the Revision Plan Questionnaire====
After creating the Revision Plan Questionnaire, it must be edited. Questions can be added by specifying the amount of questions and their type. Questions can be removed by clicking '''Remove''' in the leftmost column. Once the questionnaire is complete, it can be saved. This page will be visible during each submission period after the first and will be unavailable during all review periods.

=====Initial Wireframe=====
[[File:Edit_Revision_Plan_Wireframe.png|500px|thumb|center|Wireframe of Editing an Assignment's Revision Plan]]

=====Final UI=====
[[File:Edit_Revision_Plan.png|700px|thumb|center|Edit Revision Plan page demonstrating added view]]

=====Implementation=====
* Create a model RevisionPlanTeamMap that maps a revision plan to a team and stores information of the round it is used in.

''/app/models/revision_plan_team_map.rb''

class RevisionPlanTeamMap < ActiveRecord::Base
belongs_to :team
belongs_to :questionnaire
end

''/db/migrate/20201106020642_create_revision_plan_team_maps.rb''

class CreateRevisionPlanTeamMaps < ActiveRecord::Migration
def change
create_table :revision_plan_team_maps do |t|
t.references :team, index: true, foreign_key: true
t.references :questionnaire, index: true, foreign_key: true
t.integer :used_in_round
t.timestamps null: false
end
end
end

* Create a Revision Plan Questionnaire Model and Controller

[https://github.com/SidharthMehta/expertiza/blob/1b54cf6c7aa2b1195c5259a4ee616f21fc2f37a3/app/models/revision_plan_questionnaire.rb ''app/models/revision_plan_questionnaire.rb'']

Added a model for revision plan questionnaire. The code provides method to get questionnaire for a current round based on team.

def self.get_questionnaire_for_current_round(team_id)
assignment_team = Team.find(team_id)
assignment = assignment_team.assignment
current_round = assignment.number_of_current_round(assignment_team.topic)
questionnaire = RevisionPlanTeamMap.find_by(team: assignment_team, used_in_round: current_round).try(:questionnaire)
unless questionnaire
questionnaire = RevisionPlanQuestionnaire.new
questionnaire.name = 'Revision Plan Questionnaire'
questionnaire.instructor_id = assignment.instructor_id
questionnaire.max_question_score = 5
questionnaire.save
questionnaire_team_map = RevisionPlanTeamMap.create(team_id: assignment_team.id, used_in_round: current_round, questionnaire_id: questionnaire.id)
end
return questionnaire
end

This method returns the team associated to a revision plan questionnaire.

def team
revision_plan_team_map.team
end

''app/models/questionnaire.rb''

Added a new type of questionnaire <code>'RevisionPlanQuestionnaire'</code> to <code>QUESTIONNAIRE_TYPES</code>

QUESTIONNAIRE_TYPES = ['ReviewQuestionnaire',
...
...
'RevisionPlanQuestionnaire'].freeze

[https://github.com/SidharthMehta/expertiza/blob/1b54cf6c7aa2b1195c5259a4ee616f21fc2f37a3/app/controllers/revision_plan_questionnaires_controller.rb ''app/controllers/revision_plan_questionnaires_controller.rb'']

Added a controller <code>RevisionPlanQuestionnairesController</code>. The controller derives from <code>QuestionnairesController</code>, it overrides the <code>new</code> action to allow creation of a <code>RevisionPlanQuestionnaire</code> for a <code>team</code>. It also updates permissions for controller.

def action_allowed?
case params[:action]
when 'edit'
questionnaire = Questionnaire.find(params[:id])
questionnaire.team.users.collect { |u| u.id }.include? session[:user].id || super
(user_logged_in? && questionnaire.team.users.collect { |u| u.id }.include?(session[:user].id)) || super
else
super
end
end

def new
begin
questionnaire = RevisionPlanQuestionnaire.get_questionnaire_for_current_round(params[:team_id])
redirect_to action: 'edit', id: questionnaire.id
rescue StandardError
flash[:error] = $ERROR_INFO
end
end

''config/routes.rb''

Add routes for <code>RevisionPlanQuestionnairesController</code>.

resources :revision_plan_questionnaires, controller: :revision_plan_questionnaires, only: %i[new edit update] do
collection do
post :add_new_questions
post :save_all_questions
end
end

* Add a view to edit Revision Plan

[https://github.com/expertiza/expertiza/blob/d97eb1c1fc8d578b18f51401dc713b0d04815fe4/app/views/revision_plan_questionnaires/edit.html.erb ''app/views/revision_plan_questionnaires/edit.html.erb'']

<%= render :partial => 'question_weight' %>
<h1>Edit <%= @questionnaire.display_type %></h1>
<%= render :partial => 'questions' %>
 
<a href="javascript:window.history.back()">Back</a>

* Refactor code to display questions into a partial. Allows for code reuse.

''app/controllers/questionnaires_controller.rb''

Updated <code>redirect_to</code> links for <code>add_new_questions</code> and <code>save_all_questions</code> methods.

redirect_to action: 'edit', id: questionnaire_id.to_sym

''app/views/questionnaires/_questionnaire.html.erb''

This file generates a view to edit/create a new questionnaire. It has functionality to display questions and some functions that are not needed for revision plan questionnaire. To enable code reuse the functionality to add/edit/remove/save questions was moved to a new partial ''app/views/questionnaires/_questions.html.erb''.

[https://github.com/expertiza/expertiza/blob/d97eb1c1fc8d578b18f51401dc713b0d04815fe4/app/views/questionnaires/_questions.html.erb ''app/views/questionnaires/_questions.html.erb'']

*Add code to allow users to delete questions from revision plan questionnaire.

''app/controllers/questions_controller.rb ''

Update authorization code to allow a user to remove questions from his teams revision plan.

def action_allowed?
if ['destroy'].include?(params[:action])
question = Question.find(params[:id])
if(user_logged_in? && question.questionnaire.owner?(session[:user].id))
return true
end
end
current_user_has_ta_privileges?
end

def destroy
...
...
if(question.questionnaire.type == 'RevisionPlanQuestionnaire')
redirect_to edit_revision_plan_questionnaire_path(questionnaire_id.to_s.to_sym)
else
redirect_to edit_questionnaire_path(questionnaire_id.to_s.to_sym)
end
end

''app/models/questionnaire.rb''

Method returns true if user owns questionnaire.

def owner?(user_id)
instructor_id == user_id
end

''app/models/revision_plan_questionnaire.rb''

Overrides implementation for <code>owner?</code> defined in <code>Questionnaire</code> to return true if a team owns the questionnaire.

def owner?(user_id)
team.users.map{ |u| u.id }.include?(user_id) || super
end

*Add code to allow users to access show/edit advice functionality for revision plan questions.

''app/controllers/advice_controller.rb''

Update authorization code to allow a user to update advice his teams revision plan.

def action_allowed?
questionnaire = Questionnaire.find(params[:id])
if(user_logged_in? && questionnaire.owner?(session[:user].id))
return true
end
current_user_has_ta_privileges?
end

''app/views/advice/edit_advice.html.erb''

Update 'Back to questionnaire' link.

<% if @questionnaire.type == 'RevisionPlanQuestionnaire'%>
<%= link_to 'Back to questionnaire', :controller => 'revision_plan_questionnaires', :action => 'edit', :id => @questionnaire.id %>
<% else %>
<%= link_to 'Back to questionnaire', :controller => 'questionnaires', :action => 'edit', :id => @questionnaire.id %>
<% end %>

====Reviewing an Assignment====
Once the Revision Plan Questionnaire and its resubmission round have been finished, the Revision Plan Questionnaire is appended to the Assignment Questionnaire for that round.

=====Initial Wireframe=====
[[File:Review_Wireframe.png|400px|thumb|center|Wireframe of Completing a Review for an Assignment]]

=====Final UI=====
[[File:Revision_Plan_Questionnaire.png|400px|thumb|center|Review page demonstrating added Revision Plan questions]]

=====Implementation=====

*Add Revision Plan questions to Response

''app/controllers/response_controller.rb''

This methods gets questions sorted by sequence from review and revision questionnaire and merges them in a single list.

def set_questions_for_new_response
@questions = sort_questions(@questionnaire.questions)
if(@assignment && @assignment.is_revision_planning_enabled)
reviewees_topic = SignedUpTeam.topic_id_by_team_id(@contributor.id)
current_round = @assignment.number_of_current_round(reviewees_topic)
@revision_plan_questionnaire = RevisionPlanTeamMap.find_by(team_id: @map.reviewee_id, used_in_round: current_round).try(:questionnaire)
if(@revision_plan_questionnaire)
@questions += sort_questions(@revision_plan_questionnaire.questions)
end
end
return @questions
end

This method gets questions from a response instead of questionnaire. We use this when response has already been created.

def set_questions
@questions = @response.get_questions
end

''app/models/response.rb''

Added a method to get all the questionnaires for a response, response is a collection of answers. Answers contain reference to their question, and question have a reference to questionnaire.

def questionnaires_by_answers(answers)
answers_with_questionnaires = answers.select{ |ans| ans && ans.question && ans.question.questionnaire }
questionnaires = answers_with_questionnaires.collect{ |ans| ans.question.questionnaire }.uniq
unless(questionnaires.any?)
questionnaires = []
questionnaires << questionnaire_by_answer(answers.first)
end
questionnaires
end

This method gets all the questions for a response.

def get_questions
@questions = []
questionnaires = questionnaires_by_answers(scores)
questionnaires.each {|questionnaire| @questions += questionnaire.questions }
return @questions
end

*Separating questions by their questionnaire type

''app/models/response.rb''

<code>construct_review_response</code> method has been updated to create separate tables for review questions, revision questions and additional comment.

''app/models/questionnaire.rb''

Add a method to check if heading is to be displayed for the questionnaire.

def display_heading?
return false
end

''app/models/revision_plan_questionnaire.rb''

Overriding the method defined in questionnaire.rb to display heading for revision plan questionnaire.

def display_heading?
return true
end

''app/views/response/response.html.erb''

View groups questions by questionnaire type and then adds a heading for questionnaires that return true for <code>display_heading?</code>.

'''Bug fix:''' The existing implementation has a bug where the first question in a questionnaire is displayed last in response. This was fixed as well.

====Summary Report Page====
When a project has been reviewed at least once, a participant is able to view their team's score. The UI below shows what this looks like after the second round of reviews. For the second and all subsequent reviews, the results of questions that were created by the instructor are shown under '''Assignment Questionnaire.''' The results of the questions created by the team are shown under '''Improvement Plan.'''

=====Initial Wireframe=====
[[File:View_Scores_Wireframe.png|400px|thumb|center|Wireframe of Summary Report for an Assignment]]
=====Final UI=====
[[File:Summary_Report.png|600px|thumb|center|Summary Report page demonstrating added Revision Plan heatgrid]]

=====Implementation=====

*Show revision plan questions in heat grid.

Heat grids for review responses are currently implemented in a view model defined in ''vm_question_response.rb''. All functionality of this view model has remained in place, and additional responses are passed to it to create the additional heat grids for revision plan responses.

''app/helpers/grades_helper.rb''

The method <code>generate_heatgrid</code> was created from the original code shared by both <code>view_heatgrid</code> and <code>view_team</code> in ''app/controllers/grades_controller'', but with the additional functionality of checking for Revision Plans in responses. When <code>assignment.vary_by_round</code>, an additional heatgrid is added for each round of Revision Planning. When <code>!assignment.vary_by_round</code>, only one heatgrid is added for the Revision Plan used in the most recent round of review.

def generate_heatgrid(participant, assignment, team, team_id, questionnaires, vmlist)
counter_for_revisions = -1
counter_for_same_rubric = 0
questionnaires.each do |questionnaire|
if assignment.vary_by_round? && questionnaire.type == "ReviewQuestionnaire"
questionnaires = AssignmentQuestionnaire.where(assignment_id: assignment.id, questionnaire_id: questionnaire.id)
if questionnaires.count > 1
@round = questionnaires[counter_for_same_rubric].used_in_round
counter_for_same_rubric += 1
else
@round = questionnaires[0].used_in_round
counter_for_same_rubric = 0
end
end
add_response_to_vmlist(participant, assignment, team, questionnaire, vmlist, @round)
if assignment.vary_by_round? && assignment.is_revision_planning_enabled?
rp_questionnaire = RevisionPlanTeamMap.find_by(team: Team.find(team_id), used_in_round: counter_for_revisions).try(:questionnaire)
if rp_questionnaire
add_response_to_vmlist(participant, assignment, team, rp_questionnaire, vmlist, @round)
end
counter_for_revisions += 1
elsif assignment.is_revision_planning_enabled? && questionnaire == questionnaires.last
if assignment.get_current_stage == "Finished"
current_round = assignment.rounds_of_reviews
else
reviewees_topic = SignedUpTeam.topic_id_by_team_id(participant.id)
current_round = assignment.number_of_current_round(reviewees_topic)
end
rp_questionnaire = RevisionPlanTeamMap.find_by(team: Team.find(team_id), used_in_round: current_round).try(:questionnaire)
if rp_questionnaire
add_response_to_vmlist(participant, assignment, team, rp_questionnaire, vmlist, @round)
end
end
end
end

The method <code>add_response_to_vmlist</code> was added to refactor the code that is used multiple times to add another review to the view model.

def add_response_to_vmlist(participant, assignment, team, questionnaire, vmlist, round)
vm = VmQuestionResponse.new(questionnaire, assignment, round)
vmquestions = questionnaire.questions
vm.add_questions(vmquestions)
vm.add_team_members(team)
vm.add_reviews(participant, team, assignment.vary_by_round)
vm.number_of_comments_greater_than_10_words
vmlist << vm
end

''app/helpers/summary_helper.rb''

'''Bug Fix:''' Reverted <code>get_sentences</code> back to version in master branch to prevent Active Record Error caused by beta version of code.

def get_sentences(ans)
unless ans.comments.nil?
ans.comments.gsub!(/[.?!]/, '\1|')
sentences = ans.comments.split('|')
sentences.map!(&:strip)
end
end

''app/models/vm_question_response.rb''

Allow <code>RevisionPlanQuestionnaires</code> to be added to the view model in the same way as <code>ReviewQuestionnaires</code>.

if (@questionnaire_type == "ReviewQuestionnaire" or @questionnaire_type == "RevisionPlanQuestionnaire")

''app/views/grades/view_team.html.erb''

Print round number headings for Revision Plan heat grids.

<% if vm.questionnaire_type == "ReviewQuestionnaire" or vm.questionnaire_type == "RevisionPlanQuestionnaire"%>

*Separated Review and Revision Plan questions in response

''app/models/response.rb''

Created separate arrays for ReviewQuestionnaire questions and RevsionPlanQuestionnaire questions. Added headings for Review Responses and Revision Plan Responses.

review_questions = []
revision_plan_questions = []
#begin table for Review Responses
...
map = ResponseMap.find(self.map_id)
unless map.is_a? ReviewResponseMap
code = add_table_rows questionnaire_max, questions, answers, code, tag_prompt_deployments, current_user
else
assignment = map.assignment
questions.each do |question|
if(question.questionnaire.type == 'ReviewQuestionnaire')
review_questions.append(question)
elsif(question.questionnaire.type == 'RevisionPlanQuestionnaire')
revision_plan_questions.append(question)
end
end
code = add_table_rows questionnaire_max, review_questions, answers, code, tag_prompt_deployments, current_user
if assignment.is_revision_planning_enabled
#end table for Review Responses and begin table for Revision Plan Responses
code = add_table_rows questionnaire_max, revision_plan_questions, answers, code, tag_prompt_deployments, current_user
end

*Refactored <code>view_team</code> and <code>view_heatgrid</code>

''app/controllers/grades_controller.rb''

The code in the <code>view_team</code> method was almost identical to the code in the <code>view_heatgrid</code> method in ''app/helpers/grades_helper.rb''. It was refactored into a new method <code>generate_heatgrid</code> in ''app/helpers/grades_helper.rb'' and used in <code>view_team</code>.

def view_team
@participant = AssignmentParticipant.find(params[:id])
@assignment = @participant.assignment
@team = @participant.team
@team_id = @team.id
questionnaires = @assignment.questionnaires
@questions = retrieve_questions questionnaires, @assignment.id
@pscore = @participant.scores(@questions)
@vmlist = []
generate_heatgrid(@participant, @assignment, @team, @team_id, questionnaires, @vmlist)
@current_role_name = current_role_name
end

''app/helpers/grades_helper.rb''

The code in the <code>view_heatgrid</code> method was almost identical to the code in the <code>view_team</code> method in ''app/controllers/grades_controller.rb''. It was refactored into a new method <code>generate_heatgrid</code> and used in <code>view_heatgrid</code>.

def view_heatgrid(participant_id, type)
# get participant, team, questionnaires for assignment.
@participant = AssignmentParticipant.find(participant_id)
@assignment = @participant.assignment
@team = @participant.team
@team_id = @team.id
@type = type
questionnaires = @assignment.questionnaires
@vmlist = []
generate_heatgrid(@participant, @assignment, @team, @team_id, questionnaires, @vmlist)
render "grades/view_heatgrid.html.erb"
end

===Control Flow Diagram===
[[File:Revision Planning Control Flow Diagram.jpeg|400px|thumb|center|Control Flow of the Revision Planning Function]]

===Test Plan - Under progress===

====RSpec Testing====
The RSpec tests are written to test both controller and models.

Controllers

*spec/controllers/response_controller_spec.rb: Updated spec to pass build by stubbing <code>get_questions</code> method.
*spec/factories/revision_plan_factory.rb: Factories can be used to create objects unique to revision plans, including: <code>RevisionPlanQuestionnaire</code> <code>RevisionPlanTeamMap</code>
*spec/controllers/revision_plan_questionnaires_controller_spec.rb: Add four examples to test revision plan questionnaires controller
describe '#action_allowed?' do

context 'when params action is edit or update' do

before(:each) do
controller.params = {id: '1', action: 'edit',team_id: 1}
controller.request.session[:user] = student
end

context 'when the role name of current user is super admin or admin' do
...
context 'when current user is the student of current questionnaires' do
...
context 'when current user is a student but not of the team' do
...
context 'when params action is not edit and update' do
...
...

Models

*spec/models/response_spec.rb: Updated spec to pass build by updating expected html output.

*spec/models/review_response_map_spec.rb: Updated spec to pass build by updating expected html output.

Features

* spec/features/assignment_creation_general_tab_spec.rb

Test if checking box tag <code>Revision Planning?</code> enables revision planning.

it "should enable revision planning" do
fill_assignment_form
check("assignment_form_assignment_is_revision_planning_enabled")
click_button 'Save'
assignment = Assignment.where(name: 'edit assignment for test').first
expect(assignment).to have_attributes(
name: 'edit assignment for test',
course_id: Course.find_by(name: 'Course 2').id,
directory_path: 'testDirectory1',
spec_location: 'testLocation1',
is_revision_planning_enabled: true,
)
end

====Manual Testing====
Manual testing aims to verify the following:
* Create an assignment with revision planning enabled.
* Are participants allowed to create/edit revision plan when round 1+ (1 or greater than 1) reviews have finished.
* Is revision plan editing disabled when assignment is in review stage.
* Are reviewers shown questions created by reviewees.
* Are participants shown summary of score for revision plan after review deadline has expired.

==Team Members==
Chaitanya Mehta (cmehta)
 
Darby Madewell (demadewe)
 
Dongni Yang (dyang23)
 
Sidharth Mehta (smehta22)
 
'''Mentor:''' Yulin Zhang (yzhan114)

==References==
# Previous Implementation: [https://expertiza.csc.ncsu.edu/index.php/E1875_Revision_Planning_Tool E1875 wiki], [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2020_E2016_Revision_planning_tool E2016 wiki]
# Forked Repository: [https://github.com/SidharthMehta/expertiza/tree/beta E2083 github]
# Pull Request: [https://github.com/expertiza/expertiza/pull/1832 E2083 pull request]
# Demo: [https://youtu.be/7pdz_A8vDkk E2083 demo video]
# Deployment: [http://152.7.98.99:8080/ temporary deployment link]

CSC/ECE 517 Fall 2020 - E2083. Revision planning tool E2016

2020-11-24T15:08:36Z

Dyang23: /* RSpec Testing */

This page provides a description of the Expertiza based OSS project.

__TOC__
==Introduction==
Rounds of peer reviews may be implemented between submissions for assignments on Expertiza. In order to better track the implementation of reviewer's suggestions, a Revision Planning Tool should be implemented.

===Problem Statement===
In the first round of Expertiza reviews, we ask reviewers to give authors some guidance on how to improve their work. Then in the second round, reviewers rate how well authors have followed their suggestions. We could carry the interaction one step further if we asked authors to make up a revision plan based on the first-round reviews. That is, authors would say what they were planning to do to improve their work. Then second-round reviewers would assess how well they did it. In essence, this means that authors would be adding criteria to the second-round rubric that applied only to their submission. We are interested in having this implemented and used in a class so that we can study its effect.

===Previous Implementations===
Revision planning has been implemented twice before, once in [https://expertiza.csc.ncsu.edu/index.php/E1875_Revision_Planning_Tool E1875] and once in [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2020_E2016_Revision_planning_tool E2016]. While the functionality worked and effectively minimized changes to the code, they also had the following problems:
*Hardcoded “round” numbers in many places of the code.
*Documentation does not reflect the new changes they made.
*Revision planning responses and responses to the other items are not distinguished in heatgrid view.
*The idea of adding a <code>team_id</code> field to each question is intuitive. However, they failed to come up with a clean implementation of this idea. Specifically, they had passed some trailing parameters several methods down before reaching the place that needs them.

===Rationale===
To implement revision planning, [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2020_E2016_Revision_planning_tool E2016] added <code>is_revision_planning_enabled</code> (Boolean) to <code>Assignment</code> and <code>team_id</code> to <code>Question</code>. Adding a Boolean to enable revision planning for an assignment makes sense, but having certain questions belong to a team is not a clean implementation. Instead, we will create a new questionnaire that will belong to each team and keep track of the set of questions. We will use this new questionnaire to separate the revision plan responses and responses to other items in heat-grid view by creating a different table for each.

===Files Modified===
This is a list of files modified and added. The list will be updated as the implementation progresses.
 
''Changed'': 
*app/controllers/advice_controller.rb
*app/controllers/grades_controller.rb
*app/controllers/questionnaires_controller.rb
*app/controllers/questions_controller.rb
*app/controllers/response_controller.rb
*app/controllers/student_task_controller.rb
*app/helpers/summary_helper.rb
*app/helpers/grades_helper.rb
*app/models/questionnaire.rb
*app/models/response.rb
*app/models/student_task.rb
*app/models/vm_question_response.rb
*app/models/on_the_fly_calc.rb
*app/views/advice/edit_advice.html.erb
*app/views/assignments/edit/_general.html.erb
*app/views/grades/view_team.html.erb
*app/views/questionnaires/_questionnaire.html.erb
*app/views/response/response.html.erb
*app/views/student_task/view.html.erb
*config/routes.rb
*db/schema.rb
*spec/controllers/response_controller_spec.rb
*spec/models/response_spec.rb
*spec/models/review_response_map_spec.rb
*spec/features/assignment_creation_general_tab_spec.rb

''Added'': 
*app/controllers/revision_plan_questionnaires_controller.rb
*app/models/revision_plan_questionnaire.rb
*app/models/revision_plan_team_map.rb
*app/views/questionnaires/_questions.html.erb
*app/views/revision_plan_questionnaires/edit.html.erb
*db/migrate/20201106020642_create_revision_plan_team_maps.rb /
*db/migrate/20201105023711_add_is_revison_planning_enabled_to_assignments.rb

==Design==
===Database Design===
Items in green are additions.
[[File:E2083_Proposed_Design.png|1000px|center]]
 
Changes:
*In the assignment table we have added <code>is_revision_planning_enabled?</code> column to indicate whether the assignment accepts a revision plan along with review rubric.
*<code>RevisionPlanTeamMap</code> maps a questionnaire to an assignment team and round. This will map to a questionnaire of type revision plan that will be created by the revewee.
*<code>RevisionPlanQuestionnaire</code> extends <code>Questionnaire</code> using single table inheritance.

===User Interface===
====Enable Revision Planning====
In order to enable '''Revision Planning''', the setting must be enabled when creating or editing an assignment under the '''General''' tab. The wireframe below demonstrates creating an assignment, and editing the assignment functions similarly. '''Revision plan?''' should be checked to enable this option.

=====Initial Wireframe=====
[[File:Enable_Revision_Planning_Wireframe.png|700px|thumb|center|Wireframe of Enabling Assignment's Revision Planning]]

=====Final UI=====
[[File:Create_New_Assignment.png|700px|thumb|center|Create New Assignment page in the UI, with added Revision Planning checkbox]]

=====Implementation=====

* Added a Boolean field <code>is_revision_planning_enabled</code> to <code>Assignment</code> that saves whether revision planning is enabled.
''db/migrate/20201105023711_add_is_revison_planning_enabled_to_assignments.rb''

class AddIsRevisonPlanningEnabledToAssignments < ActiveRecord::Migration
def change
add_column :assignments, :is_revision_planning_enabled, :boolean, default: false
end
end

* Add checkbox to enable revision planning in edit assignment view.
''app/views/assignments/edit/_general.html.erb''

<tr>
<td style='padding:5px' id='assignment_revison_planning_field'>
<input name="assignment_form[assignment][is_revision_planning_enabled]" type="hidden" value="false"/>
<%= check_box_tag('assignment_form[assignment][is_revision_planning_enabled]', 'true', @assignment_form.assignment.is_revision_planning_enabled) %>
<%= label_tag('assignment_form[assignment][is_revision_planning_enabled]', 'Revision Planning?') %>
</td>
</tr>

====Assignment Overview Page (contains the Link to the revision planning page)====
The '''Revision Planning''' link is available to students during every submission period (except the first round submission) and not available during every review period. As shown in the wireframe, by clicking '''Revision Planning''' students would be redirected to the ‘Revision planning page’.

=====Initial Wireframe=====
[[File:Link_to_the_revision_planning.jpeg|600px|thumb|center|Wireframe of Assignment Overview Page]]

=====Final UI=====
[[File:Revision_Planning_Link.png|600px|thumb|center|Submit or Review Work page demonstrating added Revision Planning link]]

=====Implementation=====
* Show link to revision planning page when revision planning is enabled for an assignment, round 1 has passed and current stage is submission.

In order to check if a team is allowed to submit a revision plan <code>@can_submit_revision_plan</code> field is added to <code>StudentTaskController</code>'s view action. If a revision plan questionnaire (<code>@revision_plan_questionnaire_id</code>) exists for the current round then clicking the link performs <code>RevisionPlanQuestionnairesController</code>'s edit action else new action is performed.

''app/views/student_task/view.html.erb''

<% if @team && @assignment.is_revision_planning_enabled %>
<li>
<% if @can_submit_revision_plan %>
<% if @revision_plan_questionnaire_id %>
<%= link_to "Revision Planning", controller: 'revision_plan_questionnaires', action: 'edit', id: @revision_plan_questionnaire_id %>
<% else %>
<%= link_to "Revision Planning", controller: 'revision_plan_questionnaires', action: 'new', team_id: @team %>
<% end %>
(Add new questions to next-round rubric)
<% else %>
Revision Planning (You are not allowed to submit your revison plan right now)
<% end %>
</li>
<%end%>

''app/controllers/student_task_controller.rb''

def view
student_task = StudentTask.from_participant_id params[:id]
...
...
# Revision plan feature
@can_submit_revision_plan = student_task.can_submit_revision_plan?
@revision_plan_questionnaire_id = student_task.revision_plan_questionnaire_id
end

''app/models/student_task.rb''

def current_round
assignment.number_of_current_round(@topic.try(:id))
end
def can_submit_revision_plan?
current_stage == 'submission' && current_round > 1
end
def revision_plan_questionnaire_id
RevisionPlanTeamMap.find_by(team: @participant.team, used_in_round: current_round).try(:questionnaire_id)
end

====Editing the Revision Plan Questionnaire====
After creating the Revision Plan Questionnaire, it must be edited. Questions can be added by specifying the amount of questions and their type. Questions can be removed by clicking '''Remove''' in the leftmost column. Once the questionnaire is complete, it can be saved. This page will be visible during each submission period after the first and will be unavailable during all review periods.

=====Initial Wireframe=====
[[File:Edit_Revision_Plan_Wireframe.png|500px|thumb|center|Wireframe of Editing an Assignment's Revision Plan]]

=====Final UI=====
[[File:Edit_Revision_Plan.png|700px|thumb|center|Edit Revision Plan page demonstrating added view]]

=====Implementation=====
* Create a model RevisionPlanTeamMap that maps a revision plan to a team and stores information of the round it is used in.

''/app/models/revision_plan_team_map.rb''

class RevisionPlanTeamMap < ActiveRecord::Base
belongs_to :team
belongs_to :questionnaire
end

''/db/migrate/20201106020642_create_revision_plan_team_maps.rb''

class CreateRevisionPlanTeamMaps < ActiveRecord::Migration
def change
create_table :revision_plan_team_maps do |t|
t.references :team, index: true, foreign_key: true
t.references :questionnaire, index: true, foreign_key: true
t.integer :used_in_round
t.timestamps null: false
end
end
end

* Create a Revision Plan Questionnaire Model and Controller

[https://github.com/SidharthMehta/expertiza/blob/1b54cf6c7aa2b1195c5259a4ee616f21fc2f37a3/app/models/revision_plan_questionnaire.rb ''app/models/revision_plan_questionnaire.rb'']

Added a model for revision plan questionnaire. The code provides method to get questionnaire for a current round based on team.

def self.get_questionnaire_for_current_round(team_id)
assignment_team = Team.find(team_id)
assignment = assignment_team.assignment
current_round = assignment.number_of_current_round(assignment_team.topic)
questionnaire = RevisionPlanTeamMap.find_by(team: assignment_team, used_in_round: current_round).try(:questionnaire)
unless questionnaire
questionnaire = RevisionPlanQuestionnaire.new
questionnaire.name = 'Revision Plan Questionnaire'
questionnaire.instructor_id = assignment.instructor_id
questionnaire.max_question_score = 5
questionnaire.save
questionnaire_team_map = RevisionPlanTeamMap.create(team_id: assignment_team.id, used_in_round: current_round, questionnaire_id: questionnaire.id)
end
return questionnaire
end

This method returns the team associated to a revision plan questionnaire.

def team
revision_plan_team_map.team
end

''app/models/questionnaire.rb''

Added a new type of questionnaire <code>'RevisionPlanQuestionnaire'</code> to <code>QUESTIONNAIRE_TYPES</code>

QUESTIONNAIRE_TYPES = ['ReviewQuestionnaire',
...
...
'RevisionPlanQuestionnaire'].freeze

[https://github.com/SidharthMehta/expertiza/blob/1b54cf6c7aa2b1195c5259a4ee616f21fc2f37a3/app/controllers/revision_plan_questionnaires_controller.rb ''app/controllers/revision_plan_questionnaires_controller.rb'']

Added a controller <code>RevisionPlanQuestionnairesController</code>. The controller derives from <code>QuestionnairesController</code>, it overrides the <code>new</code> action to allow creation of a <code>RevisionPlanQuestionnaire</code> for a <code>team</code>. It also updates permissions for controller.

def action_allowed?
case params[:action]
when 'edit'
questionnaire = Questionnaire.find(params[:id])
questionnaire.team.users.collect { |u| u.id }.include? session[:user].id || super
(user_logged_in? && questionnaire.team.users.collect { |u| u.id }.include?(session[:user].id)) || super
else
super
end
end

def new
begin
questionnaire = RevisionPlanQuestionnaire.get_questionnaire_for_current_round(params[:team_id])
redirect_to action: 'edit', id: questionnaire.id
rescue StandardError
flash[:error] = $ERROR_INFO
end
end

''config/routes.rb''

Add routes for <code>RevisionPlanQuestionnairesController</code>.

resources :revision_plan_questionnaires, controller: :revision_plan_questionnaires, only: %i[new edit update] do
collection do
post :add_new_questions
post :save_all_questions
end
end

* Add a view to edit Revision Plan

[https://github.com/expertiza/expertiza/blob/d97eb1c1fc8d578b18f51401dc713b0d04815fe4/app/views/revision_plan_questionnaires/edit.html.erb ''app/views/revision_plan_questionnaires/edit.html.erb'']

<%= render :partial => 'question_weight' %>
<h1>Edit <%= @questionnaire.display_type %></h1>
<%= render :partial => 'questions' %>
 
<a href="javascript:window.history.back()">Back</a>

* Refactor code to display questions into a partial. Allows for code reuse.

''app/controllers/questionnaires_controller.rb''

Updated <code>redirect_to</code> links for <code>add_new_questions</code> and <code>save_all_questions</code> methods.

redirect_to action: 'edit', id: questionnaire_id.to_sym

''app/views/questionnaires/_questionnaire.html.erb''

This file generates a view to edit/create a new questionnaire. It has functionality to display questions and some functions that are not needed for revision plan questionnaire. To enable code reuse the functionality to add/edit/remove/save questions was moved to a new partial ''app/views/questionnaires/_questions.html.erb''.

[https://github.com/expertiza/expertiza/blob/d97eb1c1fc8d578b18f51401dc713b0d04815fe4/app/views/questionnaires/_questions.html.erb ''app/views/questionnaires/_questions.html.erb'']

*Add code to allow users to delete questions from revision plan questionnaire.

''app/controllers/questions_controller.rb ''

Update authorization code to allow a user to remove questions from his teams revision plan.

def action_allowed?
if ['destroy'].include?(params[:action])
question = Question.find(params[:id])
if(user_logged_in? && question.questionnaire.owner?(session[:user].id))
return true
end
end
current_user_has_ta_privileges?
end

def destroy
...
...
if(question.questionnaire.type == 'RevisionPlanQuestionnaire')
redirect_to edit_revision_plan_questionnaire_path(questionnaire_id.to_s.to_sym)
else
redirect_to edit_questionnaire_path(questionnaire_id.to_s.to_sym)
end
end

''app/models/questionnaire.rb''

Method returns true if user owns questionnaire.

def owner?(user_id)
instructor_id == user_id
end

''app/models/revision_plan_questionnaire.rb''

Overrides implementation for <code>owner?</code> defined in <code>Questionnaire</code> to return true if a team owns the questionnaire.

def owner?(user_id)
team.users.map{ |u| u.id }.include?(user_id) || super
end

*Add code to allow users to access show/edit advice functionality for revision plan questions.

''app/controllers/advice_controller.rb''

Update authorization code to allow a user to update advice his teams revision plan.

def action_allowed?
questionnaire = Questionnaire.find(params[:id])
if(user_logged_in? && questionnaire.owner?(session[:user].id))
return true
end
current_user_has_ta_privileges?
end

''app/views/advice/edit_advice.html.erb''

Update 'Back to questionnaire' link.

<% if @questionnaire.type == 'RevisionPlanQuestionnaire'%>
<%= link_to 'Back to questionnaire', :controller => 'revision_plan_questionnaires', :action => 'edit', :id => @questionnaire.id %>
<% else %>
<%= link_to 'Back to questionnaire', :controller => 'questionnaires', :action => 'edit', :id => @questionnaire.id %>
<% end %>

====Reviewing an Assignment====
Once the Revision Plan Questionnaire and its resubmission round have been finished, the Revision Plan Questionnaire is appended to the Assignment Questionnaire for that round.

=====Initial Wireframe=====
[[File:Review_Wireframe.png|400px|thumb|center|Wireframe of Completing a Review for an Assignment]]

=====Final UI=====
[[File:Revision_Plan_Questionnaire.png|400px|thumb|center|Review page demonstrating added Revision Plan questions]]

=====Implementation=====

*Add Revision Plan questions to Response

''app/controllers/response_controller.rb''

This methods gets questions sorted by sequence from review and revision questionnaire and merges them in a single list.

def set_questions_for_new_response
@questions = sort_questions(@questionnaire.questions)
if(@assignment && @assignment.is_revision_planning_enabled)
reviewees_topic = SignedUpTeam.topic_id_by_team_id(@contributor.id)
current_round = @assignment.number_of_current_round(reviewees_topic)
@revision_plan_questionnaire = RevisionPlanTeamMap.find_by(team_id: @map.reviewee_id, used_in_round: current_round).try(:questionnaire)
if(@revision_plan_questionnaire)
@questions += sort_questions(@revision_plan_questionnaire.questions)
end
end
return @questions
end

This method gets questions from a response instead of questionnaire. We use this when response has already been created.

def set_questions
@questions = @response.get_questions
end

''app/models/response.rb''

Added a method to get all the questionnaires for a response, response is a collection of answers. Answers contain reference to their question, and question have a reference to questionnaire.

def questionnaires_by_answers(answers)
answers_with_questionnaires = answers.select{ |ans| ans && ans.question && ans.question.questionnaire }
questionnaires = answers_with_questionnaires.collect{ |ans| ans.question.questionnaire }.uniq
unless(questionnaires.any?)
questionnaires = []
questionnaires << questionnaire_by_answer(answers.first)
end
questionnaires
end

This method gets all the questions for a response.

def get_questions
@questions = []
questionnaires = questionnaires_by_answers(scores)
questionnaires.each {|questionnaire| @questions += questionnaire.questions }
return @questions
end

*Separating questions by their questionnaire type

''app/models/response.rb''

<code>construct_review_response</code> method has been updated to create separate tables for review questions, revision questions and additional comment.

''app/models/questionnaire.rb''

Add a method to check if heading is to be displayed for the questionnaire.

def display_heading?
return false
end

''app/models/revision_plan_questionnaire.rb''

Overriding the method defined in questionnaire.rb to display heading for revision plan questionnaire.

def display_heading?
return true
end

''app/views/response/response.html.erb''

View groups questions by questionnaire type and then adds a heading for questionnaires that return true for <code>display_heading?</code>.

'''Bug fix:''' The existing implementation has a bug where the first question in a questionnaire is displayed last in response. This was fixed as well.

====Summary Report Page====
When a project has been reviewed at least once, a participant is able to view their team's score. The UI below shows what this looks like after the second round of reviews. For the second and all subsequent reviews, the results of questions that were created by the instructor are shown under '''Assignment Questionnaire.''' The results of the questions created by the team are shown under '''Improvement Plan.'''

=====Initial Wireframe=====
[[File:View_Scores_Wireframe.png|400px|thumb|center|Wireframe of Summary Report for an Assignment]]
=====Final UI=====
[[File:Summary_Report.png|600px|thumb|center|Summary Report page demonstrating added Revision Plan heatgrid]]

=====Implementation=====

*Show revision plan questions in heat grid.

Heat grids for review responses are currently implemented in a view model defined in ''vm_question_response.rb''. All functionality of this view model has remained in place, and additional responses are passed to it to create the additional heat grids for revision plan responses.

''app/helpers/grades_helper.rb''

The method <code>generate_heatgrid</code> was created from the original code shared by both <code>view_heatgrid</code> and <code>view_team</code> in ''app/controllers/grades_controller'', but with the additional functionality of checking for Revision Plans in responses. When <code>assignment.vary_by_round</code>, an additional heatgrid is added for each round of Revision Planning. When <code>!assignment.vary_by_round</code>, only one heatgrid is added for the Revision Plan used in the most recent round of review.

def generate_heatgrid(participant, assignment, team, team_id, questionnaires, vmlist)
counter_for_revisions = -1
counter_for_same_rubric = 0
questionnaires.each do |questionnaire|
if assignment.vary_by_round? && questionnaire.type == "ReviewQuestionnaire"
questionnaires = AssignmentQuestionnaire.where(assignment_id: assignment.id, questionnaire_id: questionnaire.id)
if questionnaires.count > 1
@round = questionnaires[counter_for_same_rubric].used_in_round
counter_for_same_rubric += 1
else
@round = questionnaires[0].used_in_round
counter_for_same_rubric = 0
end
end
add_response_to_vmlist(participant, assignment, team, questionnaire, vmlist, @round)
if assignment.vary_by_round? && assignment.is_revision_planning_enabled?
rp_questionnaire = RevisionPlanTeamMap.find_by(team: Team.find(team_id), used_in_round: counter_for_revisions).try(:questionnaire)
if rp_questionnaire
add_response_to_vmlist(participant, assignment, team, rp_questionnaire, vmlist, @round)
end
counter_for_revisions += 1
elsif assignment.is_revision_planning_enabled? && questionnaire == questionnaires.last
if assignment.get_current_stage == "Finished"
current_round = assignment.rounds_of_reviews
else
reviewees_topic = SignedUpTeam.topic_id_by_team_id(participant.id)
current_round = assignment.number_of_current_round(reviewees_topic)
end
rp_questionnaire = RevisionPlanTeamMap.find_by(team: Team.find(team_id), used_in_round: current_round).try(:questionnaire)
if rp_questionnaire
add_response_to_vmlist(participant, assignment, team, rp_questionnaire, vmlist, @round)
end
end
end
end

The method <code>add_response_to_vmlist</code> was added to refactor the code that is used multiple times to add another review to the view model.

def add_response_to_vmlist(participant, assignment, team, questionnaire, vmlist, round)
vm = VmQuestionResponse.new(questionnaire, assignment, round)
vmquestions = questionnaire.questions
vm.add_questions(vmquestions)
vm.add_team_members(team)
vm.add_reviews(participant, team, assignment.vary_by_round)
vm.number_of_comments_greater_than_10_words
vmlist << vm
end

''app/helpers/summary_helper.rb''

'''Bug Fix:''' Reverted <code>get_sentences</code> back to version in master branch to prevent Active Record Error caused by beta version of code.

def get_sentences(ans)
unless ans.comments.nil?
ans.comments.gsub!(/[.?!]/, '\1|')
sentences = ans.comments.split('|')
sentences.map!(&:strip)
end
end

''app/models/vm_question_response.rb''

Allow <code>RevisionPlanQuestionnaires</code> to be added to the view model in the same way as <code>ReviewQuestionnaires</code>.

if (@questionnaire_type == "ReviewQuestionnaire" or @questionnaire_type == "RevisionPlanQuestionnaire")

''app/views/grades/view_team.html.erb''

Print round number headings for Revision Plan heat grids.

<% if vm.questionnaire_type == "ReviewQuestionnaire" or vm.questionnaire_type == "RevisionPlanQuestionnaire"%>

*Separated Review and Revision Plan questions in response

''app/models/response.rb''

Created separate arrays for ReviewQuestionnaire questions and RevsionPlanQuestionnaire questions. Added headings for Review Responses and Revision Plan Responses.

review_questions = []
revision_plan_questions = []
#begin table for Review Responses
...
map = ResponseMap.find(self.map_id)
unless map.is_a? ReviewResponseMap
code = add_table_rows questionnaire_max, questions, answers, code, tag_prompt_deployments, current_user
else
assignment = map.assignment
questions.each do |question|
if(question.questionnaire.type == 'ReviewQuestionnaire')
review_questions.append(question)
elsif(question.questionnaire.type == 'RevisionPlanQuestionnaire')
revision_plan_questions.append(question)
end
end
code = add_table_rows questionnaire_max, review_questions, answers, code, tag_prompt_deployments, current_user
if assignment.is_revision_planning_enabled
#end table for Review Responses and begin table for Revision Plan Responses
code = add_table_rows questionnaire_max, revision_plan_questions, answers, code, tag_prompt_deployments, current_user
end

*Refactored <code>view_team</code> and <code>view_heatgrid</code>

''app/controllers/grades_controller.rb''

The code in the <code>view_team</code> method was almost identical to the code in the <code>view_heatgrid</code> method in ''app/helpers/grades_helper.rb''. It was refactored into a new method <code>generate_heatgrid</code> in ''app/helpers/grades_helper.rb'' and used in <code>view_team</code>.

def view_team
@participant = AssignmentParticipant.find(params[:id])
@assignment = @participant.assignment
@team = @participant.team
@team_id = @team.id
questionnaires = @assignment.questionnaires
@questions = retrieve_questions questionnaires, @assignment.id
@pscore = @participant.scores(@questions)
@vmlist = []
generate_heatgrid(@participant, @assignment, @team, @team_id, questionnaires, @vmlist)
@current_role_name = current_role_name
end

''app/helpers/grades_helper.rb''

The code in the <code>view_heatgrid</code> method was almost identical to the code in the <code>view_team</code> method in ''app/controllers/grades_controller.rb''. It was refactored into a new method <code>generate_heatgrid</code> and used in <code>view_heatgrid</code>.

def view_heatgrid(participant_id, type)
# get participant, team, questionnaires for assignment.
@participant = AssignmentParticipant.find(participant_id)
@assignment = @participant.assignment
@team = @participant.team
@team_id = @team.id
@type = type
questionnaires = @assignment.questionnaires
@vmlist = []
generate_heatgrid(@participant, @assignment, @team, @team_id, questionnaires, @vmlist)
render "grades/view_heatgrid.html.erb"
end

===Control Flow Diagram===
[[File:Revision Planning Control Flow Diagram.jpeg|400px|thumb|center|Control Flow of the Revision Planning Function]]

===Test Plan - Under progress===

====RSpec Testing====
The RSpec tests are written to test both controller and models.

Controllers

*spec/controllers/response_controller_spec.rb: Updated spec to pass build by stubbing <code>get_questions</code> method.
*spec/factories/revision_plan_factory.rb: Factories can be used to create objects unique to revision plans, including: <code>RevisionPlanQuestionnaire</code> <code>RevisionPlanTeamMap</code>
*spec/controllers/revision_plan_questionnaires_controller_spec.rb: Add four examples to test revision plan questionnaires controller
describe '#action_allowed?' do

context 'when params action is edit or update' do

before(:each) do
controller.params = {id: '1', action: 'edit',team_id: 1}
controller.request.session[:user] = student
end

context 'when the role name of current user is super admin or admin' do
...
context 'when current user is the student of current questionnaires' do
...
context 'when current user is a student but not of the team' do
...
context 'when params action is not edit and update' do
...
...

Models

*spec/models/response_spec.rb: Updated spec to pass build by updating expected html output.

*spec/models/review_response_map_spec.rb: Updated spec to pass build by updating expected html output.

Features

* spec/features/assignment_creation_general_tab_spec.rb

Test if checking box tag <code>Revision Planning?</code> enables revision planning.

it "should enable revision planning" do
fill_assignment_form
check("assignment_form_assignment_is_revision_planning_enabled")
click_button 'Save'
assignment = Assignment.where(name: 'edit assignment for test').first
expect(assignment).to have_attributes(
name: 'edit assignment for test',
course_id: Course.find_by(name: 'Course 2').id,
directory_path: 'testDirectory1',
spec_location: 'testLocation1',
is_revision_planning_enabled: true,
)
end

====Manual Testing====
Manual testing aims to verify the following:
* Create an assignment with revision planning enabled.
* Are participants allowed to create/edit revision plan when round 1+ (1 or greater than 1) reviews have finished.
* Is revision plan editing disabled when assignment is in review stage.
* Are reviewers shown questions created by reviewees.
* Are participants shown summary of score for revision plan after review deadline has expired.

==Team Members==
Chaitanya Mehta (cmehta)
 
Darby Madewell (demadewe)
 
Dongni Yang (dyang23)
 
Sidharth Mehta (smehta22)
 
'''Mentor:''' Yulin Zhang (yzhan114)

==References==
# Previous Implementation: [https://expertiza.csc.ncsu.edu/index.php/E1875_Revision_Planning_Tool E1875 wiki], [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2020_E2016_Revision_planning_tool E2016 wiki]
# Forked Repository: [https://github.com/SidharthMehta/expertiza/tree/beta E2083 github]
# Pull Request: [https://github.com/expertiza/expertiza/pull/1832 E2083 pull request]
# Demo: [https://youtu.be/7pdz_A8vDkk E2083 demo video]
# Deployment: [http://152.7.98.99:8080/ temporary deployment link]

CSC/ECE 517 Fall 2020 - E2083. Revision planning tool E2016

2020-11-18T19:57:17Z

Dyang23: /* RSpec Testing */

This page provides a description of the Expertiza based OSS project.

__TOC__
==Introduction==
Rounds of peer reviews may be implemented between submissions for assignments on Expertiza. In order to better track the implementation of reviewer's suggestions, a Revision Planning Tool should be implemented.

===Problem Statement===
In the first round of Expertiza reviews, we ask reviewers to give authors some guidance on how to improve their work. Then in the second round, reviewers rate how well authors have followed their suggestions. We could carry the interaction one step further if we asked authors to make up a revision plan based on the first-round reviews. That is, authors would say what they were planning to do to improve their work. Then second-round reviewers would assess how well they did it. In essence, this means that authors would be adding criteria to the second-round rubric that applied only to their submission. We are interested in having this implemented and used in a class so that we can study its effect.

===Previous Implementations===
Revision planning has been implemented twice before, once in [https://expertiza.csc.ncsu.edu/index.php/E1875_Revision_Planning_Tool E1875] and once in [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2020_E2016_Revision_planning_tool E2016]. While the functionality worked and effectively minimized changes to the code, they also had the following problems:
*Hardcoded “round” numbers in many places of the code.
*Documentation does not reflect the new changes they made.
*Revision planning responses and responses to the other items are not distinguished in heatgrid view.
*The idea of adding a <code>team_id</code> field to each question is intuitive. However, they failed to come up with a clean implementation of this idea. Specifically, they had passed some trailing parameters several methods down before reaching the place that needs them.

===Rationale===
To implement revision planning, [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2020_E2016_Revision_planning_tool E2016] added <code>is_revision_planning_enabled</code> (Boolean) to <code>Assignment</code> and <code>team_id</code> to <code>Question</code>. Adding a Boolean to enable revision planning for an assignment makes sense, but having certain questions belong to a team is not a clean implementation. Instead, we will create a new questionnaire that will belong to each team and keep track of the set of questions. We will use this new questionnaire to separate the revision plan responses and responses to other items in heat-grid view by creating a different table for each.

===Files Modified - Under progress===
This is a list of files modified and added. The list will be updated as the implementation progresses.
 
''Changed'': 
*app/controllers/advice_controller.rb
*app/controllers/grades_controller.rb
*app/controllers/questionnaires_controller.rb
*app/controllers/questions_controller.rb
*app/controllers/response_controller.rb
*app/controllers/student_task_controller.rb
*app/helpers/summary_helper.rb
*app/helpers/grades_helper.rb
*app/models/questionnaire.rb
*app/models/response.rb
*app/models/student_task.rb
*app/models/vm_question_response.rb
*app/models/on_the_fly_calc.rb
*app/views/advice/edit_advice.html.erb
*app/views/assignments/edit/_general.html.erb
*app/views/grades/view_team.html.erb
*app/views/questionnaires/_questionnaire.html.erb
*app/views/response/response.html.erb
*app/views/student_task/view.html.erb
*config/routes.rb
*db/schema.rb
*spec/controllers/response_controller_spec.rb
*spec/models/response_spec.rb
*spec/models/review_response_map_spec.rb

''Added'': 
*app/controllers/revision_plan_questionnaires_controller.rb
*app/models/revision_plan_questionnaire.rb
*app/models/revision_plan_team_map.rb
*app/views/questionnaires/_questions.html.erb
*app/views/revision_plan_questionnaires/edit.html.erb
*db/migrate/20201106020642_create_revision_plan_team_maps.rb /
*db/migrate/20201105023711_add_is_revison_planning_enabled_to_assignments.rb

==Design==
===Database Design===
Items in green are additions.
[[File:E2083_Proposed_Design.png|1000px|center]]
 
Changes:
*In the assignment table we have added <code>is_revision_planning_enabled?</code> column to indicate whether the assignment accepts a revision plan along with review rubric.
*<code>RevisionPlanTeamMap</code> maps a questionnaire to an assignment team and round. This will map to a questionnaire of type revision plan that will be created by the revewee.
*<code>RevisionPlanQuestionnaire</code> extends <code>Questionnaire</code> using single table inheritance.

===User Interface===
====Enable Revision Planning====
In order to enable '''Revision Planning''', the setting must be enabled when creating or editing an assignment under the '''General''' tab. The wireframe below demonstrates creating an assignment, and editing the assignment functions similarly. '''Revision plan?''' should be checked to enable this option.

=====Initial Wireframe=====
[[File:Enable_Revision_Planning_Wireframe.png|700px|thumb|center|Wireframe of Enabling Assignment's Revision Planning]]

=====Final UI=====
[[File:Create_New_Assignment.png|700px|thumb|center|Create New Assignment page in the UI, with added Revision Planning checkbox]]

=====Implementation=====

* Added a Boolean field <code>is_revision_planning_enabled</code> to <code>Assignment</code> that saves whether revision planning is enabled.
''db/migrate/20201105023711_add_is_revison_planning_enabled_to_assignments.rb''

class AddIsRevisonPlanningEnabledToAssignments < ActiveRecord::Migration
def change
add_column :assignments, :is_revision_planning_enabled, :boolean, default: false
end
end

* Add checkbox to enable revision planning in edit assignment view.
''app/views/assignments/edit/_general.html.erb''

<tr>
<td style='padding:5px' id='assignment_revison_planning_field'>
<input name="assignment_form[assignment][is_revision_planning_enabled]" type="hidden" value="false"/>
<%= check_box_tag('assignment_form[assignment][is_revision_planning_enabled]', 'true', @assignment_form.assignment.is_revision_planning_enabled) %>
<%= label_tag('assignment_form[assignment][is_revision_planning_enabled]', 'Revision Planning?') %>
</td>
</tr>

====Assignment Overview Page (contains the Link to the revision planning page)====
The '''Revision Planning''' link is available to students during every submission period (except the first round submission) and not available during every review period. As shown in the wireframe, by clicking '''Revision Planning''' students would be redirected to the ‘Revision planning page’.

=====Initial Wireframe=====
[[File:Link_to_the_revision_planning.jpeg|600px|thumb|center|Wireframe of Assignment Overview Page]]

=====Final UI=====
[[File:Revision_Planning_Link.png|600px|thumb|center|Submit or Review Work page demonstrating added Revision Planning link]]

=====Implementation=====
* Show link to revision planning page when revision planning is enabled for an assignment, round 1 has passed and current stage is submission.

In order to check if a team is allowed to submit a revision plan <code>@can_submit_revision_plan</code> field is added to <code>StudentTaskController</code>'s view action. If a revision plan questionnaire (<code>@revision_plan_questionnaire_id</code>) exists for the current round then clicking the link performs <code>RevisionPlanQuestionnairesController</code>'s edit action else new action is performed.

''app/views/student_task/view.html.erb''

<% if @team && @assignment.is_revision_planning_enabled %>
<li>
<% if @can_submit_revision_plan %>
<% if @revision_plan_questionnaire_id %>
<%= link_to "Revision Planning", controller: 'revision_plan_questionnaires', action: 'edit', id: @revision_plan_questionnaire_id %>
<% else %>
<%= link_to "Revision Planning", controller: 'revision_plan_questionnaires', action: 'new', team_id: @team %>
<% end %>
(Add new questions to next-round rubric)
<% else %>
Revision Planning (You are not allowed to submit your revison plan right now)
<% end %>
</li>
<%end%>

''app/controllers/student_task_controller.rb''

def view
student_task = StudentTask.from_participant_id params[:id]
...
...
# Revision plan feature
@can_submit_revision_plan = student_task.can_submit_revision_plan?
@revision_plan_questionnaire_id = student_task.revision_plan_questionnaire_id
end

''app/models/student_task.rb''

def current_round
assignment.number_of_current_round(@topic.try(:id))
end
def can_submit_revision_plan?
current_stage == 'submission' && current_round > 1
end
def revision_plan_questionnaire_id
RevisionPlanTeamMap.find_by(team: @participant.team, used_in_round: current_round).try(:questionnaire_id)
end

====Editing the Revision Plan Questionnaire====
After creating the Revision Plan Questionnaire, it must be edited. Questions can be added by specifying the amount of questions and their type. Questions can be removed by clicking '''Remove''' in the leftmost column. Once the questionnaire is complete, it can be saved. This page will be visible during each submission period after the first and will be unavailable during all review periods.

=====Initial Wireframe=====
[[File:Edit_Revision_Plan_Wireframe.png|500px|thumb|center|Wireframe of Editing an Assignment's Revision Plan]]

=====Final UI=====
[[File:Edit_Revision_Plan.png|700px|thumb|center|Edit Revision Plan page demonstrating added view]]

=====Implementation=====
* Create a model RevisionPlanTeamMap that maps a revision plan to a team and stores information of the round it is used in.

''/app/models/revision_plan_team_map.rb''

class RevisionPlanTeamMap < ActiveRecord::Base
belongs_to :team
belongs_to :questionnaire
end

''/db/migrate/20201106020642_create_revision_plan_team_maps.rb''

class CreateRevisionPlanTeamMaps < ActiveRecord::Migration
def change
create_table :revision_plan_team_maps do |t|
t.references :team, index: true, foreign_key: true
t.references :questionnaire, index: true, foreign_key: true
t.integer :used_in_round
t.timestamps null: false
end
end
end

* Create a Revision Plan Questionnaire Model and Controller

[https://github.com/SidharthMehta/expertiza/blob/1b54cf6c7aa2b1195c5259a4ee616f21fc2f37a3/app/models/revision_plan_questionnaire.rb ''app/models/revision_plan_questionnaire.rb'']

Added a model for revision plan questionnaire. The code provides method to get questionnaire for a current round based on team.

def self.get_questionnaire_for_current_round(team_id)
assignment_team = Team.find(team_id)
assignment = assignment_team.assignment
current_round = assignment.number_of_current_round(assignment_team.topic)
questionnaire = RevisionPlanTeamMap.find_by(team: assignment_team, used_in_round: current_round).try(:questionnaire)
unless questionnaire
questionnaire = RevisionPlanQuestionnaire.new
questionnaire.name = 'Revision Plan Questionnaire'
questionnaire.instructor_id = assignment.instructor_id
questionnaire.max_question_score = 5
questionnaire.save
questionnaire_team_map = RevisionPlanTeamMap.create(team_id: assignment_team.id, used_in_round: current_round, questionnaire_id: questionnaire.id)
end
return questionnaire
end

This method returns the team associated to a revision plan questionnaire.

def team
revision_plan_team_map.team
end

''app/models/questionnaire.rb''

Added a new type of questionnaire <code>'RevisionPlanQuestionnaire'</code> to <code>QUESTIONNAIRE_TYPES</code>

QUESTIONNAIRE_TYPES = ['ReviewQuestionnaire',
...
...
'RevisionPlanQuestionnaire'].freeze

[https://github.com/SidharthMehta/expertiza/blob/1b54cf6c7aa2b1195c5259a4ee616f21fc2f37a3/app/controllers/revision_plan_questionnaires_controller.rb ''app/controllers/revision_plan_questionnaires_controller.rb'']

Added a controller <code>RevisionPlanQuestionnairesController</code>. The controller derives from <code>QuestionnairesController</code>, it overrides the <code>new</code> action to allow creation of a <code>RevisionPlanQuestionnaire</code> for a <code>team</code>. It also updates permissions for controller.

def action_allowed?
case params[:action]
when 'edit'
questionnaire = Questionnaire.find(params[:id])
questionnaire.team.users.collect { |u| u.id }.include? session[:user].id || super
(user_logged_in? && questionnaire.team.users.collect { |u| u.id }.include?(session[:user].id)) || super
else
super
end
end

def new
begin
questionnaire = RevisionPlanQuestionnaire.get_questionnaire_for_current_round(params[:team_id])
redirect_to action: 'edit', id: questionnaire.id
rescue StandardError
flash[:error] = $ERROR_INFO
end
end

''config/routes.rb''

Add routes for <code>RevisionPlanQuestionnairesController</code>.

resources :revision_plan_questionnaires, controller: :revision_plan_questionnaires, only: %i[new edit update] do
collection do
post :add_new_questions
post :save_all_questions
end
end

* Add a view to edit Revision Plan

[https://github.com/expertiza/expertiza/blob/d97eb1c1fc8d578b18f51401dc713b0d04815fe4/app/views/revision_plan_questionnaires/edit.html.erb ''app/views/revision_plan_questionnaires/edit.html.erb'']

<%= render :partial => 'question_weight' %>
<h1>Edit <%= @questionnaire.display_type %></h1>
<%= render :partial => 'questions' %>
 
<a href="javascript:window.history.back()">Back</a>

* Refactor code to display questions into a partial. Allows for code reuse.

''app/controllers/questionnaires_controller.rb''

Updated <code>redirect_to</code> links for <code>add_new_questions</code> and <code>save_all_questions</code> methods.

redirect_to action: 'edit', id: questionnaire_id.to_sym

''app/views/questionnaires/_questionnaire.html.erb''

This file generates a view to edit/create a new questionnaire. It has functionality to display questions and some functions that are not needed for revision plan questionnaire. To enable code reuse the functionality to add/edit/remove/save questions was moved to a new partial ''app/views/questionnaires/_questions.html.erb''.

[https://github.com/expertiza/expertiza/blob/d97eb1c1fc8d578b18f51401dc713b0d04815fe4/app/views/questionnaires/_questions.html.erb ''app/views/questionnaires/_questions.html.erb'']

*Add code to allow users to delete questions from revision plan questionnaire.

''app/controllers/questions_controller.rb ''

Update authorization code to allow a user to remove questions from his teams revision plan.

def action_allowed?
if ['destroy'].include?(params[:action])
question = Question.find(params[:id])
if(user_logged_in? && question.questionnaire.owner?(session[:user].id))
return true
end
end
current_user_has_ta_privileges?
end

def destroy
...
...
if(question.questionnaire.type == 'RevisionPlanQuestionnaire')
redirect_to edit_revision_plan_questionnaire_path(questionnaire_id.to_s.to_sym)
else
redirect_to edit_questionnaire_path(questionnaire_id.to_s.to_sym)
end
end

''app/models/questionnaire.rb''

Method returns true if user owns questionnaire.

def owner?(user_id)
instructor_id == user_id
end

''app/models/revision_plan_questionnaire.rb''

Overrides implementation for <code>owner?</code> defined in <code>Questionnaire</code> to return true if a team owns the questionnaire.

def owner?(user_id)
team.users.map{ |u| u.id }.include?(user_id) || super
end

*Add code to allow users to access show/edit advice functionality for revision plan questions.

''app/controllers/advice_controller.rb''

Update authorization code to allow a user to update advice his teams revision plan.

def action_allowed?
questionnaire = Questionnaire.find(params[:id])
if(user_logged_in? && questionnaire.owner?(session[:user].id))
return true
end
current_user_has_ta_privileges?
end

''app/views/advice/edit_advice.html.erb''

Update 'Back to questionnaire' link.

<% if @questionnaire.type == 'RevisionPlanQuestionnaire'%>
<%= link_to 'Back to questionnaire', :controller => 'revision_plan_questionnaires', :action => 'edit', :id => @questionnaire.id %>
<% else %>
<%= link_to 'Back to questionnaire', :controller => 'questionnaires', :action => 'edit', :id => @questionnaire.id %>
<% end %>

====Reviewing an Assignment====
Once the Revision Plan Questionnaire and its resubmission round have been finished, the Revision Plan Questionnaire is appended to the Assignment Questionnaire for that round.

=====Initial Wireframe=====
[[File:Review_Wireframe.png|400px|thumb|center|Wireframe of Completing a Review for an Assignment]]

=====Final UI=====
[[File:Revision_Plan_Questionnaire.png|400px|thumb|center|Review page demonstrating added Revision Plan questions]]

=====Implementation=====

*Add Revision Plan questions to Response

''app/controllers/response_controller.rb''

This methods gets questions sorted by sequence from review and revision questionnaire and merges them in a single list.

def set_questions_for_new_response
@questions = sort_questions(@questionnaire.questions)
if(@assignment && @assignment.is_revision_planning_enabled)
reviewees_topic = SignedUpTeam.topic_id_by_team_id(@contributor.id)
current_round = @assignment.number_of_current_round(reviewees_topic)
@revision_plan_questionnaire = RevisionPlanTeamMap.find_by(team_id: @map.reviewee_id, used_in_round: current_round).try(:questionnaire)
if(@revision_plan_questionnaire)
@questions += sort_questions(@revision_plan_questionnaire.questions)
end
end
return @questions
end

This method gets questions from a response instead of questionnaire. We use this when response has already been created.

def set_questions
@questions = @response.get_questions
end

''app/models/response.rb''

Added a method to get all the questionnaires for a response, response is a collection of answers. Answers contain reference to their question, and question have a reference to questionnaire.

def questionnaires_by_answers(answers)
answers_with_questionnaires = answers.select{ |ans| ans && ans.question && ans.question.questionnaire }
questionnaires = answers_with_questionnaires.collect{ |ans| ans.question.questionnaire }.uniq
unless(questionnaires.any?)
questionnaires = []
questionnaires << questionnaire_by_answer(answers.first)
end
questionnaires
end

This method gets all the questions for a response.

def get_questions
@questions = []
questionnaires = questionnaires_by_answers(scores)
questionnaires.each {|questionnaire| @questions += questionnaire.questions }
return @questions
end

*Separating questions by their questionnaire type

''app/models/response.rb''

<code>construct_review_response</code> method has been updated to create separate tables for review questions, revision questions and additional comment.

''app/models/questionnaire.rb''

Add a method to check if heading is to be displayed for the questionnaire.

def display_heading?
return false
end

''app/models/revision_plan_questionnaire.rb''

Overriding the method defined in questionnaire.rb to display heading for revision plan questionnaire.

def display_heading?
return true
end

''app/views/response/response.html.erb''

View groups questions by questionnaire type and then adds a heading for questionnaires that return true for <code>display_heading?</code>.

'''Bug fix:''' The existing implementation has a bug where the first question in a questionnaire is displayed last in response. This was fixed as well.

====Summary Report Page====
When a project has been reviewed at least once, a participant is able to view their team's score. The UI below shows what this looks like after the second round of reviews. For the second and all subsequent reviews, the results of questions that were created by the instructor are shown under '''Assignment Questionnaire.''' The results of the questions created by the team are shown under '''Improvement Plan.'''

=====Initial Wireframe=====
[[File:View_Scores_Wireframe.png|400px|thumb|center|Wireframe of Summary Report for an Assignment]]
=====Final UI=====
[[File:Summary_Report.png|600px|thumb|center|Summary Report page demonstrating added Revision Plan heatgrid]]

=====Implementation=====

*Show revision plan questions in heat grid.

Heat grids for review responses are currently implemented in a view model defined in ''vm_question_response.rb''. All functionality of this view model has remained in place, and additional responses are passed to it to create the additional heat grids for revision plan responses.

''app/helpers/grades_helper.rb''

The method <code>generate_heatgrid</code> was created from the original code shared by both <code>view_heatgrid</code> and <code>view_team</code> in ''app/controllers/grades_controller'', but with the additional functionality of checking for Revision Plans in responses. When <code>assignment.vary_by_round</code>, an additional heatgrid is added for each round of Revision Planning. When <code>!assignment.vary_by_round</code>, only one heatgrid is added for the Revision Plan used in the most recent round of review.

def generate_heatgrid(participant, assignment, team, team_id, questionnaires, vmlist)
counter_for_revisions = -1
counter_for_same_rubric = 0
questionnaires.each do |questionnaire|
if assignment.vary_by_round? && questionnaire.type == "ReviewQuestionnaire"
questionnaires = AssignmentQuestionnaire.where(assignment_id: assignment.id, questionnaire_id: questionnaire.id)
if questionnaires.count > 1
@round = questionnaires[counter_for_same_rubric].used_in_round
counter_for_same_rubric += 1
else
@round = questionnaires[0].used_in_round
counter_for_same_rubric = 0
end
end
add_response_to_vmlist(participant, assignment, team, questionnaire, vmlist, @round)
if assignment.vary_by_round? && assignment.is_revision_planning_enabled?
rp_questionnaire = RevisionPlanTeamMap.find_by(team: Team.find(team_id), used_in_round: counter_for_revisions).try(:questionnaire)
if rp_questionnaire
add_response_to_vmlist(participant, assignment, team, rp_questionnaire, vmlist, @round)
end
counter_for_revisions += 1
elsif assignment.is_revision_planning_enabled? && questionnaire == questionnaires.last
if assignment.get_current_stage == "Finished"
current_round = assignment.rounds_of_reviews
else
reviewees_topic = SignedUpTeam.topic_id_by_team_id(participant.id)
current_round = assignment.number_of_current_round(reviewees_topic)
end
rp_questionnaire = RevisionPlanTeamMap.find_by(team: Team.find(team_id), used_in_round: current_round).try(:questionnaire)
if rp_questionnaire
add_response_to_vmlist(participant, assignment, team, rp_questionnaire, vmlist, @round)
end
end
end
end

The method <code>add_response_to_vmlist</code> was added to refactor the code that is used multiple times to add another review to the view model.

def add_response_to_vmlist(participant, assignment, team, questionnaire, vmlist, round)
vm = VmQuestionResponse.new(questionnaire, assignment, round)
vmquestions = questionnaire.questions
vm.add_questions(vmquestions)
vm.add_team_members(team)
vm.add_reviews(participant, team, assignment.vary_by_round)
vm.number_of_comments_greater_than_10_words
vmlist << vm
end

''app/helpers/summary_helper.rb''

'''Bug Fix:''' Reverted <code>get_sentences</code> back to version in master branch to prevent Active Record Error caused by beta version of code.

def get_sentences(ans)
unless ans.comments.nil?
ans.comments.gsub!(/[.?!]/, '\1|')
sentences = ans.comments.split('|')
sentences.map!(&:strip)
end
end

''app/models/vm_question_response.rb''

Allow <code>RevisionPlanQuestionnaires</code> to be added to the view model in the same way as <code>ReviewQuestionnaires</code>.

if (@questionnaire_type == "ReviewQuestionnaire" or @questionnaire_type == "RevisionPlanQuestionnaire")

''app/views/grades/view_team.html.erb''

Print round number headings for Revision Plan heat grids.

<% if vm.questionnaire_type == "ReviewQuestionnaire" or vm.questionnaire_type == "RevisionPlanQuestionnaire"%>

*Separated Review and Revision Plan questions in response

''app/models/response.rb''

Created separate arrays for ReviewQuestionnaire questions and RevsionPlanQuestionnaire questions. Added headings for Review Responses and Revision Plan Responses.

review_questions = []
revision_plan_questions = []
#begin table for Review Responses
...
map = ResponseMap.find(self.map_id)
unless map.is_a? ReviewResponseMap
code = add_table_rows questionnaire_max, questions, answers, code, tag_prompt_deployments, current_user
else
assignment = map.assignment
questions.each do |question|
if(question.questionnaire.type == 'ReviewQuestionnaire')
review_questions.append(question)
elsif(question.questionnaire.type == 'RevisionPlanQuestionnaire')
revision_plan_questions.append(question)
end
end
code = add_table_rows questionnaire_max, review_questions, answers, code, tag_prompt_deployments, current_user
if assignment.is_revision_planning_enabled
#end table for Review Responses and begin table for Revision Plan Responses
code = add_table_rows questionnaire_max, revision_plan_questions, answers, code, tag_prompt_deployments, current_user
end

*Refactored <code>view_team</code> and <code>view_heatgrid</code>

''app/controllers/grades_controller.rb''

The code in the <code>view_team</code> method was almost identical to the code in the <code>view_heatgrid</code> method in ''app/helpers/grades_helper.rb''. It was refactored into a new method <code>generate_heatgrid</code> in ''app/helpers/grades_helper.rb'' and used in <code>view_team</code>.

def view_team
@participant = AssignmentParticipant.find(params[:id])
@assignment = @participant.assignment
@team = @participant.team
@team_id = @team.id
questionnaires = @assignment.questionnaires
@questions = retrieve_questions questionnaires, @assignment.id
@pscore = @participant.scores(@questions)
@vmlist = []
generate_heatgrid(@participant, @assignment, @team, @team_id, questionnaires, @vmlist)
@current_role_name = current_role_name
end

''app/helpers/grades_helper.rb''

The code in the <code>view_heatgrid</code> method was almost identical to the code in the <code>view_team</code> method in ''app/controllers/grades_controller.rb''. It was refactored into a new method <code>generate_heatgrid</code> and used in <code>view_heatgrid</code>.

def view_heatgrid(participant_id, type)
# get participant, team, questionnaires for assignment.
@participant = AssignmentParticipant.find(participant_id)
@assignment = @participant.assignment
@team = @participant.team
@team_id = @team.id
@type = type
questionnaires = @assignment.questionnaires
@vmlist = []
generate_heatgrid(@participant, @assignment, @team, @team_id, questionnaires, @vmlist)
render "grades/view_heatgrid.html.erb"
end

===Control Flow Diagram===
[[File:Revision Planning Control Flow Diagram.jpeg|400px|thumb|center|Control Flow of the Revision Planning Function]]

===Test Plan - Under progress===

====RSpec Testing====
The RSpec tests are written to test both controller and models.

Controllers

*spec/controllers/grades_controller_spec.rb:

*spec/controllers/revision_plan_questionnaires_controller_spec.rb: test revision plan questionnaire can be created and edited.

*spec/controllers/assignments_controller_spec.rb: create and validate an assignment with revision planning enabled.

*spec/controllers/response_controller_spec.rb: Updated spec to pass build by stubbing <code>get_questions</code> method.

Models

*spec/models/revision_plan_questionnaire_spec.rb:

*spec/models/revision_plan_team_map_spec.rb:

*spec/models/response_spec.rb: Updated spec to pass build by updating expected html output.

*spec/models/review_response_map_spec.rb: Updated spec to pass build by updating expected html output.

Features

''spec/features/assignment_creation_general_tab_spec.rb''

Test if checking box tag <code>Revision Planning?</code> could enable revision planning.

it "should enable revision planning" do
fill_assignment_form
check("assignment_form_assignment_is_revision_planning_enabled")
click_button 'Save'
assignment = Assignment.where(name: 'edit assignment for test').first
expect(assignment).to have_attributes(
name: 'edit assignment for test',
course_id: Course.find_by(name: 'Course 2').id,
directory_path: 'testDirectory1',
spec_location: 'testLocation1',
is_revision_planning_enabled: true,
)
end

====Manual Testing====
Manual testing aims to verify the following:
* Create an assignment with revision planning enabled.
* Are participants allowed to create/edit revision plan when round 1+ (1 or greater than 1) reviews have finished.
* Is revision plan editing disabled when assignment is in review stage.
* Are reviewers shown questions created by reviewees.
* Are participants shown summary of score for revision plan after review deadline has expired.

==Team Members==
Chaitanya Mehta (cmehta)
 
Darby Madewell (demadewe)
 
Dongni Yang (dyang23)
 
Sidharth Mehta (smehta22)
 
'''Mentor:''' Yulin Zhang (yzhan114)

==References==
# Previous Implementation: [https://expertiza.csc.ncsu.edu/index.php/E1875_Revision_Planning_Tool E1875 wiki], [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2020_E2016_Revision_planning_tool E2016 wiki]
# Forked Repository: [https://github.com/SidharthMehta/expertiza/tree/beta E2083 github]
# Pull Request: [https://github.com/expertiza/expertiza/pull/1832 E2083 pull request]
# Demo: [https://youtu.be/7pdz_A8vDkk E2083 demo video]
# Deployment: [http://152.7.98.99:8080/ temporary deployment link]

CSC/ECE 517 Fall 2020 - E2083. Revision planning tool E2016

2020-10-28T19:13:13Z

Dyang23: /* Introduction */

CSC/ECE 517 Fall 2020 - E2083. Revision planning tool E2016

2020-10-20T22:59:52Z

Dyang23: /* Control Flow Diagram */

File:Revision Planning Control Flow Diagram.jpeg

2020-10-20T22:57:06Z

Dyang23:

CSC/ECE 517 Fall 2020 - E2083. Revision planning tool E2016

2020-10-20T22:05:30Z

Dyang23: /* Assignment Overview Page (contains the Link to the revision planning page) */

File:Link to the revision planning.jpeg

2020-10-20T22:05:01Z

Dyang23:

CSC/ECE 517 Fall 2020 - E2083. Revision planning tool E2016

2020-10-20T22:01:20Z

Dyang23: /* Assignment Overview Page (contains the Link to the revision planning page) */

CSC/ECE 517 Fall 2020 - E2083. Revision planning tool E2016

2020-10-20T22:00:42Z

Dyang23: /* Assignment Overview Page (contains the Link to the revision planning page) */

File:Link to the revision planning.png

2020-10-20T21:47:52Z

Dyang23:

CSC/ECE 517 Fall 2020 - E2083. Revision planning tool E2016

2020-10-20T21:39:28Z

Dyang23: /* Submission Page */

CSC/ECE 517 Fall 2020 - E2067. Refactor student teams controller.rb

2020-10-12T18:58:28Z

Dyang23: /* Test */

==Introduction==
===Background===
The '''student_teams_controller.rb''' controller used in Expertiza to manipulate teams that are created for assignments. Primary functions that this controller provides is create a new team, update team name and delete members from a team.

===Motivation===
The motivation of our work is to improve the maintainability and readability of '''student_teams_controller.rb''' Controller. Furthermore, we also fix several occasions where the '''student_teams_controller.rb''' contains code snippets that actually belongs to the model classes. This will help enforce single responsibility principle and model-view-controller pattern.

===Task Identified===
* Move '''return unless current_user_id? student.user_id''' into '''action_allowed method'''
* Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class'''
* Remove variable '''current_team'''
* Add comment for '''@users_on_waiting_list''' and also simplify the condition
* Refactor '''@teammate_review_allowed''' into '''due_date.rb'''
* Rename '''existing_assignments?''' to '''existing_teams'''
* Add method comment for the '''update method'''
* Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?'''
* Add method comment for '''Advertise_for_partners''' and '''remove_advertisement'''
* Rename '''sign_up''' into '''signup'''
* Refactor code from '''remove_participant''' into '''waitlist.rb'''
* Add method comment for the '''review method'''

===Affected Classes===
*/controllers/student_teams_controller.rb
*/models/waitlist.rb
*/models/due_date.rb

==Code Review==
The purpose of this section is to provide the before and after change comparison so the developer can perform code review to ensure the correctness of the change.

=== Move '''return unless current_user_id? student.user_id''' into '''action_allowed method''' ===
* Objective
The action_allowed method determines whether an action is permissible, depending on the privileges of the user who is performing it. In the view method, there is a clause that says, return unless current_user_id? student.user_id. This needs to be moved to the when 'view' clause in the action_allowed method.
* Change Comparison
''Student_teams_controller.rb'': 
'''action_allowed method''' 
[[File:Issue1 fix.png|1000px]] 
'''view method''' 
[[File:Issue1 fix2.png|1000px]]
* Change Explanation
Deleted 'return unless current_user_id? student.user_id' in the view method, and added it to a if clause under the when 'view' clause in the action_allowed method. So that every time before view method is called, this sentence in the action_allowed method will run first.

=== Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class''' ===
* Objective
The code beginning at line 44 deals with due_dates, which are part of the “business logic” and should be moved to an appropriate model method. Thus, @student.assignment.due_dates.each do |due_date| should invoke an appropriate method in due_date.rb. It may or may not be necessary to add a method to the DueDates class.
* Change Comparison
 
''Student_teams_controller.rb'':
[[File:current_due_date_comparison.PNG]]
 
 
''due_date.rb'': 
[[File:due_date_method_added.PNG]]
* Change Explanation
The code in the first image uses logic associated with due dates to assign a variable called '@current_due_date'. This logic was moved to a new method in the model due_dates.rb, shown in the second image. This logic was moved to to simplify the controller but also because the logic is specific to due dates and is more reasonable to be located in the due dates model.

=== Remove variable '''current_team''' ===
* Objective
The variable current_team seems unnecessary; it only saves one character vs. @student.team. Consider removing it.
* Change Comparison
[[File:current_team_change_comparison.PNG]]

* Change Explanation
In the red line 51 above, the variable 'current_team' is used. This variable is unnecessary as it is not significantly shorter or more descriptive. As shown in the changes between red line 53 and green line 49, the 'current_team' variable was removed and replaced with '@student.team'.

=== Add comment for '''@users_on_waiting_list''' and also simplify the condition ===
* Objective
The code on Line 53 (@users_on_waiting_list ...) is not clear at all. Needs at least a comment, and also the condition should not be so complicated.
* Change Comparison
[[File:updated_comment_waiting_list.PNG]]
[[File:added_requirements_method.PNG]]

* Change Explanation
The '@users_on_waiting_list' line of code needed a comment to clarify its function. The original line had a complex conditional that made the code hard to read. This conditional was moved to a new method in which it was refactored and commented to be more understandable and readable.

=== Refactor '''@teammate_review_allowed''' into '''due_date.rb''' ===
* Objective
In line 55, the @teammate_review_allowed: condition is way too complex, uses magic constants, and belongs in code in another model class (maybe due_date.rb); no way should it be in the controller!
* Change Comparison
[[File:Issue5 fix.png|1000px]]
 
''Student_teams_controller.rb'': 
[[File:Issue5 fix2.png|1000px]]

* Change Explanation
Moved '''@teammate_review_allowed''' 's condition to '''due_date.rb''' so the code is more readable.

=== Rename '''existing_assignments?''' to '''existing_teams''' ===
* Objective
Line 59: existing_assignments is evidently a team! So why is the variable name existing_assignments? It should perhaps be changed to existing_teams.
* Change Comparison
[[File:existing_assignments_comparison.PNG]]

* Change Explanation
The variable 'existing_assignments' was actually a team. To better reflect the purpose of the variable and function of the code, the name was changed from 'existing_assignments' to 'existing_teams'.

=== Add method comment for the '''update method''' ===
* Objective
The update method needs comments about what it is doing.
* Change Comparison (commit 7a9e39)
[[File:E2067_Issue7_Fix.png|1000px]]
* Change Explanation
The '''if''' condition checks if the new team name is NOT already exist, if so then update the team name. The '''elsif''' condition checks if the new team name is the same the current one, if so then nothing is changed. The '''else''' covers the case where the new team name is already in use, then print an error message and not changing the team name.
=== Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?''' ===
* Objective
Line 95: Why isn’t (matching_teams[0].name <=> team.name).zero? just (matching_teams[0].name == team.name)?
* Change Comparison (commit d60d92)
[[File:E2067_Issue8_Fix.png|1000px]]
* Change Explanation
This change is straight forward to satisfy the objective of the task.

=== Add method comment for '''Advertise_for_partners''' and '''remove_advertisement''' ===
* Objective
Line 108: advertise_for_partners needs a method comment. Since its body is only 1 line, does it make sense to have a separate method for this? It makes sense only if having a separate method improves readability. Ditto for the remove_advertisement method at line 112.
* Change Comparison
[[File:Issue9 fix.png|1000px]]
* Change Explanation
Both '''advertise_for_partners''' and '''remove_advertisement''' are necessary in terms of readability.

=== Rename '''sign_up''' into '''signup''' ===
* Objective
The name sign_up needs to be signup; it is being used as a noun (sign_up would suggest a verb, the action of signing up).
* Change Comparison (commit e393ba)
[[File:E2067_Issue10_Fix.png|1000px]]
* Change Explanation
I performed a file search on '''sign_up''' and it only appears in the places shown in the screenshot. I replaced them with signup according to the objective.
=== Refactor code from '''remove_participant''' into '''waitlist.rb''' ===
* Objective
remove_participant is generally good code, but handling of waitlists is again model activity, and should be moved to a model class, perhaps waitlist.rb.
* Change Comparison (commit 504f84)
[[File:E2067_Issue11_Fix1.png|1000px]]
[[File:E2067_Issue11_Fix2.png|1000px]]
* Change Explanation
Line 127~142 in '''student_teams_controller.rb''' are moved to Line 14~29 in '''waitlist.rb'''. The only input to the new function '''remove_from_waitlists''' in '''waitlist.rb''' requires '''params[:team_id]''' from '''student_teams_controller.rb'''.
=== Add method comment for the '''review method''' ===
* Objective
Line 169: The review method needs a method comment; its purpose is not clear.
* Change Comparison
[[File:Issue12 fix.png|1000px]]

* Change Explanation
Clarified what review method intends to do.

==Test==
File: spec/controllers/student_teams_controller_spec.rb

<pre>
describe StudentTeamsController do
let (:student_teams_controller) { StudentTeamsController.new }
let(:student) { double "student" }
describe '#view' do
it 'sets the student' do
allow(AssignmentParticipant).to receive(:find).with('12345').and_return student
allow(student_teams_controller).to receive(:current_user_id?)
allow(student_teams_controller).to receive(:params).and_return(student_id: '12345')
allow(student).to receive(:user_id)
student_teams_controller.view
end
end
end
</pre>

==Future Work==
In the update method of student_teams_controller.rb, the 'elsif' needs a comment to explain its purpose. It is unclear as to why this conditional is present.

CSC/ECE 517 Fall 2020 - E2067. Refactor student teams controller.rb

2020-10-12T18:43:24Z

Dyang23: /* Add method comment for Advertise_for_partners and remove_advertisement */

==Introduction==
===Background===
The '''student_teams_controller.rb''' controller used in Expertiza to manipulate teams that are created for assignments. Primary functions that this controller provides is create a new team, update team name and delete members from a team.

===Motivation===
The motivation of our work is to improve the maintainability and readability of '''student_teams_controller.rb''' Controller. Furthermore, we also fix several occasions where the '''student_teams_controller.rb''' contains code snippets that actually belongs to the model classes. This will help enforce single responsibility principle and model-view-controller pattern.

===Task Identified===
* Move '''return unless current_user_id? student.user_id''' into '''action_allowed method'''
* Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class'''
* Remove variable '''current_team'''
* Add comment for '''@users_on_waiting_list''' and also simplify the condition
* Refactor '''@teammate_review_allowed''' into '''due_date.rb'''
* Rename '''existing_assignments?''' to '''existing_teams'''
* Add method comment for the '''update method'''
* Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?'''
* Add method comment for '''Advertise_for_partners''' and '''remove_advertisement'''
* Rename '''sign_up''' into '''signup'''
* Refactor code from '''remove_participant''' into '''waitlist.rb'''
* Add method comment for the '''review method'''

===Affected Classes===
*/controllers/student_teams_controller.rb
*/models/waitlist.rb
*/models/due_date.rb

==Code Review==
The purpose of this section is to provide the before and after change comparison so the developer can perform code review to ensure the correctness of the change.

=== Move '''return unless current_user_id? student.user_id''' into '''action_allowed method''' ===
* Objective
The action_allowed method determines whether an action is permissible, depending on the privileges of the user who is performing it. In the view method, there is a clause that says, return unless current_user_id? student.user_id. This needs to be moved to the when 'view' clause in the action_allowed method.
* Change Comparison
''Student_teams_controller.rb'': 
'''action_allowed method''' 
[[File:Issue1 fix.png|1000px]] 
'''view method''' 
[[File:Issue1 fix2.png|1000px]]
* Change Explanation
Deleted 'return unless current_user_id? student.user_id' in the view method, and added it to a if clause under the when 'view' clause in the action_allowed method. So that every time before view method is called, this sentence in the action_allowed method will run first.

=== Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class''' ===
* Objective
The code beginning at line 44 deals with due_dates, which are part of the “business logic” and should be moved to an appropriate model method. Thus, @student.assignment.due_dates.each do |due_date| should invoke an appropriate method in due_date.rb. It may or may not be necessary to add a method to the DueDates class.
* Change Comparison
 
''Student_teams_controller.rb'':
[[File:current_due_date_comparison.PNG]]
 
 
''due_date.rb'': 
[[File:due_date_method_added.PNG]]
* Change Explanation
The code in the first image uses logic associated with due dates to assign a variable called '@current_due_date'. This logic was moved to a new method in the model due_dates.rb, shown in the second image. This logic was moved to to simplify the controller but also because the logic is specific to due dates and is more reasonable to be located in the due dates model.

=== Remove variable '''current_team''' ===
* Objective
The variable current_team seems unnecessary; it only saves one character vs. @student.team. Consider removing it.
* Change Comparison
[[File:current_team_change_comparison.PNG]]

* Change Explanation
In the red line 51 above, the variable 'current_team' is used. This variable is unnecessary as it is not significantly shorter or more descriptive. As shown in the changes between red line 53 and green line 49, the 'current_team' variable was removed and replaced with '@student.team'.

=== Add comment for '''@users_on_waiting_list''' and also simplify the condition ===
* Objective
The code on Line 53 (@users_on_waiting_list ...) is not clear at all. Needs at least a comment, and also the condition should not be so complicated.
* Change Comparison
[[File:updated_comment_waiting_list.PNG]]
[[File:added_requirements_method.PNG]]

* Change Explanation
The '@users_on_waiting_list' line of code needed a comment to clarify its function. The original line had a complex conditional that made the code hard to read. This conditional was moved to a new method in which it was refactored and commented to be more understandable and readable.

=== Refactor '''@teammate_review_allowed''' into '''due_date.rb''' ===
* Objective
In line 55, the @teammate_review_allowed: condition is way too complex, uses magic constants, and belongs in code in another model class (maybe due_date.rb); no way should it be in the controller!
* Change Comparison
[[File:Issue5 fix.png|1000px]]
 
''Student_teams_controller.rb'': 
[[File:Issue5 fix2.png|1000px]]

* Change Explanation
Moved '''@teammate_review_allowed''' 's condition to '''due_date.rb''' so the code is more readable.

=== Rename '''existing_assignments?''' to '''existing_teams''' ===
* Objective
Line 59: existing_assignments is evidently a team! So why is the variable name existing_assignments? It should perhaps be changed to existing_teams.
* Change Comparison
[[File:existing_assignments_comparison.PNG]]

* Change Explanation
The variable 'existing_assignments' was actually a team. To better reflect the purpose of the variable and function of the code, the name was changed from 'existing_assignments' to 'existing_teams'.

=== Add method comment for the '''update method''' ===
* Objective
The update method needs comments about what it is doing.
* Change Comparison (commit 7a9e39)
[[File:E2067_Issue7_Fix.png|1000px]]
* Change Explanation
The '''if''' condition checks if the new team name is NOT already exist, if so then update the team name. The '''elsif''' condition checks if the new team name is the same the current one, if so then nothing is changed. The '''else''' covers the case where the new team name is already in use, then print an error message and not changing the team name.
=== Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?''' ===
* Objective
Line 95: Why isn’t (matching_teams[0].name <=> team.name).zero? just (matching_teams[0].name == team.name)?
* Change Comparison (commit d60d92)
[[File:E2067_Issue8_Fix.png|1000px]]
* Change Explanation
This change is straight forward to satisfy the objective of the task.

=== Add method comment for '''Advertise_for_partners''' and '''remove_advertisement''' ===
* Objective
Line 108: advertise_for_partners needs a method comment. Since its body is only 1 line, does it make sense to have a separate method for this? It makes sense only if having a separate method improves readability. Ditto for the remove_advertisement method at line 112.
* Change Comparison
[[File:Issue9 fix.png|1000px]]
* Change Explanation
Both '''advertise_for_partners''' and '''remove_advertisement''' are necessary in terms of readability.

=== Rename '''sign_up''' into '''signup''' ===
* Objective
The name sign_up needs to be signup; it is being used as a noun (sign_up would suggest a verb, the action of signing up).
* Change Comparison (commit e393ba)
[[File:E2067_Issue10_Fix.png|1000px]]
* Change Explanation
I performed a file search on '''sign_up''' and it only appears in the places shown in the screenshot. I replaced them with signup according to the objective.
=== Refactor code from '''remove_participant''' into '''waitlist.rb''' ===
* Objective
remove_participant is generally good code, but handling of waitlists is again model activity, and should be moved to a model class, perhaps waitlist.rb.
* Change Comparison (commit 504f84)
[[File:E2067_Issue11_Fix1.png|1000px]]
[[File:E2067_Issue11_Fix2.png|1000px]]
* Change Explanation
Line 127~142 in '''student_teams_controller.rb''' are moved to Line 14~29 in '''waitlist.rb'''. The only input to the new function '''remove_from_waitlists''' in '''waitlist.rb''' requires '''params[:team_id]''' from '''student_teams_controller.rb'''.
=== Add method comment for the '''review method''' ===
* Objective
Line 169: The review method needs a method comment; its purpose is not clear.
* Change Comparison
[[File:Issue12 fix.png|1000px]]

* Change Explanation
Clarified what review method intends to do.

==Test==

==Future Work==
In the update method of student_teams_controller.rb, the 'elsif' needs a comment to explain its purpose. It is unclear as to why this conditional is present.

File:Issue9 fix.png

2020-10-12T18:39:20Z

Dyang23:

CSC/ECE 517 Fall 2020 - E2067. Refactor student teams controller.rb

2020-10-12T17:27:07Z

Dyang23: /* Add method comment for the review method */

==Introduction==
===Background===
The '''student_teams_controller.rb''' controller used in Expertiza to manipulate teams that are created for assignments. Primary functions that this controller provides is create a new team, update team name and delete members from a team.

===Motivation===
The motivation of our work is to improve the maintainability and readability of '''student_teams_controller.rb''' Controller. Furthermore, we also fix several occasions where the '''student_teams_controller.rb''' contains code snippets that actually belongs to the model classes. This will help enforce single responsibility principle and model-view-controller pattern.

===Task Identified===
* Move '''return unless current_user_id? student.user_id''' into '''action_allowed method'''
* Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class'''
* Remove variable '''current_team'''
* Add comment for '''@users_on_waiting_list''' and also simplify the condition
* Refactor '''@teammate_review_allowed''' into '''due_date.rb'''
* Rename '''existing_assignments?''' to '''existing_teams'''
* Add method comment for the '''update method'''
* Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?'''
* Add method comment for '''Advertise_for_partners''' and '''remove_advertisement'''
* Rename '''sign_up''' into '''signup'''
* Refactor code from '''remove_participant''' into '''waitlist.rb'''
* Add method comment for the '''review method'''

===Affected Classes===
*/controllers/student_teams_controller.rb
*/models/waitlist.rb
*/models/due_date.rb

==Code Review==
The purpose of this section is to provide the before and after change comparison so the developer can perform code review to ensure the correctness of the change.

=== Move '''return unless current_user_id? student.user_id''' into '''action_allowed method''' ===
* Objective
The action_allowed method determines whether an action is permissible, depending on the privileges of the user who is performing it. In the view method, there is a clause that says, return unless current_user_id? student.user_id. This needs to be moved to the when 'view' clause in the action_allowed method.
* Change Comparison
''Student_teams_controller.rb'': 
'''action_allowed method''' 
[[File:Issue1 fix.png|1000px]] 
'''view method''' 
[[File:Issue1 fix2.png|1000px]]
* Change Explanation
Deleted 'return unless current_user_id? student.user_id' in the view method, and added it to a if clause under the when 'view' clause in the action_allowed method. So that every time before view method is called, this sentence in the action_allowed method will run first.

=== Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class''' ===
* Objective
The code beginning at line 44 deals with due_dates, which are part of the “business logic” and should be moved to an appropriate model method. Thus, @student.assignment.due_dates.each do |due_date| should invoke an appropriate method in due_date.rb. It may or may not be necessary to add a method to the DueDates class.
* Change Comparison
 
''Student_teams_controller.rb'':
[[File:current_due_date_comparison.PNG]]
 
 
''due_date.rb'': 
[[File:due_date_method_added.PNG]]
* Change Explanation
The code in the first image uses logic associated with due dates to assign a variable called '@current_due_date'. This logic was moved to a new method in the model due_dates.rb, shown in the second image. This logic was moved to to simplify the controller but also because the logic is specific to due dates and is more reasonable to be located in the due dates model.

=== Remove variable '''current_team''' ===
* Objective
The variable current_team seems unnecessary; it only saves one character vs. @student.team. Consider removing it.
* Change Comparison
[[File:current_team_change_comparison.PNG]]

* Change Explanation
In the red line 51 above, the variable 'current_team' is used. This variable is unnecessary as it is not significantly shorter or more descriptive. As shown in the changes between red line 53 and green line 49, the 'current_team' variable was removed and replaced with '@student.team'.

=== Add comment for '''@users_on_waiting_list''' and also simplify the condition ===
* Objective
The code on Line 53 (@users_on_waiting_list ...) is not clear at all. Needs at least a comment, and also the condition should not be so complicated.
* Change Comparison
[[File:updated_comment_waiting_list.PNG]]
[[File:added_requirements_method.PNG]]

* Change Explanation
The '@users_on_waiting_list' line of code needed a comment to clarify its function. The original line had a complex conditional that made the code hard to read. This conditional was moved to a new method in which it was refactored and commented to be more understandable and readable.

=== Refactor '''@teammate_review_allowed''' into '''due_date.rb''' ===
* Objective
In line 55, the @teammate_review_allowed: condition is way too complex, uses magic constants, and belongs in code in another model class (maybe due_date.rb); no way should it be in the controller!
* Change Comparison
[[File:Issue5 fix.png|1000px]]
 
''Student_teams_controller.rb'': 
[[File:Issue5 fix2.png|1000px]]

* Change Explanation
Moved '''@teammate_review_allowed''' 's condition to '''due_date.rb''' so the code is more readable.

=== Rename '''existing_assignments?''' to '''existing_teams''' ===
* Objective
Line 59: existing_assignments is evidently a team! So why is the variable name existing_assignments? It should perhaps be changed to existing_teams.
* Change Comparison
[[File:existing_assignments_comparison.PNG]]

* Change Explanation
The variable 'existing_assignments' was actually a team. To better reflect the purpose of the variable and function of the code, the name was changed from 'existing_assignments' to 'existing_teams'.

=== Add method comment for the '''update method''' ===
* Objective
The update method needs comments about what it is doing.
* Change Comparison (commit 7a9e39)
[[File:E2067_Issue7_Fix.png|1000px]]
* Change Explanation
The '''if''' condition checks if the new team name is NOT already exist, if so then update the team name. The '''elsif''' condition checks if the new team name is the same the current one, if so then nothing is changed. The '''else''' covers the case where the new team name is already in use, then print an error message and not changing the team name.
=== Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?''' ===
* Objective
Line 95: Why isn’t (matching_teams[0].name <=> team.name).zero? just (matching_teams[0].name == team.name)?
* Change Comparison (commit d60d92)
[[File:E2067_Issue8_Fix.png|1000px]]
* Change Explanation
This change is straight forward to satisfy the objective of the task.

=== Add method comment for '''Advertise_for_partners''' and '''remove_advertisement''' ===
* Objective
Line 108: advertise_for_partners needs a method comment. Since its body is only 1 line, does it make sense to have a separate method for this? It makes sense only if having a separate method improves readability. Ditto for the remove_advertisement method at line 112.
* Change Comparison

* Change Explanation

=== Rename '''sign_up''' into '''signup''' ===
* Objective
The name sign_up needs to be signup; it is being used as a noun (sign_up would suggest a verb, the action of signing up).
* Change Comparison (commit e393ba)
[[File:E2067_Issue10_Fix.png|1000px]]
* Change Explanation
I performed a file search on '''sign_up''' and it only appears in the places shown in the screenshot. I replaced them with signup according to the objective.
=== Refactor code from '''remove_participant''' into '''waitlist.rb''' ===
* Objective
remove_participant is generally good code, but handling of waitlists is again model activity, and should be moved to a model class, perhaps waitlist.rb.
* Change Comparison (commit 504f84)
[[File:E2067_Issue11_Fix1.png|1000px]]
[[File:E2067_Issue11_Fix2.png|1000px]]
* Change Explanation
Line 127~142 in '''student_teams_controller.rb''' are moved to Line 14~29 in '''waitlist.rb'''. The only input to the new function '''remove_from_waitlists''' in '''waitlist.rb''' requires '''params[:team_id]''' from '''student_teams_controller.rb'''.
=== Add method comment for the '''review method''' ===
* Objective
Line 169: The review method needs a method comment; its purpose is not clear.
* Change Comparison
[[File:Issue12 fix.png|1000px]]

* Change Explanation
Clarified what review method intends to do.

==Test==

==Future Work==
In the update method of student_teams_controller.rb, the 'elsif' needs a comment to explain its purpose. It is unclear as to why this conditional is present.

File:Issue12 fix.png

2020-10-12T17:19:59Z

Dyang23:

CSC/ECE 517 Fall 2020 - E2067. Refactor student teams controller.rb

2020-10-12T17:17:23Z

Dyang23: /* Refactor @teammate_review_allowed into due_date.rb */

==Introduction==
===Background===
The '''student_teams_controller.rb''' controller used in Expertiza to manipulate teams that are created for assignments. Primary functions that this controller provides is create a new team, update team name and delete members from a team.

===Motivation===
The motivation of our work is to improve the maintainability and readability of '''student_teams_controller.rb''' Controller. Furthermore, we also fix several occasions where the '''student_teams_controller.rb''' contains code snippets that actually belongs to the model classes. This will help enforce single responsibility principle and model-view-controller pattern.

===Task Identified===
* Move '''return unless current_user_id? student.user_id''' into '''action_allowed method'''
* Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class'''
* Remove variable '''current_team'''
* Add comment for '''@users_on_waiting_list''' and also simplify the condition
* Refactor '''@teammate_review_allowed''' into '''due_date.rb'''
* Rename '''existing_assignments?''' to '''existing_teams'''
* Add method comment for the '''update method'''
* Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?'''
* Add method comment for '''Advertise_for_partners''' and '''remove_advertisement'''
* Rename '''sign_up''' into '''signup'''
* Refactor code from '''remove_participant''' into '''waitlist.rb'''
* Add method comment for the '''review method'''

===Affected Classes===
*/controllers/student_teams_controller.rb
*/models/waitlist.rb
*/models/due_date.rb

==Code Review==
The purpose of this section is to provide the before and after change comparison so the developer can perform code review to ensure the correctness of the change.

=== Move '''return unless current_user_id? student.user_id''' into '''action_allowed method''' ===
* Objective
The action_allowed method determines whether an action is permissible, depending on the privileges of the user who is performing it. In the view method, there is a clause that says, return unless current_user_id? student.user_id. This needs to be moved to the when 'view' clause in the action_allowed method.
* Change Comparison
''Student_teams_controller.rb'': 
'''action_allowed method''' 
[[File:Issue1 fix.png|1000px]] 
'''view method''' 
[[File:Issue1 fix2.png|1000px]]
* Change Explanation
Deleted 'return unless current_user_id? student.user_id' in the view method, and added it to a if clause under the when 'view' clause in the action_allowed method. So that every time before view method is called, this sentence in the action_allowed method will run first.

=== Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class''' ===
* Objective
The code beginning at line 44 deals with due_dates, which are part of the “business logic” and should be moved to an appropriate model method. Thus, @student.assignment.due_dates.each do |due_date| should invoke an appropriate method in due_date.rb. It may or may not be necessary to add a method to the DueDates class.
* Change Comparison
 
''Student_teams_controller.rb'':
[[File:current_due_date_comparison.PNG]]
 
 
''due_date.rb'': 
[[File:due_date_method_added.PNG]]
* Change Explanation
The code in the first image uses logic associated with due dates to assign a variable called '@current_due_date'. This logic was moved to a new method in the model due_dates.rb, shown in the second image. This logic was moved to to simplify the controller but also because the logic is specific to due dates and is more reasonable to be located in the due dates model.

=== Remove variable '''current_team''' ===
* Objective
The variable current_team seems unnecessary; it only saves one character vs. @student.team. Consider removing it.
* Change Comparison
[[File:current_team_change_comparison.PNG]]

* Change Explanation
In the red line 51 above, the variable 'current_team' is used. This variable is unnecessary as it is not significantly shorter or more descriptive. As shown in the changes between red line 53 and green line 49, the 'current_team' variable was removed and replaced with '@student.team'.

=== Add comment for '''@users_on_waiting_list''' and also simplify the condition ===
* Objective
The code on Line 53 (@users_on_waiting_list ...) is not clear at all. Needs at least a comment, and also the condition should not be so complicated.
* Change Comparison
[[File:updated_comment_waiting_list.PNG]]
[[File:added_requirements_method.PNG]]

* Change Explanation
The '@users_on_waiting_list' line of code needed a comment to clarify its function. The original line had a complex conditional that made the code hard to read. This conditional was moved to a new method in which it was refactored and commented to be more understandable and readable.

=== Refactor '''@teammate_review_allowed''' into '''due_date.rb''' ===
* Objective
In line 55, the @teammate_review_allowed: condition is way too complex, uses magic constants, and belongs in code in another model class (maybe due_date.rb); no way should it be in the controller!
* Change Comparison
[[File:Issue5 fix.png|1000px]]
 
''Student_teams_controller.rb'': 
[[File:Issue5 fix2.png|1000px]]

* Change Explanation
Moved '''@teammate_review_allowed''' 's condition to '''due_date.rb''' so the code is more readable.

=== Rename '''existing_assignments?''' to '''existing_teams''' ===
* Objective
Line 59: existing_assignments is evidently a team! So why is the variable name existing_assignments? It should perhaps be changed to existing_teams.
* Change Comparison
[[File:existing_assignments_comparison.PNG]]

* Change Explanation
The variable 'existing_assignments' was actually a team. To better reflect the purpose of the variable and function of the code, the name was changed from 'existing_assignments' to 'existing_teams'.

=== Add method comment for the '''update method''' ===
* Objective
The update method needs comments about what it is doing.
* Change Comparison (commit 7a9e39)
[[File:E2067_Issue7_Fix.png|1000px]]
* Change Explanation
The '''if''' condition checks if the new team name is NOT already exist, if so then update the team name. The '''elsif''' condition checks if the new team name is the same the current one, if so then nothing is changed. The '''else''' covers the case where the new team name is already in use, then print an error message and not changing the team name.
=== Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?''' ===
* Objective
Line 95: Why isn’t (matching_teams[0].name <=> team.name).zero? just (matching_teams[0].name == team.name)?
* Change Comparison (commit d60d92)
[[File:E2067_Issue8_Fix.png|1000px]]
* Change Explanation
This change is straight forward to satisfy the objective of the task.

=== Add method comment for '''Advertise_for_partners''' and '''remove_advertisement''' ===
* Objective
Line 108: advertise_for_partners needs a method comment. Since its body is only 1 line, does it make sense to have a separate method for this? It makes sense only if having a separate method improves readability. Ditto for the remove_advertisement method at line 112.
* Change Comparison

* Change Explanation

=== Rename '''sign_up''' into '''signup''' ===
* Objective
The name sign_up needs to be signup; it is being used as a noun (sign_up would suggest a verb, the action of signing up).
* Change Comparison (commit e393ba)
[[File:E2067_Issue10_Fix.png|1000px]]
* Change Explanation
I performed a file search on '''sign_up''' and it only appears in the places shown in the screenshot. I replaced them with signup according to the objective.
=== Refactor code from '''remove_participant''' into '''waitlist.rb''' ===
* Objective
remove_participant is generally good code, but handling of waitlists is again model activity, and should be moved to a model class, perhaps waitlist.rb.
* Change Comparison (commit 504f84)
[[File:E2067_Issue11_Fix1.png|1000px]]
[[File:E2067_Issue11_Fix2.png|1000px]]
* Change Explanation
Line 127~142 in '''student_teams_controller.rb''' are moved to Line 14~29 in '''waitlist.rb'''. The only input to the new function '''remove_from_waitlists''' in '''waitlist.rb''' requires '''params[:team_id]''' from '''student_teams_controller.rb'''.
=== Add method comment for the '''review method''' ===
* Objective
Line 169: The review method needs a method comment; its purpose is not clear.
* Change Comparison

* Change Explanation

==Test==

==Future Work==
In the update method of student_teams_controller.rb, the 'elsif' needs a comment to explain its purpose. It is unclear as to why this conditional is present.

CSC/ECE 517 Fall 2020 - E2067. Refactor student teams controller.rb

2020-10-12T17:14:34Z

Dyang23: /* Move return unless current_user_id? student.user_id into action_allowed method */

==Introduction==
===Background===
The '''student_teams_controller.rb''' controller used in Expertiza to manipulate teams that are created for assignments. Primary functions that this controller provides is create a new team, update team name and delete members from a team.

===Motivation===
The motivation of our work is to improve the maintainability and readability of '''student_teams_controller.rb''' Controller. Furthermore, we also fix several occasions where the '''student_teams_controller.rb''' contains code snippets that actually belongs to the model classes. This will help enforce single responsibility principle and model-view-controller pattern.

===Task Identified===
* Move '''return unless current_user_id? student.user_id''' into '''action_allowed method'''
* Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class'''
* Remove variable '''current_team'''
* Add comment for '''@users_on_waiting_list''' and also simplify the condition
* Refactor '''@teammate_review_allowed''' into '''due_date.rb'''
* Rename '''existing_assignments?''' to '''existing_teams'''
* Add method comment for the '''update method'''
* Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?'''
* Add method comment for '''Advertise_for_partners''' and '''remove_advertisement'''
* Rename '''sign_up''' into '''signup'''
* Refactor code from '''remove_participant''' into '''waitlist.rb'''
* Add method comment for the '''review method'''

===Affected Classes===
*/controllers/student_teams_controller.rb
*/models/waitlist.rb
*/models/due_date.rb

==Code Review==
The purpose of this section is to provide the before and after change comparison so the developer can perform code review to ensure the correctness of the change.

=== Move '''return unless current_user_id? student.user_id''' into '''action_allowed method''' ===
* Objective
The action_allowed method determines whether an action is permissible, depending on the privileges of the user who is performing it. In the view method, there is a clause that says, return unless current_user_id? student.user_id. This needs to be moved to the when 'view' clause in the action_allowed method.
* Change Comparison
''Student_teams_controller.rb'': 
'''action_allowed method''' 
[[File:Issue1 fix.png|1000px]] 
'''view method''' 
[[File:Issue1 fix2.png|1000px]]
* Change Explanation
Deleted 'return unless current_user_id? student.user_id' in the view method, and added it to a if clause under the when 'view' clause in the action_allowed method. So that every time before view method is called, this sentence in the action_allowed method will run first.

=== Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class''' ===
* Objective
The code beginning at line 44 deals with due_dates, which are part of the “business logic” and should be moved to an appropriate model method. Thus, @student.assignment.due_dates.each do |due_date| should invoke an appropriate method in due_date.rb. It may or may not be necessary to add a method to the DueDates class.
* Change Comparison
 
''Student_teams_controller.rb'':
[[File:current_due_date_comparison.PNG]]
 
 
''due_date.rb'': 
[[File:due_date_method_added.PNG]]
* Change Explanation
The code in the first image uses logic associated with due dates to assign a variable called '@current_due_date'. This logic was moved to a new method in the model due_dates.rb, shown in the second image. This logic was moved to to simplify the controller but also because the logic is specific to due dates and is more reasonable to be located in the due dates model.

=== Remove variable '''current_team''' ===
* Objective
The variable current_team seems unnecessary; it only saves one character vs. @student.team. Consider removing it.
* Change Comparison
[[File:current_team_change_comparison.PNG]]

* Change Explanation
In the red line 51 above, the variable 'current_team' is used. This variable is unnecessary as it is not significantly shorter or more descriptive. As shown in the changes between red line 53 and green line 49, the 'current_team' variable was removed and replaced with '@student.team'.

=== Add comment for '''@users_on_waiting_list''' and also simplify the condition ===
* Objective
The code on Line 53 (@users_on_waiting_list ...) is not clear at all. Needs at least a comment, and also the condition should not be so complicated.
* Change Comparison
[[File:updated_comment_waiting_list.PNG]]
[[File:added_requirements_method.PNG]]

* Change Explanation
The '@users_on_waiting_list' line of code needed a comment to clarify its function. The original line had a complex conditional that made the code hard to read. This conditional was moved to a new method in which it was refactored and commented to be more understandable and readable.

=== Refactor '''@teammate_review_allowed''' into '''due_date.rb''' ===
* Objective
In line 55, the @teammate_review_allowed: condition is way too complex, uses magic constants, and belongs in code in another model class (maybe due_date.rb); no way should it be in the controller!
* Change Comparison
[[File:Issue5 fix.png]]
 
''Student_teams_controller.rb'': 
[[File:Issue5 fix2.png]]

* Change Explanation
Moved '''@teammate_review_allowed''' 's condition to '''due_date.rb''' so the code is more readable.

=== Rename '''existing_assignments?''' to '''existing_teams''' ===
* Objective
Line 59: existing_assignments is evidently a team! So why is the variable name existing_assignments? It should perhaps be changed to existing_teams.
* Change Comparison
[[File:existing_assignments_comparison.PNG]]

* Change Explanation
The variable 'existing_assignments' was actually a team. To better reflect the purpose of the variable and function of the code, the name was changed from 'existing_assignments' to 'existing_teams'.

=== Add method comment for the '''update method''' ===
* Objective
The update method needs comments about what it is doing.
* Change Comparison (commit 7a9e39)
[[File:E2067_Issue7_Fix.png|1000px]]
* Change Explanation
The '''if''' condition checks if the new team name is NOT already exist, if so then update the team name. The '''elsif''' condition checks if the new team name is the same the current one, if so then nothing is changed. The '''else''' covers the case where the new team name is already in use, then print an error message and not changing the team name.
=== Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?''' ===
* Objective
Line 95: Why isn’t (matching_teams[0].name <=> team.name).zero? just (matching_teams[0].name == team.name)?
* Change Comparison (commit d60d92)
[[File:E2067_Issue8_Fix.png|1000px]]
* Change Explanation
This change is straight forward to satisfy the objective of the task.

=== Add method comment for '''Advertise_for_partners''' and '''remove_advertisement''' ===
* Objective
Line 108: advertise_for_partners needs a method comment. Since its body is only 1 line, does it make sense to have a separate method for this? It makes sense only if having a separate method improves readability. Ditto for the remove_advertisement method at line 112.
* Change Comparison

* Change Explanation

=== Rename '''sign_up''' into '''signup''' ===
* Objective
The name sign_up needs to be signup; it is being used as a noun (sign_up would suggest a verb, the action of signing up).
* Change Comparison (commit e393ba)
[[File:E2067_Issue10_Fix.png|1000px]]
* Change Explanation
I performed a file search on '''sign_up''' and it only appears in the places shown in the screenshot. I replaced them with signup according to the objective.
=== Refactor code from '''remove_participant''' into '''waitlist.rb''' ===
* Objective
remove_participant is generally good code, but handling of waitlists is again model activity, and should be moved to a model class, perhaps waitlist.rb.
* Change Comparison (commit 504f84)
[[File:E2067_Issue11_Fix1.png|1000px]]
[[File:E2067_Issue11_Fix2.png|1000px]]
* Change Explanation
Line 127~142 in '''student_teams_controller.rb''' are moved to Line 14~29 in '''waitlist.rb'''. The only input to the new function '''remove_from_waitlists''' in '''waitlist.rb''' requires '''params[:team_id]''' from '''student_teams_controller.rb'''.
=== Add method comment for the '''review method''' ===
* Objective
Line 169: The review method needs a method comment; its purpose is not clear.
* Change Comparison

* Change Explanation

==Test==

==Future Work==
In the update method of student_teams_controller.rb, the 'elsif' needs a comment to explain its purpose. It is unclear as to why this conditional is present.

CSC/ECE 517 Fall 2020 - E2067. Refactor student teams controller.rb

2020-10-12T17:11:08Z

Dyang23: /* Refactor @teammate_review_allowed into due_date.rb */

==Introduction==
===Background===
The '''student_teams_controller.rb''' controller used in Expertiza to manipulate teams that are created for assignments. Primary functions that this controller provides is create a new team, update team name and delete members from a team.

===Motivation===
The motivation of our work is to improve the maintainability and readability of '''student_teams_controller.rb''' Controller. Furthermore, we also fix several occasions where the '''student_teams_controller.rb''' contains code snippets that actually belongs to the model classes. This will help enforce single responsibility principle and model-view-controller pattern.

===Task Identified===
* Move '''return unless current_user_id? student.user_id''' into '''action_allowed method'''
* Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class'''
* Remove variable '''current_team'''
* Add comment for '''@users_on_waiting_list''' and also simplify the condition
* Refactor '''@teammate_review_allowed''' into '''due_date.rb'''
* Rename '''existing_assignments?''' to '''existing_teams'''
* Add method comment for the '''update method'''
* Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?'''
* Add method comment for '''Advertise_for_partners''' and '''remove_advertisement'''
* Rename '''sign_up''' into '''signup'''
* Refactor code from '''remove_participant''' into '''waitlist.rb'''
* Add method comment for the '''review method'''

===Affected Classes===
*/controllers/student_teams_controller.rb
*/models/waitlist.rb
*/models/due_date.rb

==Code Review==
The purpose of this section is to provide the before and after change comparison so the developer can perform code review to ensure the correctness of the change.

=== Move '''return unless current_user_id? student.user_id''' into '''action_allowed method''' ===
* Objective
The action_allowed method determines whether an action is permissible, depending on the privileges of the user who is performing it. In the view method, there is a clause that says, return unless current_user_id? student.user_id. This needs to be moved to the when 'view' clause in the action_allowed method.
* Change Comparison
''Student_teams_controller.rb'': 
 
[[File:Issue1 fix2.png|1000px]]
[[File:Issue1 fix.png|1000px]]
* Change Explanation
Deleted 'return unless current_user_id? student.user_id' in the view method, and added it to a if clause under the when 'view' clause in the action_allowed method. So that every time before view method is called, this sentence in the action_allowed method will run first.

=== Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class''' ===
* Objective
The code beginning at line 44 deals with due_dates, which are part of the “business logic” and should be moved to an appropriate model method. Thus, @student.assignment.due_dates.each do |due_date| should invoke an appropriate method in due_date.rb. It may or may not be necessary to add a method to the DueDates class.
* Change Comparison
 
''Student_teams_controller.rb'':
[[File:current_due_date_comparison.PNG]]
 
 
''due_date.rb'': 
[[File:due_date_method_added.PNG]]
* Change Explanation
The code in the first image uses logic associated with due dates to assign a variable called '@current_due_date'. This logic was moved to a new method in the model due_dates.rb, shown in the second image. This logic was moved to to simplify the controller but also because the logic is specific to due dates and is more reasonable to be located in the due dates model.

=== Remove variable '''current_team''' ===
* Objective
The variable current_team seems unnecessary; it only saves one character vs. @student.team. Consider removing it.
* Change Comparison
[[File:current_team_change_comparison.PNG]]

* Change Explanation
In the red line 51 above, the variable 'current_team' is used. This variable is unnecessary as it is not significantly shorter or more descriptive. As shown in the changes between red line 53 and green line 49, the 'current_team' variable was removed and replaced with '@student.team'.

=== Add comment for '''@users_on_waiting_list''' and also simplify the condition ===
* Objective
The code on Line 53 (@users_on_waiting_list ...) is not clear at all. Needs at least a comment, and also the condition should not be so complicated.
* Change Comparison
[[File:waiting_list_comment_added.PNG]]

* Change Explanation
The '@users_on_waiting_list' line of code needed a comment to clarify its function. At the suggestion of the teams mentor, the condition was not simplified but instead explained clearly in the comment.

=== Refactor '''@teammate_review_allowed''' into '''due_date.rb''' ===
* Objective
In line 55, the @teammate_review_allowed: condition is way too complex, uses magic constants, and belongs in code in another model class (maybe due_date.rb); no way should it be in the controller!
* Change Comparison
[[File:Issue5 fix.png]]
 
''Student_teams_controller.rb'': 
[[File:Issue5 fix2.png]]

* Change Explanation
Moved '''@teammate_review_allowed''' 's condition to '''due_date.rb''' so the code is more readable.

=== Rename '''existing_assignments?''' to '''existing_teams''' ===
* Objective
Line 59: existing_assignments is evidently a team! So why is the variable name existing_assignments? It should perhaps be changed to existing_teams.
* Change Comparison
[[File:existing_assignments_comparison.PNG]]

* Change Explanation
The variable 'existing_assignments' was actually a team. To better reflect the purpose of the variable and function of the code, the name was changed from 'existing_assignments' to 'existing_teams'.

=== Add method comment for the '''update method''' ===
* Objective
The update method needs comments about what it is doing.
* Change Comparison (commit 7a9e39)
[[File:E2067_Issue7_Fix.png|1000px]]
* Change Explanation
The '''if''' condition checks if the new team name is NOT already exist, if so then update the team name. The '''elsif''' condition checks if the new team name is the same the current one, if so then nothing is changed. The '''else''' covers the case where the new team name is already in use, then print an error message and not changing the team name.
=== Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?''' ===
* Objective
Line 95: Why isn’t (matching_teams[0].name <=> team.name).zero? just (matching_teams[0].name == team.name)?
* Change Comparison (commit d60d92)
[[File:E2067_Issue8_Fix.png|1000px]]
* Change Explanation
This change is straight forward to satisfy the objective of the task.

=== Add method comment for '''Advertise_for_partners''' and '''remove_advertisement''' ===
* Objective
Line 108: advertise_for_partners needs a method comment. Since its body is only 1 line, does it make sense to have a separate method for this? It makes sense only if having a separate method improves readability. Ditto for the remove_advertisement method at line 112.
* Change Comparison

* Change Explanation

=== Rename '''sign_up''' into '''signup''' ===
* Objective
The name sign_up needs to be signup; it is being used as a noun (sign_up would suggest a verb, the action of signing up).
* Change Comparison (commit e393ba)
[[File:E2067_Issue10_Fix.png|1000px]]
* Change Explanation
I performed a file search on '''sign_up''' and it only appears in the places shown in the screenshot. I replaced them with signup according to the objective.
=== Refactor code from '''remove_participant''' into '''waitlist.rb''' ===
* Objective
remove_participant is generally good code, but handling of waitlists is again model activity, and should be moved to a model class, perhaps waitlist.rb.
* Change Comparison (commit 504f84)
[[File:E2067_Issue11_Fix1.png|1000px]]
[[File:E2067_Issue11_Fix2.png|1000px]]
* Change Explanation
Line 127~142 in '''student_teams_controller.rb''' are moved to Line 14~29 in '''waitlist.rb'''. The only input to the new function '''remove_from_waitlists''' in '''waitlist.rb''' requires '''params[:team_id]''' from '''student_teams_controller.rb'''.
=== Add method comment for the '''review method''' ===
* Objective
Line 169: The review method needs a method comment; its purpose is not clear.
* Change Comparison

* Change Explanation

==Test==

==Future Work==
In the update method of student_teams_controller.rb, the 'elsif' needs a comment to explain its purpose. It is unclear as to why this conditional is present.

CSC/ECE 517 Fall 2020 - E2067. Refactor student teams controller.rb

2020-10-12T17:05:13Z

Dyang23: /* Move return unless current_user_id? student.user_id into action_allowed method */

==Introduction==
===Background===
The '''student_teams_controller.rb''' controller used in Expertiza to manipulate teams that are created for assignments. Primary functions that this controller provides is create a new team, update team name and delete members from a team.

===Motivation===
The motivation of our work is to improve the maintainability and readability of '''student_teams_controller.rb''' Controller. Furthermore, we also fix several occasions where the '''student_teams_controller.rb''' contains code snippets that actually belongs to the model classes. This will help enforce single responsibility principle and model-view-controller pattern.

===Task Identified===
* Move '''return unless current_user_id? student.user_id''' into '''action_allowed method'''
* Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class'''
* Remove variable '''current_team'''
* Add comment for '''@users_on_waiting_list''' and also simplify the condition
* Refactor '''@teammate_review_allowed''' into '''due_date.rb'''
* Rename '''existing_assignments?''' to '''existing_teams'''
* Add method comment for the '''update method'''
* Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?'''
* Add method comment for '''Advertise_for_partners''' and '''remove_advertisement'''
* Rename '''sign_up''' into '''signup'''
* Refactor code from '''remove_participant''' into '''waitlist.rb'''
* Add method comment for the '''review method'''

===Affected Classes===
*/controllers/student_teams_controller.rb
*/models/waitlist.rb
*/models/due_date.rb

==Code Review==
The purpose of this section is to provide the before and after change comparison so the developer can perform code review to ensure the correctness of the change.

=== Move '''return unless current_user_id? student.user_id''' into '''action_allowed method''' ===
* Objective
The action_allowed method determines whether an action is permissible, depending on the privileges of the user who is performing it. In the view method, there is a clause that says, return unless current_user_id? student.user_id. This needs to be moved to the when 'view' clause in the action_allowed method.
* Change Comparison
''Student_teams_controller.rb'': 
 
[[File:Issue1 fix2.png|1000px]]
[[File:Issue1 fix.png|1000px]]
* Change Explanation
Deleted 'return unless current_user_id? student.user_id' in the view method, and added it to a if clause under the when 'view' clause in the action_allowed method. So that every time before view method is called, this sentence in the action_allowed method will run first.

=== Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class''' ===
* Objective
The code beginning at line 44 deals with due_dates, which are part of the “business logic” and should be moved to an appropriate model method. Thus, @student.assignment.due_dates.each do |due_date| should invoke an appropriate method in due_date.rb. It may or may not be necessary to add a method to the DueDates class.
* Change Comparison
 
''Student_teams_controller.rb'':
[[File:current_due_date_comparison.PNG]]
 
 
''due_date.rb'': 
[[File:due_date_method_added.PNG]]
* Change Explanation
The code in the first image uses logic associated with due dates to assign a variable called '@current_due_date'. This logic was moved to a new method in the model due_dates.rb, shown in the second image. This logic was moved to to simplify the controller but also because the logic is specific to due dates and is more reasonable to be located in the due dates model.

=== Remove variable '''current_team''' ===
* Objective
The variable current_team seems unnecessary; it only saves one character vs. @student.team. Consider removing it.
* Change Comparison
[[File:current_team_change_comparison.PNG]]

* Change Explanation
In the red line 51 above, the variable 'current_team' is used. This variable is unnecessary as it is not significantly shorter or more descriptive. As shown in the changes between red line 53 and green line 49, the 'current_team' variable was removed and replaced with '@student.team'.

=== Add comment for '''@users_on_waiting_list''' and also simplify the condition ===
* Objective
The code on Line 53 (@users_on_waiting_list ...) is not clear at all. Needs at least a comment, and also the condition should not be so complicated.
* Change Comparison
[[File:waiting_list_comment_added.PNG]]

* Change Explanation
The '@users_on_waiting_list' line of code needed a comment to clarify its function. At the suggestion of the teams mentor, the condition was not simplified but instead explained clearly in the comment.

=== Refactor '''@teammate_review_allowed''' into '''due_date.rb''' ===
* Objective
In line 55, the @teammate_review_allowed: condition is way too complex, uses magic constants, and belongs in code in another model class (maybe due_date.rb); no way should it be in the controller!
* Change Comparison

* Change Explanation

=== Rename '''existing_assignments?''' to '''existing_teams''' ===
* Objective
Line 59: existing_assignments is evidently a team! So why is the variable name existing_assignments? It should perhaps be changed to existing_teams.
* Change Comparison
[[File:existing_assignments_comparison.PNG]]

* Change Explanation
The variable 'existing_assignments' was actually a team. To better reflect the purpose of the variable and function of the code, the name was changed from 'existing_assignments' to 'existing_teams'.

=== Add method comment for the '''update method''' ===
* Objective
The update method needs comments about what it is doing.
* Change Comparison (commit 7a9e39)
[[File:E2067_Issue7_Fix.png|1000px]]
* Change Explanation
The '''if''' condition checks if the new team name is NOT already exist, if so then update the team name. The '''elsif''' condition checks if the new team name is the same the current one, if so then nothing is changed. The '''else''' covers the case where the new team name is already in use, then print an error message and not changing the team name.
=== Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?''' ===
* Objective
Line 95: Why isn’t (matching_teams[0].name <=> team.name).zero? just (matching_teams[0].name == team.name)?
* Change Comparison (commit d60d92)
[[File:E2067_Issue8_Fix.png|1000px]]
* Change Explanation
This change is straight forward to satisfy the objective of the task.

=== Add method comment for '''Advertise_for_partners''' and '''remove_advertisement''' ===
* Objective
Line 108: advertise_for_partners needs a method comment. Since its body is only 1 line, does it make sense to have a separate method for this? It makes sense only if having a separate method improves readability. Ditto for the remove_advertisement method at line 112.
* Change Comparison

* Change Explanation

=== Rename '''sign_up''' into '''signup''' ===
* Objective
The name sign_up needs to be signup; it is being used as a noun (sign_up would suggest a verb, the action of signing up).
* Change Comparison (commit e393ba)
[[File:E2067_Issue10_Fix.png|1000px]]
* Change Explanation
I performed a file search on '''sign_up''' and it only appears in the places shown in the screenshot. I replaced them with signup according to the objective.
=== Refactor code from '''remove_participant''' into '''waitlist.rb''' ===
* Objective
remove_participant is generally good code, but handling of waitlists is again model activity, and should be moved to a model class, perhaps waitlist.rb.
* Change Comparison (commit 504f84)
[[File:E2067_Issue11_Fix1.png|1000px]]
[[File:E2067_Issue11_Fix2.png|1000px]]
* Change Explanation
Line 127~142 in '''student_teams_controller.rb''' are moved to Line 14~29 in '''waitlist.rb'''. The only input to the new function '''remove_from_waitlists''' in '''waitlist.rb''' requires '''params[:team_id]''' from '''student_teams_controller.rb'''.
=== Add method comment for the '''review method''' ===
* Objective
Line 169: The review method needs a method comment; its purpose is not clear.
* Change Comparison

* Change Explanation

==Test==

==Future Work==
In the update method of student_teams_controller.rb, the 'elsif' needs a comment to explain its purpose. It is unclear as to why this conditional is present.

File:Issue5 fix2.png

2020-10-12T17:02:01Z

Dyang23:

File:Issue5 fix.png

2020-10-12T17:01:37Z

Dyang23:

CSC/ECE 517 Fall 2020 - E2067. Refactor student teams controller.rb

2020-10-12T16:55:40Z

Dyang23: /* Move return unless current_user_id? student.user_id into action_allowed method */

==Introduction==
===Background===
The '''student_teams_controller.rb''' controller used in Expertiza to manipulate teams that are created for assignments. Primary functions that this controller provides is create a new team, update team name and delete members from a team.

===Motivation===
The motivation of our work is to improve the maintainability and readability of '''student_teams_controller.rb''' Controller. Furthermore, we also fix several occasions where the '''student_teams_controller.rb''' contains code snippets that actually belongs to the model classes. This will help enforce single responsibility principle and model-view-controller pattern.

===Task Identified===
* Move '''return unless current_user_id? student.user_id''' into '''action_allowed method'''
* Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class'''
* Remove variable '''current_team'''
* Add comment for '''@users_on_waiting_list''' and also simplify the condition
* Refactor '''@teammate_review_allowed''' into '''due_date.rb'''
* Rename '''existing_assignments?''' to '''existing_teams'''
* Add method comment for the '''update method'''
* Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?'''
* Add method comment for '''Advertise_for_partners''' and '''remove_advertisement'''
* Rename '''sign_up''' into '''signup'''
* Refactor code from '''remove_participant''' into '''waitlist.rb'''
* Add method comment for the '''review method'''

===Affected Classes===
*/controllers/student_teams_controller.rb
*/models/waitlist.rb
*/models/due_date.rb

==Code Review==
The purpose of this section is to provide the before and after change comparison so the developer can perform code review to ensure the correctness of the change.

=== Move '''return unless current_user_id? student.user_id''' into '''action_allowed method''' ===
* Objective
The action_allowed method determines whether an action is permissible, depending on the privileges of the user who is performing it. In the view method, there is a clause that says, return unless current_user_id? student.user_id. This needs to be moved to the when 'view' clause in the action_allowed method.
* Change Comparison
''Student_teams_controller.rb'': 
 
[[File:Issue1 fix2.png]]
[[File:Issue1 fix.png]]
* Change Explanation
Deleted 'return unless current_user_id? student.user_id' in the view method, and added it to a if clause under the when 'view' clause in the action_allowed method. So that every time before view method is called, this sentence in the action_allowed method will run first.

=== Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class''' ===
* Objective
The code beginning at line 44 deals with due_dates, which are part of the “business logic” and should be moved to an appropriate model method. Thus, @student.assignment.due_dates.each do |due_date| should invoke an appropriate method in due_date.rb. It may or may not be necessary to add a method to the DueDates class.
* Change Comparison
 
''Student_teams_controller.rb'':
[[File:current_due_date_comparison.PNG]]
 
 
''due_date.rb'': 
[[File:due_date_method_added.PNG]]
* Change Explanation
The code in the first image uses logic associated with due dates to assign a variable called '@current_due_date'. This logic was moved to a new method in the model due_dates.rb, shown in the second image. This logic was moved to to simplify the controller but also because the logic is specific to due dates and is more reasonable to be located in the due dates model.

=== Remove variable '''current_team''' ===
* Objective
The variable current_team seems unnecessary; it only saves one character vs. @student.team. Consider removing it.
* Change Comparison
[[File:current_team_change_comparison.PNG]]

* Change Explanation
In the red line 51 above, the variable 'current_team' is used. This variable is unnecessary as it is not significantly shorter or more descriptive. As shown in the changes between red line 53 and green line 49, the 'current_team' variable was removed and replaced with '@student.team'.

=== Add comment for '''@users_on_waiting_list''' and also simplify the condition ===
* Objective
The code on Line 53 (@users_on_waiting_list ...) is not clear at all. Needs at least a comment, and also the condition should not be so complicated.
* Change Comparison
[[File:waiting_list_comment_added.PNG]]

* Change Explanation
The '@users_on_waiting_list' line of code needed a comment to clarify its function. At the suggestion of the teams mentor, the condition was not simplified but instead explained clearly in the comment.

=== Refactor '''@teammate_review_allowed''' into '''due_date.rb''' ===
* Objective
In line 55, the @teammate_review_allowed: condition is way too complex, uses magic constants, and belongs in code in another model class (maybe due_date.rb); no way should it be in the controller!
* Change Comparison

* Change Explanation

=== Rename '''existing_assignments?''' to '''existing_teams''' ===
* Objective
Line 59: existing_assignments is evidently a team! So why is the variable name existing_assignments? It should perhaps be changed to existing_teams.
* Change Comparison
[[File:existing_assignments_comparison.PNG]]

* Change Explanation
The variable 'existing_assignments' was actually a team. To better reflect the purpose of the variable and function of the code, the name was changed from 'existing_assignments' to 'existing_teams'.

=== Add method comment for the '''update method''' ===
* Objective
The update method needs comments about what it is doing.
* Change Comparison (commit 7a9e39)
[[File:E2067_Issue7_Fix.png|1000px]]
* Change Explanation
The '''if''' condition checks if the new team name is NOT already exist, if so then update the team name. The '''elsif''' condition checks if the new team name is the same the current one, if so then nothing is changed. The '''else''' covers the case where the new team name is already in use, then print an error message and not changing the team name.
=== Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?''' ===
* Objective
Line 95: Why isn’t (matching_teams[0].name <=> team.name).zero? just (matching_teams[0].name == team.name)?
* Change Comparison (commit d60d92)
[[File:E2067_Issue8_Fix.png|1000px]]
* Change Explanation
This change is straight forward to satisfy the objective of the task.

=== Add method comment for '''Advertise_for_partners''' and '''remove_advertisement''' ===
* Objective
Line 108: advertise_for_partners needs a method comment. Since its body is only 1 line, does it make sense to have a separate method for this? It makes sense only if having a separate method improves readability. Ditto for the remove_advertisement method at line 112.
* Change Comparison

* Change Explanation

=== Rename '''sign_up''' into '''signup''' ===
* Objective
The name sign_up needs to be signup; it is being used as a noun (sign_up would suggest a verb, the action of signing up).
* Change Comparison (commit e393ba)
[[File:E2067_Issue10_Fix.png|1000px]]
* Change Explanation
I performed a file search on '''sign_up''' and it only appears in the places shown in the screenshot. I replaced them with signup according to the objective.
=== Refactor code from '''remove_participant''' into '''waitlist.rb''' ===
* Objective
remove_participant is generally good code, but handling of waitlists is again model activity, and should be moved to a model class, perhaps waitlist.rb.
* Change Comparison (commit 504f84)
[[File:E2067_Issue11_Fix1.png|1000px]]
[[File:E2067_Issue11_Fix2.png|1000px]]
* Change Explanation
Line 127~142 in '''student_teams_controller.rb''' are moved to Line 14~29 in '''waitlist.rb'''. The only input to the new function '''remove_from_waitlists''' in '''waitlist.rb''' requires '''params[:team_id]''' from '''student_teams_controller.rb'''.
=== Add method comment for the '''review method''' ===
* Objective
Line 169: The review method needs a method comment; its purpose is not clear.
* Change Comparison

* Change Explanation

==Test==

==Future Work==
In the update method of student_teams_controller.rb, the 'elsif' needs a comment to explain its purpose. It is unclear as to why this conditional is present.

CSC/ECE 517 Fall 2020 - E2067. Refactor student teams controller.rb

2020-10-12T16:55:06Z

Dyang23: /* Move return unless current_user_id? student.user_id into action_allowed method */

==Introduction==
===Background===
The '''student_teams_controller.rb''' controller used in Expertiza to manipulate teams that are created for assignments. Primary functions that this controller provides is create a new team, update team name and delete members from a team.

===Motivation===
The motivation of our work is to improve the maintainability and readability of '''student_teams_controller.rb''' Controller. Furthermore, we also fix several occasions where the '''student_teams_controller.rb''' contains code snippets that actually belongs to the model classes. This will help enforce single responsibility principle and model-view-controller pattern.

===Task Identified===
* Move '''return unless current_user_id? student.user_id''' into '''action_allowed method'''
* Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class'''
* Remove variable '''current_team'''
* Add comment for '''@users_on_waiting_list''' and also simplify the condition
* Refactor '''@teammate_review_allowed''' into '''due_date.rb'''
* Rename '''existing_assignments?''' to '''existing_teams'''
* Add method comment for the '''update method'''
* Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?'''
* Add method comment for '''Advertise_for_partners''' and '''remove_advertisement'''
* Rename '''sign_up''' into '''signup'''
* Refactor code from '''remove_participant''' into '''waitlist.rb'''
* Add method comment for the '''review method'''

===Affected Classes===
*/controllers/student_teams_controller.rb
*/models/waitlist.rb
*/models/due_date.rb

==Code Review==
The purpose of this section is to provide the before and after change comparison so the developer can perform code review to ensure the correctness of the change.

=== Move '''return unless current_user_id? student.user_id''' into '''action_allowed method''' ===
* Objective
The action_allowed method determines whether an action is permissible, depending on the privileges of the user who is performing it. In the view method, there is a clause that says, return unless current_user_id? student.user_id. This needs to be moved to the when 'view' clause in the action_allowed method.
* Change Comparison
''Student_teams_controller.rb'': 
[[File:Issue1 fix2.png]]
[[File:Issue1 fix.png]]
* Change Explanation
Deleted 'return unless current_user_id? student.user_id' in the view method, and added it to a if clause under the when 'view' clause in the action_allowed method. So that every time before view method is called, this sentence in the action_allowed method will run first.

=== Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class''' ===
* Objective
The code beginning at line 44 deals with due_dates, which are part of the “business logic” and should be moved to an appropriate model method. Thus, @student.assignment.due_dates.each do |due_date| should invoke an appropriate method in due_date.rb. It may or may not be necessary to add a method to the DueDates class.
* Change Comparison
 
''Student_teams_controller.rb'':
[[File:current_due_date_comparison.PNG]]
 
 
''due_date.rb'': 
[[File:due_date_method_added.PNG]]
* Change Explanation
The code in the first image uses logic associated with due dates to assign a variable called '@current_due_date'. This logic was moved to a new method in the model due_dates.rb, shown in the second image. This logic was moved to to simplify the controller but also because the logic is specific to due dates and is more reasonable to be located in the due dates model.

=== Remove variable '''current_team''' ===
* Objective
The variable current_team seems unnecessary; it only saves one character vs. @student.team. Consider removing it.
* Change Comparison
[[File:current_team_change_comparison.PNG]]

* Change Explanation
In the red line 51 above, the variable 'current_team' is used. This variable is unnecessary as it is not significantly shorter or more descriptive. As shown in the changes between red line 53 and green line 49, the 'current_team' variable was removed and replaced with '@student.team'.

=== Add comment for '''@users_on_waiting_list''' and also simplify the condition ===
* Objective
The code on Line 53 (@users_on_waiting_list ...) is not clear at all. Needs at least a comment, and also the condition should not be so complicated.
* Change Comparison
[[File:waiting_list_comment_added.PNG]]

* Change Explanation
The '@users_on_waiting_list' line of code needed a comment to clarify its function. At the suggestion of the teams mentor, the condition was not simplified but instead explained clearly in the comment.

=== Refactor '''@teammate_review_allowed''' into '''due_date.rb''' ===
* Objective
In line 55, the @teammate_review_allowed: condition is way too complex, uses magic constants, and belongs in code in another model class (maybe due_date.rb); no way should it be in the controller!
* Change Comparison

* Change Explanation

=== Rename '''existing_assignments?''' to '''existing_teams''' ===
* Objective
Line 59: existing_assignments is evidently a team! So why is the variable name existing_assignments? It should perhaps be changed to existing_teams.
* Change Comparison
[[File:existing_assignments_comparison.PNG]]

* Change Explanation
The variable 'existing_assignments' was actually a team. To better reflect the purpose of the variable and function of the code, the name was changed from 'existing_assignments' to 'existing_teams'.

=== Add method comment for the '''update method''' ===
* Objective
The update method needs comments about what it is doing.
* Change Comparison (commit 7a9e39)
[[File:E2067_Issue7_Fix.png|1000px]]
* Change Explanation
The '''if''' condition checks if the new team name is NOT already exist, if so then update the team name. The '''elsif''' condition checks if the new team name is the same the current one, if so then nothing is changed. The '''else''' covers the case where the new team name is already in use, then print an error message and not changing the team name.
=== Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?''' ===
* Objective
Line 95: Why isn’t (matching_teams[0].name <=> team.name).zero? just (matching_teams[0].name == team.name)?
* Change Comparison (commit d60d92)
[[File:E2067_Issue8_Fix.png|1000px]]
* Change Explanation
This change is straight forward to satisfy the objective of the task.

=== Add method comment for '''Advertise_for_partners''' and '''remove_advertisement''' ===
* Objective
Line 108: advertise_for_partners needs a method comment. Since its body is only 1 line, does it make sense to have a separate method for this? It makes sense only if having a separate method improves readability. Ditto for the remove_advertisement method at line 112.
* Change Comparison

* Change Explanation

=== Rename '''sign_up''' into '''signup''' ===
* Objective
The name sign_up needs to be signup; it is being used as a noun (sign_up would suggest a verb, the action of signing up).
* Change Comparison (commit e393ba)
[[File:E2067_Issue10_Fix.png|1000px]]
* Change Explanation
I performed a file search on '''sign_up''' and it only appears in the places shown in the screenshot. I replaced them with signup according to the objective.
=== Refactor code from '''remove_participant''' into '''waitlist.rb''' ===
* Objective
remove_participant is generally good code, but handling of waitlists is again model activity, and should be moved to a model class, perhaps waitlist.rb.
* Change Comparison (commit 504f84)
[[File:E2067_Issue11_Fix1.png|1000px]]
[[File:E2067_Issue11_Fix2.png|1000px]]
* Change Explanation
Line 127~142 in '''student_teams_controller.rb''' are moved to Line 14~29 in '''waitlist.rb'''. The only input to the new function '''remove_from_waitlists''' in '''waitlist.rb''' requires '''params[:team_id]''' from '''student_teams_controller.rb'''.
=== Add method comment for the '''review method''' ===
* Objective
Line 169: The review method needs a method comment; its purpose is not clear.
* Change Comparison

* Change Explanation

==Test==

==Future Work==
In the update method of student_teams_controller.rb, the 'elsif' needs a comment to explain its purpose. It is unclear as to why this conditional is present.

CSC/ECE 517 Fall 2020 - E2067. Refactor student teams controller.rb

2020-10-12T16:54:45Z

Dyang23: /* Move return unless current_user_id? student.user_id into action_allowed method */

==Introduction==
===Background===
The '''student_teams_controller.rb''' controller used in Expertiza to manipulate teams that are created for assignments. Primary functions that this controller provides is create a new team, update team name and delete members from a team.

===Motivation===
The motivation of our work is to improve the maintainability and readability of '''student_teams_controller.rb''' Controller. Furthermore, we also fix several occasions where the '''student_teams_controller.rb''' contains code snippets that actually belongs to the model classes. This will help enforce single responsibility principle and model-view-controller pattern.

===Task Identified===
* Move '''return unless current_user_id? student.user_id''' into '''action_allowed method'''
* Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class'''
* Remove variable '''current_team'''
* Add comment for '''@users_on_waiting_list''' and also simplify the condition
* Refactor '''@teammate_review_allowed''' into '''due_date.rb'''
* Rename '''existing_assignments?''' to '''existing_teams'''
* Add method comment for the '''update method'''
* Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?'''
* Add method comment for '''Advertise_for_partners''' and '''remove_advertisement'''
* Rename '''sign_up''' into '''signup'''
* Refactor code from '''remove_participant''' into '''waitlist.rb'''
* Add method comment for the '''review method'''

===Affected Classes===
*/controllers/student_teams_controller.rb
*/models/waitlist.rb
*/models/due_date.rb

==Code Review==
The purpose of this section is to provide the before and after change comparison so the developer can perform code review to ensure the correctness of the change.

=== Move '''return unless current_user_id? student.user_id''' into '''action_allowed method''' ===
* Objective
The action_allowed method determines whether an action is permissible, depending on the privileges of the user who is performing it. In the view method, there is a clause that says, return unless current_user_id? student.user_id. This needs to be moved to the when 'view' clause in the action_allowed method.
* Change Comparison

''Student_teams_controller.rb'': 
[[File:Issue1 fix2.png]]
 
[[File:Issue1 fix.png]]
 
* Change Explanation
Deleted 'return unless current_user_id? student.user_id' in the view method, and added it to a if clause under the when 'view' clause in the action_allowed method. So that every time before view method is called, this sentence in the action_allowed method will run first.

=== Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class''' ===
* Objective
The code beginning at line 44 deals with due_dates, which are part of the “business logic” and should be moved to an appropriate model method. Thus, @student.assignment.due_dates.each do |due_date| should invoke an appropriate method in due_date.rb. It may or may not be necessary to add a method to the DueDates class.
* Change Comparison
 
''Student_teams_controller.rb'':
[[File:current_due_date_comparison.PNG]]
 
 
''due_date.rb'': 
[[File:due_date_method_added.PNG]]
* Change Explanation
The code in the first image uses logic associated with due dates to assign a variable called '@current_due_date'. This logic was moved to a new method in the model due_dates.rb, shown in the second image. This logic was moved to to simplify the controller but also because the logic is specific to due dates and is more reasonable to be located in the due dates model.

=== Remove variable '''current_team''' ===
* Objective
The variable current_team seems unnecessary; it only saves one character vs. @student.team. Consider removing it.
* Change Comparison
[[File:current_team_change_comparison.PNG]]

* Change Explanation
In the red line 51 above, the variable 'current_team' is used. This variable is unnecessary as it is not significantly shorter or more descriptive. As shown in the changes between red line 53 and green line 49, the 'current_team' variable was removed and replaced with '@student.team'.

=== Add comment for '''@users_on_waiting_list''' and also simplify the condition ===
* Objective
The code on Line 53 (@users_on_waiting_list ...) is not clear at all. Needs at least a comment, and also the condition should not be so complicated.
* Change Comparison
[[File:waiting_list_comment_added.PNG]]

* Change Explanation
The '@users_on_waiting_list' line of code needed a comment to clarify its function. At the suggestion of the teams mentor, the condition was not simplified but instead explained clearly in the comment.

=== Refactor '''@teammate_review_allowed''' into '''due_date.rb''' ===
* Objective
In line 55, the @teammate_review_allowed: condition is way too complex, uses magic constants, and belongs in code in another model class (maybe due_date.rb); no way should it be in the controller!
* Change Comparison

* Change Explanation

=== Rename '''existing_assignments?''' to '''existing_teams''' ===
* Objective
Line 59: existing_assignments is evidently a team! So why is the variable name existing_assignments? It should perhaps be changed to existing_teams.
* Change Comparison
[[File:existing_assignments_comparison.PNG]]

* Change Explanation
The variable 'existing_assignments' was actually a team. To better reflect the purpose of the variable and function of the code, the name was changed from 'existing_assignments' to 'existing_teams'.

=== Add method comment for the '''update method''' ===
* Objective
The update method needs comments about what it is doing.
* Change Comparison (commit 7a9e39)
[[File:E2067_Issue7_Fix.png|1000px]]
* Change Explanation
The '''if''' condition checks if the new team name is NOT already exist, if so then update the team name. The '''elsif''' condition checks if the new team name is the same the current one, if so then nothing is changed. The '''else''' covers the case where the new team name is already in use, then print an error message and not changing the team name.
=== Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?''' ===
* Objective
Line 95: Why isn’t (matching_teams[0].name <=> team.name).zero? just (matching_teams[0].name == team.name)?
* Change Comparison (commit d60d92)
[[File:E2067_Issue8_Fix.png|1000px]]
* Change Explanation
This change is straight forward to satisfy the objective of the task.

=== Add method comment for '''Advertise_for_partners''' and '''remove_advertisement''' ===
* Objective
Line 108: advertise_for_partners needs a method comment. Since its body is only 1 line, does it make sense to have a separate method for this? It makes sense only if having a separate method improves readability. Ditto for the remove_advertisement method at line 112.
* Change Comparison

* Change Explanation

=== Rename '''sign_up''' into '''signup''' ===
* Objective
The name sign_up needs to be signup; it is being used as a noun (sign_up would suggest a verb, the action of signing up).
* Change Comparison (commit e393ba)
[[File:E2067_Issue10_Fix.png|1000px]]
* Change Explanation
I performed a file search on '''sign_up''' and it only appears in the places shown in the screenshot. I replaced them with signup according to the objective.
=== Refactor code from '''remove_participant''' into '''waitlist.rb''' ===
* Objective
remove_participant is generally good code, but handling of waitlists is again model activity, and should be moved to a model class, perhaps waitlist.rb.
* Change Comparison (commit 504f84)
[[File:E2067_Issue11_Fix1.png|1000px]]
[[File:E2067_Issue11_Fix2.png|1000px]]
* Change Explanation
Line 127~142 in '''student_teams_controller.rb''' are moved to Line 14~29 in '''waitlist.rb'''. The only input to the new function '''remove_from_waitlists''' in '''waitlist.rb''' requires '''params[:team_id]''' from '''student_teams_controller.rb'''.
=== Add method comment for the '''review method''' ===
* Objective
Line 169: The review method needs a method comment; its purpose is not clear.
* Change Comparison

* Change Explanation

==Test==

==Future Work==
In the update method of student_teams_controller.rb, the 'elsif' needs a comment to explain its purpose. It is unclear as to why this conditional is present.

CSC/ECE 517 Fall 2020 - E2067. Refactor student teams controller.rb

2020-10-12T16:54:13Z

Dyang23: /* Move return unless current_user_id? student.user_id into action_allowed method */

==Introduction==
===Background===
The '''student_teams_controller.rb''' controller used in Expertiza to manipulate teams that are created for assignments. Primary functions that this controller provides is create a new team, update team name and delete members from a team.

===Motivation===
The motivation of our work is to improve the maintainability and readability of '''student_teams_controller.rb''' Controller. Furthermore, we also fix several occasions where the '''student_teams_controller.rb''' contains code snippets that actually belongs to the model classes. This will help enforce single responsibility principle and model-view-controller pattern.

===Task Identified===
* Move '''return unless current_user_id? student.user_id''' into '''action_allowed method'''
* Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class'''
* Remove variable '''current_team'''
* Add comment for '''@users_on_waiting_list''' and also simplify the condition
* Refactor '''@teammate_review_allowed''' into '''due_date.rb'''
* Rename '''existing_assignments?''' to '''existing_teams'''
* Add method comment for the '''update method'''
* Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?'''
* Add method comment for '''Advertise_for_partners''' and '''remove_advertisement'''
* Rename '''sign_up''' into '''signup'''
* Refactor code from '''remove_participant''' into '''waitlist.rb'''
* Add method comment for the '''review method'''

===Affected Classes===
*/controllers/student_teams_controller.rb
*/models/waitlist.rb
*/models/due_date.rb

==Code Review==
The purpose of this section is to provide the before and after change comparison so the developer can perform code review to ensure the correctness of the change.

=== Move '''return unless current_user_id? student.user_id''' into '''action_allowed method''' ===
* Objective
The action_allowed method determines whether an action is permissible, depending on the privileges of the user who is performing it. In the view method, there is a clause that says, return unless current_user_id? student.user_id. This needs to be moved to the when 'view' clause in the action_allowed method.
* Change Comparison
 
''Student_teams_controller.rb'': 
[[File:Issue1 fix2.png]]
 
[[File:Issue1 fix.png]]
 
* Change Explanation
Deleted 'return unless current_user_id? student.user_id' in the view method, and added it to a if clause under the when 'view' clause in the action_allowed method. So that every time before view method is called, this sentence in the action_allowed method will run first.

=== Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class''' ===
* Objective
The code beginning at line 44 deals with due_dates, which are part of the “business logic” and should be moved to an appropriate model method. Thus, @student.assignment.due_dates.each do |due_date| should invoke an appropriate method in due_date.rb. It may or may not be necessary to add a method to the DueDates class.
* Change Comparison
 
''Student_teams_controller.rb'':
[[File:current_due_date_comparison.PNG]]
 
 
''due_date.rb'': 
[[File:due_date_method_added.PNG]]
* Change Explanation
The code in the first image uses logic associated with due dates to assign a variable called '@current_due_date'. This logic was moved to a new method in the model due_dates.rb, shown in the second image. This logic was moved to to simplify the controller but also because the logic is specific to due dates and is more reasonable to be located in the due dates model.

=== Remove variable '''current_team''' ===
* Objective
The variable current_team seems unnecessary; it only saves one character vs. @student.team. Consider removing it.
* Change Comparison
[[File:current_team_change_comparison.PNG]]

* Change Explanation
In the red line 51 above, the variable 'current_team' is used. This variable is unnecessary as it is not significantly shorter or more descriptive. As shown in the changes between red line 53 and green line 49, the 'current_team' variable was removed and replaced with '@student.team'.

=== Add comment for '''@users_on_waiting_list''' and also simplify the condition ===
* Objective
The code on Line 53 (@users_on_waiting_list ...) is not clear at all. Needs at least a comment, and also the condition should not be so complicated.
* Change Comparison
[[File:waiting_list_comment_added.PNG]]

* Change Explanation
The '@users_on_waiting_list' line of code needed a comment to clarify its function. At the suggestion of the teams mentor, the condition was not simplified but instead explained clearly in the comment.

=== Refactor '''@teammate_review_allowed''' into '''due_date.rb''' ===
* Objective
In line 55, the @teammate_review_allowed: condition is way too complex, uses magic constants, and belongs in code in another model class (maybe due_date.rb); no way should it be in the controller!
* Change Comparison

* Change Explanation

=== Rename '''existing_assignments?''' to '''existing_teams''' ===
* Objective
Line 59: existing_assignments is evidently a team! So why is the variable name existing_assignments? It should perhaps be changed to existing_teams.
* Change Comparison
[[File:existing_assignments_comparison.PNG]]

* Change Explanation
The variable 'existing_assignments' was actually a team. To better reflect the purpose of the variable and function of the code, the name was changed from 'existing_assignments' to 'existing_teams'.

=== Add method comment for the '''update method''' ===
* Objective
The update method needs comments about what it is doing.
* Change Comparison (commit 7a9e39)
[[File:E2067_Issue7_Fix.png|1000px]]
* Change Explanation
The '''if''' condition checks if the new team name is NOT already exist, if so then update the team name. The '''elsif''' condition checks if the new team name is the same the current one, if so then nothing is changed. The '''else''' covers the case where the new team name is already in use, then print an error message and not changing the team name.
=== Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?''' ===
* Objective
Line 95: Why isn’t (matching_teams[0].name <=> team.name).zero? just (matching_teams[0].name == team.name)?
* Change Comparison (commit d60d92)
[[File:E2067_Issue8_Fix.png|1000px]]
* Change Explanation
This change is straight forward to satisfy the objective of the task.

=== Add method comment for '''Advertise_for_partners''' and '''remove_advertisement''' ===
* Objective
Line 108: advertise_for_partners needs a method comment. Since its body is only 1 line, does it make sense to have a separate method for this? It makes sense only if having a separate method improves readability. Ditto for the remove_advertisement method at line 112.
* Change Comparison

* Change Explanation

=== Rename '''sign_up''' into '''signup''' ===
* Objective
The name sign_up needs to be signup; it is being used as a noun (sign_up would suggest a verb, the action of signing up).
* Change Comparison (commit e393ba)
[[File:E2067_Issue10_Fix.png|1000px]]
* Change Explanation
I performed a file search on '''sign_up''' and it only appears in the places shown in the screenshot. I replaced them with signup according to the objective.
=== Refactor code from '''remove_participant''' into '''waitlist.rb''' ===
* Objective
remove_participant is generally good code, but handling of waitlists is again model activity, and should be moved to a model class, perhaps waitlist.rb.
* Change Comparison (commit 504f84)
[[File:E2067_Issue11_Fix1.png|1000px]]
[[File:E2067_Issue11_Fix2.png|1000px]]
* Change Explanation
Line 127~142 in '''student_teams_controller.rb''' are moved to Line 14~29 in '''waitlist.rb'''. The only input to the new function '''remove_from_waitlists''' in '''waitlist.rb''' requires '''params[:team_id]''' from '''student_teams_controller.rb'''.
=== Add method comment for the '''review method''' ===
* Objective
Line 169: The review method needs a method comment; its purpose is not clear.
* Change Comparison

* Change Explanation

==Test==

==Future Work==
In the update method of student_teams_controller.rb, the 'elsif' needs a comment to explain its purpose. It is unclear as to why this conditional is present.

CSC/ECE 517 Fall 2020 - E2067. Refactor student teams controller.rb

2020-10-12T16:52:19Z

Dyang23: /* Move return unless current_user_id? student.user_id into action_allowed method */

==Introduction==
===Background===
The '''student_teams_controller.rb''' controller used in Expertiza to manipulate teams that are created for assignments. Primary functions that this controller provides is create a new team, update team name and delete members from a team.

===Motivation===
The motivation of our work is to improve the maintainability and readability of '''student_teams_controller.rb''' Controller. Furthermore, we also fix several occasions where the '''student_teams_controller.rb''' contains code snippets that actually belongs to the model classes. This will help enforce single responsibility principle and model-view-controller pattern.

===Task Identified===
* Move '''return unless current_user_id? student.user_id''' into '''action_allowed method'''
* Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class'''
* Remove variable '''current_team'''
* Add comment for '''@users_on_waiting_list''' and also simplify the condition
* Refactor '''@teammate_review_allowed''' into '''due_date.rb'''
* Rename '''existing_assignments?''' to '''existing_teams'''
* Add method comment for the '''update method'''
* Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?'''
* Add method comment for '''Advertise_for_partners''' and '''remove_advertisement'''
* Rename '''sign_up''' into '''signup'''
* Refactor code from '''remove_participant''' into '''waitlist.rb'''
* Add method comment for the '''review method'''

===Affected Classes===
*/controllers/student_teams_controller.rb
*/models/waitlist.rb
*/models/due_date.rb

==Code Review==
The purpose of this section is to provide the before and after change comparison so the developer can perform code review to ensure the correctness of the change.

=== Move '''return unless current_user_id? student.user_id''' into '''action_allowed method''' ===
* Objective
The action_allowed method determines whether an action is permissible, depending on the privileges of the user who is performing it. In the view method, there is a clause that says, return unless current_user_id? student.user_id. This needs to be moved to the when 'view' clause in the action_allowed method.
* Change Comparison
 
''Student_teams_controller.rb'': 
 
[[File:Issue1 fix2.png]]
 
 
[[File:Issue1 fix.png]]
 
* Change Explanation
Deleted 'return unless current_user_id? student.user_id' in the view method, and added it to a if clause under the when 'view' clause in the action_allowed method. So that every time before view method is called, this sentence in the action_allowed method will run first.

=== Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class''' ===
* Objective
The code beginning at line 44 deals with due_dates, which are part of the “business logic” and should be moved to an appropriate model method. Thus, @student.assignment.due_dates.each do |due_date| should invoke an appropriate method in due_date.rb. It may or may not be necessary to add a method to the DueDates class.
* Change Comparison
 
''Student_teams_controller.rb'':
[[File:current_due_date_comparison.PNG]]
 
 
''due_date.rb'': 
[[File:due_date_method_added.PNG]]
* Change Explanation
The code in the first image uses logic associated with due dates to assign a variable called '@current_due_date'. This logic was moved to a new method in the model due_dates.rb, shown in the second image. This logic was moved to to simplify the controller but also because the logic is specific to due dates and is more reasonable to be located in the due dates model.

=== Remove variable '''current_team''' ===
* Objective
The variable current_team seems unnecessary; it only saves one character vs. @student.team. Consider removing it.
* Change Comparison
[[File:current_team_change_comparison.PNG]]

* Change Explanation
In the red line 51 above, the variable 'current_team' is used. This variable is unnecessary as it is not significantly shorter or more descriptive. As shown in the changes between red line 53 and green line 49, the 'current_team' variable was removed and replaced with '@student.team'.

=== Add comment for '''@users_on_waiting_list''' and also simplify the condition ===
* Objective
The code on Line 53 (@users_on_waiting_list ...) is not clear at all. Needs at least a comment, and also the condition should not be so complicated.
* Change Comparison
[[File:waiting_list_comment_added.PNG]]

* Change Explanation
The '@users_on_waiting_list' line of code needed a comment to clarify its function. At the suggestion of the teams mentor, the condition was not simplified but instead explained clearly in the comment.

=== Refactor '''@teammate_review_allowed''' into '''due_date.rb''' ===
* Objective
In line 55, the @teammate_review_allowed: condition is way too complex, uses magic constants, and belongs in code in another model class (maybe due_date.rb); no way should it be in the controller!
* Change Comparison

* Change Explanation

=== Rename '''existing_assignments?''' to '''existing_teams''' ===
* Objective
Line 59: existing_assignments is evidently a team! So why is the variable name existing_assignments? It should perhaps be changed to existing_teams.
* Change Comparison
[[File:existing_assignments_comparison.PNG]]

* Change Explanation
The variable 'existing_assignments' was actually a team. To better reflect the purpose of the variable and function of the code, the name was changed from 'existing_assignments' to 'existing_teams'.

=== Add method comment for the '''update method''' ===
* Objective
The update method needs comments about what it is doing.
* Change Comparison (commit 7a9e39)
[[File:E2067_Issue7_Fix.png|1000px]]
* Change Explanation
The '''if''' condition checks if the new team name is NOT already exist, if so then update the team name. The '''elsif''' condition checks if the new team name is the same the current one, if so then nothing is changed. The '''else''' covers the case where the new team name is already in use, then print an error message and not changing the team name.
=== Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?''' ===
* Objective
Line 95: Why isn’t (matching_teams[0].name <=> team.name).zero? just (matching_teams[0].name == team.name)?
* Change Comparison (commit d60d92)
[[File:E2067_Issue8_Fix.png|1000px]]
* Change Explanation
This change is straight forward to satisfy the objective of the task.

=== Add method comment for '''Advertise_for_partners''' and '''remove_advertisement''' ===
* Objective
Line 108: advertise_for_partners needs a method comment. Since its body is only 1 line, does it make sense to have a separate method for this? It makes sense only if having a separate method improves readability. Ditto for the remove_advertisement method at line 112.
* Change Comparison

* Change Explanation

=== Rename '''sign_up''' into '''signup''' ===
* Objective
The name sign_up needs to be signup; it is being used as a noun (sign_up would suggest a verb, the action of signing up).
* Change Comparison (commit e393ba)
[[File:E2067_Issue10_Fix.png|1000px]]
* Change Explanation
I performed a file search on '''sign_up''' and it only appears in the places shown in the screenshot. I replaced them with signup according to the objective.
=== Refactor code from '''remove_participant''' into '''waitlist.rb''' ===
* Objective
remove_participant is generally good code, but handling of waitlists is again model activity, and should be moved to a model class, perhaps waitlist.rb.
* Change Comparison (commit 504f84)
[[File:E2067_Issue11_Fix1.png|1000px]]
[[File:E2067_Issue11_Fix2.png|1000px]]
* Change Explanation
Line 127~142 in '''student_teams_controller.rb''' are moved to Line 14~29 in '''waitlist.rb'''. The only input to the new function '''remove_from_waitlists''' in '''waitlist.rb''' requires '''params[:team_id]''' from '''student_teams_controller.rb'''.
=== Add method comment for the '''review method''' ===
* Objective
Line 169: The review method needs a method comment; its purpose is not clear.
* Change Comparison

* Change Explanation

==Test==

==Future Work==
In the update method of student_teams_controller.rb, the 'elsif' needs a comment to explain its purpose. It is unclear as to why this conditional is present.

CSC/ECE 517 Fall 2020 - E2067. Refactor student teams controller.rb

2020-10-12T16:50:30Z

Dyang23: /* Move return unless current_user_id? student.user_id into action_allowed method */

==Introduction==
===Background===
The '''student_teams_controller.rb''' controller used in Expertiza to manipulate teams that are created for assignments. Primary functions that this controller provides is create a new team, update team name and delete members from a team.

===Motivation===
The motivation of our work is to improve the maintainability and readability of '''student_teams_controller.rb''' Controller. Furthermore, we also fix several occasions where the '''student_teams_controller.rb''' contains code snippets that actually belongs to the model classes. This will help enforce single responsibility principle and model-view-controller pattern.

===Task Identified===
* Move '''return unless current_user_id? student.user_id''' into '''action_allowed method'''
* Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class'''
* Remove variable '''current_team'''
* Add comment for '''@users_on_waiting_list''' and also simplify the condition
* Refactor '''@teammate_review_allowed''' into '''due_date.rb'''
* Rename '''existing_assignments?''' to '''existing_teams'''
* Add method comment for the '''update method'''
* Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?'''
* Add method comment for '''Advertise_for_partners''' and '''remove_advertisement'''
* Rename '''sign_up''' into '''signup'''
* Refactor code from '''remove_participant''' into '''waitlist.rb'''
* Add method comment for the '''review method'''

===Affected Classes===
*/controllers/student_teams_controller.rb
*/models/waitlist.rb
*/models/due_date.rb

==Code Review==
The purpose of this section is to provide the before and after change comparison so the developer can perform code review to ensure the correctness of the change.

=== Move '''return unless current_user_id? student.user_id''' into '''action_allowed method''' ===
* Objective
The action_allowed method determines whether an action is permissible, depending on the privileges of the user who is performing it. In the view method, there is a clause that says, return unless current_user_id? student.user_id. This needs to be moved to the when 'view' clause in the action_allowed method.
* Change Comparison
 
''Student_teams_controller.rb'':
 
[[File:Issue1 fix2.png]]
 
 
[[File:Issue1 fix.png]]
* Change Explanation
Deleted 'return unless current_user_id? student.user_id' in the view method, and added it to a if clause under the when 'view' clause in the action_allowed method. So that every time before view method is called, this sentence in the action_allowed method will run first.

=== Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class''' ===
* Objective
The code beginning at line 44 deals with due_dates, which are part of the “business logic” and should be moved to an appropriate model method. Thus, @student.assignment.due_dates.each do |due_date| should invoke an appropriate method in due_date.rb. It may or may not be necessary to add a method to the DueDates class.
* Change Comparison
 
''Student_teams_controller.rb'':
[[File:current_due_date_comparison.PNG]]
 
 
''due_date.rb'': 
[[File:due_date_method_added.PNG]]
* Change Explanation
The code in the first image uses logic associated with due dates to assign a variable called '@current_due_date'. This logic was moved to a new method in the model due_dates.rb, shown in the second image. This logic was moved to to simplify the controller but also because the logic is specific to due dates and is more reasonable to be located in the due dates model.

=== Remove variable '''current_team''' ===
* Objective
The variable current_team seems unnecessary; it only saves one character vs. @student.team. Consider removing it.
* Change Comparison
[[File:current_team_change_comparison.PNG]]

* Change Explanation
In the red line 51 above, the variable 'current_team' is used. This variable is unnecessary as it is not significantly shorter or more descriptive. As shown in the changes between red line 53 and green line 49, the 'current_team' variable was removed and replaced with '@student.team'.

=== Add comment for '''@users_on_waiting_list''' and also simplify the condition ===
* Objective
The code on Line 53 (@users_on_waiting_list ...) is not clear at all. Needs at least a comment, and also the condition should not be so complicated.
* Change Comparison
[[File:waiting_list_comment_added.PNG]]

* Change Explanation
The '@users_on_waiting_list' line of code needed a comment to clarify its function. At the suggestion of the teams mentor, the condition was not simplified but instead explained clearly in the comment.

=== Refactor '''@teammate_review_allowed''' into '''due_date.rb''' ===
* Objective
In line 55, the @teammate_review_allowed: condition is way too complex, uses magic constants, and belongs in code in another model class (maybe due_date.rb); no way should it be in the controller!
* Change Comparison

* Change Explanation

=== Rename '''existing_assignments?''' to '''existing_teams''' ===
* Objective
Line 59: existing_assignments is evidently a team! So why is the variable name existing_assignments? It should perhaps be changed to existing_teams.
* Change Comparison
[[File:existing_assignments_comparison.PNG]]

* Change Explanation
The variable 'existing_assignments' was actually a team. To better reflect the purpose of the variable and function of the code, the name was changed from 'existing_assignments' to 'existing_teams'.

=== Add method comment for the '''update method''' ===
* Objective
The update method needs comments about what it is doing.
* Change Comparison (commit 7a9e39)
[[File:E2067_Issue7_Fix.png|1000px]]
* Change Explanation
The '''if''' condition checks if the new team name is NOT already exist, if so then update the team name. The '''elsif''' condition checks if the new team name is the same the current one, if so then nothing is changed. The '''else''' covers the case where the new team name is already in use, then print an error message and not changing the team name.
=== Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?''' ===
* Objective
Line 95: Why isn’t (matching_teams[0].name <=> team.name).zero? just (matching_teams[0].name == team.name)?
* Change Comparison (commit d60d92)
[[File:E2067_Issue8_Fix.png|1000px]]
* Change Explanation
This change is straight forward to satisfy the objective of the task.

=== Add method comment for '''Advertise_for_partners''' and '''remove_advertisement''' ===
* Objective
Line 108: advertise_for_partners needs a method comment. Since its body is only 1 line, does it make sense to have a separate method for this? It makes sense only if having a separate method improves readability. Ditto for the remove_advertisement method at line 112.
* Change Comparison

* Change Explanation

=== Rename '''sign_up''' into '''signup''' ===
* Objective
The name sign_up needs to be signup; it is being used as a noun (sign_up would suggest a verb, the action of signing up).
* Change Comparison (commit e393ba)
[[File:E2067_Issue10_Fix.png|1000px]]
* Change Explanation
I performed a file search on '''sign_up''' and it only appears in the places shown in the screenshot. I replaced them with signup according to the objective.
=== Refactor code from '''remove_participant''' into '''waitlist.rb''' ===
* Objective
remove_participant is generally good code, but handling of waitlists is again model activity, and should be moved to a model class, perhaps waitlist.rb.
* Change Comparison (commit 504f84)
[[File:E2067_Issue11_Fix1.png|1000px]]
[[File:E2067_Issue11_Fix2.png|1000px]]
* Change Explanation
Line 127~142 in '''student_teams_controller.rb''' are moved to Line 14~29 in '''waitlist.rb'''. The only input to the new function '''remove_from_waitlists''' in '''waitlist.rb''' requires '''params[:team_id]''' from '''student_teams_controller.rb'''.
=== Add method comment for the '''review method''' ===
* Objective
Line 169: The review method needs a method comment; its purpose is not clear.
* Change Comparison

* Change Explanation

==Test==

==Future Work==
In the update method of student_teams_controller.rb, the 'elsif' needs a comment to explain its purpose. It is unclear as to why this conditional is present.

CSC/ECE 517 Fall 2020 - E2067. Refactor student teams controller.rb

2020-10-12T16:49:57Z

Dyang23: /* Move return unless current_user_id? student.user_id into action_allowed method */

==Introduction==
===Background===
The '''student_teams_controller.rb''' controller used in Expertiza to manipulate teams that are created for assignments. Primary functions that this controller provides is create a new team, update team name and delete members from a team.

===Motivation===
The motivation of our work is to improve the maintainability and readability of '''student_teams_controller.rb''' Controller. Furthermore, we also fix several occasions where the '''student_teams_controller.rb''' contains code snippets that actually belongs to the model classes. This will help enforce single responsibility principle and model-view-controller pattern.

===Task Identified===
* Move '''return unless current_user_id? student.user_id''' into '''action_allowed method'''
* Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class'''
* Remove variable '''current_team'''
* Add comment for '''@users_on_waiting_list''' and also simplify the condition
* Refactor '''@teammate_review_allowed''' into '''due_date.rb'''
* Rename '''existing_assignments?''' to '''existing_teams'''
* Add method comment for the '''update method'''
* Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?'''
* Add method comment for '''Advertise_for_partners''' and '''remove_advertisement'''
* Rename '''sign_up''' into '''signup'''
* Refactor code from '''remove_participant''' into '''waitlist.rb'''
* Add method comment for the '''review method'''

===Affected Classes===
*/controllers/student_teams_controller.rb
*/models/waitlist.rb
*/models/due_date.rb

==Code Review==
The purpose of this section is to provide the before and after change comparison so the developer can perform code review to ensure the correctness of the change.

=== Move '''return unless current_user_id? student.user_id''' into '''action_allowed method''' ===
* Objective
The action_allowed method determines whether an action is permissible, depending on the privileges of the user who is performing it. In the view method, there is a clause that says, return unless current_user_id? student.user_id. This needs to be moved to the when 'view' clause in the action_allowed method.
* Change Comparison
 
''Student_teams_controller.rb'':
[[File:Issue1 fix2.png]]
 
 
[[File:Issue1 fix.png]]
* Change Explanation
Deleted 'return unless current_user_id? student.user_id' in the view method, and added it to a if clause under the when 'view' clause in the action_allowed method. So that every time before view method is called, this sentence in the action_allowed method will run first.

=== Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class''' ===
* Objective
The code beginning at line 44 deals with due_dates, which are part of the “business logic” and should be moved to an appropriate model method. Thus, @student.assignment.due_dates.each do |due_date| should invoke an appropriate method in due_date.rb. It may or may not be necessary to add a method to the DueDates class.
* Change Comparison
 
''Student_teams_controller.rb'':
[[File:current_due_date_comparison.PNG]]
 
 
''due_date.rb'': 
[[File:due_date_method_added.PNG]]
* Change Explanation
The code in the first image uses logic associated with due dates to assign a variable called '@current_due_date'. This logic was moved to a new method in the model due_dates.rb, shown in the second image. This logic was moved to to simplify the controller but also because the logic is specific to due dates and is more reasonable to be located in the due dates model.

=== Remove variable '''current_team''' ===
* Objective
The variable current_team seems unnecessary; it only saves one character vs. @student.team. Consider removing it.
* Change Comparison
[[File:current_team_change_comparison.PNG]]

* Change Explanation
In the red line 51 above, the variable 'current_team' is used. This variable is unnecessary as it is not significantly shorter or more descriptive. As shown in the changes between red line 53 and green line 49, the 'current_team' variable was removed and replaced with '@student.team'.

=== Add comment for '''@users_on_waiting_list''' and also simplify the condition ===
* Objective
The code on Line 53 (@users_on_waiting_list ...) is not clear at all. Needs at least a comment, and also the condition should not be so complicated.
* Change Comparison
[[File:waiting_list_comment_added.PNG]]

* Change Explanation
The '@users_on_waiting_list' line of code needed a comment to clarify its function. At the suggestion of the teams mentor, the condition was not simplified but instead explained clearly in the comment.

=== Refactor '''@teammate_review_allowed''' into '''due_date.rb''' ===
* Objective
In line 55, the @teammate_review_allowed: condition is way too complex, uses magic constants, and belongs in code in another model class (maybe due_date.rb); no way should it be in the controller!
* Change Comparison

* Change Explanation

=== Rename '''existing_assignments?''' to '''existing_teams''' ===
* Objective
Line 59: existing_assignments is evidently a team! So why is the variable name existing_assignments? It should perhaps be changed to existing_teams.
* Change Comparison
[[File:existing_assignments_comparison.PNG]]

* Change Explanation
The variable 'existing_assignments' was actually a team. To better reflect the purpose of the variable and function of the code, the name was changed from 'existing_assignments' to 'existing_teams'.

=== Add method comment for the '''update method''' ===
* Objective
The update method needs comments about what it is doing.
* Change Comparison (commit 7a9e39)
[[File:E2067_Issue7_Fix.png|1000px]]
* Change Explanation
The '''if''' condition checks if the new team name is NOT already exist, if so then update the team name. The '''elsif''' condition checks if the new team name is the same the current one, if so then nothing is changed. The '''else''' covers the case where the new team name is already in use, then print an error message and not changing the team name.
=== Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?''' ===
* Objective
Line 95: Why isn’t (matching_teams[0].name <=> team.name).zero? just (matching_teams[0].name == team.name)?
* Change Comparison (commit d60d92)
[[File:E2067_Issue8_Fix.png|1000px]]
* Change Explanation
This change is straight forward to satisfy the objective of the task.

=== Add method comment for '''Advertise_for_partners''' and '''remove_advertisement''' ===
* Objective
Line 108: advertise_for_partners needs a method comment. Since its body is only 1 line, does it make sense to have a separate method for this? It makes sense only if having a separate method improves readability. Ditto for the remove_advertisement method at line 112.
* Change Comparison

* Change Explanation

=== Rename '''sign_up''' into '''signup''' ===
* Objective
The name sign_up needs to be signup; it is being used as a noun (sign_up would suggest a verb, the action of signing up).
* Change Comparison (commit e393ba)
[[File:E2067_Issue10_Fix.png|1000px]]
* Change Explanation
I performed a file search on '''sign_up''' and it only appears in the places shown in the screenshot. I replaced them with signup according to the objective.
=== Refactor code from '''remove_participant''' into '''waitlist.rb''' ===
* Objective
remove_participant is generally good code, but handling of waitlists is again model activity, and should be moved to a model class, perhaps waitlist.rb.
* Change Comparison (commit 504f84)
[[File:E2067_Issue11_Fix1.png|1000px]]
[[File:E2067_Issue11_Fix2.png|1000px]]
* Change Explanation
Line 127~142 in '''student_teams_controller.rb''' are moved to Line 14~29 in '''waitlist.rb'''. The only input to the new function '''remove_from_waitlists''' in '''waitlist.rb''' requires '''params[:team_id]''' from '''student_teams_controller.rb'''.
=== Add method comment for the '''review method''' ===
* Objective
Line 169: The review method needs a method comment; its purpose is not clear.
* Change Comparison

* Change Explanation

==Test==

==Future Work==
In the update method of student_teams_controller.rb, the 'elsif' needs a comment to explain its purpose. It is unclear as to why this conditional is present.

CSC/ECE 517 Fall 2020 - E2067. Refactor student teams controller.rb

2020-10-12T16:43:54Z

Dyang23: /* Move return unless current_user_id? student.user_id into action_allowed method */

==Introduction==
===Background===
The '''student_teams_controller.rb''' controller used in Expertiza to manipulate teams that are created for assignments. Primary functions that this controller provides is create a new team, update team name and delete members from a team.

===Motivation===
The motivation of our work is to improve the maintainability and readability of '''student_teams_controller.rb''' Controller. Furthermore, we also fix several occasions where the '''student_teams_controller.rb''' contains code snippets that actually belongs to the model classes. This will help enforce single responsibility principle and model-view-controller pattern.

===Task Identified===
* Move '''return unless current_user_id? student.user_id''' into '''action_allowed method'''
* Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class'''
* Remove variable '''current_team'''
* Add comment for '''@users_on_waiting_list''' and also simplify the condition
* Refactor '''@teammate_review_allowed''' into '''due_date.rb'''
* Rename '''existing_assignments?''' to '''existing_teams'''
* Add method comment for the '''update method'''
* Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?'''
* Add method comment for '''Advertise_for_partners''' and '''remove_advertisement'''
* Rename '''sign_up''' into '''signup'''
* Refactor code from '''remove_participant''' into '''waitlist.rb'''
* Add method comment for the '''review method'''

===Affected Classes===
*/controllers/student_teams_controller.rb
*/models/waitlist.rb
*/models/due_date.rb

==Code Review==
The purpose of this section is to provide the before and after change comparison so the developer can perform code review to ensure the correctness of the change.

=== Move '''return unless current_user_id? student.user_id''' into '''action_allowed method''' ===
* Objective
The action_allowed method determines whether an action is permissible, depending on the privileges of the user who is performing it. In the view method, there is a clause that says, return unless current_user_id? student.user_id. This needs to be moved to the when 'view' clause in the action_allowed method.
* Change Comparison
[[File:Issue1 fix2.png]]
[[File:Issue1 fix.png]]
* Change Explanation
Deleted '''return unless current_user_id? student.user_id''' in the view method, and added it to a if clause under the when 'view' clause in the action_allowed method.

=== Refactor code from '''view function''' into may or may not be necessary to add a method to the '''DueDates class''' ===
* Objective
The code beginning at line 44 deals with due_dates, which are part of the “business logic” and should be moved to an appropriate model method. Thus, @student.assignment.due_dates.each do |due_date| should invoke an appropriate method in due_date.rb. It may or may not be necessary to add a method to the DueDates class.
* Change Comparison
 
''Student_teams_controller.rb'':
[[File:current_due_date_comparison.PNG]]
 
 
''due_date.rb'': 
[[File:due_date_method_added.PNG]]
* Change Explanation
The code in the first image uses logic associated with due dates to assign a variable called '@current_due_date'. This logic was moved to a new method in the model due_dates.rb, shown in the second image. This logic was moved to to simplify the controller but also because the logic is specific to due dates and is more reasonable to be located in the due dates model.

=== Remove variable '''current_team''' ===
* Objective
The variable current_team seems unnecessary; it only saves one character vs. @student.team. Consider removing it.
* Change Comparison
[[File:current_team_change_comparison.PNG]]

* Change Explanation
In the red line 51 above, the variable 'current_team' is used. This variable is unnecessary as it is not significantly shorter or more descriptive. As shown in the changes between red line 53 and green line 49, the 'current_team' variable was removed and replaced with '@student.team'.

=== Add comment for '''@users_on_waiting_list''' and also simplify the condition ===
* Objective
The code on Line 53 (@users_on_waiting_list ...) is not clear at all. Needs at least a comment, and also the condition should not be so complicated.
* Change Comparison
[[File:waiting_list_comment_added.PNG]]

* Change Explanation
The '@users_on_waiting_list' line of code needed a comment to clarify its function. At the suggestion of the teams mentor, the condition was not simplified but instead explained clearly in the comment.

=== Refactor '''@teammate_review_allowed''' into '''due_date.rb''' ===
* Objective
In line 55, the @teammate_review_allowed: condition is way too complex, uses magic constants, and belongs in code in another model class (maybe due_date.rb); no way should it be in the controller!
* Change Comparison

* Change Explanation

=== Rename '''existing_assignments?''' to '''existing_teams''' ===
* Objective
Line 59: existing_assignments is evidently a team! So why is the variable name existing_assignments? It should perhaps be changed to existing_teams.
* Change Comparison
[[File:existing_assignments_comparison.PNG]]

* Change Explanation
The variable 'existing_assignments' was actually a team. To better reflect the purpose of the variable and function of the code, the name was changed from 'existing_assignments' to 'existing_teams'.

=== Add method comment for the '''update method''' ===
* Objective
The update method needs comments about what it is doing.
* Change Comparison (commit 7a9e39)
[[File:E2067_Issue7_Fix.png|1000px]]
* Change Explanation
The '''if''' condition checks if the new team name is NOT already exist, if so then update the team name. The '''elsif''' condition checks if the new team name is the same the current one, if so then nothing is changed. The '''else''' covers the case where the new team name is already in use, then print an error message and not changing the team name.
=== Change '''(matching_teams[0].name <=> team.name).zero?''' to '''(matching_teams[0].name == team.name)?''' ===
* Objective
Line 95: Why isn’t (matching_teams[0].name <=> team.name).zero? just (matching_teams[0].name == team.name)?
* Change Comparison (commit d60d92)
[[File:E2067_Issue8_Fix.png|1000px]]
* Change Explanation
This change is straight forward to satisfy the objective of the task.

=== Add method comment for '''Advertise_for_partners''' and '''remove_advertisement''' ===
* Objective
Line 108: advertise_for_partners needs a method comment. Since its body is only 1 line, does it make sense to have a separate method for this? It makes sense only if having a separate method improves readability. Ditto for the remove_advertisement method at line 112.
* Change Comparison

* Change Explanation

=== Rename '''sign_up''' into '''signup''' ===
* Objective
The name sign_up needs to be signup; it is being used as a noun (sign_up would suggest a verb, the action of signing up).
* Change Comparison (commit e393ba)
[[File:E2067_Issue10_Fix.png|1000px]]
* Change Explanation
I performed a file search on '''sign_up''' and it only appears in the places shown in the screenshot. I replaced them with signup according to the objective.
=== Refactor code from '''remove_participant''' into '''waitlist.rb''' ===
* Objective
remove_participant is generally good code, but handling of waitlists is again model activity, and should be moved to a model class, perhaps waitlist.rb.
* Change Comparison (commit 504f84)
[[File:E2067_Issue11_Fix1.png|1000px]]
[[File:E2067_Issue11_Fix2.png|1000px]]
* Change Explanation
Line 127~142 in '''student_teams_controller.rb''' are moved to Line 14~29 in '''waitlist.rb'''. The only input to the new function '''remove_from_waitlists''' in '''waitlist.rb''' requires '''params[:team_id]''' from '''student_teams_controller.rb'''.
=== Add method comment for the '''review method''' ===
* Objective
Line 169: The review method needs a method comment; its purpose is not clear.
* Change Comparison

* Change Explanation

==Test==

==Future Work==
In the update method of student_teams_controller.rb, the 'elsif' needs a comment to explain its purpose. It is unclear as to why this conditional is present.

File:Issue1 fix2.png

2020-10-12T16:36:28Z

Dyang23:

File:Issue1 fix.png

2020-10-12T16:35:42Z

Dyang23: