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

From Expertiza_Wiki
Jump to navigation Jump to search
m (testing a template)
(Moved the entire content from sandbox to ncsu wiki)
Line 1: Line 1:
Testing a template. This wiki page is for the description of changes made under E1555 OSS assignment for Fall 2015, CSC/ECE 517.
This wiki documentation is for the open source refactoring project E1601: Refactor methods related to submitted_hyperlinks for the course ECE/CSC 517.


== Peer Review Information ==
== Peer Review Information ==


For users intending to view the deployed Expertiza associated with this assignment, the credentials are below:
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
* Instructor login: username -> instructor6,  password -> password
* Student  login: username -> student4340,  password -> password  
* Student  login: username -> student5884,  password -> password  
* Student login: username -> student4405,  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 Background==
== Expertiza Introduction==


Expertiza is an educational web application created and maintained by the joint efforts of the students and the faculty at NCSU. It’s an open source project developed on Ruby on Rails platform and it’s code is available on Github. It allows students to review each other’s work and improve their work upon this feedback.
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!


== Description of the current project ==
== Problem statement ==


The following is an Expertiza based OSS project which deals primarily with the GradesController and GradesHelper. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.
'''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.


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


A controller and a helper file were modified for this project namely:<br/>
'''What refactoring we have done to improve''':
1. GradesController <br/>
* Removed “hyperlinks_array” method and “hyperlinks” method from assignment_participant.rb. Created “hyperlinks” method in assignment_team.rb.
2. GradesHelper <br/>
* Removed “submit_hyperlink” and “remove_hyperlink” from assignment_pariticpant.rb and created equivalent methods in assignment_team.rb.      


=== GradesController ===
* Removed “has_submissions?” from assginment_participant.rb and created equivalent method in assignment_team.rb       
* All the calls to the above methods were refactored to call the appropriate method's functions.
This is a controller that helps students and instructors view grades and reviews, update scores, check for grading conflicts and calculate penalties. A couple of long and complex methods were refactored from this controller along with removal of some non-functional code and a few language changes to make it Ruby style.
* Wrote tests for submitting and removing submitted hyperlinks and files.
Three methods in particular, namely conflict_notification ,calculate_all_penalties and edit were found to be too long and were in need of refactoring into smaller, easier to manage methods. Few more compact methods were created for this purpose.
 
There were no existing test cases for the controller. We have added a spec file named 'grades_spec.rb' under the spec folder. As no changes were done for the model, no tests for the model were included.
 
=== GradesHelper ===
 
This is a helper class which contains methods for constructing a table(construct_table) and to check whether an assignment has a team and metareveiw(has_team_and_metareview)
 
== List of changes ==
We worked on the following work items(WIs)<br/>
WI1 : Refactor calculate_all_penalties method into smaller methods<br/>
WI2 : Move the repeated code in conflict_notification & edit methods to a separate method list_questions.<br/>
WI3 : Refactor the code as per the Ruby style guidelines and incorporate the good practices<br/>
WI4 : Test the conflict_notification method to test the changes made.<br/>
WI5 : Move the repeated code in view and view_my_scores methods to a separate method retrieve_questions
 
=== Solutions Implemented and Delivered ===
 
*Refactoring calculate_all_penalties method
 
This is used to calculate various penalty values for each assignment if penalty is applicable.
 
The following changes were made:


1. This method was very complex, performing too many functions within a single method and had to be broken into 3 smaller methods each having a more well defined function.
== Key changes ==
2. The following 3 methods were created after splitting the first method<br>
Files with major modifications:
  i. calculate_all_penalties<br>
# The model: assignment_participant.rb
  ii. calculate_penatly_attributes<br>
# The model: assignment_team.rb
  iii. assign_all_penalties<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 />
3. Changes were also made to make the code follow ruby style.The language was made more ruby friendly.
4. Finally some redundant code was commented out as it was non-functional.


1.a) The below hyperlinks_array and hyperlinks method are removed from the assignment_participant.rb:<syntaxhighlight lang="ruby">
def hyperlinks
    team.try(:hyperlinks) || []
  end
   
   
  def hyperlinks_array
    self.team.submitted_hyperlinks.blank? ? [] : YAML::load(self.team.submitted_hyperlinks)
  end
</syntaxhighlight>b) They are both merged into a single method in assignment_team.rb under the hyperlinks mehtod:<syntaxhighlight lang="ruby">
def hyperlinks
        self.submitted_hyperlinks.blank? ? [] : YAML::load(self.submitted_hyperlinks)
end
</syntaxhighlight>


Refactoring into smaller more specific methods:
2. a) Earlier, the following method submit_hyperlink was in assignment_participant.rb:<syntaxhighlight lang="ruby">
 
  def submit_hyperlink(hyperlink)
[[File:Change6_new.png]]
    hyperlink.strip!
 
    raise "The hyperlink cannot be empty" if hyperlink.empty?
Removal of non-functional code :
    url = URI.parse(hyperlink)
 
    # If not a valid URL, it will throw an exception
[[File:Change5_new.png]]
    Net::HTTP.start(url.host, url.port)
 
    hyperlinks = self.hyperlinks_array
 
    hyperlinks << hyperlink
Change of language to make it more Ruby friendly:
    team_object = self.team
 
    team_object.submitted_hyperlinks = YAML::dump(hyperlinks)
[[File:Change1_new.png]]
    team_object.save
  end
</syntaxhighlight>b) It is now moved to assignment_team.rb:<syntaxhighlight lang="ruby">
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
</syntaxhighlight>


3) Similarly the methods "has_submissions?", "remove_hyperlink" were moved.


*Move the redundant piece of code from conflict_notification & edit methods to a new method list_questions
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.
 
The conflict_notification method is used to help the instructors decide if one of the reviews are unfair or inaccurate.
This was again split into 2 methods with some part of the code which is repeated in another method  refactored into a new method.
 
[[File:Change3_new.png]]
 
 
Refactored #Created a method which was a duplicate in conflict_notification and edit methods
 
[[File:Change4_new.png]]
 
edit method:
 
This method is used to edit the questionnaires. This method again has code which is repeated in the conflict_notification method and thus the repeated section was split into a new method.
 
[[File:Change2_new.png]]
 
New method:
Refactored #Created a method which was a duplicate in conflict_notification and edit methods
 
[[File:Change4_new.png]]
 
Similar refactoring was performed to obtain the retrieve_questions method:
 
[[File:Latest1.png]]
 
This is the new method created after the above refactoring:
 
[[File:Latest2.png]]
 
== Testing Details==


== Testing Details ==
=== RSpec ===
=== RSpec ===
There were no existing test cases for the GradesController. We have added a new spec file 'grades_spec.rb' which covers testing scenario for the newly added method. The specs were run on the previous and current files and they return the same results implying that the refactored code does not break anything.
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".
As the model was not changed, no test cases were added for the model.


=== UI Testing ===
=== UI Testing ===


Following steps needs to be performed to test this code from UI:<br/>
Following steps needs to be performed to test this code from UI:
1. Login as instructor. Create a course and an assignment under that course.<br/>
# Login as a student.
2. Keep the has team checkbox checked while creating the assignment. Add a grading rubric to it. Add at least two students as participants to the assignment.<br/>
# In an assignment, that is still due, add one or more hyperlinks.
3. Create topics for the assignment.<br/>
# Login as another student who is a teammate of the first student working on the same assignment.
4. Sign in as one of the students who were added to the assignment.<br/>
# You should be able to see the newly uploaded link(s) by the other team member.
5. Go to the assignment and sign up for a topic.<br/>
# Try adding a link that is not a valid http or https link. It will give an error message.
6. Submit student's work by clicking 'Your work' under that assignment.<br/>
# Try submitting the same link once again. This will give an error message too.
7. Sign in as a different student which is participant of the assignment.<br/>
8. Go to Assignments--><assignment name>-->Others' work (If the link is disabled, login as instructor and change the due date of the assignment to current time).<br/>
9. Give reviews on first student's work.<br/>
10. Login as instructor or first student to look at the review grades.<br/>
 
 
== Scope for future improvement ==
1. The construct_table method in GradesHelper is not used anywhere. It has no reference in the project. So we feel it can be safely removed.<br/>
2. The has_team_and_metareview? method in GradesHelper can be broken down into separate methods, one each for team and metareview. This will provide improved flexibility. It needs some analysis though, as both the entities(team & metareview) are currently checked in conjuction from all the views they are referenced from.

Revision as of 00:36, 24 March 2016

This wiki documentation 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 submit 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 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

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 hyperlinks_array and hyperlinks method are 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 single method in assignment_team.rb under the hyperlinks mehtod:

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) Similarly the methods "has_submissions?", "remove_hyperlink" were moved.

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.