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.

Key changes

Controller Files:

1. sign_up_sheet_controller.rb
2. submitted_content_controller.rb

View Files:

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

Model 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

Files with major modifications:

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

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 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 the 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 removed the need to invoke the team method. A similar refactoring was done for remove_hyperlinks method 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
       hyperlinks << hyperlink
       self.submitted_hyperlinks = YAML::dump(hyperlinks)
       self.save
 end

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 above was that has_submission (AssignmentTeam method) used to call has_submission for each of the Assignment Participants 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 participants again ended up again going to the team for these fields.

So we removed has_submissions from the AssignmentParticipant and refactored the one already present in AssignmentTeam to just the 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 new methods in assignment_team.rb.

Sample example:

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

was refactored as:

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

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

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:

1. Login as a student.
2. In an assignment, that is still due, add one or more hyperlinks.
3. Login as another student who is a teammate of the first student working on the same assignment.
4. You should be able to see the newly uploaded link(s) by the other team member.
5. Try adding a link that is not a valid http or https link. It will give an error message.
6. Try submitting the same link once again. This will give an error message too.