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

The reviewers may use the below student and instructor logins for the purpose of reviewing and testing the functionality. These students belong to the same team and work on an assignment created 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 be displayed to the enrolled students only if assignment deadline is in future and the assignment is not in "Finish" state.

Expertiza Introduction

Expertiza is an open source web application which enhances the student's 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 a student submitted files and hyperlinks for an assignment, that content was submitted on behalf of his team, and not individually (even if there is only one person in the team i.e a student working on an assignment cannot be without a team even if he is working alone). Some of the previous contributors had worked on a project which made this change and associated all the submitted content to teams instead of the individual participants: the submitted_hyperlinks and dirctory_num fields were moved to teams table. All the hyperlinks were now recorded in one submitted_hyperlinks field in the team model instead of being present in all the participants of the team. All the submitted files similarly were uploaded to the common 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 give the impression that the author is submitting the hyperlinks on behalf of himself and not the team even though the relevant fields have been moved to teams model. These methods are to be moved to assignment_teams.rb for improving the clarity and the efficiency of code.

What refactoring we have done to improve the code:

Task1:: Removed “hyperlinks_array” method and “hyperlinks” method from assignment_participant.rb. Created “hyperlinks” method in assignment_team.rb.

Task2:: Removed “submit_hyperlink” and “remove_hyperlink” from assignment_pariticpant.rb and created equivalent methods in assignment_team.rb.      

Task3:: Removed “has_submissions?” from assginment_participant.rb and created the equivalent method in assignment_team.rb.        

Task4:: All the calls to the above methods were refactored to call the appropriate method's functions.

Task5:: Refactored some of the logic of controller methods.

Task6:: 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.

Task1:: 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

Task2:: 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.

Task3:: 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

Task4:: Most importantly, at all the places in the Expertiza code, where there were calls to the above mentioned methods in assignment_participant.rb, they 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.

Task5 :: 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".

Factory Girl

Factory Girl is a replacement for fixtures. Fixtures have to be updated whenever we change a data model whereas adding and removing fields is much easier in Factory Girl. Fixture definitions are global whereas Factories can be local, so isolated cases can be tested. Factories are defined to create objects for testing. <ref>https://stackoverflow.com/questions/5183975/factory-girl-whats-the-purpose</ref>

UI Testing

Following are some of the steps which we performed to test this code from the User interface.

Before the test:: Have two students in an assignment team (say student1 and student2, you can also use the student logins listed in this wiki) for an active assignment. Active assignment simply means that the due date is in future and the assignment is not in "Finish" state. You can use an instructor login (example username: instructor6 password: password) to create a new assignment or update the due dates of some previous assignment and assign the team 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 the other team. Review should be submitted successfully.