CSC/ECE 517 Fall 2015/oss E1571

Introduction

Background

Expertiza has objects, which represent an assignment that is done by some number of users. An instructor or a TA can perform several kinds of operations on assignments, such as, ”Add participants”, “Create teams”, and “View scores”. These operations can be seen: First, on the homepage, under the “Actions” column in assignment list when a user (instructor or TA or admin) logs in and navigates to Manage -> Assignments.

Second, when editing an assignment (by clicking on edit logo above), on the tab called “Other stuff”.

Motivation

This project in particular intends that the students collaborate with each other and work on making enhancements to the code base by applying the concepts of Rails,RSpec, DRY code,Test driven development etc. This provides an opportunity for students to contribute to an open source project and learn further about software deployment etc.

Currently, the Assignment controller holds methods which are deprecated and also has a few lines code in its Edit and Update methods which could be removed to make the code DRY. In addition to that, it houses a few actions where the code could be refactored to make it DRY and follow the Ruby style guide and Rails 4 syntax.

Tasks Identified

• Refactor edit and update method
• Remove irrelevant comments from the create action.
• associate_assignment_with_course and remove_assignment_from_course logic should be moved to model.
• Change to new redirect method rather using controller and action explicitly.
• Refactor the update_due_dates from AssignmentForm. using the DRY principle
• Refactor code to follow ruby style guide and Rails4 syntax.

Classes

• controllers/assignments_controller.rb
• models/Assignment.rb

Refactoring Edit function

Edit function contains many manipulations that need be refactored into separate functions. The following block contains manipulations such as checking if due_date name or description is not defined.

  @due_date_all.each do |dd| 
  if((!dd.deadline_name.nil?&&!dd.deadline_name.empty?)||(!dd.description_url.nil?&&!dd.description_url.empty?))
      @due_date_nameurl_notempty = true
      @due_date_nameurl_notempty_checkbox = true
    end
   ....
  end

This part of the code is moved into a separate function.

  def is_due_date_nameurl_notempty(dd)
   (!dd.deadline_name.nil?&&!dd.deadline_name.empty?)||(!dd.description_url.nil?&&!dd.description_url.empty?)
  end

There are lots of redundant code in the following snippet.And, constants are hard coded inside the code rather than defining constants.

if dd.deadline_type_id==5
       @metareview_allowed = true
     end
     if @due_date_nameurl_notempty && @due_date_nameurl_notempty_checkbox && @metareview_allowed
       break
     end
     if dd.deadline_type_id==6
       @drop_topic_allowed = true
     end
     if @due_date_nameurl_notempty && @due_date_nameurl_notempty_checkbox && @drop_topic_allowed
       break
     end
     if dd.deadline_type_id==7
       @signup_allowed = true
     end
     if @due_date_nameurl_notempty && @due_date_nameurl_notempty_checkbox && @signup_allowed
       break
     end
     if dd.deadline_type_id==8
       @team_formation_allowed = true
     end
     if @due_date_nameurl_notempty && @due_date_nameurl_notempty_checkbox && @team_formation_allowed
       break
     end
   end

Redundant code is removed and made it more efficient and understandable.

     @metareview_allowed = is_meta_review_allowed?(dd);
     @drop_topic_allowed = is_drop_topic_allowed?(dd);
     @signup_allowed = is_signup_allowed?(dd);
     @team_formation_allowed = is_team_formation_allowed?(dd);

     if dd.due_at.present?
       dd.due_at = dd.due_at.to_s.in_time_zone(current_user.timezonepref)
     end
     if  @due_date_nameurl_notempty && @due_date_nameurl_notempty_checkbox &&
         (@metareview_allowed || @drop_topic_allowed || @signup_allowed || @team_formation_allowed)
       break
     end

Hard coded constants are removed and necessary constants are defined in the deadline_helper.rb helper file.

 DEADLINE_TYPE_METAREVIEW = 5
 DEADLINE_TYPE_DROP_TOPIC = 6
 DEADLINE_TYPE_SIGN_UP = 7
 DEADLINE_TYPE_TEAM_FORMATION = 8

And, manipulations such as checking if metareview, drop_topic,singn_up and team_formation are allowed need to be refactored into separate functions.

 def is_meta_review_allowed?(dd)
   status = false
   if dd.deadline_type_id== DeadlineHelper::DEADLINE_TYPE_METAREVIEW
     status = true
   end
   status
 end

 def is_drop_topic_allowed?(dd)
   status = false
   if dd.deadline_type_id==DeadlineHelper::DEADLINE_TYPE_DROP_TOPIC
     status = true
   end
   status
 end

 def is_signup_allowed?(dd)
   status = false
   if dd.deadline_type_id==DeadlineHelper::DEADLINE_TYPE_SIGN_UP
     status = true
   end
   status
 end

 def is_team_formation_allowed?(dd)
   status = false
   if dd.deadline_type_id==DeadlineHelper::DEADLINE_TYPE_TEAM_FORMATION
     status = true
   end
   status
 end

The following snippet constructs a string that need to be shown to the user when rubrics are not set by the instructor.

if !empty_rubrics_list.empty? and request.original_fullpath == "/assignments/#{@assignment_form.assignment.id}/edit"
     empty_rubrics = "["
     empty_rubrics_list.each do |item|
       empty_rubrics += item[0...-13] + ", "
     end
     empty_rubrics = empty_rubrics[0...-2]
     empty_rubrics += "] "
     flash.now[:error] = "You did not specify all necessary rubrics: " +empty_rubrics+" of assignment #{@assignment_form.assignment.name} before saving the assignment. You can assign rubrics <a id='go_to_tabs2' style='color: blue;'>here</a>."
   end

Adding code such as above directly into the controller action made the edit function bulkier. The above snippet is also refactored into a separate function.

def needed_rubrics(empty_rubrics_list)
   ...
 end

Refactoring out into separate functions makes the code more readable and understandable.

Refactoring Update function

Update function of the Assignment controller helps in updating the resources after editing it. The following code retrieves the data of the user logged in.

  session[:user]

The above statement checks the cookies every time, when we want to retrieve information about current user. As it is inefficient and wrong, we need to use helper functions to retrieve such information.

Explicit URL creation using controller name and controller action is refactored in such a way that helper functions create the URL when required.

redirect_to :action => 'edit', :id => @assignment.id  # Incorrect
redirect_to edit_assignment_path @assignment.id    # Correct

Remove irrelevant comments from the create action

The Create action has a few comments which are irrelevant and make the method bulky. These could be safely removed from the code.

associate_assignment_with_course moved to model

This is currently defined as an action in AssignmentsController. The action logic could be moved to Model by passing the current user object to the corresponding method we create in the Model. The method in the model would then return a list of courses which would be used in the View corresponding to this action.

Inside controller action,

 def associate_assignment_with_course
   @assignment = Assignment.find(params[:id])
   @courses = Assignment.set_courses_to_assignment(current_user)
 end

Inside model,

 def self.set_courses_to_assignment(user)
   @courses=Course.where(instructor_id: user.id).order(:name)
 end

remove_assignment_from_course moved to model

All the current logic here, except for the "save" and "redirect_to" calls could be moved to the model. There is a need to create a method in the Model with the same name and then pass an assignment object. All operations can be performed inside this method and then the user can be redirected back to the assignment list page. Inside controller action,

 def remove_assignment_from_course
   assignment = Assignment.find(params[:id])
   Assignment.remove_assignment_from_course(assignment)
   redirect_to controller: 'tree_display', action: 'list'
 end

Inside model,

def self.remove_assignment_from_course(assignment)
   oldpath = assignment.path rescue nil
   assignment.course_id = nil
   assignment.save
   newpath = assignment.path rescue nil
   FileHelper.update_file_location(oldpath, newpath)
 end

Change to new redirect method rather using controller and action explicitly.

According to Ruby style guidelines explicit use of action and controller in redirect calls should be avoided. This helps in easy refactoring in case action names are changed in future. All the redirect calls in assignment controller have been modified to use url path helpers. In controller assignment and action toggle_access, redirect call has been changes as follows

 def toggle_access
   assignment = Assignment.find(params[:id])
   assignment.private = !assignment.private
   assignment.save
   redirect_to list_tree_display_index_path
 end

Refactor the update_due_dates from AssignmentForm using the DRY principle

There exist variables which are declared and initialized but are not used in any part of the code. Such variables could be safely removed without impacting the functionality of the action. Conditional statements which check whether due_date object is present can be avoided. The same piece of code is used at various points of code making it redundant and hence can be converted to a method. There exist dead code which tries to remove due_dates not in update or new list from database. There can never be such a case, so the code can be safely removed.

Changes to the View

Impact Analysis

• Tested using Selenium IDE, the associated objects , assignment and course before and after the change. Testing confirms that the objects and their interactions have not changed.
• Addition of new methods do not coincide with the performance of currently existent methods.
• Removal of existing code does not lead to reduced/loss of functionality.

Affected Classes

Changed existent classes

• controllers/assignments_controller.rb
• controllers/application_controller.rb
• models/Assignment.rb
• models/User.rb
• views/tree_display/actions/_assignments_actions.html.erb

Running the Project Locally

The project could be run locally by cloning the Github repository expertiza and then running the following commands sequentially.

bundle install
rake db:create:all
rake db:migrate
rails s

Testing using Selenium IDE

Selenium IDEis implemented as a Firefox extension. It allows user to record edit and debug test cases. It has a recording feature, which records user actions as they are performed and then exports them as a reusable script in one of many programming languages that can be later executed. The UI automation helps in testing quickly yet rigorously. Major test cases identified are following

• Loading assignments
• Creating assignments
• Editing assignments
• Updating due dates for assignments
• Associating assignments to a course
• Deleting assignments

The test cases were manually run and recorded once. Then the test cases were re run with changed parameters on the deployed version of the application. Test scripts can be found here.

Testing Instructions

Follow the instructions below to test the application using selenium IDE

• Install Selenium IDE plugin for Firefox.
• Login to Expertiza using username: instructor6 & password: password.
• Download the test scripts from here
• Open the test script from Selenium IDE. File -> open -> select test script html file
• Set the testing speed to slow on the speed slidebar, otherwise test may fail due to slow page response
• Run the script from Selenium IDE using 'Play current test case' icon.

To add a new assignment or topic and to remove an assignment or topic, we need to change the script every time to avoid duplicate record IDs. When we run the same script more than once, the test case will fail. Thus, we haven't provided test case for those scenarios.

Future Work

The tasks accomplish only a part of the refactoring and as always with any software application there is scope for even more. The following are a few which were identified as part of this project.

• There exists lot of redundant code which code be refactored into methods.
• Unused variables and deprecated methods could be ridden off from the code base.
• The test cases are written only for the methods that came under the scope of this project. But there is room for implementing test cases for the other actions too.
• In an MVC framework, most of the business logic should be within the Model. Work needs to be done on this front to make it more close to this convention of Rails.