CSC/ECE 517 Spring 2016/OSS E1601: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
No edit summary
No edit summary
Line 37: Line 37:
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.<br />
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.<br />


1.a) The below hyperlinks_array and hyperlinks method are removed from the assignment_participant.rb:
1.a) The below methods 'hyperlinks_array' and 'hyperlinks' were removed from the assignment_participant.rb.
  def hyperlinks
   
  def hyperlinks
     team.try(:hyperlinks) || []
     team.try(:hyperlinks) || []
   end
   end
Line 46: Line 47:
   end
   end


b) They are both merged into a single method in assignment_team.rb under the hyperlinks method:
b) They are both merged into a new single 'hyperlinks' method in assignment_team.rb:


   def hyperlinks
   def hyperlinks
Line 81: Line 82:
         self.save
         self.save
   end
   end




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


In assignment_team.rb
#In assignment_team.rb


   def has_submissions?
   def has_submissions?
Line 92: Line 92:
         list_of_users.each { |participant| return true if participant.has_submissions? }
         list_of_users.each { |participant| return true if participant.has_submissions? }
         false
         false
        return ((self.submitted_files.length > 0) or (hyperlinks.length > 0))
   end
   end
    
    
   
   
In assignment_participant.rb  
#In assignment_participant.rb  


   def has_submissions?
   def has_submissions?
Line 107: Line 106:
So we removed has_submissions from the AssignmentParticipant and refactored the one already present in AssignmentTeam to just the code below:
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  
#In assignment_team.rb  


   def has_submissions?
   def has_submissions?

Revision as of 19:32, 28 March 2016

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

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.a) The below methods 'hyperlinks_array' and 'hyperlinks' were removed from the assignment_participant.rb.

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

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

b) They are both merged into a new single 'hyperlinks' method in assignment_team.rb:

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


2. a) 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

b) It is now moved to assignment_team.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.

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

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

  1. 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 methods in assignment_participant.rb, which are now moved to assignment_team.rb, were refactored accordingly to call them appropriately.

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.