CSC/ECE 517 Spring 2020 - E2006. Refactor Tree Display Controller: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
 
(47 intermediate revisions by 4 users not shown)
Line 3: Line 3:


__TOC__
__TOC__
===About Expertiza===
[http://wiki.expertiza.ncsu.edu/index.php/Expertiza_documentation Expertiza] is a web application through which students can submit and peer-review learning objects (articles, code, web sites, etc). The Expertiza project is supported by the National Science Foundation.


==Project Description==
==Project Description==


===Problem Statement===
===Problem Statement===
E2006. Refactor tree_display_controller.rb
[https://docs.google.com/document/d/1_-rgdE4Fh_Fz_Jhxw1Qj4vehbu84qiUKAiLWGcFUl5A/edit#heading=h.h37uqoqtoks4 E2006. Refactor tree_display_controller.rb]
 
===Background===
===Background===
The tree_display_controller is responsible for rendering hierarchical lists in the instructor user interface.  Here is a sample instructor homepage:
The tree_display_controller is responsible for rendering hierarchical lists in the instructor user interface.  Here is a sample instructor homepage:
Line 30: Line 27:


From the problem background three main goals were derived to focus on in the refactor of tree display controller:
From the problem background three main goals were derived to focus on in the refactor of tree display controller:
===Remove unnecessary methods===
# '''Remove unnecessary methods''' - Search the 45 methods defined in the controller to see where and why they are called. Remove outdated and unused methods, and see where it is fit to merge or replace methods.
Search the 45 methods defined in the controller to see where and why they are called. Remove outdated and unused methods, and see where it is fit to merge or replace methods.
# '''Save code and avoid DRY issues''' - In addition to removing unnecessary methods, code should be reduced wherever possible to make the controller smaller and easier to read.
===Save code and avoid DRY issues===
# '''Improve understanding''' - For all necessary, complex methods in the controller, comments should be added to improve understanding of what methods do. This way, future reviewers won't have to search for implementations to understand the functionality when working on other issues.
In addition to removing unnecessary methods, code should be reduced wherever possible to make the controller smaller and easier to read.


===Improve understanding===
==Changes==
For all necessary, complex methods in the controller, comments should be added to improve understanding of what methods do. This way, future reviewers won't have to search for implementations to understand the functionality when working on other issues.
The sections below include a link to the commits where the changes were made following the description. We believe that by providing the links to code changes that we will improve the flow of the wiki while giving a better idea of what the code changes look like in each commit. All code changes can be found in pull request [https://github.com/expertiza/expertiza/pull/1698 #1698]


==Proposed Changes==
=== Make all file scope methods private ===
=== Make all file scope methods private ===
 
Code that is only called within the <code>tree_display_controller.rb</code> was moved to the private scope. This makes the code easier to read and easier to understand what is getting called outside the controller. [https://github.com/expertiza/expertiza/pull/1698/commits/0f3cda31e9e16eb3b435ca8e2a80f26f0e094f39 Assign private designation to internal methods]
Code that is only called within the '''tree_display_controller.rb''' was moved to the private scope. This makes the code easier to read and easier to understand what is getting called outside the controller. [https://github.com/expertiza/expertiza/pull/1698/commits/0f3cda31e9e16eb3b435ca8e2a80f26f0e094f39]


=== Combine folder_node_ng_getter with children_node_ng ===
=== Combine folder_node_ng_getter with children_node_ng ===
'''folder_node_ng_getter''' was being called by the view to return '''FolderNode.get'''. The resulting json of this call was then passed, without modification, into '''children_node_ng'''. Since this was the only place '''folder_node_ng_getter''' was being called it was combined with '''children_node_ng''' to make things easier to read when when viewing controller code.
<code>folder_node_ng_getter</code> was being called by the view to return <code>FolderNode.get</code>. The resulting json of this call was then passed, without modification, into <code>children_node_ng</code>. Since this was the only place <code>folder_node_ng_getter</code> was being called it was combined with <code>children_node_ng</code> to make things easier to read when when viewing controller code. [https://github.com/expertiza/expertiza/pull/1698/commits/1d6401807b0fee302b765ea67b8024ca0f1658a7 Refactor children_node_ng]


===Refactor children_node_ng===
===Refactor children_node_ng===
<code>children_node_ng</code> was a misleading name. The method has been renamed <code>get_folder_contents</code> since it returns the contents of each top level folder (assignments, courses, and questionnaires) in json format. In addition to the name change, the <code>get_folder_contents</code> method was refactored to include the behavior of <code>folder_node_ng_getter</code>. This addition simplifies the code because the method no longer needs to convert the object that was converted to json back to an object. A single call <code>serialize_folder_to_json</code> makes the code easier to read instead of chasing nested function calls through <code>initialize_fnode_update_children</code> which ultimately leads to the same result. Another key change was changing the method from a post to a get, to better represent what it was doing. Changes to <code>tree_display.jsx</code> and <code>routes.rb</code> had to be made to account for the name change and method type change. [https://github.com/expertiza/expertiza/pull/1698/commits/1d6401807b0fee302b765ea67b8024ca0f1658a7 Refactor children_node_ng]


We determined that the '''children_node_ng''' method had a misleading name for its function. We propose that the method be renamed to '''get_folder_contents''' since it returns the contents of each top level folder (assignments, courses, and questionnaires) in json format. We believe that the proposed name change and comment make the code easier to understand (Goal 3). In addition to the name change, the '''get_folder_contents''' method is refactored to make the method itself more understandable. We also changed the method from a post to a get, to better represent what it was doing, so changes had to be made to account for the name change and method type change. In total, changes were made to '''tree_display.jsx''', '''tree_display_controller.rb''', and '''routes.rb'''.
===Refactor children_node_ng_2===
<code>children_node_ng_2</code> was a misleading name. The method has been renamed <code>get_sub_folder_contents</code> since it returns the subfolder items for the folders obtained by <code>get_folder_contents</code> in json format. Like the <code>get_folder_contents</code> refactor <code>get_sub_folder_contents</code> was simplified to make a single call to <code>serialize_sub_folder_to_json</code> making the code easier to read. Again this method was changed from a post to a get, to better represent what it was doing. Changes to <code>tree_display.jsx</code> and <code>routes.rb</code> had to be made to account for the name change and method type change. [https://github.com/expertiza/expertiza/pull/1698/commits/d7a364edce289f0efb8487429ba8fec8038f3557 Refactor children_node_ng_2] [https://github.com/expertiza/expertiza/pull/1698/commits/9a1d5caec559d15e576faf10fb55952cf9d99bf7 Rename children_node_2_ng to get_sub_folder_contents] [https://github.com/expertiza/expertiza/pull/1698/commits/867fa9d3f9d5cc988824ce2890e5ebc90708bf79 Rename serialize_to_json to serialize_sub_folder_to_json]


====Changes to tree_display_controller.rb====
=== Inline function calls to make json serialization more clear ===
Where it could be done without making the existing functions unwieldy functions that updated json properties were moved up one level. This makes it easier to see in one place what properties are being modified by the serialization functions. [https://github.com/expertiza/expertiza/pull/1698/commits/d7acad1904a5431d1714008b738bc127582d53a7 Inline function calls to make json serialization more clear]


[[File:Diff_get_folder_contents_png.PNG|Diff_get_folder_contents_png.PNG]]
=== Simplify checks for determining a users access to view a course ===
 
With guidance from Dr. Gehringer it was determined a TA should have access to view all items for a given course if and only if they are mapped to that course. Likewise a instructor should be able to view all of the items for courses they teach. Admins and super admins have access to all items on the website. These facts allowed us to greatly simplify the checks necessary to determine if a course was available to the current user and if it should be private or not. [https://github.com/expertiza/expertiza/pull/1698/commits/2ea9a2286688ef173613efc1d19e1fdf460af4d0 Simplified is_available checks] [https://github.com/expertiza/expertiza/pull/1698/commits/a9dcbe459ffbab9effa0589b9f618b0a7fb19810 Removed unused is_available checks]
====Changes to tree_display.jsx====
 
[[File:Diff_get_folder_js.PNG]]
 
====Changes to routes.rb====
 
[[File:Diff_config_routes_get_folder.PNG]]


===Collapse 'goto' methods===
===Collapse 'goto' methods===
In the legacy code found in <code>tree_display_controller.rb</code>, 60 lines are dedicated to the 12 methods related to traversing the menu system. 11 of the methods call the <code>goto_controller()</code> method with different values. Those 11 methods were condensed into single line methods with one overarching comment to describe them. This change makes the code easier to read, since each of the 11 methods condensed carry out the same functionality, and easier to understand since we have added in a better comment for understanding what the code does (Goal 3). This also saved 41 lines of code in the controller (Goal 2). [https://github.com/expertiza/expertiza/pull/1698/commits/2732cef5eb3bcf258b61447277bd82d8d58f7c54 Compact gotos]


In the legacy code found in '''tree_display_controller.rb''', 60 lines are dedicated to the 12 methods related to traversing the menu system. 11 of the methods call the '''goto_controller()''' method with different values, so we decided that those 11 methods could be condensed into single line methods with one overarching comment to describe them. We believe that this change makes the code easier to read, since each of the 11 methods condensed carry out the same functionality, and easier to understand since we have added in a better comment for understanding what the code does (Goal 3). This also saved 41 lines of code in the controller (Goal 2).
===Remove unnecessary code to sort assignments by instructor and creation date===
====Original Implementation====
By default assignments are received from <code>FolderNode.get</code> sorted by instructor and creation date. There is no need to resort this object. [https://github.com/expertiza/expertiza/pull/1698/commits/81a401083e5a4fd2d9720fd015dd7d515b808f83 Remove unnecessary code to sort assignments by instructor and creation date]
<pre style="color: red">
  # refactored method to provide direct access to parameters
  def goto_controller(name_parameter)
    node_object = TreeFolder.find_by(name: name_parameter)
    session[:root] = FolderNode.find_by(node_object_id: node_object.id).id
    redirect_to controller: 'tree_display', action: 'list'
  end
  # direct access to questionnaires
  def goto_questionnaires
    goto_controller('Questionnaires')
  end
  # direct access to review rubrics
  def goto_review_rubrics
    goto_controller('Review')
  end
  # direct access to metareview rubrics
  def goto_metareview_rubrics
    goto_controller('Metareview')
  end
  # direct access to teammate review rubrics
  def goto_teammatereview_rubrics
    goto_controller('Teammate Review')
  end
  # direct access to author feedbacks
  def goto_author_feedbacks
    goto_controller('Author Feedback')
  end
  # direct access to global survey
  def goto_global_survey
    goto_controller('Global Survey')
  end
  # direct access to surveys
  def goto_surveys
    goto_controller('Assignment Survey')
  end
  # direct access to course surveys
  def goto_course_surveys
    goto_controller('Course Survey')
  end
  # direct access to courses
  def goto_courses
    goto_controller('Courses')
  end
  def goto_bookmarkrating_rubrics
    goto_controller('Bookmarkrating')
  end
  # direct access to assignments
  def goto_assignments
    goto_controller('Assignments')
  end
</pre>
====Proposed Implementation====
<pre style="color: green">
  # The goto_ methods listed below are used to traverse the menu system. It is  
  # hard to tell exactly where they are called from, but at least some (if not all)
  # are necessary. These functions may be better suited for another controller.
  def goto_controller(name_parameter)
    node_object = TreeFolder.find_by(name: name_parameter)
    session[:root] = FolderNode.find_by(node_object_id: node_object.id).id
    redirect_to controller: 'tree_display', action: 'list'
  end
  def goto_questionnaires; goto_controller('Questionnaires') end
  def goto_review_rubrics; goto_controller('Review') end
  def goto_metareview_rubrics; goto_controller('Metareview') end
  def goto_teammatereview_rubrics; goto_controller('Teammate Review') end
  def goto_author_feedbacks; goto_controller('Author Feedback') end
  def goto_global_survey; goto_controller('Global Survey') end
  def goto_surveys; goto_controller('Assignment Survey') end
  def goto_course_surveys; goto_controller('Course Survey') end
  def goto_courses; goto_controller('Courses') end
  def goto_bookmarkrating_rubrics; goto_controller('Bookmarkrating') end
  def goto_assignments; goto_controller('Assignments') end
</pre>


===Remove unused functions===
===Remove unused functions===
By searching for the implementations of all functions in the controller we were able to recognize some of them as being unused and remove them (Goal 1). Others became unused once refactoring was completed.
By searching for the implementations of all functions in the controller we were able to recognize some of them as being unused and remove them (Goal 1). Others became unused once refactoring was completed.
* filter()
* <code>filter()</code>
* filter_node_is_qan()
* <code>filter_node_is_qan()</code>
* courses_assignments_obj()
These methods were both removed because they were only used in the page_header partial for tree display, which was [https://github.com/expertiza/expertiza/commit/167cd7893e2c7eab660495ea29ff2678024d9c47 removed approximately 3 years prior.]
* confirm_notifications_access()
* <code>courses_assignments_obj()</code>
* child_nodes_from_params()
* <code>confirm_notifications_access()</code>
* res_node_for_child()
* <code>child_nodes_from_params()</code>
* bridge_to_is_available()
* <code>res_node_for_child()</code>
* update_tmp_obj()
These methods are not used in the app.
* action()
* <code>update_tmp_obj()</code>
This method was used only once in the app, and it was in the tree display controller, since the purpose of the method was aligned with the purpose of the <code>serialize_to_json</code> method which called it there was no reason to write a new method to call. Especially since the <code>update_tmp_obj</code> method was only making minor changes to the json.
* <code>bridge_to_is_available()</code>
* <code>action()</code>
These methods are not called in the app. References to these methods were removed from <code>routes.rb</code>


====Changes to tree_display_controller.rb====
== Test Plan ==


The '''filter''' and '''filter_node_is_qan''' methods were both removed because they were only used in the page_header partial for tree display, which was [https://github.com/expertiza/expertiza/commit/167cd7893e2c7eab660495ea29ff2678024d9c47 removed approximately 3 years prior.]
All tests described below have been implemented in the [https://github.com/RPHolloway/expertiza/blob/beta/spec/controllers/tree_display_controller_spec.rb tree_display_controller_spec.rb file]. Since this project was focused on refactoring the tree_display_controller.rb code, and not on adding any functionality, we have edited the spec file to accommodate for our changes. [https://github.com/expertiza/expertiza/pull/1698/files?file-filters%5B%5D=.rb#diff-daf992d845e06cb7f3dff81021736967 All changes to the spec file] correspond to the refactoring that we have done.
;<code>#list</code> should not redirect if current user is an instructor.
:A user is defined to be an instructor. When the <code>get "list"</code> method is called, the response should not redirect to the tree_display/list


<pre style="color: red">
;<code>#list</code> should not redirect if current user is a TA.
  def filter
:A user is defined to be an instructor. When the <code>get "list"</code> method is called, the response should not redirect to the tree_display/list
    qid = 'filter+'
    search = params[:filter_string]
    filter_node = params[:filternode]
    if filter_node == 'QAN'                      # Questionaire Assignment Name
      qid = filter_node_is_qan(search, qid)
    elsif filter_node == 'ACN'                    # Assignment Course Name
      session[:root] = 2
      qid << search
    end
    qid
  end
</pre>


<pre style="color: red">
;<code>#list</code> should redirect to student_task#list if current user is a student
  # if filter node is 'QAN', get the corresponding assignment questionnaires
:A user is defined to be a student. Instead of redirecting to the tree_display list, it should redirect to the student_task list.  
  def filter_node_is_qan(search, qid)
    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}+" }
        session[:root] = 1
      end
    end
    qid
  end
</pre>


The '''course_assignments_obj''' method is not called anywhere else in the app.
;<code>#list</code> should redirect to login page if current user is nil.
:This is a catch all for if a user somehow directs to the list without signing in, they shouldn't be able to see anything without signing in first.


<pre style="color: red">
;<code>#ta_for_current_mappings?</code> should return true if the current user is a TA for current course
  def courses_assignments_obj(node_type, tmp_object, node)
:This test should check that the method properly returns true if it receives a ta as the user.
    update_tmp_obj(tmp_object, node)
    # tmpObject["private"] = node.get_private
    instructor_id = node.get_instructor_id
    ## if current user's role is TA for a course, then that course will be listed under his course listing.
    update_in_ta_course_listing(instructor_id, node, tmp_object)
    update_instructor(tmp_object, instructor_id)
    update_is_available(tmp_object, instructor_id, node)
    assignments_method(node, tmp_object) if node_type == "Assignments"
  end
</pre>


The '''confirm_notifications_access''' method is not called anywhere else in the app.
;<code>#drill</code> should redirect to list action
:The drill method should redirect to the tree_display controller with the list action.


<pre style="color: red">
;<code>GET #session_last_open_tab</code> should return a status of 200.
  def confirm_notifications_access
:The HTTP status 200 is the OK SUCCESS status to show that the get request succeeds.
    redirect_to controller: :notifications, action: :list if current_user.try(:student?)
  end
</pre>


The '''child_nodes_from_params''' method is not called anywhere else in the app
;<code>GET #set_session_last_open_tab</code> should return a status of 200.
:The HTTP status 200 is the OK SUCCESS status to show that the get request succeeds.


<pre style="color: red">
;<code>GET #get_folder_contents</code> should return a list of course objects(private) as a json.
  # finding out child_nodes from params
:When there is a course (private) and an assignment defined, the get_folder_contents method should match the json file provided in the test.
  def child_nodes_from_params(child_nodes)
    if child_nodes.is_a? String and !child_nodes.empty?
      JSON.parse(child_nodes)
    else
      child_nodes
    end
  end
</pre>


The '''res_node_for_child''' method is not called anywhere else in the app.
;<code>GET #get_folder_contents</code> should return an empty list if no courses
:When the public and private courses have been deleted/aren't present, then the list returned should be empty, consisting of 'Courses: []'


<pre style="color: red">
;<code>GET #get_folder_contents</code> should return a list of course objects(public) as a json.
  # getting result nodes for child
:When there is a course (public) and an assignment defined, the get_folder_contents method should match the json file provided in the test.
  # Changes to this method were done as part of E1788_OSS_project_Maroon_Heatmap_fixes
  #
  # courses_assignments_obj method makes a call to update_in_ta_course_listing which
  # separates out courses based on if he/she is the TA for the course passed
  # by marking private to be true in that case
  #
  # this also ensures that instructors (who are not ta) would have update_in_ta_course_listing
  # not changing the private value if he/she is not TA which was set to true for all courses before filtering
  # in update_tmp_obj in courses_assignments_obj
  #
  # below objects/variable names were part of the project as before and
  # refactoring could have affected other functionalities too, so it was avoided in this fix
  #
  # fix comment end
  #
  def res_node_for_child(tmp_res)
    res = {}
    tmp_res.each_key do |node_type|
      res[node_type] = []
      tmp_res[node_type].each do |node|
        tmp_object = {
          "nodeinfo" => node,
          "name" => node.get_name,
          "type" => node.type
        }
        courses_assignments_obj(node_type, tmp_object, node) if %w[Courses Assignments].include? node_type
        res[node_type] << tmp_object
      end
    end
    res
  end
</pre>


The '''bridge_to_is_available''' method is not called anywhere else in the app.
;<code>GET #get_folder_contents for TA</code> should return an empty array if the logged in user is not a TA.
:Edge case testing to ensure that a user with the role of student results in an empty array.


<pre style="color: red">
;<code>GET #get_folder_contents for TA</code> should return a list of course objects the TA is supposed to TA in as json.
  def bridge_to_is_available
:If the logged in user is a TA, then return should be a json of all courses that the user is a ta for.
    user = session[:user]
    owner_id = params[:owner_id]
    is_available(user, owner_id)
  end
</pre>


The '''update_tmp_obj''' method was used only once in the app, and it was in the tree display controller. We decided that since the purpose of the method was aligned with the purpose of the '''serialize_to_json''' method which called it, there was no reason to write a new method to call, especially since the '''update_tmp_obj''' method seemed like it was only making minor changes to the json.
;<code>GET #get_folder_contents for TA</code> should return an empty list when there is no mapping between a ta and a course.
:If the TA is not mapped to a course then the return should be empty.


<pre style="color: red">
;<code>GET #get_folder_contents for TA</code> should return only courses that a TA is a TA for. It should not return any courses that the user is a student in, but not a TA in.
  def update_tmp_obj(tmp_object, node)
:Edge case test for when the user is a student in some classes, but a TA in others.
    tmp = {
      "directory" => node.get_directory,
      "creation_date" => node.get_creation_date,
      "updated_date" => node.get_modified_date,
      "institution" => Institution.where(id: node.retrieve_institution_id),
      "private" => node.get_instructor_id == session[:user].id
    }
    tmp_object.merge!(tmp)
  end
</pre>


====Changes to routes.rb====
;<code>GET #get_folder_contents for TA</code> should return only the course he/she is TA of when the TA is a student of the same course.
:Edge case test for a student in a course also being a TA, most likely because of an error in assignment.


We removed the call for '''bridge_to_is_available''' and found that the '''action''' method was also unused.
;<code>GET #get_folder_contents for TA</code> returns only the assignments which belongs to the course he/she is TA of.
:All assignments in the courses that the TA is assigned to should be returned.


[[File:Diff_config_routes_remove_functions.PNG]]
;<code>GET #get_folder_contents for TA</code> should return an empty list if none of the assignments belong to the course he/she is TA of.
:Edge case test for when there are assignments, but they don't belong to courses which he/she is a TA.


=== Assume an object with FolderNode properties is passed to children_node_2_ng ===
=== Test Results ===


=== Populate JSON object within serialize methods ===
[[File:E2006_test_results.png]]
 
==Code Coverage==
To be completed.


==Team Information==
==Team Information==
Line 318: Line 143:


Special thanks to our mentor and professor: <b>Dr. Edward Gehringer</b>
Special thanks to our mentor and professor: <b>Dr. Edward Gehringer</b>
==References==
To be completed.

Latest revision as of 02:56, 1 April 2020

E2006. Refactor tree_display_controller.rb

Project Description

Problem Statement

E2006. Refactor tree_display_controller.rb

Background

The tree_display_controller is responsible for rendering hierarchical lists in the instructor user interface. Here is a sample instructor homepage:

The image below shows that specific questionnaires are nested under different questionnaire categories. This means that the node for a questionnaire has a parent_id that points to a node for a questionnaire category. Similarly, listings of courses and their assignments are hierarchical.

The various rubrics, as well as surveys, are types of questionnaires. So the parent_id of a particular rubric will be the type of rubric (e.g., teammate review) that it is.

The tree_display_controller was 379 lines long, and contained 45 methods. Many of those methods had similar names and were performing very similar functions. In an effort to save code and avoid DRY issues this file was refactored.

This effort reduced the controller to less than 200 lines, 20 public methods (15 of which are necessary scaffold for menu navigation), and 6 private methods.

Goals

From the problem background three main goals were derived to focus on in the refactor of tree display controller:

  1. Remove unnecessary methods - Search the 45 methods defined in the controller to see where and why they are called. Remove outdated and unused methods, and see where it is fit to merge or replace methods.
  2. Save code and avoid DRY issues - In addition to removing unnecessary methods, code should be reduced wherever possible to make the controller smaller and easier to read.
  3. Improve understanding - For all necessary, complex methods in the controller, comments should be added to improve understanding of what methods do. This way, future reviewers won't have to search for implementations to understand the functionality when working on other issues.

Changes

The sections below include a link to the commits where the changes were made following the description. We believe that by providing the links to code changes that we will improve the flow of the wiki while giving a better idea of what the code changes look like in each commit. All code changes can be found in pull request #1698

Make all file scope methods private

Code that is only called within the tree_display_controller.rb was moved to the private scope. This makes the code easier to read and easier to understand what is getting called outside the controller. Assign private designation to internal methods

Combine folder_node_ng_getter with children_node_ng

folder_node_ng_getter was being called by the view to return FolderNode.get. The resulting json of this call was then passed, without modification, into children_node_ng. Since this was the only place folder_node_ng_getter was being called it was combined with children_node_ng to make things easier to read when when viewing controller code. Refactor children_node_ng

Refactor children_node_ng

children_node_ng was a misleading name. The method has been renamed get_folder_contents since it returns the contents of each top level folder (assignments, courses, and questionnaires) in json format. In addition to the name change, the get_folder_contents method was refactored to include the behavior of folder_node_ng_getter. This addition simplifies the code because the method no longer needs to convert the object that was converted to json back to an object. A single call serialize_folder_to_json makes the code easier to read instead of chasing nested function calls through initialize_fnode_update_children which ultimately leads to the same result. Another key change was changing the method from a post to a get, to better represent what it was doing. Changes to tree_display.jsx and routes.rb had to be made to account for the name change and method type change. Refactor children_node_ng

Refactor children_node_ng_2

children_node_ng_2 was a misleading name. The method has been renamed get_sub_folder_contents since it returns the subfolder items for the folders obtained by get_folder_contents in json format. Like the get_folder_contents refactor get_sub_folder_contents was simplified to make a single call to serialize_sub_folder_to_json making the code easier to read. Again this method was changed from a post to a get, to better represent what it was doing. Changes to tree_display.jsx and routes.rb had to be made to account for the name change and method type change. Refactor children_node_ng_2 Rename children_node_2_ng to get_sub_folder_contents Rename serialize_to_json to serialize_sub_folder_to_json

Inline function calls to make json serialization more clear

Where it could be done without making the existing functions unwieldy functions that updated json properties were moved up one level. This makes it easier to see in one place what properties are being modified by the serialization functions. Inline function calls to make json serialization more clear

Simplify checks for determining a users access to view a course

With guidance from Dr. Gehringer it was determined a TA should have access to view all items for a given course if and only if they are mapped to that course. Likewise a instructor should be able to view all of the items for courses they teach. Admins and super admins have access to all items on the website. These facts allowed us to greatly simplify the checks necessary to determine if a course was available to the current user and if it should be private or not. Simplified is_available checks Removed unused is_available checks

Collapse 'goto' methods

In the legacy code found in tree_display_controller.rb, 60 lines are dedicated to the 12 methods related to traversing the menu system. 11 of the methods call the goto_controller() method with different values. Those 11 methods were condensed into single line methods with one overarching comment to describe them. This change makes the code easier to read, since each of the 11 methods condensed carry out the same functionality, and easier to understand since we have added in a better comment for understanding what the code does (Goal 3). This also saved 41 lines of code in the controller (Goal 2). Compact gotos

Remove unnecessary code to sort assignments by instructor and creation date

By default assignments are received from FolderNode.get sorted by instructor and creation date. There is no need to resort this object. Remove unnecessary code to sort assignments by instructor and creation date

Remove unused functions

By searching for the implementations of all functions in the controller we were able to recognize some of them as being unused and remove them (Goal 1). Others became unused once refactoring was completed.

  • filter()
  • filter_node_is_qan()

These methods were both removed because they were only used in the page_header partial for tree display, which was removed approximately 3 years prior.

  • courses_assignments_obj()
  • confirm_notifications_access()
  • child_nodes_from_params()
  • res_node_for_child()

These methods are not used in the app.

  • update_tmp_obj()

This method was used only once in the app, and it was in the tree display controller, since the purpose of the method was aligned with the purpose of the serialize_to_json method which called it there was no reason to write a new method to call. Especially since the update_tmp_obj method was only making minor changes to the json.

  • bridge_to_is_available()
  • action()

These methods are not called in the app. References to these methods were removed from routes.rb

Test Plan

All tests described below have been implemented in the tree_display_controller_spec.rb file. Since this project was focused on refactoring the tree_display_controller.rb code, and not on adding any functionality, we have edited the spec file to accommodate for our changes. All changes to the spec file correspond to the refactoring that we have done.

#list should not redirect if current user is an instructor.
A user is defined to be an instructor. When the get "list" method is called, the response should not redirect to the tree_display/list
#list should not redirect if current user is a TA.
A user is defined to be an instructor. When the get "list" method is called, the response should not redirect to the tree_display/list
#list should redirect to student_task#list if current user is a student
A user is defined to be a student. Instead of redirecting to the tree_display list, it should redirect to the student_task list.
#list should redirect to login page if current user is nil.
This is a catch all for if a user somehow directs to the list without signing in, they shouldn't be able to see anything without signing in first.
#ta_for_current_mappings? should return true if the current user is a TA for current course
This test should check that the method properly returns true if it receives a ta as the user.
#drill should redirect to list action
The drill method should redirect to the tree_display controller with the list action.
GET #session_last_open_tab should return a status of 200.
The HTTP status 200 is the OK SUCCESS status to show that the get request succeeds.
GET #set_session_last_open_tab should return a status of 200.
The HTTP status 200 is the OK SUCCESS status to show that the get request succeeds.
GET #get_folder_contents should return a list of course objects(private) as a json.
When there is a course (private) and an assignment defined, the get_folder_contents method should match the json file provided in the test.
GET #get_folder_contents should return an empty list if no courses
When the public and private courses have been deleted/aren't present, then the list returned should be empty, consisting of 'Courses: []'
GET #get_folder_contents should return a list of course objects(public) as a json.
When there is a course (public) and an assignment defined, the get_folder_contents method should match the json file provided in the test.
GET #get_folder_contents for TA should return an empty array if the logged in user is not a TA.
Edge case testing to ensure that a user with the role of student results in an empty array.
GET #get_folder_contents for TA should return a list of course objects the TA is supposed to TA in as json.
If the logged in user is a TA, then return should be a json of all courses that the user is a ta for.
GET #get_folder_contents for TA should return an empty list when there is no mapping between a ta and a course.
If the TA is not mapped to a course then the return should be empty.
GET #get_folder_contents for TA should return only courses that a TA is a TA for. It should not return any courses that the user is a student in, but not a TA in.
Edge case test for when the user is a student in some classes, but a TA in others.
GET #get_folder_contents for TA should return only the course he/she is TA of when the TA is a student of the same course.
Edge case test for a student in a course also being a TA, most likely because of an error in assignment.
GET #get_folder_contents for TA returns only the assignments which belongs to the course he/she is TA of.
All assignments in the courses that the TA is assigned to should be returned.
GET #get_folder_contents for TA should return an empty list if none of the assignments belong to the course he/she is TA of.
Edge case test for when there are assignments, but they don't belong to courses which he/she is a TA.

Test Results

Team Information

Brooks Anderson

Rick Holloway

PJ Loheide

Special thanks to our mentor and professor: Dr. Edward Gehringer

Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2020_-_E2006._Refactor_Tree_Display_Controller&oldid=132611"