CSC/ECE 517 Spring 2016/OSS E1601

E1601: Refactor methods related to submitted_hyperlinks.

This wiki is for the open source refactoring project E1601: Refactor methods related to submitted_hyperlinks for the course ECE/CSC 517.

Peer Review Information

We suggest the reviewers to use the below students and instructor for the purpose of reviewing. These students belong to the same team and work on an assignment given by the below instructor:

• Instructor login: username -> instructor6, password -> password
• Student login: username -> student5884, password -> password
• Student login: username -> student6420, password -> password

Please note: Please do not delete the above users or their team. If you wish to do so, please add them back so as to aid other reviewers. Also note that the option to submit hyperlinks in the UI will only be displayed to the enrolled students only before the assignment deadline.

Expertiza Introduction

Expertiza is an open source web application which enhances learning through peer reviews. The students can form teams and work on assignments given by course instructors. The peers can review the submissions and give feedback. The students also get to review the reviewers!

Problem statement

Before: When student submitted files and hyperlinks, that content was submitted on behalf of a team, not individuals (even there is only one person in the team). Some of the previous contributors worked on a project which associated all the submitted content to teams: there were submitted_hyperlinks and dirctory_num fields in teams table. All the hyperlinks should ideally be recorded in one submitted_hyperlinks field. All the submitted files were uploaded to the file space and in the “course_folder/assignment_folder/team_folder” path.

What was wrong with that: There are multiple methods in assignment_participant.rb which still look like the author submitted the hyperlinks on behalf of him/herself. They should be moved to assignment_teams.rb.

What refactoring we have done to improve:

• Removed “hyperlinks_array” method and “hyperlinks” method from assignment_participant.rb. Created “hyperlinks” method in assignment_team.rb.
• Removed “submit_hyperlink” and “remove_hyperlink” from assignment_pariticpant.rb and created equivalent methods in assignment_team.rb.      

• Removed “has_submissions?” from assginment_participant.rb and created the equivalent method in assignment_team.rb.        
• All the calls to the above methods were refactored to call the appropriate method's functions.
• Wrote tests for submitting and removing submitted hyperlinks and files.

List of files changed

Model Files:

1. assignment_participant.rb
2. assignment_team.rb
3. student_task.rb

View Files:

1. submitted_content/_hyperlink.html.erb
2. assignments/edit/_calibration.html.erb
3. assignments/list_submissions.html.erb
4. submitted_content/_main.html.erb

Controller Files:

1. sign_up_sheet_controller.rb
2. submitted_content_controller.rb

Files with major modifications:

1. The model: assignment_participant.rb
2. The model: assignment_team.rb
3. The controller: submitted_content_controller.rb

Key changes

Four methods hyperlinks, has_submissions?, submit_hyperlink and remove_hyperlink were moved from the assignment_participant.rb model to assignment_team.rb, and the hyperlinks_array method was removed.

1) The below methods hyperlinks_array and hyperlinks were removed from the assignment_participant.rb. The hyperlinks method which existed here seemed suspect as there was no hyperlinks method in the AssignmentTeam/Team models so the try would always fail and return an empty array. Also the hyperlinks_array method retrieved the hyperlinks from the submitted_hyperlinks field of the team model so in moving to AssignmentTeam model we just had to remove the .team quantifier.

 def hyperlinks
   team.try(:hyperlinks) || []
 end

 def hyperlinks_array
   self.team.submitted_hyperlinks.blank? ? [] : YAML::load(self.team.submitted_hyperlinks)
 end

They are both merged into a new single hyperlinks method in assignment_team.rb. All callers of both methods were refactored to use hyperlinks method of AssignmentTeam.

 def hyperlinks
       self.submitted_hyperlinks.blank? ? [] : YAML::load(self.submitted_hyperlinks)
 end

2) Earlier, the following method submit_hyperlink was in assignment_participant.rb:

 def submit_hyperlink(hyperlink)
   hyperlink.strip!
   raise "The hyperlink cannot be empty" if hyperlink.empty?
   url = URI.parse(hyperlink)
   # If not a valid URL, it will throw an exception
   Net::HTTP.start(url.host, url.port)
   hyperlinks = self.hyperlinks_array
   hyperlinks << hyperlink
   team_object = self.team
   team_object.submitted_hyperlinks = YAML::dump(hyperlinks)
   team_object.save
 end

It has been now moved to assignment_team.rb as below. Again, as the method was moved to AssignmentTeam model, it obviated the need to invoke the team method.

 def submit_hyperlink(hyperlink)
       hyperlink.strip!
       raise "The hyperlink cannot be empty" if hyperlink.empty?
       url = URI.parse(hyperlink)
       # If not a valid URL, it will throw an exception
       Net::HTTP.start(url.host, url.port)
       hyperlinks = self.hyperlinks
       hyperlinks << hyperlink
       self.submitted_hyperlinks = YAML::dump(hyperlinks)
       self.save
 end

A similar refactoring was done for remove_hyperlinks method in assignment_participant.rb.

3) The methods has_submissions? earlier existed both in AssignmentTeam model and AssignmentParticipant model.

In assignment_team.rb

 def has_submissions?
       list_of_users = participants;
       list_of_users.each { |participant| return true if participant.has_submissions? }
       false
 end

In assignment_participant.rb

 def has_submissions?
       return ((self.team.submitted_files.length > 0) or (hyperlinks_array.length > 0))
 end

What was happening earlier was that has_submissions? (AssignmentTeam method) used to call has_submissions? for each of the AssignmentParticipants in the team. This was redundant as the submitted_files and submitted_hyperlinks fields had been moved from the participant to the team model in earlier projects. So the participant objects again ended up again going to the team for retrieving these fields.

So we removed has_submissions? from the AssignmentParticipant and refactored the one already present in AssignmentTeam to the simple code below:

In assignment_team.rb

 def has_submissions?
       return ((self.submitted_files.length > 0) or (hyperlinks.length > 0))
 end

4) Most importantly, at all the places in the Expertiza code, where there were calls to the above mentioned methods in assignment_participant.rb, were refactored appropriately to call the newly created methods in assignment_team.rb.

Sample example :

       if (participant.submitted_at.nil? && participant.hyperlinks.empty?)

This was refactored as

       if (participant.submitted_at.nil? && participant.team.hyperlinks.empty?)

In refactoring the call points, we used the existing team method of AssignmentParticipant to get the AssignmentTeam object associated with the particular participant and then invoked the equivalent methods on the team object.

5) Refactoring some of controller code

In submitted_content_controller, remove_hyperlink method used to cycle through all the participants belonging to the team of current participant, and for each participant call the remove_hyperlink method of the AssignmentParticipant Model to delete the instance of input hyperlink.

Since all the required hyperlink methods and fields are now in AssignmentTeam this would be redundant and waste cycles. So we refactored it to directly call the AssignmentTeam remove_hyperlink method.

Earlier in submitted_content_controller.rb

 def remove_hyperlink
   @participant = AssignmentParticipant.find(params[:hyperlinks][:participant_id])

   return unless current_user_id?(@participant.user_id)
   hyperlink_to_delete = @participant.hyperlinks_array[params['chk_links'].to_i]
 
   team_id = TeamsUser.team_id(@participant.parent_id, @participant.user_id)
   team_participants = Array.new
   if Team.exists?(team_id)
     team_users = TeamsUser.where(team_id: team_id)
     team_users.each do |team_user|
       team_participants << AssignmentParticipant.where(parent_id: @participant.parent_id, user_id: team_user.user_id).first
     end
   else
     team_participants << @participant
   end
 
   team_participants.each do |team_participant|
     team_participant.remove_hyperlink(hyperlink_to_delete)
   end

Refactored:

 def remove_hyperlink
   @participant = AssignmentParticipant.find(params[:hyperlinks][:participant_id])

   return unless current_user_id?(@participant.user_id)

   team = @participant.team
   hyperlink_to_delete = team.hyperlinks[params['chk_links'].to_i]
   team.remove_hyperlink(hyperlink_to_delete)

Testing Details

RSpec

There were no existing tests for the hyperlinks related methods. We used RSpec to write test cases using TTD approach. The assignment_team_spec.rb in the spec folder will have these tests. All the tests can be executed by rspec spec command, or can also be executed individually using the command "rspec spec/models/assignment_team_spec.rb".

UI Testing

Following steps needs to be performed to test this code from UI:

Before the test Have two students in an assignment team (say student1 and student2) for an active assignment. Active assignment implies that due date is in that future. You can use instructor login (example username: instructor6 password: password) to create a new assignment or update the due date of some previous assignment and assign student/teams to it.

Test1 Login as student1. Submit a hyperlink (say : linkA) as student1. Expected Submit should go through. The hyperlink should be visible when you login as student2

Test2 Now as student2, submit another hyperlink (say linkB). Expected Submit should go through. Now both hyperlinks should be visible from both student1 and student2.

Test3 As student1, try submitting linkB. Expected As linkB already exists in team's hyperlinks the submit should not go through.

Test4 Remove a hyperlink (say linkA). Expected Remove goes through. Now only linkB should be visible from both student accounts.

Test5 Remove all hyperlinks. Expected Hyperlinks list is empty for team and this can be seen from both student accounts.

Test6 Submit some hyperlink. Try dropping topic after submitting. Expected: You are not allowed to drop if you have submitted some work. Only if you remove the submitted work it is possible for you to drop.

Test7 Submit some hyperlink. Do a review of the team after logging in as a student in another team. The assignment should be in review stage for this to be possible, you can move the due dates via instructor login to achieve this. Expected Peer review should show the links submitted by other team. Review can be submitted successfully.