Latest revision as of 22:11, 5 November 2015

E1560. Refactoring PopUpController.rb and ParticipantsController.rb

This page provides a description of the Expertiza based OSS project. This project is aimed at refactoring PopUpController.rb and ParticipantsController.rb.

Introduction to Expertiza

Expertiza is an Open Source Rails application which is used by instructors and students for creating assignments and submitting peer reviews. Expertiza allows the instructor to create and customize assignments, create a list of topics the students can sign up for, have students work on teams and then review each other's assignments at the end. The Source code of the application can be cloned from Github.

Project Desicription<ref>https://docs.google.com/document/d/1uWs3zyrupTmrOFuv5IbVWCF4NRvCXqJmg8dZ0wCqgus/edit</ref>

Files involved:

popUpController.rb

participantsController.rb

What they do

PopUpController displays a review when a user (instructor or student) clicks on a list of reviews.

ParticipantsController handles participants in an assignment.

What's wrong with them:

PopUpController contains only two prominent methods, but they are very big.

ParticipantsController contains redundant code.

What needs to be done:

1 popUpController.rb

1.1 Action_allowed method always returns true. It needs to be fixed.

1.2 Team_user_popup method needs refactoring. @teamId is assigned but never used. Add comments to make code more readable. Rename variables based on their usage

1.3 Refactor Participants_popup and team_users_popup methods into smaller private methods.

2 participantsController.rb

2.1 In add and update_authorization methods, permissions collection object can be used to reference its elements without assigning each element to individual private variables.

2.2 Inherit and bequeath_all methods are similar. Common statements can be migrated to private method. Add comments in code to make it easy to understand.

2.3 Cluster all the flash messages under one private method to make the code more manageable

2.4 Fix email_sent method. It contains a dummy email address.

Changes Made<ref>https://github.com/viveksubbarao/expertiza/commits/master</ref>

PopUpController.rb

Modified `Action_allowed` method

Modified Action_allowed method to return true only if...

Before Refactoring	After Refactoring
def action_allowed? true end	def action_allowed? if @allowed_actions.include? params[:action] true else false end end

Refactored `Team_user_popup` method

Removed unused instance variable @teamId Changed redundant instance variables to local variables

Before Refactoring

After Refactoring

 def team_users_popup
    @sum = 0
    @count = 0
    @teamid = params[:id]
    @team = Team.find(params[:id])
    @assignment = Assignment.find(@team.parent_id)
    @assignment_id = @assignment.id
    @id=params[:assignment_id]
    #  @teamname = Team.find(params[:id]).name
    @teamusers = TeamsUser.where(team_id: params[:id])

    #id2 seems to be a response_map
    if(params[:id2] == nil)
      #  if(@reviewid == nil)
      @scores = nil
    else
      #get the last response from response_map id
      response = Response.where(map_id:params[:id2]).last
      @reviewid = response.id
      @pid = ResponseMap.find(params[:id2]).reviewer_id
      @reviewer_id = Participant.find(@pid).user_id

      @scores = Answer.where(response_id: @reviewid)

      questionnaire =Response.find(@reviewid).questionnaire_by_answer(@scores.first)
      @maxscore = questionnaire.max_question_score

      if(@maxscore == nil)
        @maxscore = 5
      end

      @total_percentage = response.get_average_score
      @sum = response.get_total_score
      @total_possible = response.get_maximum_score
    end

    #    @review_questionnaire = Questionnaire.find(@assignment.review_questionnaire_id)
    #    @review_questions = @review_questionnaire.questions
    #@maxscore = @review_questionnaire.max_question_score


 end

 def team_users_popup
    @sum = 0
    @team = Team.find(params[:id])
    @id = params[:assignment_id]
    @teamusers = TeamsUser.where(team_id: params[:id])

    puts params
    # response_map id can be used to index into the Response
    # table and find the desired response.
    if (params[:response_map_id] == nil)
      @scores = nil
    else
      #get the last response from response_map id
      response = Response.where(map_id: params[:response_map_id]).last
      @reviewid = response.id
      @pid = ResponseMap.find(params[:response_map_id]).reviewer_id
      @reviewer_id = Participant.find(@pid).user_id

      @scores = Answer.where(response_id: response.id)
      questionnaire = Response.find(@reviewid).questionnaire_by_answer(@scores.first)

      if (questionnaire.max_question_score == nil)
        @maxscore = 5 # This instance variable is needed. It's used in the team_users_popup view.
      end

      @total_percentage = response.get_average_score
      @sum = response.get_total_score
      @total_possible = response.get_maximum_score
    end
 end

Refactored Participants_popup and team_users_popup methods into smaller private methods

Before Refactoring	After Refactoring

ParticipantsController.rb

Permissions collection object referenced directly in `add` and `update_authorization methods`

Local variables like can_submit, can_review, etc removed and permissions object referenced directly

Before Refactoring

After Refactoring

def add
......
    permissions = Participant.get_permissions(params[:authorization])
    can_submit = permissions[:can_submit]   
    can_review = permissions[:can_review]
    can_take_quiz = permissions[:can_take_quiz]
    curr_object.add_participant(params[:user][:name],can_submit,can_review,can_take_quiz)
......
end

def update_authorizations
......
    permissions = Participant.get_permissions(params[:authorization])
    can_submit = permissions[:can_submit]
    can_review = permissions[:can_review]
    can_take_quiz = permissions[:can_take_quiz]
    participant = Participant.find(params[:id])
    parent_id = participant.parent_id
    participant.update_attributes(:can_submit => can_submit, :can_review => can_review, :can_take_quiz => can_take_quiz)
......
end

def add
......
    permissions = Participant.get_permissions(params[:authorization])
    curr_object.add_participant(params[:user][:name], permissions[:can_submit], 
              permissions[:can_review], permissions[:can_take_quiz])
......
end


def update_authorizations
......
    permissions = Participant.get_permissions(params[:authorization])
    participant = Participant.find(params[:id])
    parent_id = participant.parent_id
    participant.update_attributes(:can_submit => permissions[:can_submit], 
           :can_review => permissions[:can_review], :can_take_quiz => permissions[:can_take_quiz])
......
end

Moved common functionality in `Inherit` and `bequeath_all` to private methods

Moved common functionality of Inherit and bequeath_all to private method populate_copied_participants

Before Refactoring

After Refactoring

def inherit
    assignment = Assignment.find(params[:id])
    course = assignment.course
    @copied_participants = []

    if course
      participants = course.participants
      if participants.length > 0
        participants.each{|participant|
          new_participant = participant.copy(params[:id])

          if new_participant
            @copied_participants.push new_participant
          end
        }

        # Only display undo link if copies of participants are created
        if @copied_participants.length > 0
          undo_link("Participants from \"#{course.name}\" has been copied to this assignment successfully. ")
        else
          flash[:note] = 'All course participants are already in this assignment'
        end

      else
        flash[:note] = "No participants were found to inherit."
      end
    else
      flash[:error] = "No course was found for this assignment."
    end


    redirect_to :controller => 'participants', :action => 'list', :id => assignment.id, :model => 'Assignment'
end

def bequeath_all
    @copied_participants = []
    assignment = Assignment.find(params[:id])
    if assignment.course
      course = assignment.course
      assignment.participants.each{ |participant|
        new_participant = participant.copy(course.id)

        if new_participant
          @copied_participants.push new_participant
        end
      }
      # only display undo link if copies of participants are created
      if @copied_participants.length > 0
        undo_link("All participants were successfully copied to \"#{course.name}\". " )
      else
        flash[:note] = 'All assignment participants are already part of the course'
      end

      #flash[:note] = "All participants were successfully copied to \""+course.name+"\""
    else
      flash[:error] = "This assignment is not associated with a course."
    end



    redirect_to :controller => 'participants', :action => 'list', :id => assignment.id, :model => 'Assignment'
end

def inherit
    assignment = Assignment.find(params[:id])

    if assignment.course
      if assignment.course.participants.length > 0
        @copied_participants = populate_copied_participants(assignment.courses.participants, params[:id])
        if @copied_participants.length > 0
          undo_link("Participants from \"#{course.name}\" have been copied to this assignment successfully. ")
        else
          flash[:note] = 'All course participants are already in this assignment'
        end
      else
        flash[:note] = "No participants were found to inherit."
      end
    else
      flash[:error] = "No course was found for this assignment."
    end

    redirect_to :controller => 'participants', :action => 'list', :id => assignment.id, :model => 'Assignment'
 end


 def bequeath_all
    assignment = Assignment.find(params[:id])

    if assignment.course
      @copied_participants = populate_copied_participants(assignment.participants, assignment.course.id)
      if @copied_participants.length > 0
        undo_link("All participants were successfully copied to \"#{course.name}\". ")
      else
        flash[:note] = 'All assignment participants are already part of the course'
      end
    else
      flash[:error] = "This assignment is not associated with a course."
    end

    redirect_to :controller => 'participants', :action => 'list', :id => assignment.id, :model => 'Assignment'
 end


def populate_copied_participants(participants, val)
    @copied_participants = []

    participants.each do |participant|
      new_participant = participant.copy(val)
      if new_participant
        @copied_participants.push new_participant
      end
    end

    return @copied_participants
 end

Flash messages clustered under a single private method

Before Refactoring	After Refactoring

Fixed `email_sent` method

Before Refactoring	After Refactoring

How to test from UI

Login as a instructor "instructor6/password". Click on Assignments -> review report -> and on a team

References

CSC/ECE 517 Fall 2015/oss E1560 PSV: Difference between revisions

Latest revision as of 22:11, 5 November 2015

Contents

Introduction to Expertiza

Project Desicription<ref>https://docs.google.com/document/d/1uWs3zyrupTmrOFuv5IbVWCF4NRvCXqJmg8dZ0wCqgus/edit</ref>

Changes Made<ref>https://github.com/viveksubbarao/expertiza/commits/master</ref>

PopUpController.rb

Modified `Action_allowed` method

Refactored `Team_user_popup` method

Refactored Participants_popup and team_users_popup methods into smaller private methods

ParticipantsController.rb

Permissions collection object referenced directly in `add` and `update_authorization methods`

Moved common functionality in `Inherit` and `bequeath_all` to private methods

Flash messages clustered under a single private method

Fixed `email_sent` method

How to test from UI

References

Navigation menu

@@ Line 5: / Line 5: @@
 =Introduction to Expertiza=
-[http://http://expertiza.ncsu.edu/ Expertiza] is an [http://en.wikipedia.org/wiki/Open-source_software Open Source] [http://rubyonrails.org/ Rails] application which is used by instructors and students for creating assignments and submitting peer reviews.  Expertiza allows the instructor to create and customize assignments, create a list of topics the students can sign up for, have students work on teams and then review each other's assignments at the end. The Source code of the application can be cloned from [https://github.com/expertiza/expertiza/ Github], and the latest active branch is [https://github.com/expertiza/expertiza/tree/rails4 "Rails 4"].
+[http://http://expertiza.ncsu.edu/ Expertiza] is an [http://en.wikipedia.org/wiki/Open-source_software Open Source] [http://rubyonrails.org/ Rails] application which is used by instructors and students for creating assignments and submitting peer reviews.  Expertiza allows the instructor to create and customize assignments, create a list of topics the students can sign up for, have students work on teams and then review each other's assignments at the end. The Source code of the application can be cloned from [https://github.com/expertiza/expertiza/ Github].
-__TOC__
 =Project Desicription<ref>https://docs.google.com/document/d/1uWs3zyrupTmrOFuv5IbVWCF4NRvCXqJmg8dZ0wCqgus/edit</ref>=
 '''Files involved:'''
-<p>PopUpController.rb</p>
+<p>popUpController.rb</p>
-<p>ParticipantsController.rb</p>
+<p>participantsController.rb</p>
 '''What they do'''
@@ Line 23: / Line 22: @@
 '''What needs to be done:'''
-<p>'''1 PopUpController.rb'''</p>
+<p>'''1 popUpController.rb'''</p>
-<p>1.1 Action_allowed method always returns true</p>
+<p>1.1 Action_allowed method always returns true. It needs to be fixed.</p>
-<p>1.2 Team_user_popup method needs refactoring. @teamId is assigned but never used, what is id2.Comments can be added at make code more readable. Rename variables based on their usage</p>
+<p>1.2 Team_user_popup method needs refactoring. @teamId is assigned but never used. Add comments to make code more readable. Rename variables based on their usage</p>
-<p>1.3 Change instance variables to local variables where possible.</p>
+<p>1.3 Refactor Participants_popup and team_users_popup methods into smaller private methods.</p>
-<p>1.4 Participants_popup and team_users_popup methods must be refactored into smaller private methods.</p>
 <p></p>
-<p>'''2 ParticipantsController.rb'''</p>
+<p>'''2 participantsController.rb'''</p>
-<p>2.1 In add and update_authorization methods, permissions collection object can be used to reference its elements without assigning each element to individual private variables. It will make the code more concise.</p>
+<p>2.1 In add and update_authorization methods, permissions collection object can be used to reference its elements without assigning each element to individual private variables.</p>
 <p>2.2 Inherit and bequeath_all methods are similar. Common statements can be migrated to private method. Add comments in code to make it easy to understand.</p>
-<p>2.3 Cluster all the flash messages under one private method. It will make them easy to manage.</p>
+<p>2.3 Cluster all the flash messages under one private method to make the code more manageable</p>
-<p>2.4 Fix email_sent method. It contains a dummy email address. Check if this method can be removed.</p>
+<p>2.4 Fix email_sent method. It contains a dummy email address.</p>
 =Changes Made<ref>https://github.com/viveksubbarao/expertiza/commits/master</ref>=
+==<p>'''PopUpController.rb'''</p>==
+=== Modified <code>Action_allowed</code> method ===
+Modified <code>Action_allowed</code> method to return true only if...
+{| class="wikitable"
+|-
+! |Before Refactoring
+! |After Refactoring
+|- style="vertical-align:top;"
+|<pre>
+def action_allowed?
+    true
+end
+</pre>
+|<pre>
+def action_allowed?
+    if @allowed_actions.include? params[:action]
+        true
+    else
+        false
+    end
+end
+</pre>
+|}
+=== Refactored <code>Team_user_popup</code> method ===
+Removed unused instance variable @teamId
+Changed redundant instance variables to local variables
+{| class="wikitable"
+|-
+! |Before Refactoring
+! |After Refactoring
+|- style="vertical-align:top;"
+|<pre>
+ def team_users_popup
+    @sum = 0
+    @count = 0
+    @teamid = params[:id]
+    @team = Team.find(params[:id])
+    @assignment = Assignment.find(@team.parent_id)
+    @assignment_id = @assignment.id
+    @id=params[:assignment_id]
+    #  @teamname = Team.find(params[:id]).name
+    @teamusers = TeamsUser.where(team_id: params[:id])
+    #id2 seems to be a response_map
+    if(params[:id2] == nil)
+      #  if(@reviewid == nil)
+      @scores = nil
+    else
+      #get the last response from response_map id
+      response = Response.where(map_id:params[:id2]).last
+      @reviewid = response.id
+      @pid = ResponseMap.find(params[:id2]).reviewer_id
+      @reviewer_id = Participant.find(@pid).user_id
+      @scores = Answer.where(response_id: @reviewid)
+      questionnaire =Response.find(@reviewid).questionnaire_by_answer(@scores.first)
+      @maxscore = questionnaire.max_question_score
+      if(@maxscore == nil)
+        @maxscore = 5
+      end
+      @total_percentage = response.get_average_score
+      @sum = response.get_total_score
+      @total_possible = response.get_maximum_score
+    end
+    #    @review_questionnaire = Questionnaire.find(@assignment.review_questionnaire_id)
+    #    @review_questions = @review_questionnaire.questions
+    #@maxscore = @review_questionnaire.max_question_score
+ end
+</pre>
+|<pre>
+ def team_users_popup
+    @sum = 0
+    @team = Team.find(params[:id])
+    @id = params[:assignment_id]
+    @teamusers = TeamsUser.where(team_id: params[:id])
+    puts params
+    # response_map id can be used to index into the Response
+    # table and find the desired response.
+    if (params[:response_map_id] == nil)
+      @scores = nil
+    else
+      #get the last response from response_map id
+      response = Response.where(map_id: params[:response_map_id]).last
+      @reviewid = response.id
+      @pid = ResponseMap.find(params[:response_map_id]).reviewer_id
+      @reviewer_id = Participant.find(@pid).user_id
+      @scores = Answer.where(response_id: response.id)
+      questionnaire = Response.find(@reviewid).questionnaire_by_answer(@scores.first)
+      if (questionnaire.max_question_score == nil)
+        @maxscore = 5 # This instance variable is needed. It's used in the team_users_popup view.
+      end
+      @total_percentage = response.get_average_score
+      @sum = response.get_total_score
+      @total_possible = response.get_maximum_score
+    end
+ end
+</pre>
+|}
+=== Refactored Participants_popup and team_users_popup methods into smaller private methods ===
+{| class="wikitable"
+|-
+! |Before Refactoring
+! |After Refactoring
+|- style="vertical-align:top;"
+|<pre>
+</pre>
+|<pre>
+</pre>
+|}
+<p></p>
+==<p>'''ParticipantsController.rb'''</p>==
+=== Permissions collection object referenced directly in <code>add</code> and  <code>update_authorization methods</code>===
+Local variables like can_submit, can_review, etc removed and permissions object referenced directly
+{| class="wikitable"
+|-
+! |Before Refactoring
+! |After Refactoring
+|- style="vertical-align:top;"
+|<pre>
+def add
+......
+    permissions = Participant.get_permissions(params[:authorization])
+    can_submit = permissions[:can_submit]
+    can_review = permissions[:can_review]
+    can_take_quiz = permissions[:can_take_quiz]
+    curr_object.add_participant(params[:user][:name],can_submit,can_review,can_take_quiz)
+......
+end
+def update_authorizations
+......
+    permissions = Participant.get_permissions(params[:authorization])
+    can_submit = permissions[:can_submit]
+    can_review = permissions[:can_review]
+    can_take_quiz = permissions[:can_take_quiz]
+    participant = Participant.find(params[:id])
+    parent_id = participant.parent_id
+    participant.update_attributes(:can_submit => can_submit, :can_review => can_review, :can_take_quiz => can_take_quiz)
+......
+end
+</pre>
+|<pre>
+def add
+......
+    permissions = Participant.get_permissions(params[:authorization])
+    curr_object.add_participant(params[:user][:name], permissions[:can_submit],
+              permissions[:can_review], permissions[:can_take_quiz])
+......
+end
+def update_authorizations
+......
+    permissions = Participant.get_permissions(params[:authorization])
+    participant = Participant.find(params[:id])
+    parent_id = participant.parent_id
+    participant.update_attributes(:can_submit => permissions[:can_submit],
+           :can_review => permissions[:can_review], :can_take_quiz => permissions[:can_take_quiz])
+......
+end
+</pre>
+|}
+=== Moved common functionality in <code>Inherit</code> and <code>bequeath_all</code> to private methods ===
+Moved common functionality of <code>Inherit</code> and <code>bequeath_all</code> to private method <code>populate_copied_participants</code>
+{| class="wikitable"
+|-
+! |Before Refactoring
+! |After Refactoring
+|- style="vertical-align:top;"
+|<pre>
+def inherit
+    assignment = Assignment.find(params[:id])
+    course = assignment.course
+    @copied_participants = []
+    if course
+      participants = course.participants
+      if participants.length > 0
+        participants.each{|participant|
+          new_participant = participant.copy(params[:id])
+          if new_participant
+            @copied_participants.push new_participant
+          end
+        }
+        # Only display undo link if copies of participants are created
+        if @copied_participants.length > 0
+          undo_link("Participants from \"#{course.name}\" has been copied to this assignment successfully. ")
+        else
+          flash[:note] = 'All course participants are already in this assignment'
+        end
+      else
+        flash[:note] = "No participants were found to inherit."
+      end
+    else
+      flash[:error] = "No course was found for this assignment."
+    end
+    redirect_to :controller => 'participants', :action => 'list', :id => assignment.id, :model => 'Assignment'
+end
+def bequeath_all
+    @copied_participants = []
+    assignment = Assignment.find(params[:id])
+    if assignment.course
+      course = assignment.course
+      assignment.participants.each{ |participant|
+        new_participant = participant.copy(course.id)
+        if new_participant
+          @copied_participants.push new_participant
+        end
+      }
+      # only display undo link if copies of participants are created
+      if @copied_participants.length > 0
+        undo_link("All participants were successfully copied to \"#{course.name}\". " )
+      else
+        flash[:note] = 'All assignment participants are already part of the course'
+      end
+      #flash[:note] = "All participants were successfully copied to \""+course.name+"\""
+    else
+      flash[:error] = "This assignment is not associated with a course."
+    end
+    redirect_to :controller => 'participants', :action => 'list', :id => assignment.id, :model => 'Assignment'
+end
+</pre>
+|<pre>
+def inherit
+    assignment = Assignment.find(params[:id])
+    if assignment.course
+      if assignment.course.participants.length > 0
+        @copied_participants = populate_copied_participants(assignment.courses.participants, params[:id])
+        if @copied_participants.length > 0
+          undo_link("Participants from \"#{course.name}\" have been copied to this assignment successfully. ")
+        else
+          flash[:note] = 'All course participants are already in this assignment'
+        end
+      else
+        flash[:note] = "No participants were found to inherit."
+      end
+    else
+      flash[:error] = "No course was found for this assignment."
+    end
+    redirect_to :controller => 'participants', :action => 'list', :id => assignment.id, :model => 'Assignment'
+ end
+ def bequeath_all
+    assignment = Assignment.find(params[:id])
+    if assignment.course
+      @copied_participants = populate_copied_participants(assignment.participants, assignment.course.id)
+      if @copied_participants.length > 0
+        undo_link("All participants were successfully copied to \"#{course.name}\". ")
+      else
+        flash[:note] = 'All assignment participants are already part of the course'
+      end
+    else
+      flash[:error] = "This assignment is not associated with a course."
+    end
+    redirect_to :controller => 'participants', :action => 'list', :id => assignment.id, :model => 'Assignment'
+ end
+def populate_copied_participants(participants, val)
+    @copied_participants = []
+    participants.each do |participant|
+      new_participant = participant.copy(val)
+      if new_participant
+        @copied_participants.push new_participant
+      end
+    end
+    return @copied_participants
+ end
+</pre>
+|}
+=== Flash messages clustered under a single private method ===
+{| class="wikitable"
+|-
+! |Before Refactoring
+! |After Refactoring
+|- style="vertical-align:top;"
+|<pre>
+</pre>
+|<pre>
+</pre>
+|}
+=== Fixed <code>email_sent</code> method ===
+{| class="wikitable"
+|-
+! |Before Refactoring
+! |After Refactoring
+|- style="vertical-align:top;"
+|<pre>
+</pre>
+|<pre>
+</pre>
+|}
+= How to test from UI =
+Login as a instructor "instructor6/password". Click on Assignments -> review report -> and on a team
 = References=
 <references/>

CSC/ECE 517 Fall 2015/oss E1560 PSV: Difference between revisions

Latest revision as of 22:11, 5 November 2015

Introduction to Expertiza

Project Desicription<ref>https://docs.google.com/document/d/1uWs3zyrupTmrOFuv5IbVWCF4NRvCXqJmg8dZ0wCqgus/edit</ref>

Changes Made<ref>https://github.com/viveksubbarao/expertiza/commits/master</ref>

PopUpController.rb

Modified Action_allowed method

Refactored Team_user_popup method

Refactored Participants_popup and team_users_popup methods into smaller private methods

ParticipantsController.rb

Permissions collection object referenced directly in add and update_authorization methods

Moved common functionality in Inherit and bequeath_all to private methods

Flash messages clustered under a single private method

Fixed email_sent method

How to test from UI

References

Navigation menu

Search

Modified `Action_allowed` method

Refactored `Team_user_popup` method

Permissions collection object referenced directly in `add` and `update_authorization methods`

Moved common functionality in `Inherit` and `bequeath_all` to private methods

Fixed `email_sent` method