CSC/ECE 517 Fall 2015/oss E1560 PSV

2015-11-05T22:11:22Z

Vsubbar: Added line telling how to test from UI

'''E1560. Refactoring PopUpController.rb and ParticipantsController.rb'''

This page provides a description of the Expertiza based OSS project. This project is aimed at refactoring PopUpController.rb and ParticipantsController.rb.

=Introduction to Expertiza=
[http://http://expertiza.ncsu.edu/ Expertiza] is an [http://en.wikipedia.org/wiki/Open-source_software Open Source] [http://rubyonrails.org/ Rails] application which is used by instructors and students for creating assignments and submitting peer reviews. Expertiza allows the instructor to create and customize assignments, create a list of topics the students can sign up for, have students work on teams and then review each other's assignments at the end. The Source code of the application can be cloned from [https://github.com/expertiza/expertiza/ Github].

=Project Desicription<ref>https://docs.google.com/document/d/1uWs3zyrupTmrOFuv5IbVWCF4NRvCXqJmg8dZ0wCqgus/edit</ref>=
'''Files involved:'''
popUpController.rb
participantsController.rb

'''What they do'''
PopUpController displays a review when a user (instructor or student) clicks on a list of reviews.
ParticipantsController handles participants in an assignment.

'''What's wrong with them:'''
PopUpController contains only two prominent methods, but they are very big.
ParticipantsController contains redundant code.

'''What needs to be done:'''
'''1 popUpController.rb'''
1.1 Action_allowed method always returns true. It needs to be fixed.
1.2 Team_user_popup method needs refactoring. @teamId is assigned but never used. Add comments to make code more readable. Rename variables based on their usage
1.3 Refactor Participants_popup and team_users_popup methods into smaller private methods.


'''2 participantsController.rb'''
2.1 In add and update_authorization methods, permissions collection object can be used to reference its elements without assigning each element to individual private variables.
2.2 Inherit and bequeath_all methods are similar. Common statements can be migrated to private method. Add comments in code to make it easy to understand.
2.3 Cluster all the flash messages under one private method to make the code more manageable
2.4 Fix email_sent method. It contains a dummy email address.

=Changes Made<ref>https://github.com/viveksubbarao/expertiza/commits/master</ref>=

=='''PopUpController.rb'''==
=== Modified <code>Action_allowed</code> method ===
Modified <code>Action_allowed</code> method to return true only if...
{| class="wikitable"
|-
! |Before Refactoring
! |After Refactoring
|- style="vertical-align:top;"
|<pre>
def action_allowed?
true
end
</pre>
|<pre>
def action_allowed?
if @allowed_actions.include? params[:action]
true
else
false
end
end
</pre>
|}
=== Refactored <code>Team_user_popup</code> method ===
Removed unused instance variable @teamId
Changed redundant instance variables to local variables
{| class="wikitable"
|-
! |Before Refactoring
! |After Refactoring
|- style="vertical-align:top;"
|<pre>
def team_users_popup
@sum = 0
@count = 0
@teamid = params[:id]
@team = Team.find(params[:id])
@assignment = Assignment.find(@team.parent_id)
@assignment_id = @assignment.id
@id=params[:assignment_id]
# @teamname = Team.find(params[:id]).name
@teamusers = TeamsUser.where(team_id: params[:id])

#id2 seems to be a response_map
if(params[:id2] == nil)
# if(@reviewid == nil)
@scores = nil
else
#get the last response from response_map id
response = Response.where(map_id:params[:id2]).last
@reviewid = response.id
@pid = ResponseMap.find(params[:id2]).reviewer_id
@reviewer_id = Participant.find(@pid).user_id

@scores = Answer.where(response_id: @reviewid)

questionnaire =Response.find(@reviewid).questionnaire_by_answer(@scores.first)
@maxscore = questionnaire.max_question_score

if(@maxscore == nil)
@maxscore = 5
end

@total_percentage = response.get_average_score
@sum = response.get_total_score
@total_possible = response.get_maximum_score
end

# @review_questionnaire = Questionnaire.find(@assignment.review_questionnaire_id)
# @review_questions = @review_questionnaire.questions
#@maxscore = @review_questionnaire.max_question_score

end
</pre>
|<pre>
def team_users_popup
@sum = 0
@team = Team.find(params[:id])
@id = params[:assignment_id]
@teamusers = TeamsUser.where(team_id: params[:id])

puts params
# response_map id can be used to index into the Response
# table and find the desired response.
if (params[:response_map_id] == nil)
@scores = nil
else
#get the last response from response_map id
response = Response.where(map_id: params[:response_map_id]).last
@reviewid = response.id
@pid = ResponseMap.find(params[:response_map_id]).reviewer_id
@reviewer_id = Participant.find(@pid).user_id

@scores = Answer.where(response_id: response.id)
questionnaire = Response.find(@reviewid).questionnaire_by_answer(@scores.first)

if (questionnaire.max_question_score == nil)
@maxscore = 5 # This instance variable is needed. It's used in the team_users_popup view.
end

@total_percentage = response.get_average_score
@sum = response.get_total_score
@total_possible = response.get_maximum_score
end
end
</pre>
|}

=== Refactored Participants_popup and team_users_popup methods into smaller private methods ===
{| class="wikitable"
|-
! |Before Refactoring
! |After Refactoring
|- style="vertical-align:top;"
|<pre>

</pre>
|<pre>

</pre>
|}



=='''ParticipantsController.rb'''==
=== Permissions collection object referenced directly in <code>add</code> and <code>update_authorization methods</code>===
Local variables like can_submit, can_review, etc removed and permissions object referenced directly
{| class="wikitable"
|-
! |Before Refactoring
! |After Refactoring
|- style="vertical-align:top;"
|<pre>
def add
......
permissions = Participant.get_permissions(params[:authorization])
can_submit = permissions[:can_submit]
can_review = permissions[:can_review]
can_take_quiz = permissions[:can_take_quiz]
curr_object.add_participant(params[:user][:name],can_submit,can_review,can_take_quiz)
......
end

def update_authorizations
......
permissions = Participant.get_permissions(params[:authorization])
can_submit = permissions[:can_submit]
can_review = permissions[:can_review]
can_take_quiz = permissions[:can_take_quiz]
participant = Participant.find(params[:id])
parent_id = participant.parent_id
participant.update_attributes(:can_submit => can_submit, :can_review => can_review, :can_take_quiz => can_take_quiz)
......
end
</pre>
|<pre>
def add
......
permissions = Participant.get_permissions(params[:authorization])
curr_object.add_participant(params[:user][:name], permissions[:can_submit],
permissions[:can_review], permissions[:can_take_quiz])
......
end

def update_authorizations
......
permissions = Participant.get_permissions(params[:authorization])
participant = Participant.find(params[:id])
parent_id = participant.parent_id
participant.update_attributes(:can_submit => permissions[:can_submit],
:can_review => permissions[:can_review], :can_take_quiz => permissions[:can_take_quiz])
......
end
</pre>
|}
=== Moved common functionality in <code>Inherit</code> and <code>bequeath_all</code> to private methods ===
Moved common functionality of <code>Inherit</code> and <code>bequeath_all</code> to private method <code>populate_copied_participants</code>
{| class="wikitable"
|-
! |Before Refactoring
! |After Refactoring
|- style="vertical-align:top;"
|<pre>
def inherit
assignment = Assignment.find(params[:id])
course = assignment.course
@copied_participants = []

if course
participants = course.participants
if participants.length > 0
participants.each{|participant|
new_participant = participant.copy(params[:id])

if new_participant
@copied_participants.push new_participant
end
}

# Only display undo link if copies of participants are created
if @copied_participants.length > 0
undo_link("Participants from \"#{course.name}\" has been copied to this assignment successfully. ")
else
flash[:note] = 'All course participants are already in this assignment'
end

else
flash[:note] = "No participants were found to inherit."
end
else
flash[:error] = "No course was found for this assignment."
end

redirect_to :controller => 'participants', :action => 'list', :id => assignment.id, :model => 'Assignment'
end

def bequeath_all
@copied_participants = []
assignment = Assignment.find(params[:id])
if assignment.course
course = assignment.course
assignment.participants.each{ |participant|
new_participant = participant.copy(course.id)

if new_participant
@copied_participants.push new_participant
end
}
# only display undo link if copies of participants are created
if @copied_participants.length > 0
undo_link("All participants were successfully copied to \"#{course.name}\". " )
else
flash[:note] = 'All assignment participants are already part of the course'
end

#flash[:note] = "All participants were successfully copied to \""+course.name+"\""
else
flash[:error] = "This assignment is not associated with a course."
end

redirect_to :controller => 'participants', :action => 'list', :id => assignment.id, :model => 'Assignment'
end
</pre>
|<pre>
def inherit
assignment = Assignment.find(params[:id])

if assignment.course
if assignment.course.participants.length > 0
@copied_participants = populate_copied_participants(assignment.courses.participants, params[:id])
if @copied_participants.length > 0
undo_link("Participants from \"#{course.name}\" have been copied to this assignment successfully. ")
else
flash[:note] = 'All course participants are already in this assignment'
end
else
flash[:note] = "No participants were found to inherit."
end
else
flash[:error] = "No course was found for this assignment."
end

redirect_to :controller => 'participants', :action => 'list', :id => assignment.id, :model => 'Assignment'
end

def bequeath_all
assignment = Assignment.find(params[:id])

if assignment.course
@copied_participants = populate_copied_participants(assignment.participants, assignment.course.id)
if @copied_participants.length > 0
undo_link("All participants were successfully copied to \"#{course.name}\". ")
else
flash[:note] = 'All assignment participants are already part of the course'
end
else
flash[:error] = "This assignment is not associated with a course."
end

redirect_to :controller => 'participants', :action => 'list', :id => assignment.id, :model => 'Assignment'
end

def populate_copied_participants(participants, val)
@copied_participants = []

participants.each do |participant|
new_participant = participant.copy(val)
if new_participant
@copied_participants.push new_participant
end
end

return @copied_participants
end
</pre>
|}
=== Flash messages clustered under a single private method ===
{| class="wikitable"
|-
! |Before Refactoring
! |After Refactoring
|- style="vertical-align:top;"
|<pre>

</pre>
|<pre>

</pre>
|}
=== Fixed <code>email_sent</code> method ===
{| class="wikitable"
|-
! |Before Refactoring
! |After Refactoring
|- style="vertical-align:top;"
|<pre>

</pre>
|<pre>

</pre>
|}

= How to test from UI =
Login as a instructor "instructor6/password". Click on Assignments -> review report -> and on a team
= References=
<references/>

Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Fall 2015/oss E1560 PSV