CSC/ECE 517 Spring 2015/oss E1507 DG

E1507. Refactoring Review Files Controller & Submitted Content Controller

This page provides a description of the Expertiza based OSS project. This project aimed at refactoring the Review Files Controller & Submitted Content Controller as well as fixing the bug #483

What is Expertiza

Expertiza is a Open Source Rails application where students can submit and peer-review learning objects (articles, code, web sites, etc). It is used in select courses at NC State and by professors at several other colleges and universities. This Open Source application can be cloned from Github, the latest active branch is "Rails 4".

Project Desicription<ref>https://docs.google.com/document/d/1J0WUBtYV_MDhpEQ-50z8gXE-OrvVkpEaZxvn_9RCAsM/edit#</ref>

There are 3 major objects for this project:

1. Fix Bug #483<ref>https://github.com/expertiza/expertiza/issues/483</ref>, which causes some error and prevent users to get access to the submitted_content page.

2. Refactor Review file controller and Submitted content controller, so that large methods can be divided into smaller methods and make the code more readable and understandable.

3. Replace some statements based on Global Rules<ref>https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit?usp=sharing</ref>, for the uniformity of the whole project.

Bug #483

In "Your works" view, students can only submit links but no files.

This is caused by commit f313711 where the line 13 in submitted_content/_main.html.erb was commented out.

<%#= render partial: 'submitted_content/submitted_files', locals: {participant: @participant, stage: @stage} %>

But after uncommented line 13 in submitted_content/_main.html.erb and refresh the page, the Rails Server stopped responding.

Refactoring Review Files Controller & Submitted Content Controller

What they do

This review_files_controller.rb is responsible for handling the review files of the participants. This controller helps in uploading newer versions of reviews for an assignment and also in displaying all the versions (and diff between versions/code) with the help of submitted_content_controller.rb.

What’s wrong with them

There are quite a few complex functions inside these controllers. These functions can easily be broken up into much more smaller methods with specific functionalities. They also need to be refactored to meet the Global Rules.

What needs to be done

1.Fix this bug #483

2.Break up the complex methods such as get_comments, show_code_file_diff. Anything that can be modularized should be modularized (Single Responsibility)

3.While breaking up the complex methods, look out for possible helper functions. If there are any, move them to the review_files_helper.rb

4.Remove commented out code from the controller

5.Refactor the file based on Global Rules provided at the top of this document

Changes Made

Files involved

review_files_controller.rb
submitted_content_controller.rb
review_files_helper.rb
review_comments_helper.rb
submitted_content/_main.html.erb
model/assignment.rb

For Bug #483 fixing

Git log can be viewed in commit aa37e2e

1. In submitted_content_controller.rb, line 19.

if @assignment.max_team_size > 1 && @participant.team.nil?
     flash[:error] = "This is a team assignment. Before submitting your work, you must <a style='color: blue;' href='../../student_teams/view/?student_id=#{params[:id]}'>create a team</a>, 
                     even if you will be the only member of the team"
     redirect_to controller: 'student_task', action: 'view', id: params[:id]
else if @participant.team.nil?
     #create a new team for current user before submission
     team = AssignmentTeam.create_team_and_node(@assignment.id)
     team.add_member(User.find(@participant.user_id),@assignment.id)
end

This else if must be a mistake, so we changed it to:

elsif @participant.team.nil?

2. In model/assignment.rb, line 489.

Course.find(self.course_id).get_path

The Course model doesn't have an attribute called get_path, it should be directory_path, so we changed it to

Course.find(self.course_id).directory_path

3. In submitted_content/_main.html.erb, line 18.

<% if participant.assignment.is_coding_assignment%>
     <%= link_to "Code Review Dashboard", :controller => 'review_files',
       :action => 'show_all_submitted_files',
       :participant_id => assignment_participant.id,
       :stage => stage %>
   

<% end %>

As in this page there doesn't have a parameter called assignment_participant, as it need to get the participant's id, so we changed this line to:

:participant_id => participant.id

4. In config/routes.rb. Added one more routes

post :submit_file

Refactoring review_files_controller.rb

1. Changed and and or to && and || to meet the requirements in the Global Rules.

2. In def get_comments, deleted unused variable:

newobj = ReviewComment.where(review_file_id: params[:file_id]);

3. In def show_all_submitted_files, deleted unused variable:

file_path = ReviewFile.get_file(code_review_dir, versions.sort.last,base_filename)

4. In def show_all_submitted_files, deleted unused variable:

code_review_dir = ReviewFilesHelper::get_code_review_file_dir(AssignmentParticipant.find(auth[base_filename][versions.sort.last]))

5. The method def get_comments is too complex, so we modularized part of its code in review_comments_helper.rb, we created a new method: def self.populate_comments, which returns handle, comment, authorflag these three variables.

 def self.populate_comments(params, authorflag, comment)
   assignmentparticipant = AssignmentParticipant.find(params[:participant_id])
   current_participant = AssignmentParticipant.where(parent_id: assignmentparticipant[:parent_id], user_id: session[:user].id).first
   if current_participant.id.to_s == params[:participant_id]
     authorflag = 1
   else
     authorflag = 0
   end
   member = []
   if assignmentparticipant.assignment.team_assignment
     assignmentparticipant.team.get_participants.each_with_index {|member1, index|
       member[index] = member1.id
     }
   end
   if (comment[:reviewer_participant_id] ==  current_participant.id)
     handle = "Me :"
     authorflag = 0
   elsif member.include? comment[:reviewer_participant_id] || comment[:reviewer_participant_id] == assignmentparticipant.id
     handle = "Author :"
     authorflag = 0
   else
     handle = "Reviewer"+comment[:reviewer_participant_id].to_s
   end
   return handle, comment, authorflag
 end

6. Rewrited all :key => value to key: value format according to the requirements in the Global Rules<ref>https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit</ref>.

7. The method def show_code_files_diffIn is too complex, we created a new method: def self.populate_shareObj in review_files_helper.rb to perform part of the show_code_files_diff.

 def self.populate_shareObj(processor)
   first_line_num = []
   second_line_num = []
   first_offset = []
   second_offset = []
   offsetswithcomments_file1 = []
   offsetswithcomments_file2 = []
   first_offset << 0
   second_offset << 0
   first_count = 0
   second_count = 0
   for i in (0..processor.absolute_line_num)
     first_offset = ReviewFile.get_first_offset(processor, i, @first_offset)
     second_offset = ReviewFile.get_second_offset(processor, i, @second_offset)
     first_line_num_new = Hash.new
     first_line_num_new = ReviewFile.get_first_line_num(processor, i, first_count)
     first_line_num << first_line_num[:first_line_num]
     first_count = first_line_num[:first_count]
     second_line_num_new = Hash.new
     second_line_num_new = ReviewFile.get_second_line_num(processor, i,second_count)
     second_line_num << second_line_num[:second_line_num]
     second_count = second_line_num[:second_count]
     # Remove newlines at the end of this line of code
     processor = ReviewFile.get_first_file_array(processor, i)
     processor = ReviewFile.get_second_file_array(processor, i)
     shareObj = Hash.new()
     shareObj['linearray1'] = processor.first_file_array
     shareObj['linearray2'] = processor.second_file_array
     shareObj['comparator'] = processor.comparison_array
     shareObj['linenumarray1'] = first_line_num
     shareObj['linenumarray2'] = second_line_num
     shareObj['offsetarray1'] = first_offset
     shareObj['offsetarray2'] = second_offset
     return first_line_num, second_line_num, first_count, second_count, shareObj
   end
 end

8. In review_files_helper.rb, created a new method: def self.find_review_files, which contains some statements in def show_all_submitted_files.

 def self.find_review_files(participant) 
   # Find all files over all versions submitted by the team
   all_review_files = Array.new
   if participant.assignment.team_assignment
     participant.team.get_participants.each_with_index { |member,index|
       all_review_files += ReviewFile.where(author_participant_id: member.id)
     }
   else
     all_review_files = ReviewFile.where(author_participant_id: @participant.id)
   end
   all_review_files
 end

9. In review_files_helper.rb, created a new method: def self.find_review_versions, which contains some statements in def show_all_submitted_files.

 def self.find_review_versions(all_review_files) 
   file_version_map = Hash.new
   all_review_files.each_with_index do |each_file,index|
     file_version_map[File.basename(each_file.filepath)] = Array.new unless
     file_version_map[File.basename(each_file.filepath)]
     file_version_map[File.basename(each_file.filepath)] << each_file.version_number
   end
   return file_version_map
 end

Refactoring submitted_content_controller.rb

1. Changed and and or to && and || to meet the requirements in the Global Rules.

2. Rewrited all :key => value to key: value format.

3. Rewrited all param['string'] to params[:string].

4. Added one more routes post :folder_action to config/routes.rb

5. Changed all array.size == 0 to array.zero?

6. Changed all find_by_x(val) and where("x=val") to where(x: val)

7. In def remove_hyperlink, deleted redundant line:

@participant = AssignmentParticipant.find(params[:id])

8. In def download, deleted commented line:

#folder_name = FileHelper::sanitize_folder(@current_folder.name)

Results Screenshot

After fixing the bug #483, now the submit files function works.

The file is stored at:

References

<references/>