CSC/ECE 517 Fall 2020 - E2063. Refactor tree-display.js and tree display controller.rb: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
Line 71: Line 71:


===New Implementation===
===New Implementation===
*The method paginate_list has been split into 2 methods now.
** BuildSearchCriteria – as the name suggests the sole purpose of this method is to build a search criteria based on the input search filters when the current user initiates a search in versions.
** paginate_list – this method will call the paginate API.
:First the search criteria is built, then the criteria is applied to versions in the database to get all versions which matches the criteria and then the retrieved versions are paginated.
<pre>
  # pagination.
  def paginate_list(versions)
    paginate(versions, VERSIONS_PER_PAGE);
  end
  def BuildSearchCriteria(id, user_id, item_type, event)
    # Set up the search criteria
    search_criteria = ''
    search_criteria = search_criteria + add_id_filter_if_valid(id).to_s
    if current_user_role? == 'Super-Administrator'
      search_criteria = search_criteria + add_user_filter_for_super_admin(user_id).to_s
    end
    search_criteria = search_criteria + add_user_filter
    search_criteria = search_criteria + add_version_type_filter(item_type).to_s
    search_criteria = search_criteria + add_event_filter(event).to_s
    search_criteria = search_criteria + add_date_time_filter
    search_criteria
  end
</pre>
* The string literals and conditions in the method paginate_list were replaced with methods with intuitive names so that the programmer can understand the code more easily. We also removed an empty if clause and a redundant statement.
<pre>
  def add_id_filter_if_valid (id)
    "id = #{id} AND " if id && id.to_i > 0
  end
  def add_user_filter_for_super_admin (user_id)
    "whodunnit = #{user_id} AND " if user_id && user_id.to_i > 0
  end
  def add_user_filter
    "whodunnit = #{current_user.try(:id)} AND " if current_user.try(:id) && current_user.try(:id).to_i > 0
  end
  def add_event_filter (event)
    "event = '#{event}' AND " if event && !(event.eql? 'Any')
  end
  def add_date_time_filter
    "created_at >= '#{time_to_string(params[:start_time])}' AND " +
        "created_at <= '#{time_to_string(params[:end_time])}'"
  end
  def add_version_type_filter (version_type)
    "item_type = '#{version_type}' AND " if version_type && !(version_type.eql? 'Any')
  end
</pre>
* The paginate method has been moved to the helper class Pagination_Helper. This new method can be now reused by the different components like UsersController etc. The method receives two parameters, first the list to paginate and second the number of items to be displayed in a page.
<pre>
module PaginationHelper
  def paginate (items, number_of_items_per_page)
    items.page(params[:page]).per_page(number_of_items_per_page)
  end
end
</pre>


===Code improvements===
===Code improvements===

Revision as of 03:16, 14 October 2020

E2063. Refactoring the tree-display.js and tree display controller.rb

This page provides a description of the Expertiza based OSS project.



About Expertiza

Expertiza is an open source project based on Ruby on Rails framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

Background

The tree-display.js and its tree_display_controller.rb files are designed to allow Expertiza users to view their Assignments, Courses and Questionnaires at one place. This is the primary control page, as well as the home page for instructors on Expertiza which allows them to create, modify, delete and view Assignments.

The primary problem with this is that both the files, due to their bulky and unoptimized methods, slow the rendering of UI on screen. The methods in these files can be studied and refactored to improve the overall performance of this controller and its corresponding UI. Moreover, any obsolete or unused methods can be removed and DRY principle should be implemented. This project mostly revolves around these 2 files, and would involve refactoring JavaScript more than Ruby on Rails. Knowledge of JavaScript is a prerequisite for this project.


Accomplishments

The following tasks were accomplished in this project:


  • Removed methods that were not used
  • Implemented previous changes from past groups on this assignment if useful
  • Modified tree display controller to better parse data prior to being accessed by the react client



About Tree Display Controller

This class manages different the different tabs: courses, assignments, and questionaires.

Current Implementation

Functionality
  • Instructors can view the course, assignment and questionnaire tabs.


Drawbacks and Solutions
  • Problem 1: Checks in tree_display.jsx are too complicated.
In
 showElement={_this.state.expandedRow.indexOf(entry.type+'_'(parseInt(entry.nodeinfo.node_object_id)*2).toString()+'_'+i) 
> -1 ? "" : "none"}
  • Solution: For now, assign showElement as true.
 showElement={true}
  • Problem 2: The beta branch isn't getting a prop related to the data to be displayed in the dropdown


 
var FilterableTable = React.createClass({
   ....
}
  • Solution:
  • Problem 3: Tree display controller needs to better parse the data prior to being access to the react client
  • Solution:
  • Problem 4: Components in tree-display are passed too far down when only a small amount of it is needed at a time.
  • Solution:

New Implementation

Code improvements

  • Introduced a constant VERSIONS_PER_PAGE and assigned the value 25 to it. The pagination algorithm for VersionsController displays at most 25 versions in a page. The existing implementation uses the value 25 straight in the code and there are few problems associated with such an approach.
    • It is not easy to understand what 25 is unless the programmer takes a close look at the code.
    • In case if the value 25 is used at more than one places and in future a new requirement comes to show at most 30 versions in a page, all the values will have to be modified. It is not very DRY.
  • The VersionsController was overriding AccessHelper - action_allowed? method to return true in all the cases. This was violating the whole purpose of the method action_allowed?. The purpose of this method is to determine whether the user who is triggering a CRUD operation is allowed to do so. So when the current user invokes a CRUD operation, the action_allowed? method is invoked first and if the method returns true the CRUD operation is triggered or else the user is intimated with a message and gracefully exited. Hence, when the action_allowed? method is overridden to return true always, it results in providing unauthorized access to certain users.
def action_allowed?
    true
  end
With the new implementation the AccessHelper - action_allowed? method has been modified in such a way that unauthorized access is prevented. As per the new algorithm, 'new', 'create', 'edit', 'update' cannot be invoked by any user. These operations can be accessed only by ‘papertrail’ gem. Only an ‘Administrator’ or ‘Super-Administrator’ can call 'destroy_all' method. All the other methods are accessible to ‘Administrator’, ‘Super-Administrator’, ‘Instructor’, ‘Teaching Assistant’ and ‘Student’.
  def action_allowed?
    case params[:action]
    when 'new', 'create', 'edit', 'update'
    #Modifications can only be done by papertrail
      return false
    when 'destroy_all'
      ['Super-Administrator',
       'Administrator'].include? current_role_name
    else
      #Allow all others
      ['Super-Administrator',
       'Administrator',
       'Instructor',
       'Teaching Assistant',
       'Student'].include? current_role_name
    end
  end

Automated Testing using RSPEC

The current version of expertiza did not have any test for VersionsController. Using the test driven development(TDD) approach, we have added an exhaustive set of RSPEC tests for VersionsController, to test all the modifications we have done to the code of the controller class. The tests use double and stub features of rspec-rails gem, to fake the log in by different users - Administrator, Instructor, Student etc. The tests can be executed "rpec spec" command as shown below. 

user-expertiza $rspec spec
.
.
.
Finished in 5.39 seconds (files took 25.33 seconds to load)
66 examples, 0 failures

Randomized with seed 19254
.
.

Testing from UI

Following are a few testcases with respectto our code changes that can be tried from UI: 1. To go to versions index page, type in the following url after logging in: 

  http://152.46.16.81:3000/versions

2. After logging in as student/instructor or admin : Try accessing the new, create, edit, update actions. These actions are not allowed to any of the users. 

  http://152.46.16.81:3000/versions/new
  This calls the new action. In the current production version of expertiza, it is unhandled and application gives a default 404 page.

3. Another feature that can be tested from UI is Pagination. Try searching for a user's versions and see if the results are paginated or not. Search here: 

  http://152.46.16.81:3000/versions/search

4. Visit the same URL as step 3, you should see only the students under that instructor in the users dropdown.

References

  1. Expertiza on GitHub
  2. GitHub Project Repository Fork
  3. The live Expertiza website
  4. Demo link
  5. Expertiza project documentation wiki
  6. Rspec Documentation
  7. Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin
Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2020_-_E2063._Refactor_tree-display.js_and_tree_display_controller.rb&oldid=135145"