CSC/ECE 517 Fall 2014/oss E1453 syy: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
m (Created page with "'''E1453. Refactoring AssignmentsController''' This page provides a description of the Expertiza based OSS project. This project aimed at refactoring the AssignmentsController a...")
 
(Undo revision 94945 by Rlcoble (talk))
 
(27 intermediate revisions by 3 users not shown)
Line 5: Line 5:
'''Introduction to Expertiza'''<br>
'''Introduction to Expertiza'''<br>


[http://expertiza.ncsu.edu/ Expertiza] is a project developed using Ruby on Rails platform. It provides features like peer review, team assignments and submission of projects. This can be achieved by submitting code base, URL of hosted code on remote server and Wiki submissions. It is an open source application and the code can be cloned from [https://github.com/expertiza/expertiza github]. This application provides an efficient way to manage assignments, grades and reviews. This makes the process easier and faster when the class strength is large.  
[http://expertiza.ncsu.edu/ Expertiza] is a project developed using the [http://rubyonrails.org/ Ruby on Rails] platform. It provides features like peer review, team assignments and submission of projects. This can be achieved by submitting code base, URL of hosted code on remote server and Wiki submissions. It is an open source application and the code can be cloned from [https://github.com/expertiza/expertiza GitHub]. This application provides an efficient way to manage assignments, grades and reviews, which makes the process easier and faster when the class strength is large.  


Expertiza is supported by National Science Foundation under Grant No. 0536558. Additional funding from the NCSU Learning in a Technology-Rich Environment (LITRE) program, the NCSU Faculty Center for Teaching and Learning, the NCSU STEM Initiative, and the Center for Advanced Computing and Communication.
Expertiza is supported by National Science Foundation under Grant No. 0536558. Additional funding from the [http://www.ncsu.edu/ NCSU] [http://litre.ncsu.edu/ Learning in a Technology-Rich Environment] (LITRE) program, the NCSU [http://ofd.ncsu.edu/teaching-learning/ Faculty Center for Teaching and Learning], the NCSU [http://stem.ncsu.edu/ STEM] Initiative, and the Center for Advanced Computing and Communication.


= Project Resources =


[https://github.com/itsmylifesoham/expertiza GitHub Project Repository]
__TOC__
 
[http://152.46.18.5:3000/ VCL IP Address]
 
[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki]


= Problem Statement =
= Problem Statement =
Currently the topics tab is not usable to the instructor when he is editing a current assignment or creating a new one. This functionality is necessary as it enables the instructor to create/delete/edit/view topics present under the selected assignment. Currently, the instructor achieves this using the pop-up menu that appears adjacent every assignment on the page that lists all the assignments. We were required to implement the topics tab.
'''Classes involved:'''
assignment_controller.rb
sign_up_sheet_controller.rb
assignment.rb
sign_up_sheet.rb


The functionality to display the topics under an assignment is duplicated between the AssignmentController views and the SignupSheetController views. Both the instructor and the student can view the topics by selecting an assignment and we were required to remove this duplication in the views.
'''What they do:'''
The assignment model and controller are used to create and edit assignments.  It provides tabs for rubrics, review strategy, due dates, and topics. The sign_up_sheet model and controller handle operations on the topics associated with each assignment.


'''What needs to be done:'''
The <b>Topics</b> tab in the <b>Assignments --> Edit</b> view is not usable right now by an instructor when he/she is editing an assignment.  The functionality that displays topics also needs to be usable by the student view, since students also have to view the list of topics (when they sign up for a topic).  Instructor and student functionality is a little different: instructors can add topics and change the number of slots, but they can’t sign up for topics. The Topics tab needs to be fixed, edit/delete topic functionality is to be added for instructors and common/duplicate code needs to be refactored.


= Re-factored Code Cases =
The functionality to display the topics under an assignment is duplicated between the AssignmentController views and the SignupSheetController views. Both the instructor and the student can view the topics by selecting an assignment and we were required to remove this duplication in the views. Consequently, most of our work dealt with modifying the SignupSheetController and its corresponding views.


== Case 1 : Refactoring in sign_up_sheet_controller.rb ==


===Current Scenario===
=Changes Made=
Different method definitions for setting up staggered and non-staggered assignment sign up sheet.


<b>method:add_signup_topics_staggered</b><br>
==SignupSheet Controller==
This method lists all the available topics for an assignment and allows admin to set due dates for assignment.Staggered means that different topics can have different deadlines.
  def add_signup_topics_staggered
    load_add_signup_topics(params[:id])
    ....
   
    #below functionality finds allows to set due dates for topics of assignment
    @duedates = SignUpTopic.find_by_sql("SELECT s.id as topic_id FROM sign_up_topics s WHERE s.assignment_id = " + params[:id].to_s)
    ....
  end


<b>method:add_signup_topics</b><br>
{| class="wikitable"
It is similar to the above function except that all the topics and review/submission rounds have the similar deadlines.
|-
  def add_signup_topics
! style="width:13%;"|Method Name
    load_add_signup_topics(params[:id])
! style="width:33%;"|Changes Made
  end
! style="width:43%;"|Reason For Change
|- style="vertical-align:top;"
| create
| Fixed redirection after topic creation to Topics tab
| On successful creation of a topic, the action was merely redirecting to the assignments/edit view
|-
| update
| Fixed redirection after topic updation to Topics tab
| Once a topic was successfully edited, the view rendered was just a list of topics
|-
| edit
| Initialized @assignment_id variable
| The variable is used in the edit view
|-
| rowspan="3" valign="middle" |
destroy
| Renamed the 'delete' method to 'destroy'
| This was done for better Rails 4 routing compatibility
|-
| Initialized the assignment_id parameter
| This is used further on and is not passed from the link URL
|-
| Fixed redirection after topic deletion to Topics tab
| Once a topic was successfully deleted, the view rendered was just a list of topics
|}


<b>Conditional call</b><br>
==Views==
We had to make conditional check every time the function is called for the assignment type. For instance, below method redirects to the /add_signup_topics or the /add_signup_topics_staggered page depending on assignment type
def redirect_to_sign_up(assignment_id)
    assignment = Assignment.find(assignment_id)
    if assignment.staggered_deadline == true
      redirect_to :action => 'add_signup_topics_staggered', :id => assignment_id
    else
      redirect_to :action => 'add_signup_topics', :id => assignment_id
    end
end


===After Changes===
{| class="wikitable"
We have redundant code in two methods so we merged add_signup_topics_staggered and add_signup_topics into add_signup_topics. The action calls single method for both type of assignments and renders views based on condition of staggered or non-staggered assignment.
|-
! style="width:18%;"|View Name
! style="width:33%;"|Changes Made
! style="width:43%;"|Reason For Change
|- style="vertical-align:top;"
| rowspan="5" valign="middle" |
sign_up_sheet/_add_signup_topcis
| Fixed title
| The page title displayed for an instructor was "Sign-up sheet for ... "
|-
| Commented out View/Manage bookmark links
| These links were broken, and according to existing code were not meant for the instructor view.
|-
| Added variable initialization for @sign_up_topics
| The variable was referenced further on in the view, which was causing an 'nil value' error
|-
| Fixed flash message rendering for new topic creation
| The flash message was rendering incorrectly with HTML tags visible on the web page
|-
| Added full path names to partial names
| The shared partials were also being used by views in the Assignments controller, so full paths were needed
|-
| sign_up_sheet/_add_topics
| Added separation between the Import Topics and Manage Bookmarks links
| The two links were clumped together and it was difficult to distinguish between them
|-
| sign_up_sheet/_actions
| Replaced rendering of the reserve_topic partial with the _all_actions partial
| The reserve_topic partial is no longer being used and is replaced by the newly created _all_actions partial
|-
| sign_up_sheet/_all_actions
| Created this new partial
| We created the _all_actions partial to conditionally render the "Actions" table field for both instructors and students. This now also includes working links through which instructors or administrators can edit or delete topics.
|-
| sign_up_sheet/_table_line
| Gave edit/delete rights to Super Admin
| The Super-Administrator user wasn't included in the list of users with these permissions
|-
| sign_up_sheet/edit
| Modified title and included @assignment_id variable to be passed as a parameter variable to the update action
| The view was initially trying to access the params[:assignment_id] variable which was not getting initialized
|-
| assignments/edit
| Changed the partial rendered to /sign_up_sheet/add_signup_topcis.html
| The assignments/edit/add_signup_topics partial (with duplicated code) was otherwise being rendered
|}


def add_signup_topics
= Re-factored Code Cases =
    load_add_signup_topics(params[:id])
    if @assignment.staggered_deadline ==true
      ....
     
      #below functionality finds allows to set due dates for topics of assignment
      @duedates = SignUpTopic.find_by_sql("SELECT s.id as topic_id FROM sign_up_topics s WHERE s.assignment_id = " + params[:id].to_s)
      ....
     
      #Render a view different for staggered deadlines assignment from the add_sign_up.html.erb
      render "add_signup_topics_staggered"
  end
end
 
<b>Single method call</b><br>
At several places in the project, there was separate method call based on staggered and non-staggered assignments. We have made a single method call and condition check in various files ,for type of assignment, has been removed as part of re-factoring.
 
def redirect_to_sign_up(assignment_id)
    assignment = Assignment.find(assignment_id)
    redirect_to :action => 'add_signup_topics', :id => assignment_id
end
 
Similarly the method call is re-factored in various views.
For instance, for view _assignments_actions.html.erb
 
<b>Before Changes</b><br>
<% <b>if @assignment.staggered_deadline == true</b> %>
    <%= link_to image_tag('/images/tree_view/edit-signup-sheet-24.png', :title => 'Edit signup sheet'),
      {:controller => 'sign_up_sheet', <b>:action => 'add_signup_topics_staggered'</b>, :id => node.node_object_id},
      {:title => 'Edit signup sheet'} %>
<% else %>
    <%= link_to image_tag('/images/tree_view/edit-signup-sheet-24.png', :title => 'Edit signup sheet'),
      {:controller => 'sign_up_sheet', <b>:action => 'add_signup_topics'</b>, :id => node.node_object_id},
      {:title => 'Edit signup sheet'} %>
<% end %>
 
<b>After Changes</b><br>
  <%= link_to image_tag('/images/tree_view/edit-signup-sheet-24.png', :title => 'Edit signup sheet'),
      {:controller => 'sign_up_sheet', <b>:action => 'add_signup_topics'</b>, :id => node.node_object_id},
      {:title => 'Edit signup sheet'} %>
 
== Case 2 : Method Extraction  ==
 
Improved readability by [http://sourcemaking.com/refactoring/extract-method extracting] two separate functionalities from the same method into separate methods with appropriate naming convention and responsibilities.


===Current Scenario===
== Case 1 : Refactoring the AssignmentsController and SignupSheetController ==


The add_signup_topics calculated due dates for separate topics for a Staggered assignment with individual Topic deadlines.
===Removing duplicate _add_signup_topics partials===
The AssignmentsController displayed topics to the instructor while he was editing an assignment. It used _add_signup_topics.html.erb partial to render topics under the topics tab. The SignupSheetController displayed topics to a student who wanted to view signup topics under an assignment. It was found that it had its own copy namely _add_signup_topcis.html.erb to display topics to the student. We wanted to have these two controllers use the same views to render the same thing which was list of topics under an assignment. We made the AssignmentsController reuse the _add_signup_topcis.html.erb partial under the SignupSheetController.


def add_signup_topics
<b>view:assignments/edit.html.erb</b><br>
  load_add_signup_topics(params[:id])
This view contains all the tabs to display like general,rubrics,review_strategy, topics which are related to the selected assignment. tabs-5 is the topics tab which we are interested in.
  if @assignment.staggered_deadline ==true
<pre>
      @review_rounds = Assignment.find(params[:id]).get_review_rounds
  <div id="tabs-3">
      @topics = SignUpTopic.find_all_by_assignment_id(params[:id])
         <%= render 'assignments/edit/review_strategy' %>
     
  </div>
    <b><i>#below functionality finds allows to set due dates for topics of assignment
  <div id="tabs-4">
      @duedates = SignUpTopic.find_by_sql("SELECT s.id as topic_id FROM sign_up_topics s WHERE s.assignment_id = " + params[:id].to_s)
         <%= render 'assignments/edit/due_dates' %>
      if !@topics.nil?
  </div>
         i=0
  <div id="tabs-5">
        @topics.each { |topic|
        <%= render 'assignments/edit/add_signup_topics' %>
        ......
  </div>
         i = i + 1
</pre>
      end</i></b>
     
      #Render a view different for staggered deadlines assignment from the add_sign_up.html.erb
      render "add_signup_topics_staggered"
  end
end


===After Changes===
===After Changes===
We have redundant code in two methods so we merged add_signup_topics_staggered and add_signup_topics into add_signup_topics. The action calls single method for both type of assignments and renders views based on condition of staggered or non-staggered assignment.


The logic to calculate due_dates was moved to a separate method called assignment_due_date , as this is only valid for staggered assignments 
  <pre>
and hence it should be placed in a different method instead of merging it the add_signup_topics
  <div id="tabs-3">
         <%= render 'assignments/edit/review_strategy' %>
  def add_signup_topics
  </div>
    load_add_signup_topics(params[:id])
  <div id="tabs-4">
  if @assignment.staggered_deadline ==true
        <%= render 'assignments/edit/due_dates' %>
      @review_rounds = Assignment.find(params[:id]).get_review_rounds
  </div>
      @topics = SignUpTopic.find_all_by_assignment_id(params[:id])
   <div id="tabs-5">
   
        <%= render '/sign_up_sheet/add_signup_topcis.html' %>
  <b><i>#Function call to set up due dates
  </div>
      assignment_due_date</i></b>
</pre>
     
  #Render a view different from the add_sign_up.html.erb  , so rendering a view for staggered deadlines.
      render "add_signup_topics_staggered"
    end
  end
 
  def assignment_due_date
    #below functionality finds allows to set due dates for topics of assignment
      @duedates = SignUpTopic.find_by_sql("SELECT s.id as topic_id FROM sign_up_topics s WHERE s.assignment_id = " + params[:id].to_s)
      if !@topics.nil?
        i=0
        @topics.each { |topic|
        ......
        i = i + 1
      end
  end
 
==Case 3 : Modularize Code in application.rb==
===Current Scenario===
In the assignment.rb model, there exists one method(check_condition) which checks whether review, meta-review, etc are allowed or not. This method makes database calls to TopicDeadline table, DueDate and DeadlineType tables. Following is the code which was existing before the changes were made. It has the logic to find the nearest due date that hasn't passed.
 
<b>assignment.rb</b>
 
    drop_topic_deadline_id = DeadlineType.find_by_name('drop_topic').id
    if self.staggered_deadline?
     
      if topic_id
          
        next_due_date = TopicDeadline.find(:first, :conditions => ['topic_id = ? and due_at >= ? and deadline_type_id <> ?',
                                  topic_id, Time.now, drop_topic_deadline_id], :order => 'due_at')
      else
       
        next_due_date = TopicDeadline.find(:first, :conditions => ['assignment_id = ? and due_at >= ? and deadline_type_id <> ?',
                                  self.id, Time.now, drop_topic_deadline_id], :joins => {:topic => :assignment}, :order => 'due_at')
      end
    else
      next_due_date = DueDate.find(:first, :conditions => ['assignment_id = ? and due_at >= ? and deadline_type_id <> ?',
                                  self.id, Time.now, drop_topic_deadline_id], :order => 'due_at')
    end
 
===After changes===
The database queries have been moved to their respective models. The queries to TopicDeadline table, DueDate table and DeadlineType table have been moved to their models. Below is the new code in assignments.rb model which calls TopicDeadline.rb, DueDate.rb and DeadlineType.rb respectively for the queries in their tables:
 
<b>assignment.rb</b> model
 
if self.staggered_deadline?
   next_due_date = TopicDeadline.find_next_due_date(topic_id,self.id)
else
  next_due_date = DueDate.find_next_due_date(self.id)
end
 
New <b>find_next_due_date</b> defined in <b>topic_deadline.rb</b>
def self.find_next_due_date(topic_id,assignment_id)
    drop_topic_deadline_id = DeadlineType.find_drop_topic_deadline_id()
    if topic_id
      TopicDeadline.find(:first, :conditions => ['topic_id = ? and due_at >= ? and deadline_type_id <> ?',
                        topic_id, Time.now, drop_topic_deadline_id], :order => 'due_at')
    else
      TopicDeadline.find(:first, :conditions => ['assignment_id = ? and due_at >= ? and deadline_type_id <> ?',
                        assignment_id, Time.now, drop_topic_deadline_id], :joins => {:topic => :assignment}, :order => 'due_at')
    end
end


New <b>find_next_due_date</b> method defined in <b>due_date.rb</b>
=== Refactoring view code into relevant views ===
There were no edit/delete links to edit/delete a topic in the instructor view. These had to be conditionally added based on the role of the currently logged in user. This was to prevent students from editing/deleting topics and to keep instructors from signing up for a topic. This was implemented as follows.


def self.find_next_due_date(assignment_id
===Adding Edit/Delete functionality to topics tab===
  drop_topic_deadline_id = DeadlineType.find_drop_topic_deadline_id()
  DueDate.find(:first, :conditions => ['assignment_id = ? and due_at >= ? and deadline_type_id <> ?',
                assignment_id, Time.now, drop_topic_deadline_id], :order => 'due_at')
end


New <b>find_drop_topic_deadline_id</b> method defined in <b>deadline_type.rb</b>
The current expertiza project had two views to display the Actions column in the topics table. These were _reserve_topics.html.erb and _actions.html. The first one was displaying the signup button for the student and the latter was displaying the bookmarks and rendering the _reserve_topics.html.erb after the bookmarks. On top of this we had to add the edit/delete links for the instructor. A refactor was called for. We decided to create a _all_actions.html.erb partial which had the responsibility to display the relevant actions depending upon the role of the currently logged in user. This helped us remove the separate partial to render actions pertaining to a student, namely _reserve_topics.html.erb . Also the _actions.html.erb only took care of displaying the bookmark links rather than dealing with both bookmarks and actions.  


def self.find_drop_topic_deadline_id()
<b>partial_view:_all_actions,html.erb</b>
  DeadlineType.find_by_name('drop_topic').id
<pre>
end
<% if ['Instructor', 'Teaching Assistant', 'Administrator', 'Super-Administrator'].include? current_user_role?.name %>
    <td align="center">
      <%= link_to image_tag('edit_icon.png', :border => 0, :title => 'Edit Topic', :align => 'middle'), :controller=>'sign_up_sheet', :action=> 'edit', :id => topic.id %>
      <%= link_to image_tag('delete_icon.png', :border => 0, :title => 'Delete Topic', :align => 'middle'), sign_up_sheet_path(topic.id), method: :delete, data: {confirm: 'Are you sure?'} %>
<%elsif @show_actions %>
    ...
    #Render signup button for student here. The @show_actions will be true if this partial is navigated from the SignupSheetController which is accessible only to students.
</pre>


== Case 4 : DeadCode Elimination in  sign_up_sheet_controller.rb ==


The local variables topic_set and topic under the staggered deadline check statement were never used in the entire scope of the project , it introduced code smells. So it was checked if there was any reference to this code, and upon discovering it was not used anywhere, we decided to eliminate this [http://en.wikipedia.org/wiki/Dead_code_elimination deadcode].
== Case 2 : Cleaning up unused code ==
The Refactors mentioned above enable us to delete unused partials namely:
===Current Scenario===


def create_default_for_microtask
*assignments controller partials :
    .....
**_add_signup_topics.html.erb
 
**_reserve_topic.html.erb
    if @assignment.staggered_deadline?
      topic_set = Array.new
      topic = @sign_up_topic.id
    end
   
    .....
  end


===After Changes===
*signup_sheet_controller partials :
**_reserve_topic.html.erb
def create_default_for_microtask
<br>
    .....
 
    .....
  end


=Steps to verify changes=
=Steps to verify changes=
Log into the application with the user having instructor's role or super user's role.
1. Go to Manage -> Assignments and hover over the assignment “StaggeredDeadlines_Refactoring”.


2. Click on “New Topic”.
===Instructor Role===
Log into the application with the user having instructor's role (user3, password). [http://152.46.18.5:3000/ VCL application link]


3. Enter the details of the topic you want to create. Create at least 2 topics for better analysis of the project.
1. Go to Manage -> Assignments and expand the assignments section.  


[[File:AddTopic.png]]
2. Hover over the options symbol adjacent to the assignment and select the edit icon from the popup.


4. Now once you have created two new topics , then Click on  “Show start/due date”.
3. We have pre-created an assignment with multiple topics with id. 253, so you can click this link to edit it - http://152.46.18.5:3000/assignments/253/edit


5. You should be able to view the new start and due dates of your topics confirming these are the staggered deadlines for “StaggeredDeadlines_Refactoring” which has many topics with different deadlines.
4. Click on topics tab.


[[File:ShowdueDate.png]]
[[File:Topics tab.png]]


6. Now to view the difference between the Stagerred and Non-Staggered Deadlines, follow steps.


7. Go to Manage -> Assignments and hover over “SALT Demo” and click on “Edit  Signup sheet”.
<br>
5. You can edit or delete any topic here as you are in instructor role.


8. Click on “New Topic” and fill in the details for a new topic.
[[File:Edit-topic.png]]


[[File:NonstaggeredAddTopic.png]]
6.  Also you can undo, redo any changes you have made.


9. Now you will be able to see that there is no option for “Show Start/Due Date” which is because of the reason that this is not a staggered assignment.
[[File:Undo.png]]


[[File:NodueDate.png]]
===Student Role===
1. Log into the application with a user with Student role (user10, password).


10. Hence there is only one due date and has no dependencies on others.
2. We have added this student from the instructor view into assignment id. 253 that we used above. So this is accessible to the student. The name of the assignment appears in the
student view as Copy of assignment 1.


= Concise Report on Refactoring =
[[File:Student-view.png]]


The project had redundant code, multiple functionalities in a method and extra conditional statements in controller, model and view files. We analyzed each occurrence of these issues, thus creating a list which needed to be refactored. Controllers like sign_up_sheet_controler, assignment.rb, due_date.rb,deadline_types.rb along with other files were modified. It's very difficult to explain the sequential and detailed procedure, but it has been explained in the sections above within the capacity of the Wiki.
3. Click on the Copy of assignment 1 -> SignupSheet and the student can see the edited topics for the assignment with a signup icon and no edit/delete options which are instructor specific.


= Additional Learning and Future Work=
[[File:Signupsheet.png]]


In current implementation, our database has two tables for staggered and non-staggered deadlines. For any future improvements, we can make use of [http://blog.thirst.co/post/14885390861/rails-single-table-inheritance STI (Single Table Inheritance)] and analyze if applying STI would really impact query performance when application runs and extracts data from database.
=See Also=
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/itsmylifesoham/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
#[http://152.46.18.5:3000/ VCL link] - This might not be available after Nov. 2014
#[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza project documentation wiki]
#[https://docs.google.com/document/d/1FZCL9KWSdVNsX9BowuZ3gxbCOJoiWX-GVLctSZei3No The OSS project requirements doc (Fall 2014 - Expertiza)]


= References=
= References=
[http://expertiza.ncsu.edu/ Expertiza]<br>
[http://expertiza.ncsu.edu/ Expertiza]<br>
[http://eewang.github.io/blog/2013/03/12/how-and-when-to-use-single-table-inheritance-in-rails/ Single Table Inheritance]<br>
[http://en.wikipedia.org/wiki/Code_refactoring Code Refactoring]<br>
[http://en.wikipedia.org/wiki/Code_refactoring Code Refactoring]<br>
[http://en.wikipedia.org/wiki/Code_smell Code smell]
[http://en.wikipedia.org/wiki/Code_smell Code smell]

Latest revision as of 02:40, 19 March 2015

E1453. Refactoring AssignmentsController

This page provides a description of the Expertiza based OSS project. This project aimed at refactoring the AssignmentsController as well as adding some missing functionality related to topics within an assignment.

Introduction to Expertiza

Expertiza is a project developed using the Ruby on Rails platform. It provides features like peer review, team assignments and submission of projects. This can be achieved by submitting code base, URL of hosted code on remote server and Wiki submissions. It is an open source application and the code can be cloned from GitHub. This application provides an efficient way to manage assignments, grades and reviews, which makes the process easier and faster when the class strength is large.

Expertiza is supported by National Science Foundation under Grant No. 0536558. Additional funding from the NCSU Learning in a Technology-Rich Environment (LITRE) program, the NCSU Faculty Center for Teaching and Learning, the NCSU STEM Initiative, and the Center for Advanced Computing and Communication.


Problem Statement

Classes involved: 

assignment_controller.rb
sign_up_sheet_controller.rb
assignment.rb
sign_up_sheet.rb

What they do: The assignment model and controller are used to create and edit assignments. It provides tabs for rubrics, review strategy, due dates, and topics. The sign_up_sheet model and controller handle operations on the topics associated with each assignment.

What needs to be done: The Topics tab in the Assignments --> Edit view is not usable right now by an instructor when he/she is editing an assignment. The functionality that displays topics also needs to be usable by the student view, since students also have to view the list of topics (when they sign up for a topic). Instructor and student functionality is a little different: instructors can add topics and change the number of slots, but they can’t sign up for topics. The Topics tab needs to be fixed, edit/delete topic functionality is to be added for instructors and common/duplicate code needs to be refactored.

The functionality to display the topics under an assignment is duplicated between the AssignmentController views and the SignupSheetController views. Both the instructor and the student can view the topics by selecting an assignment and we were required to remove this duplication in the views. Consequently, most of our work dealt with modifying the SignupSheetController and its corresponding views.


Changes Made

SignupSheet Controller

Method Name Changes Made Reason For Change
create Fixed redirection after topic creation to Topics tab On successful creation of a topic, the action was merely redirecting to the assignments/edit view
update Fixed redirection after topic updation to Topics tab Once a topic was successfully edited, the view rendered was just a list of topics
edit Initialized @assignment_id variable The variable is used in the edit view

destroy

Renamed the 'delete' method to 'destroy' This was done for better Rails 4 routing compatibility
Initialized the assignment_id parameter This is used further on and is not passed from the link URL
Fixed redirection after topic deletion to Topics tab Once a topic was successfully deleted, the view rendered was just a list of topics

Views

View Name Changes Made Reason For Change

sign_up_sheet/_add_signup_topcis

Fixed title The page title displayed for an instructor was "Sign-up sheet for ... "
Commented out View/Manage bookmark links These links were broken, and according to existing code were not meant for the instructor view.
Added variable initialization for @sign_up_topics The variable was referenced further on in the view, which was causing an 'nil value' error
Fixed flash message rendering for new topic creation The flash message was rendering incorrectly with HTML tags visible on the web page
Added full path names to partial names The shared partials were also being used by views in the Assignments controller, so full paths were needed
sign_up_sheet/_add_topics Added separation between the Import Topics and Manage Bookmarks links The two links were clumped together and it was difficult to distinguish between them
sign_up_sheet/_actions Replaced rendering of the reserve_topic partial with the _all_actions partial The reserve_topic partial is no longer being used and is replaced by the newly created _all_actions partial
sign_up_sheet/_all_actions Created this new partial We created the _all_actions partial to conditionally render the "Actions" table field for both instructors and students. This now also includes working links through which instructors or administrators can edit or delete topics.
sign_up_sheet/_table_line Gave edit/delete rights to Super Admin The Super-Administrator user wasn't included in the list of users with these permissions
sign_up_sheet/edit Modified title and included @assignment_id variable to be passed as a parameter variable to the update action The view was initially trying to access the params[:assignment_id] variable which was not getting initialized
assignments/edit Changed the partial rendered to /sign_up_sheet/add_signup_topcis.html The assignments/edit/add_signup_topics partial (with duplicated code) was otherwise being rendered

Re-factored Code Cases

Case 1 : Refactoring the AssignmentsController and SignupSheetController

Removing duplicate _add_signup_topics partials

The AssignmentsController displayed topics to the instructor while he was editing an assignment. It used _add_signup_topics.html.erb partial to render topics under the topics tab. The SignupSheetController displayed topics to a student who wanted to view signup topics under an assignment. It was found that it had its own copy namely _add_signup_topcis.html.erb to display topics to the student. We wanted to have these two controllers use the same views to render the same thing which was list of topics under an assignment. We made the AssignmentsController reuse the _add_signup_topcis.html.erb partial under the SignupSheetController.

view:assignments/edit.html.erb
This view contains all the tabs to display like general,rubrics,review_strategy, topics which are related to the selected assignment. tabs-5 is the topics tab which we are interested in. 

   <div id="tabs-3">
        <%= render 'assignments/edit/review_strategy' %>
   </div>
   <div id="tabs-4">
        <%= render 'assignments/edit/due_dates' %>
   </div>
   <div id="tabs-5">
        <%= render 'assignments/edit/add_signup_topics' %>
   </div>

After Changes

We have redundant code in two methods so we merged add_signup_topics_staggered and add_signup_topics into add_signup_topics. The action calls single method for both type of assignments and renders views based on condition of staggered or non-staggered assignment. 

   <div id="tabs-3">
        <%= render 'assignments/edit/review_strategy' %>
   </div>
   <div id="tabs-4">
        <%= render 'assignments/edit/due_dates' %>
   </div>
   <div id="tabs-5">
        <%= render '/sign_up_sheet/add_signup_topcis.html' %>
   </div>

Refactoring view code into relevant views

There were no edit/delete links to edit/delete a topic in the instructor view. These had to be conditionally added based on the role of the currently logged in user. This was to prevent students from editing/deleting topics and to keep instructors from signing up for a topic. This was implemented as follows.

Adding Edit/Delete functionality to topics tab

The current expertiza project had two views to display the Actions column in the topics table. These were _reserve_topics.html.erb and _actions.html. The first one was displaying the signup button for the student and the latter was displaying the bookmarks and rendering the _reserve_topics.html.erb after the bookmarks. On top of this we had to add the edit/delete links for the instructor. A refactor was called for. We decided to create a _all_actions.html.erb partial which had the responsibility to display the relevant actions depending upon the role of the currently logged in user. This helped us remove the separate partial to render actions pertaining to a student, namely _reserve_topics.html.erb . Also the _actions.html.erb only took care of displaying the bookmark links rather than dealing with both bookmarks and actions.

partial_view:_all_actions,html.erb 

<% if ['Instructor', 'Teaching Assistant', 'Administrator', 'Super-Administrator'].include? current_user_role?.name %>
    <td align="center">
      <%= link_to image_tag('edit_icon.png', :border => 0, :title => 'Edit Topic', :align => 'middle'), :controller=>'sign_up_sheet', :action=> 'edit', :id => topic.id %>
      <%= link_to image_tag('delete_icon.png', :border => 0, :title => 'Delete Topic', :align => 'middle'), sign_up_sheet_path(topic.id), method: :delete, data: {confirm: 'Are you sure?'} %>
<%elsif @show_actions %> 
    ... 
    #Render signup button for student here. The @show_actions will be true if this partial is navigated from the SignupSheetController which is accessible only to students.


Case 2 : Cleaning up unused code

The Refactors mentioned above enable us to delete unused partials namely:

  • assignments controller partials :
    • _add_signup_topics.html.erb
    • _reserve_topic.html.erb
  • signup_sheet_controller partials :
    • _reserve_topic.html.erb


Steps to verify changes

Instructor Role

Log into the application with the user having instructor's role (user3, password). VCL application link

1. Go to Manage -> Assignments and expand the assignments section.

2. Hover over the options symbol adjacent to the assignment and select the edit icon from the popup.

3. We have pre-created an assignment with multiple topics with id. 253, so you can click this link to edit it - http://152.46.18.5:3000/assignments/253/edit

4. Click on topics tab.



5. You can edit or delete any topic here as you are in instructor role.

6. Also you can undo, redo any changes you have made.

Student Role

1. Log into the application with a user with Student role (user10, password).

2. We have added this student from the instructor view into assignment id. 253 that we used above. So this is accessible to the student. The name of the assignment appears in the student view as Copy of assignment 1.

3. Click on the Copy of assignment 1 -> SignupSheet and the student can see the edited topics for the assignment with a signup icon and no edit/delete options which are instructor specific.

See Also

  1. Expertiza on GitHub
  2. GitHub Project Repository Fork
  3. The live Expertiza website
  4. VCL link - This might not be available after Nov. 2014
  5. Expertiza project documentation wiki
  6. The OSS project requirements doc (Fall 2014 - Expertiza)

References

Expertiza
Code Refactoring
Code smell

Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2014/oss_E1453_syy&oldid=94950"