Expertiza_Wiki - User contributions [en]

E1908 signupsheet

2019-03-26T02:01:11Z

Spadala:

==E1908. Refactoring the Sign-up sheet Controller==

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

__TOC__

===About Expertiza===
[http://expertiza.ncsu.edu/Expertiza] is an open source project dependent on [http://rubyonrails.org/Ruby on Rails] structure. Expertiza enables the teacher to make new assignments and alter new or existing assignments. It additionally enables the educator to make a rundown of subjects the students can agree to accept. Students can shape groups in Expertiza to chip away at different undertakings and projects. Students can likewise peer audit other students' entries. Expertiza underpins accommodation crosswise over different record types, including the URLs and wiki pages.

===Problem Statement===
The following tasks were accomplished in this project:

* Improved the clarity of code by improving the variable and parameter names.
* Followed naming conventions throughout and renamed methods with inconsistent names including the calling methods.
* Rectified several unwanted if-else conditions in methods and optimized the code.
* Refactored all instance variables and removed unnecessarily defined variables.
* Removed certain unwanted flash messages that occur for some user actions.
* Included comments for functionalities throughout for better understanding.

===About Sign-up sheet Controller===
Sign-up sheet controller contains all functions related to management of the signup sheet for an assignment function to add new topics to an assignment, edit properties of a particular topic, delete a topic, etc are included here.

=====Drawbacks and Solutions=====

* '''Problem 1''': Create method has an if-else condition determining if create or update should be called. Create method should not be responsible for calling update. Identify why the if-else condition exists. The if-else condition exists because the current implementation calls update if a signup sheet with the same name already exists.
* '''Solution''': Rectified this method by removing the call to update and flashing an error instead.
<pre>

if topic.nil?
setup_new_topic
else
update_existing_topic topic
flash[:error] = "The topic already exists."
end
end

</pre>

* '''Problem 2''': Update method has a plethora of instance variables defined before updating. These are not necessary (For e.g., look at update method of bookmarks_controller).
* '''Solution''': Refactored the variables not needed out.
<pre>

update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
# Replaced unnecessary variables and save with a single update call for all the parameters

</pre>

* '''Problem 3''': Destroy has a misleading else flash message.
* '''Solution''': Refactored the mislleading flash messages not needed out.
<pre>

@topic.destroy
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully deleted. ")
else
flash[:error] = "The topic could not be deleted."
flash[:error] = "This topic could not be found." # error message mage more specific
end
# changing the redirection url to topics tab in edit assignment view.
redirect_to edit_assignment_path(params[:assignment_id]) + "#tabs-5"

</pre>

* '''Problem 4''': Add_signup_topics_staggered does not do anything.
* '''Solution''': Renamed participants variable to 'teams'.
<pre>
#add_signup_topics_staggered calls add_signup_topics and does nothing else.
def add_signup_topics_staggered
add_signup_topics
end
</pre>

* '''Problem 5''': Several method names are renamed to be more intuitive.
* '''Solution''': load_add_signup_topics is renamed to get_assignment_data and ad_info is renamed to get_ad.
<pre>
# Contains links that let an admin or Instructor edit, delete, view enrolled/waitlisted members for each topic
# Also contains links to delete topics and modify the deadlines for individual topics. Staggered means that different topics can have different deadlines.
def add_signup_topics
load_add_signup_topics(params[:id])
get_assignment_data(params[:id])
SignUpSheet.add_signup_topic(params[:id])
end

@@ -109,7 +109,7 @@ def add_signup_topics_staggered
end

# retrieves all the data associated with the given assignment. Includes all topics,
def load_add_signup_topics(assignment_id)
def get_assignment_data(assignment_id)
@id = assignment_id
@sign_up_topics = SignUpTopic.where('assignment_id = ?', assignment_id)
@slots_filled = SignUpTopic.find_slots_filled(assignment_id)
@@ -374,7 +374,7 @@ def save_topic_deadlines
# This method is called when a student click on the trumpet icon. So this is a bad method name. --Yang
def show_team
if !(assignment = Assignment.find(params[:assignment_id])).nil? and !(topic = SignUpTopic.find(params[:id])).nil?
@results = ad_info(assignment.id, topic.id)
@results = get_ad(assignment.id, topic.id)
@results.each do |result|
result.keys.each do |key|
@current_team_name = result[key] if key.equal? :name
</pre>

* '''Problem 6''': The list method is too long and is sparsely commented.
* '''Solution''': Added comments.
<pre>

# function to list all topics and bids to a participant
def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
@@ -155,6 +156,8 @@ def list
@max_team_size = @assignment.max_team_size
team_id = @participant.team.try(:id)

# If the assignment supports bidding, add all the bids of an
# individual or team to the list of signed topics
if @assignment.is_intelligent
@bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
signed_up_topics = []

</pre>

* '''Problem 7''': What are the differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed. Provide comments as to what each method does.
* '''Solution''': Removed metod which is not required

<pre>

end

# routes to new page to specficy student
# Its just to specify the student, its a get request. Other than that it doesnt do anything.
def signup_as_instructor; end

# its an action that usually follows the aove action, its a post request to signup the student.
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user

</pre>

* '''Problem 8''': Participants variable in load_add_signup_topics actually means teams that signed up for a topic.
* '''Solution''': Renamed participants variable to 'teams'.
<pre>
# to treat all assignments as team assignments
# Though called participants, @participants are actually records in signed_up_teams table, which
# is a mapping table between teams and topics (waitlisted recored are also counted)
@participants = SignedUpTeam.find_team_participants(assignment_id, session[:ip])
@teams = SignedUpTeam.find_team_participants(assignment_id, session[:ip])
end
</pre>

* '''Problem 9''': Signup_as_instructor_action has if-else ladder.
* '''Solution''': It has been made more elegant using a helper function.

<pre>

def signup_as_instructor; end

def signup_student user
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
end

def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
end
if !user.nil? and AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
signup_student(user);
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end

# Checks if participant has permission to delete a topic, reports errors otherwise
def can_delete_topic?(is_instructor?, participant, assignment, drop_topic_deadline)
def can_delete_topic? is_instructor, participant, assignment, drop_topic_deadline
submission_error_message = ""
deadline_error_message = ""

@@ -237,7 +239,7 @@ def can_delete_topic?(is_instructor?, participant, assignment, drop_topic_deadli
else
submission_error_message = "You have already submitted your work, so you are not allowed to drop your topic."
deadline_error_message = "You cannot drop your topic after the drop topic deadline!"

end
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = submission_error_message
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
@@ -246,7 +248,7 @@ def can_delete_topic?(is_instructor?, participant, assignment, drop_topic_deadli
flash[:error] = deadline_error_message
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
return false

end
return true
end

@@ -274,7 +276,6 @@ def delete_signup_as_instructor
user = TeamsUser.find_by(team_id: team.id).user
participant = AssignmentParticipant.find_by(user_id: user.id, parent_id: assignment.id)
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)

if can_delete_topic?(true, participant, assignment, drop_topic_deadline)
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"
@@ -481,4 +482,4 @@ def ad_info(_assignment_id, topic_id)
def delete_signup_for_topic(assignment_id, topic_id, user_id)
SignUpTopic.reassign_topic(user_id, assignment_id, topic_id)
end
end
end

</pre>

* '''Problem 10''': Delete_signup and delete_signup_as_instructor have much in common and violates the DRY principle.
* '''Solution''': Refactored them by moving the duplicate code to a helper function.
<pre>

def can_delete_topic?(is_instructor?, participant, assignment, drop_topic_deadline)
submission_error_message = ""
deadline_error_message = ""

if is_instructor?
submission_error_message = "The student has already submitted their work, so you are not allowed to remove them"
deadline_error_message = "You cannot drop a student after the drop topic deadline!"
else
submission_error_message = "You have already submitted your work, so you are not allowed to drop your topic."
deadline_error_message = "You cannot drop your topic after the drop topic deadline!"

if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = submission_error_message
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
return false
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = deadline_error_message
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
return false

return true
end

# this function is used to delete a previous signup
def delete_signup
participant = AssignmentParticipant.find(params[:id])
@@ -235,13 +259,7 @@ def delete_signup
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
if can_delete_topic?(false, participant, assignment, drop_topic_deadline)
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
@@ -256,13 +274,8 @@ def delete_signup_as_instructor
user = TeamsUser.find_by(team_id: team.id).user
participant = AssignmentParticipant.find_by(user_id: user.id, parent_id: assignment.id)
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop a student after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
else

if can_delete_topic?(true, participant, assignment, drop_topic_deadline)
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"

</pre>

===References===

#https://github.com/expertiza/expertiza
#http://expertiza.ncsu.edu/ The live Expertiza website

E1908 signupsheet

2019-03-26T01:37:17Z

Spadala:

==E1908. Refactoring the Sign-up sheet Controller==

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

__TOC__

===About Expertiza===
[http://expertiza.ncsu.edu/Expertiza] is an open source project dependent on [http://rubyonrails.org/Ruby on Rails] structure. Expertiza enables the teacher to make new assignments and alter new or existing assignments. It additionally enables the educator to make a rundown of subjects the students can agree to accept. Students can shape groups in Expertiza to chip away at different undertakings and projects. Students can likewise peer audit other students' entries. Expertiza underpins accommodation crosswise over different record types, including the URLs and wiki pages.

===Problem Statement===
The following tasks were accomplished in this project:

* Improved the clarity of code by improving the variable and parameter names.
* Followed naming conventions throughout and renamed methods with inconsistent names including the calling methods.
* Rectified several unwanted if-else conditions in methods and optimized the code.
* Refactored all instance variables and removed unnecessarily defined variables.
* Removed certain unwanted flash messages that occur for some user actions.
* Included comments for functionalities throughout for better understanding.

===About Sign-up sheet Controller===
Sign-up sheet controller contains all functions related to management of the signup sheet for an assignment function to add new topics to an assignment, edit properties of a particular topic, delete a topic, etc are included here.

=====Drawbacks and Solutions=====

* '''Problem 1''': Create method has an if-else condition determining if create or update should be called. Create method should not be responsible for calling update. Identify why the if-else condition exists. The if-else condition exists because the current implementation calls update if a signup sheet with the same name already exists.
* '''Solution''': Rectified this method by removing the call to update and flashing an error instead.
<pre>

</pre>

* '''Problem 2''': Update method has a plethora of instance variables defined before updating. These are not necessary (For e.g., look at update method of bookmarks_controller).
* '''Solution''': Refactored the variables not needed out.
<pre>

</pre>

* '''Problem 4''': The list method is too long and is sparsely commented.
* '''Solution''': Added comments.
<pre>

# function to list all topics and bids to a participant
def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
@@ -155,6 +156,8 @@ def list
@max_team_size = @assignment.max_team_size
team_id = @participant.team.try(:id)

# If the assignment supports bidding, add all the bids of an
# individual or team to the list of signed topics
if @assignment.is_intelligent
@bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
signed_up_topics = []

</pre>

* '''Problem 5''': Add_signup_topics_staggered does not do anything.
* '''Solution''': Renamed participants variable to 'teams'.
<pre>
#add_signup_topics_staggered calls add_signup_topics and does nothing else.
def add_signup_topics_staggered
add_signup_topics
end
</pre>

* '''Problem 6''': Several method names are renamed to be more intuitive.
* '''Solution''': load_add_signup_topics is renamed to get_assignment_data and ad_info is renamed to get_ad.
<pre>
# Contains links that let an admin or Instructor edit, delete, view enrolled/waitlisted members for each topic
# Also contains links to delete topics and modify the deadlines for individual topics. Staggered means that different topics can have different deadlines.
def add_signup_topics
load_add_signup_topics(params[:id])
get_assignment_data(params[:id])
SignUpSheet.add_signup_topic(params[:id])
end

@@ -109,7 +109,7 @@ def add_signup_topics_staggered
end

# retrieves all the data associated with the given assignment. Includes all topics,
def load_add_signup_topics(assignment_id)
def get_assignment_data(assignment_id)
@id = assignment_id
@sign_up_topics = SignUpTopic.where('assignment_id = ?', assignment_id)
@slots_filled = SignUpTopic.find_slots_filled(assignment_id)
@@ -374,7 +374,7 @@ def save_topic_deadlines
# This method is called when a student click on the trumpet icon. So this is a bad method name. --Yang
def show_team
if !(assignment = Assignment.find(params[:assignment_id])).nil? and !(topic = SignUpTopic.find(params[:id])).nil?
@results = ad_info(assignment.id, topic.id)
@results = get_ad(assignment.id, topic.id)
@results.each do |result|
result.keys.each do |key|
@current_team_name = result[key] if key.equal? :name
</pre>

* '''Problem 9''': Participants variable in load_add_signup_topics actually means teams that signed up for a topic.
* '''Solution''': Renamed participants variable to 'teams'.
<pre>
# to treat all assignments as team assignments
# Though called participants, @participants are actually records in signed_up_teams table, which
# is a mapping table between teams and topics (waitlisted recored are also counted)
@participants = SignedUpTeam.find_team_participants(assignment_id, session[:ip])
@teams = SignedUpTeam.find_team_participants(assignment_id, session[:ip])
end
</pre>

* '''Problem 10''': Signup_as_instructor_action has if-else ladder.
* '''Solution''': It has been made more elegant using a helper function.

<pre>

def signup_as_instructor; end

def signup_student user
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
end

def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
end
if !user.nil? and AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
signup_student(user);
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end

# Checks if participant has permission to delete a topic, reports errors otherwise
def can_delete_topic?(is_instructor?, participant, assignment, drop_topic_deadline)
def can_delete_topic? is_instructor, participant, assignment, drop_topic_deadline
submission_error_message = ""
deadline_error_message = ""

@@ -237,7 +239,7 @@ def can_delete_topic?(is_instructor?, participant, assignment, drop_topic_deadli
else
submission_error_message = "You have already submitted your work, so you are not allowed to drop your topic."
deadline_error_message = "You cannot drop your topic after the drop topic deadline!"

end
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = submission_error_message
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
@@ -246,7 +248,7 @@ def can_delete_topic?(is_instructor?, participant, assignment, drop_topic_deadli
flash[:error] = deadline_error_message
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
return false

end
return true
end

@@ -274,7 +276,6 @@ def delete_signup_as_instructor
user = TeamsUser.find_by(team_id: team.id).user
participant = AssignmentParticipant.find_by(user_id: user.id, parent_id: assignment.id)
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)

if can_delete_topic?(true, participant, assignment, drop_topic_deadline)
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"
@@ -481,4 +482,4 @@ def ad_info(_assignment_id, topic_id)
def delete_signup_for_topic(assignment_id, topic_id, user_id)
SignUpTopic.reassign_topic(user_id, assignment_id, topic_id)
end
end
end

</pre>

* '''Problem 11''': Delete_signup and delete_signup_as_instructor have much in common and violates the DRY principle.
* '''Solution''': Refactored them by moving the duplicate code to a helper function.
<pre>

def can_delete_topic?(is_instructor?, participant, assignment, drop_topic_deadline)
submission_error_message = ""
deadline_error_message = ""

if is_instructor?
submission_error_message = "The student has already submitted their work, so you are not allowed to remove them"
deadline_error_message = "You cannot drop a student after the drop topic deadline!"
else
submission_error_message = "You have already submitted your work, so you are not allowed to drop your topic."
deadline_error_message = "You cannot drop your topic after the drop topic deadline!"

if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = submission_error_message
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
return false
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = deadline_error_message
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
return false

return true
end

# this function is used to delete a previous signup
def delete_signup
participant = AssignmentParticipant.find(params[:id])
@@ -235,13 +259,7 @@ def delete_signup
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
if can_delete_topic?(false, participant, assignment, drop_topic_deadline)
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
@@ -256,13 +274,8 @@ def delete_signup_as_instructor
user = TeamsUser.find_by(team_id: team.id).user
participant = AssignmentParticipant.find_by(user_id: user.id, parent_id: assignment.id)
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop a student after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
else

if can_delete_topic?(true, participant, assignment, drop_topic_deadline)
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"

</pre>

===References===

#https://github.com/expertiza/expertiza
#http://expertiza.ncsu.edu/ The live Expertiza website

E1908 signupsheet

2019-03-26T01:35:01Z

Spadala:

E1908 signupsheet

2019-03-26T01:29:39Z

Spadala:

E1908 signupsheet

2019-03-26T01:25:44Z

Spadala:

E1908 signupsheet

2019-03-26T01:07:37Z

Spadala:

E1908 signupsheet

2019-03-26T01:05:34Z

Spadala:

E1908 signupsheet

2019-03-26T00:19:45Z

Spadala:

E1908 signupsheet

2019-03-25T23:55:15Z

Spadala:

==E1908. Refactoring the Sign-up sheet Controller==

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

__TOC__

===About Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

===Problem Statement===
The following tasks were accomplished in this project:

* Improved the clarity of code by improving the variable and parameter names.
* Followed naming conventions throughout and renamed methods with inconsistent names including the calling methods.
* Rectified several unwanted if-else conditions in methods and optimized the code.
* Refactored all instance variables and removed unnecessarily defined variables.
*
* Added missing CRUD methods to Versions Controller
* Added RSPEC test cases for testing changes done in Versions Controller

===About Versions Controller===
This class manages different versions of reviews. If a reviewer reviews a submission, and after that, the author revises the submission, the next time the reviewer does a review (s)he will create a new version. Sometimes it’s necessary to find the current version of a review; sometimes it’s necessary to find all versions. Similarly, a user may want to delete the current version of a review, or all versions of a review. Pagination of versions helps the user to view a subset of versions at a time. Considering the huge number of versions in the system, it is very useful to have a pagination mechanism and a filtering mechanism which can be applied on the whole set of versions. The idea is to display the versions in an ordered, comprehensible and logical manner. In Expertiza the gem ‘will_paginate’ is used to achieve pagination.

===Current Implementation===

=====Functionality=====
* Any user irrespective of his/ her privileges can view all the versions.
::The versions which a particular user can view should be restricted based on the privileges of the user. For instance, only a user with Administrator privileges should be able to view all the versions in the system. However, that is not the case now. Every user can view all the versions irrespective of whether the user is a student or an administrator.
* Any user can delete any version
::The versions which a particular user can delete should be restricted based on the privileges of the user. For instance, a student should not be allowed to delete any version. According to the current implementation any user can delete any version in the system.
* Filtering of versions were restricted to the current user
::The filtering options on versions were restricted to the current user. Sometimes a user might want to view versions associated with other users. For instance, an instructor might want to view the list of versions created by a particular student. This is not possible with the current implementation.

=====Drawbacks and Solutions=====
* '''Problem 1''': The method paginate_list is doing more than one thing.
::The method paginate_list was building a complex search criteria based on the input params, getting the list of versions from the Database matching this search criteria and then calling the Page API. All these tasks in a single method made it difficult to understand.
<pre>
# For filtering the versions list with proper search and pagination.
def paginate_list(id, user_id, item_type, event, datetime)
# Set up the search criteria
criteria = ''
criteria = criteria + "id = #{id} AND " if id && id.to_i > 0
if current_user_role? == 'Super-Administrator'
criteria = criteria + "whodunnit = #{user_id} AND " if user_id && user_id.to_i > 0
end
criteria = criteria + "whodunnit = #{current_user.try(:id)} AND " if current_user.try(:id) && current_user.try(:id).to_i > 0
criteria = criteria + "item_type = '#{item_type}' AND " if item_type && !(item_type.eql? 'Any')
criteria = criteria + "event = '#{event}' AND " if event && !(event.eql? 'Any')
criteria = criteria + "created_at >= '#{time_to_string(params[:start_time])}' AND "
criteria = criteria + "created_at <= '#{time_to_string(params[:end_time])}' AND "

if current_role == 'Instructor' || current_role == 'Administrator'

end

# Remove the last ' AND '
criteria = criteria[0..-5]

versions = Version.page(params[:page]).order('id').per_page(25).where(criteria)
versions
end
</pre>
* '''Solution''': The implementation has been changed in such a way that the versions which a user is allowed to see depends on the privileges of the user. The approach we have taken is as follows:
**An administrator can see all the versions
**An instructor can see all the versions created by him and other users who are in his course or are participants in the assignments he creates.
**A TA can see all the versions created by him and other users who are in the course for which he/ she assists.
**A Student can see all the versions created by him/ her.
* '''Problem 2''': The search criteria created in the method paginate_list was difficult to comprehend.
::The code which builds the search criteria in the method paginate_list uses many string literals and conditions and is hardly intuitive. The programmer will have to spend some time to understand what the code is really doing.
* '''Solution''': The implementation has been changed. A student is not allowed to delete any versions now. Other types of users, for instance administrators, instructors and TAs are allowed to delete only the versions they are authorized to view.
* '''Problem 3''': The paginate method can be moved to a helper class.
::VersionsController is not the only component which require to paginate items. There are other components too. For instance, the UsersController has to paginate the list of users. Hence the Paginate method can be moved to a helper class which can be accessed by other components as well.
* '''Solution''': The filtering options has also been enhanced. The current user can now choose as part of the version search filter any user from a list of users if the current user is authorized to see the versions created by that user.

===New Implementation===
*The method paginate_list has been split into 2 methods now.
** BuildSearchCriteria – as the name suggests the sole purpose of this method is to build a search criteria based on the input search filters when the current user initiates a search in versions.
** paginate_list – this method will call the paginate API.
:First the search criteria is built, then the criteria is applied to versions in the database to get all versions which matches the criteria and then the retrieved versions are paginated.
<pre>
# pagination.
def paginate_list(versions)
paginate(versions, VERSIONS_PER_PAGE);
end

def BuildSearchCriteria(id, user_id, item_type, event)
# Set up the search criteria
search_criteria = ''
search_criteria = search_criteria + add_id_filter_if_valid(id).to_s
if current_user_role? == 'Super-Administrator'
search_criteria = search_criteria + add_user_filter_for_super_admin(user_id).to_s
end
search_criteria = search_criteria + add_user_filter
search_criteria = search_criteria + add_version_type_filter(item_type).to_s
search_criteria = search_criteria + add_event_filter(event).to_s
search_criteria = search_criteria + add_date_time_filter
search_criteria
end
</pre>
* The string literals and conditions in the method paginate_list were replaced with methods with intuitive names so that the programmer can understand the code more easily. We also removed an empty if clause and a redundant statement.
<pre>
def add_id_filter_if_valid (id)
"id = #{id} AND " if id && id.to_i > 0
end

def add_user_filter_for_super_admin (user_id)
"whodunnit = #{user_id} AND " if user_id && user_id.to_i > 0
end

def add_user_filter
"whodunnit = #{current_user.try(:id)} AND " if current_user.try(:id) && current_user.try(:id).to_i > 0
end

def add_event_filter (event)
"event = '#{event}' AND " if event && !(event.eql? 'Any')
end

def add_date_time_filter
"created_at >= '#{time_to_string(params[:start_time])}' AND " +
"created_at <= '#{time_to_string(params[:end_time])}'"
end

def add_version_type_filter (version_type)
"item_type = '#{version_type}' AND " if version_type && !(version_type.eql? 'Any')
end
</pre>
* The paginate method has been moved to the helper class Pagination_Helper. This new method can be now reused by the different components like UsersController etc. The method receives two parameters, first the list to paginate and second the number of items to be displayed in a page.

<pre>
module PaginationHelper

def paginate (items, number_of_items_per_page)
items.page(params[:page]).per_page(number_of_items_per_page)
end

end
</pre>

===Code improvements===
* Introduced a constant VERSIONS_PER_PAGE and assigned the value 25 to it. The pagination algorithm for VersionsController displays at most 25 versions in a page. The existing implementation uses the value 25 straight in the code and there are few problems associated with such an approach.
** It is not easy to understand what 25 is unless the programmer takes a close look at the code.
** In case if the value 25 is used at more than one places and in future a new requirement comes to show at most 30 versions in a page, all the values will have to be modified. It is not very DRY.
* The VersionsController was overriding AccessHelper - action_allowed? method to return true in all the cases. This was violating the whole purpose of the method action_allowed?. The purpose of this method is to determine whether the user who is triggering a CRUD operation is allowed to do so. So when the current user invokes a CRUD operation, the action_allowed? method is invoked first and if the method returns true the CRUD operation is triggered or else the user is intimated with a message and gracefully exited. Hence, when the action_allowed? method is overridden to return true always, it results in providing unauthorized access to certain users.

<pre>
def action_allowed?
true
end
</pre>

:With the new implementation the AccessHelper - action_allowed? method has been modified in such a way that unauthorized access is prevented. As per the new algorithm, 'new', 'create', 'edit', 'update' cannot be invoked by any user. These operations can be accessed only by ‘papertrail’ gem. Only an ‘Administrator’ or ‘Super-Administrator’ can call 'destroy_all' method. All the other methods are accessible to ‘Administrator’, ‘Super-Administrator’, ‘Instructor’, ‘Teaching Assistant’ and ‘Student’.

<pre>
def action_allowed?
case params[:action]
when 'new', 'create', 'edit', 'update'
#Modifications can only be done by papertrail
return false
when 'destroy_all'
['Super-Administrator',
'Administrator'].include? current_role_name
else
#Allow all others
['Super-Administrator',
'Administrator',
'Instructor',
'Teaching Assistant',
'Student'].include? current_role_name
end
end
</pre>

===Automated Testing using RSPEC===
The current version of expertiza did not have any test for VersionsController. Using the test driven development(TDD) approach, we have added an exhaustive set of RSPEC tests for VersionsController, to test all the modifications we have done to the code of the controller class. The tests use double and stub features of rspec-rails gem, to fake the log in by different users - Administrator, Instructor, Student etc. The tests can be executed "rpec spec" command as shown below.
<pre>
user-expertiza $rspec spec
.
.
.
Finished in 5.39 seconds (files took 25.33 seconds to load)
66 examples, 0 failures

Randomized with seed 19254
.
.
</pre>

===Testing from UI===
Following are a few testcases with respectto our code changes that can be tried from UI:
1. To go to versions index page, type in the following url after logging in:
http://152.46.16.81:3000/versions

2. After logging in as student/instructor or admin : Try accessing the new, create, edit, update actions. These actions are not allowed to any of the users.
http://152.46.16.81:3000/versions/new
This calls the new action. In the current production version of expertiza, it is unhandled and application gives a default 404 page.

3. Another feature that can be tested from UI is Pagination. Try searching for a user's versions and see if the results are paginated or not. Search here:
http://152.46.16.81:3000/versions/search

4. Visit the same URL as step 3, you should see only the students under that instructor in the users dropdown.

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
#[http://bit.ly/myexpertiza Demo link]
#[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza project documentation wiki]
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

E1908 signupsheet

2019-03-25T19:27:11Z

Spadala: Created page with "==E1908. Refactoring the Versions Controller== This page provides a description of the Expertiza based OSS project. __TOC__ ===About Expertiza=== [http://expertiza.ncsu.ed..."

==E1908. Refactoring the Versions Controller==

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

__TOC__

===About Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

===Problem Statement===
The following tasks were accomplished in this project:

* Improved the clarity of code by improving the variable and parameter names.
* Long character strings were taken and given appropriate names.
* Handled pagination by a separate helper module, which can be used by multiple controllers.
* Implemented action_allowed for access_control to prevent unauthorized access of methods.
* Prevented displaying of all versions for all users and tables when a user views the index page.
* Added missing CRUD methods to Versions Controller
* Added RSPEC testcases for testing changes done in Versions Controller

===About Versions Controller===
This class manages different versions of reviews. If a reviewer reviews a submission, and after that, the author revises the submission, the next time the reviewer does a review (s)he will create a new version. Sometimes it’s necessary to find the current version of a review; sometimes it’s necessary to find all versions. Similarly, a user may want to delete the current version of a review, or all versions of a review. Pagination of versions helps the user to view a subset of versions at a time. Considering the huge number of versions in the system, it is very useful to have a pagination mechanism and a filtering mechanism which can be applied on the whole set of versions. The idea is to display the versions in an ordered, comprehensible and logical manner. In Expertiza the gem ‘will_paginate’ is used to achieve pagination.

===Current Implementation===

=====Functionality=====
* Any user irrespective of his/ her privileges can view all the versions.
::The versions which a particular user can view should be restricted based on the privileges of the user. For instance, only a user with Administrator privileges should be able to view all the versions in the system. However, that is not the case now. Every user can view all the versions irrespective of whether the user is a student or an administrator.
* Any user can delete any version
::The versions which a particular user can delete should be restricted based on the privileges of the user. For instance, a student should not be allowed to delete any version. According to the current implementation any user can delete any version in the system.
* Filtering of versions were restricted to the current user
::The filtering options on versions were restricted to the current user. Sometimes a user might want to view versions associated with other users. For instance, an instructor might want to view the list of versions created by a particular student. This is not possible with the current implementation.

=====Drawbacks and Solutions=====
* '''Problem 1''': The method paginate_list is doing more than one thing.
::The method paginate_list was building a complex search criteria based on the input params, getting the list of versions from the Database matching this search criteria and then calling the Page API. All these tasks in a single method made it difficult to understand.
<pre>
# For filtering the versions list with proper search and pagination.
def paginate_list(id, user_id, item_type, event, datetime)
# Set up the search criteria
criteria = ''
criteria = criteria + "id = #{id} AND " if id && id.to_i > 0
if current_user_role? == 'Super-Administrator'
criteria = criteria + "whodunnit = #{user_id} AND " if user_id && user_id.to_i > 0
end
criteria = criteria + "whodunnit = #{current_user.try(:id)} AND " if current_user.try(:id) && current_user.try(:id).to_i > 0
criteria = criteria + "item_type = '#{item_type}' AND " if item_type && !(item_type.eql? 'Any')
criteria = criteria + "event = '#{event}' AND " if event && !(event.eql? 'Any')
criteria = criteria + "created_at >= '#{time_to_string(params[:start_time])}' AND "
criteria = criteria + "created_at <= '#{time_to_string(params[:end_time])}' AND "

if current_role == 'Instructor' || current_role == 'Administrator'

end

# Remove the last ' AND '
criteria = criteria[0..-5]

versions = Version.page(params[:page]).order('id').per_page(25).where(criteria)
versions
end
</pre>
* '''Solution''': The implementation has been changed in such a way that the versions which a user is allowed to see depends on the privileges of the user. The approach we have taken is as follows:
**An administrator can see all the versions
**An instructor can see all the versions created by him and other users who are in his course or are participants in the assignments he creates.
**A TA can see all the versions created by him and other users who are in the course for which he/ she assists.
**A Student can see all the versions created by him/ her.
* '''Problem 2''': The search criteria created in the method paginate_list was difficult to comprehend.
::The code which builds the search criteria in the method paginate_list uses many string literals and conditions and is hardly intuitive. The programmer will have to spend some time to understand what the code is really doing.
* '''Solution''': The implementation has been changed. A student is not allowed to delete any versions now. Other types of users, for instance administrators, instructors and TAs are allowed to delete only the versions they are authorized to view.
* '''Problem 3''': The paginate method can be moved to a helper class.
::VersionsController is not the only component which require to paginate items. There are other components too. For instance, the UsersController has to paginate the list of users. Hence the Paginate method can be moved to a helper class which can be accessed by other components as well.
* '''Solution''': The filtering options has also been enhanced. The current user can now choose as part of the version search filter any user from a list of users if the current user is authorized to see the versions created by that user.

===New Implementation===
*The method paginate_list has been split into 2 methods now.
** BuildSearchCriteria – as the name suggests the sole purpose of this method is to build a search criteria based on the input search filters when the current user initiates a search in versions.
** paginate_list – this method will call the paginate API.
:First the search criteria is built, then the criteria is applied to versions in the database to get all versions which matches the criteria and then the retrieved versions are paginated.
<pre>
# pagination.
def paginate_list(versions)
paginate(versions, VERSIONS_PER_PAGE);
end

def BuildSearchCriteria(id, user_id, item_type, event)
# Set up the search criteria
search_criteria = ''
search_criteria = search_criteria + add_id_filter_if_valid(id).to_s
if current_user_role? == 'Super-Administrator'
search_criteria = search_criteria + add_user_filter_for_super_admin(user_id).to_s
end
search_criteria = search_criteria + add_user_filter
search_criteria = search_criteria + add_version_type_filter(item_type).to_s
search_criteria = search_criteria + add_event_filter(event).to_s
search_criteria = search_criteria + add_date_time_filter
search_criteria
end
</pre>
* The string literals and conditions in the method paginate_list were replaced with methods with intuitive names so that the programmer can understand the code more easily. We also removed an empty if clause and a redundant statement.
<pre>
def add_id_filter_if_valid (id)
"id = #{id} AND " if id && id.to_i > 0
end

def add_user_filter_for_super_admin (user_id)
"whodunnit = #{user_id} AND " if user_id && user_id.to_i > 0
end

def add_user_filter
"whodunnit = #{current_user.try(:id)} AND " if current_user.try(:id) && current_user.try(:id).to_i > 0
end

def add_event_filter (event)
"event = '#{event}' AND " if event && !(event.eql? 'Any')
end

def add_date_time_filter
"created_at >= '#{time_to_string(params[:start_time])}' AND " +
"created_at <= '#{time_to_string(params[:end_time])}'"
end

def add_version_type_filter (version_type)
"item_type = '#{version_type}' AND " if version_type && !(version_type.eql? 'Any')
end
</pre>
* The paginate method has been moved to the helper class Pagination_Helper. This new method can be now reused by the different components like UsersController etc. The method receives two parameters, first the list to paginate and second the number of items to be displayed in a page.

<pre>
module PaginationHelper

def paginate (items, number_of_items_per_page)
items.page(params[:page]).per_page(number_of_items_per_page)
end

end
</pre>

===Code improvements===
* Introduced a constant VERSIONS_PER_PAGE and assigned the value 25 to it. The pagination algorithm for VersionsController displays at most 25 versions in a page. The existing implementation uses the value 25 straight in the code and there are few problems associated with such an approach.
** It is not easy to understand what 25 is unless the programmer takes a close look at the code.
** In case if the value 25 is used at more than one places and in future a new requirement comes to show at most 30 versions in a page, all the values will have to be modified. It is not very DRY.
* The VersionsController was overriding AccessHelper - action_allowed? method to return true in all the cases. This was violating the whole purpose of the method action_allowed?. The purpose of this method is to determine whether the user who is triggering a CRUD operation is allowed to do so. So when the current user invokes a CRUD operation, the action_allowed? method is invoked first and if the method returns true the CRUD operation is triggered or else the user is intimated with a message and gracefully exited. Hence, when the action_allowed? method is overridden to return true always, it results in providing unauthorized access to certain users.

<pre>
def action_allowed?
true
end
</pre>

:With the new implementation the AccessHelper - action_allowed? method has been modified in such a way that unauthorized access is prevented. As per the new algorithm, 'new', 'create', 'edit', 'update' cannot be invoked by any user. These operations can be accessed only by ‘papertrail’ gem. Only an ‘Administrator’ or ‘Super-Administrator’ can call 'destroy_all' method. All the other methods are accessible to ‘Administrator’, ‘Super-Administrator’, ‘Instructor’, ‘Teaching Assistant’ and ‘Student’.

<pre>
def action_allowed?
case params[:action]
when 'new', 'create', 'edit', 'update'
#Modifications can only be done by papertrail
return false
when 'destroy_all'
['Super-Administrator',
'Administrator'].include? current_role_name
else
#Allow all others
['Super-Administrator',
'Administrator',
'Instructor',
'Teaching Assistant',
'Student'].include? current_role_name
end
end
</pre>

===Automated Testing using RSPEC===
The current version of expertiza did not have any test for VersionsController. Using the test driven development(TDD) approach, we have added an exhaustive set of RSPEC tests for VersionsController, to test all the modifications we have done to the code of the controller class. The tests use double and stub features of rspec-rails gem, to fake the log in by different users - Administrator, Instructor, Student etc. The tests can be executed "rpec spec" command as shown below.
<pre>
user-expertiza $rspec spec
.
.
.
Finished in 5.39 seconds (files took 25.33 seconds to load)
66 examples, 0 failures

Randomized with seed 19254
.
.
</pre>

===Testing from UI===
Following are a few testcases with respectto our code changes that can be tried from UI:
1. To go to versions index page, type in the following url after logging in:
http://152.46.16.81:3000/versions

2. After logging in as student/instructor or admin : Try accessing the new, create, edit, update actions. These actions are not allowed to any of the users.
http://152.46.16.81:3000/versions/new
This calls the new action. In the current production version of expertiza, it is unhandled and application gives a default 404 page.

3. Another feature that can be tested from UI is Pagination. Try searching for a user's versions and see if the results are paginated or not. Search here:
http://152.46.16.81:3000/versions/search

4. Visit the same URL as step 3, you should see only the students under that instructor in the users dropdown.

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
#[http://bit.ly/myexpertiza Demo link]
#[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza project documentation wiki]
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin