CSC/ECE 517 Fall 2015/oss E1570 avr

E1570. Refactoring TreeDisplayController

This page provides a description of the Expertiza based OSS project. This project aimed at refactoring the TreeDisplayController.

Introduction to Expertiza

Expertiza is a web application where students can submit and peer-review learning objects (articles, code, web sites, etc). The Expertiza project is supported by the National Science Foundation.<ref>Expertiza</ref><ref>Wiki</ref>

Problem Statement

Classes involved:

tree_display_controller.rb

Controller Responsibilities: This controller deals with displaying “trees” of objects to the instructor. When an instructor logs in, (s)he is greeted with a homepage that lists questionnaires (rubrics, quizzes, surveys), courses, and assignments. These objects are displayed in a “tree,” allowing the user to click on the top-level object and see the objects beneath it.

Bad practices followed:

• Many Duplicate Methods.
• Very Long Methods combining many Functionalities.
• Redundant methods not being used anywhere.

Refactoring to be done:

1. Split get_children_node_ng and get_children_node_2_ng into smaller methods and give reasonable names to them and make sure implement common code in a single method.
2. Merge all the repeating methods into a single method.
3. Write functional tests for the TreeDispayController.
4. Remove commented code in list method.

Changes Made

We used the DRY principle while refactoring our code to remove duplicates in get_children_node_ng and get_children_node_2_ng methods.

TreeDisplayController

Method Name	Changes Made	Reason For Change
goto_questionnaires, goto_review_rubrics, goto_metareview_rubrics, goto_teammatereview_rubrics, goto_author_feedbacks, goto_global_survey, goto_surveys, goto_course_evaluations, goto_courses, goto_assignments	All these methods were merged into a single method with name go_to_menu_items which receives a parameter of the name of tree_folder object, such as "Questionnaires".	Since all the methods were performing the same action, i.e passing the node object id to list method in TreeDisplayController, the name is passed a parameter to the go_to_menu_items method from "link" method in MenuItemsController. Using this parameter the respective object and its ID are found and is redirected to "list" method.
get_children_node_ng	Moved part of the code from these methods to newly created methods: populate_rows,populate_1_row,page_render methods.	These Methods had many functionalities being implemented which made the method look lengthy and moreover these methods were performing almost similar functions which could be grouped together and reduce the overall code length.
get_children_node_2_ng

MenuItemsController

Method Name	Changes Made	Reason For Change
link	Redirect url only for the menu_items altered in TreeDisplayController is changed to go_to_menu_items_url.	Since the 10 methods had been merged into one method in TreeDisplayController, the redirect url is set to go_to_menu_items url.

How to test the changes from UI

• Login in as an instructor or admin using credentials (admin with password: admin or instructor6 with password: password)
• To check the changes made in go_to_menu_items method, hover over the Manage tab in the navigation bar on the top and click all the links and check whether they are being redirected to correct pages. For example, if Questionnaires is clicked, you should be | redirected directly to the page displaying all the questionnaires with its sub-categories like Reviews, Surveys etc.
• To test the changes made in get_children_node_ng and get_children_node_2_ng methods, click on the sub categories under each of the parent tree displays (Courses, Assignments and Questionnaires) and all of them will further expand to show the details. (Click on the name of the Course/Asignment/Questionnaire to expand them)

Direct access to sub categories in tree display

Checking the direct access tabs from navigation bar

Checking the sub categories in each parent Tree

Re-factored Code

Refactoring get_children_node_ng method

Before Changes

After Changes

def get_children_node_ng childNodes = {} if params[:reactParams][:child_nodes].is_a? String childNodes = JSON.parse(params[:reactParams][:child_nodes]) else childNodes = params[:reactParams][:child_nodes] end tmpRes = {} res = {} for node in childNodes fnode = eval(params[:reactParams][:nodeType]).new for a in node fnode[a[0]] = a[1] end # fnode is the parent node # ch_nodes are childrens ch_nodes = fnode.get_children(nil, nil, session[:user].id, nil, nil) tmpRes[fnode.get_name] = ch_nodes # cnode = fnode.get_children("created_at", "desc", 2, nil, nil) end for nodeType in tmpRes.keys res[nodeType] = Array.new for node in tmpRes[nodeType] tmpObject = {} tmpObject["nodeinfo"] = node tmpObject["name"] = node.get_name tmpObject["type"] = node.type if nodeType == 'Courses' || nodeType == "Assignments" tmpObject["directory"] = node.get_directory tmpObject["creation_date"] = node.get_creation_date tmpObject["updated_date"] = node.get_modified_date tmpObject["private"] = node.get_private instructor_id = node.get_instructor_id tmpObject["instructor_id"] = instructor_id unless (instructor_id.nil?) tmpObject["instructor"] = User.find(instructor_id).name else tmpObject["instructor"] = nil end tmpObject["is_available"] = is_available(session[:user], instructor_id) || (session[:user].role_id == 6 && Ta.get_my_instructors(session[:user].id).include?(instructor_id) && ta_for_current_course?(node)) if nodeType == "Assignments" tmpObject["course_id"] = node.get_course_id tmpObject["max_team_size"] = node.get_max_team_size tmpObject["is_intelligent"] = node.get_is_intelligent tmpObject["require_quiz"] = node.get_require_quiz tmpObject["allow_suggestions"] = node.get_allow_suggestions tmpObject["has_topic"] = SignUpTopic.where(['assignment_id = ?', node.node_object_id]).first ? true : false end end res[nodeType] << tmpObject end end respond_to do |format| format.html {render json: res} end end

def get_children_node_ng childNodes = {} if params[:reactParams][:child_nodes].is_a? String childNodes = JSON.parse(params[:reactParams][:child_nodes]) else childNodes = params[:reactParams][:child_nodes] end ch_nodes = {} for node in childNodes # Declaring Foldernode Object as New fnode = eval(params[:reactParams][:nodeType]).new for a in node fnode[a[0]] = a[1] end # fnode is the parent node # ch_nodes are childrens ch_nodes[fnode.get_name] = fnode.get_children(nil, nil, session[:user].id, nil, nil) # cnode = fnode.get_children("created_at", "desc", 2, nil, nil) end call_function ="get_children_node_ng" #Render JSON of the child nodes populate_rows(ch_nodes,call_function) end

Refactoring get_children_node_2_ng method

Before Changes

After Changes

def get_children_node_2_ng childNodes = {} if params[:reactParams2][:child_nodes].is_a? String childNodes = JSON.parse(params[:reactParams2][:child_nodes]) else childNodes = params[:reactParams2][:child_nodes] end tmpRes = {} res = [] fnode = eval(params[:reactParams2][:nodeType]).new childNodes.each do |key, value| fnode[key] = value end ch_nodes = fnode.get_children(nil, nil, session[:user].id, nil, nil) tmpRes = ch_nodes if tmpRes for child in tmpRes nodeType = child.type res2 = {} res2["nodeinfo"] = child res2["name"] = child.get_name res2["key"] = params[:reactParams2][:key] res2["type"] = nodeType res2["private"] = child.get_private res2["creation_date"] = child.get_creation_date res2["updated_date"] = child.get_modified_date if nodeType == 'CourseNode' || nodeType == "AssignmentNode" res2["directory"] = child.get_directory instructor_id = child.get_instructor_id res2["instructor_id"] = instructor_id unless (instructor_id.nil?) res2["instructor"] = User.find(instructor_id).name else res2["instructor"] = nil end res2["is_available"] = is_available(session[:user], instructor_id) || (session[:user].role_id == 6 && Ta.get_my_instructors(session[:user].id).include?(instructor_id) && ta_for_current_course?(child)) if nodeType == "AssignmentNode" res2["course_id"] = child.get_course_id res2["max_team_size"] = child.get_max_team_size res2["is_intelligent"] = child.get_is_intelligent res2["require_quiz"] = child.get_require_quiz res2["allow_suggestions"] = child.get_allow_suggestions res2["has_topic"] = SignUpTopic.where(['assignment_id = ?', child.node_object_id]).first ? true : false end end res << res2 end end respond_to do |format| format.html {render json: res} end end

def get_children_node_2_ng # Second Level of Tree Display with Children Nodes childNodes = {} if params[:reactParams2][:child_nodes].is_a? String childNodes = JSON.parse(params[:reactParams2][:child_nodes]) else childNodes = params[:reactParams2][:child_nodes] end res = [] fnode = eval(params[:reactParams2][:nodeType]).new childNodes.each do |key, value| fnode[key] = value end ch_nodes = fnode.get_children(nil, nil, session[:user].id, nil, nil) call_function = "get_children_node_2_ng" populate_rows(ch_nodes, call_function) end

Common Methods Added to get_children_node_ng and get_children_node_2_ng controllers

populate_rows

This is a new Common Method for both get_children_node_ng and get_children_node_2_ng that does the functionality of rendering a page with data from all nodes by calling another method populate_1_row(). There is another Method call that happens in this method which as a whole renders the List View.

  def populate_rows(list,call_function) #render page with data for all nodes in list
    if call_function == "get_children_node_ng"
      tmpRes ={}
      tmpRes = list
      res = {}
       for nodeType in tmpRes.keys
        # declaring a new array
        res[nodeType] =  Array.new
        for node in tmpRes[nodeType]
          res[nodeType] << populate_1_row(node)
        end
       end

    else
      tmpRes = list
      res = []
      if tmpRes
        for child in tmpRes
          res_node = {}
          res_node = populate_1_row(child)
          res_node["key"] = params[:reactParams2][:key]
          res << res_node
        end
      end
    end
    page_render(res)
  end

populate_1_row

'This is a new Method that is called from Method populate_row() and fetches all the child node names from respective Controller actions and then populates the tmpObject.

  def populate_1_row(node) #return JSON for 1 Node
    tmpObject = {}
    tmpObject["nodeinfo"] = node
    # all the child nodes names got and put in tmpObject from respective controller actions
    tmpObject["name"] = node.get_name
    tmpObject["type"] = node.type
    if node.type == 'CourseNode' || node.type == "AssignmentNode"
      tmpObject["directory"] = node.get_directory
      tmpObject["creation_date"] = node.get_creation_date
      tmpObject["updated_date"] = node.get_modified_date
      tmpObject["private"] = node.get_private
      instructor_id = node.get_instructor_id
      tmpObject["instructor_id"] = instructor_id
      unless (instructor_id.nil?)
        tmpObject["instructor"] = User.find(instructor_id).name
      else
        tmpObject["instructor"] = nil
      end

      tmpObject["is_available"] = is_available(session[:user], instructor_id) || (session[:user].role_id == 6 && Ta.get_my_instructors(session[:user].id).include?(instructor_id) && ta_for_current_course?(node))
      if node.type == "AssignmentNode"
        tmpObject["course_id"] = node.get_course_id
        tmpObject["max_team_size"] = node.get_max_team_size
        tmpObject["is_intelligent"] = node.get_is_intelligent
        tmpObject["require_quiz"] = node.get_require_quiz
        tmpObject["allow_suggestions"] = node.get_allow_suggestions
        tmpObject["has_topic"] = SignUpTopic.where(['assignment_id = ?', node.node_object_id]).first ? true : false
      end
    end
    tmpObject
  end

page_render

This is also a common Method which integrates the rendering functionality of get_children_node_ng and get_children_node_2_ng making sure both levels are properly populated in the view.'

  def page_render(list) # Common Render Functionality for get_children_node_ng and get_children_node_2_ng methods
    respond_to do |format|
      format.html {render json: list}
    end
  end

Method Removal

There was a Method which dint have any Routes defined in the Routes.rb file and was not called in any of the Controller . After Careful analysis we descided to remove the method from the tree Display Controller.

filter

  def filter
    search = params[:filter_string]
    filter_node = params[:filternode]
    qid = 'filter+'

    if filter_node == 'QAN'
      assignment = Assignment.find_by_name(search)
      if assignment
        assignment_questionnaires = AssignmentQuestionnaire.where(assignment_id: assignment.id)
        if assignment_questionnaires
          assignment_questionnaires.each { |q|  qid << "#{q.questionnaire_id.to_s}+" }
          session[:root] = 1
        end
      end
    elsif filter_node == 'ACN'
      session[:root] = 2
      qid <<  search
    end
    return qid
  end

Removing duplicate methods

Before Changes

After Changes

# direct access to questionnaires def goto_questionnaires node_object = TreeFolder.find_by_name('Questionnaires') session[:root] = FolderNode.find_by_node_object_id(node_object.id).id redirect_to :controller => 'tree_display', :action => 'list' end # direct access to review rubrics def goto_review_rubrics node_object = TreeFolder.find_by_name('Review') session[:root] = FolderNode.find_by_node_object_id(node_object.id).id redirect_to :controller => 'tree_display', :action => 'list' end # direct access to metareview rubrics def goto_metareview_rubrics node_object = TreeFolder.find_by_name('Metareview') session[:root] = FolderNode.find_by_node_object_id(node_object.id).id redirect_to :controller => 'tree_display', :action => 'list' end # direct access to teammate review rubrics def goto_teammatereview_rubrics node_object = TreeFolder.find_by_name('Teammate Review') session[:root] = FolderNode.find_by_node_object_id(node_object.id).id redirect_to :controller => 'tree_display', :action => 'list' end # direct access to author feedbacks def goto_author_feedbacks node_object = TreeFolder.find_by_name('Author Feedback') session[:root] = FolderNode.find_by_node_object_id(node_object.id).id redirect_to :controller => 'tree_display', :action => 'list' end # direct access to global survey def goto_global_survey node_object = TreeFolder.find_by_name('Global Survey') session[:root] = FolderNode.find_by_node_object_id(node_object.id).id redirect_to :controller => 'tree_display', :action => 'list' end # direct access to surveys def goto_surveys node_object = TreeFolder.find_by_name('Survey') session[:root] = FolderNode.find_by_node_object_id(node_object.id).id redirect_to :controller => 'tree_display', :action => 'list' end # direct access to course evaluations def goto_course_evaluations node_object = TreeFolder.find_by_name('Course Evaluation') session[:root] = FolderNode.find_by_node_object_id(node_object.id).id redirect_to :controller => 'tree_display', :action => 'list' end # direct access to courses def goto_courses node_object = TreeFolder.find_by_name('Courses') session[:root] = FolderNode.find_by_node_object_id(node_object.id).id redirect_to :controller => 'tree_display', :action => 'list' end # direct access to assignments def goto_assignments node_object = TreeFolder.find_by_name('Assignments') session[:root] = FolderNode.find_by_node_object_id(node_object.id).id redirect_to :controller => 'tree_display', :action => 'list' end

# direct access def go_to_menu_items name = params[:params1] if name if name == "Review rubrics" name = "Review" elsif name == "Teammate review rubrics" name = "Teammate Review" elsif name == "Metareview rubrics" name = "Metareview" elsif name == "Author feedbacks" name = "Author Feedback" elsif name == "Global surveys" name = "Global Survey" elsif name == "Course evaluations" name = "Course Evaluation" elsif name == "Surveys" name = "Survey" end node_object = TreeFolder.find_by_name(name) session[:root] = FolderNode.find_by_node_object_id(node_object.id).id if node_object.name == "Courses" session[:last_open_tab] = 1 elsif node_object.name == "Assignments" session[:last_open_tab] = 2 elsif (node_object.id ==1 ||node_object.id ==4 ||node_object.id ==5 || node_object.id ==6 ||node_object.id ==7 ||node_object.id ==8 ||node_object.id ==9|| node_object.id ==10) # if node_object_name is Questionnaires or its child_nodes session[:last_open_tab] = 3 # for Questionnaires and all its childnodes end redirect_to :controller => 'tree_display', :action => 'list' else # if the passed params is null, redirect to root redirect_to "/" end

Rspec Test

We have written Rspec tests for the tree_display_controller and have run the same with success.

From the home directory of the application, run the test as "rspec spec/controllers/tree_display_controller_spec.rb"

require 'rails_helper'

describe TreeDisplayController do

describe "#list" do
    it "should not redirect to student_task controller if current user is an instructor" do
      allow(session[:user]).to receive("student?").and_return(false)
      post "list"
      response.should_not redirect_to(list_student_task_index_path)
end
    it "should redirect to student_task controller if current user is a student" do
       allow(session[:user]).to receive("student?").and_return(true)
       post "list"
       response.should redirect_to(list_student_task_index_path)
    end
end
describe "#ta_for_current_mappings?" do
    it "should return true if current user is a TA for current course" do
        allow(session[:user]).to receive("ta?").and_return(true)
    end
end

describe "#populate_rows" do
    let(:dbl) { double }
  before { expect(dbl).to receive(:populate_rows).with(Hash, String)}
     it "passes when the arguments match" do
      dbl.populate_rows({},"")
     end
end

describe "#populate_1_row" do
    let(:dbl) { double }
  before { expect(dbl).to receive(:populate_1_row).with(Node) }
     it "passes when the arguments match" do
        dbl.populate_1_row(Node.new)
     end
  end

  describe "#go_to_menu_items" do
    before do
          allow(nil).to receive(:find_by_node_object_id).and_return(nil)
          allow(nil).to receive(:id).and_return(nil)
          allow(nil).to receive(:name).and_return(true)
    end
      it "should receive Review Rubrics and redirect to list" do
          allow(nil).to receive(:find_by_name).with("Review").and_return(nil)
          get "go_to_menu_items", params1: "Review Rubrics"
          expect(response).to redirect_to(list_tree_display_index_path)
      end
      it "should receive Teammate review rubrics and redirect to list" do
          allow(nil).to receive(:find_by_name).with("Teammate review").and_return(nil)
          get "go_to_menu_items", params1: "Teammate review rubrics"
          expect(response).to redirect_to(list_tree_display_index_path)
      end
      it "should receive Metareview rubrics and redirect to list" do
          allow(nil).to receive(:find_by_name).with("Metareview").and_return(nil)
          get "go_to_menu_items", params1: "Metareview rubrics"
          expect(response).to redirect_to(list_tree_display_index_path)
      end
      it "should receive Author feedbacks and redirect to list" do
          allow(nil).to receive(:find_by_name).with("Author Feedback").and_return(nil)
          get "go_to_menu_items", params1: "Author feedbacks"
          expect(response).to redirect_to(list_tree_display_index_path)
      end
      it "should receive Global surveys and redirect to list" do
          allow(nil).to receive(:find_by_name).with("Global Survey").and_return(nil)
          get "go_to_menu_items", params1: "Global surveys"
          expect(response).to redirect_to(list_tree_display_index_path)
      end
      it "should receive Course evaluations and redirect to list" do
          allow(nil).to receive(:find_by_name).with("Course Evaluation").and_return(nil)
          get "go_to_menu_items", params1: "Course evaluations"
          expect(response).to redirect_to(list_tree_display_index_path)
      end
      it "should receive Surveys and redirect to list" do
          allow(nil).to receive(:find_by_name).with("Survey").and_return(nil)
          get "go_to_menu_items", params1: "Surveys"
          expect(response).to redirect_to(list_tree_display_index_path)
      end
      it "should redirect to root_url if request parameter is invalid" do
          allow(nil).to receive(:find_by_name).with(nil).and_return(nil)
          get "go_to_menu_items"
          expect(response).to redirect_to(root_path)
      end
    end
    describe "#drill" do
      it "redirect to list action" do
           get "drill" , root: 1
           session[:root].should == "1"
           expect(response).to redirect_to(list_tree_display_index_path)
      end
    end
end

References

<references></references>