CSC/ECE 517 Fall 2016/E1648: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
(Created page with "==E1648. Adding past due assignments to the task list== This page provides a description of the Expertiza based OSS project for Fall 2016. __TOC__ ===About Expertiza=== [h...")
 
(Added references)
 
(8 intermediate revisions by 2 users not shown)
Line 15: Line 15:


* Adding past due assignments to the task list.
* Adding past due assignments to the task list.
* Highlighting the next immediate due date of an assignment.
* Highlighting the next immediate due date on the assignment list.
* Correcting the due date of finished assignments.
* Correcting the stage deadline of finished assignments.  
 
===About Versions Controller===
This class manages different versions of reviews. If a reviewer reviews a submission, and after that, the author revises the submission, the next time the reviewer does a review (s)he will create a new version. Sometimes it’s necessary to find the current version of a review; sometimes it’s necessary to find all versions. Similarly, a user may want to delete the current version of a review, or all versions of a review. Pagination of versions helps the user to view a subset of versions at a time. Considering the huge number of versions in the system, it is very useful to have a pagination mechanism and a filtering mechanism which can be applied on the whole set of versions. The idea is to display the versions in an ordered, comprehensible and logical manner. In Expertiza the gem ‘will_paginate’ is used to achieve pagination.


===Current Implementation===
===Current Implementation===


===== Functionality =====
*Assignments that are past due are not present in the task list
:The assignments that the user is a participant of and are past the deadline but not yet completed are not part of the task list of the user. Currently only tasks that are not yet started by the user are being shown in the task list.
*Next immediate due date
:As per the current implementation, it is hard to pick the when and which assignment has the next immediate due.
*Stage deadline of a finished assignment
:According to the existing implementation, the stage deadline of any finished assignment is being shown as one year from the current time which is misleading from the actual final stage deadline of that assignment.
===== Problems and Solutions =====
*'''Problem 1:'''next_due_date is being assigned string value instead of DueDate object
:While finding the current stage of the assignment, the next due date returned from the DueDate model currently returns nil for the assignments which have their due dates past the current time. Then, if the next due date is found to be ''nil'' or 'Finished' it is being assigned the value 'Finished'. In the list.html.erb of the student_task view, the stage deadline of the student task for an assignment is fetched as the due_at attribute of the next due date object. As the finished assignments have a next due date as a string they are being caught as an exception and rescued by showing the stage deadline as an year from the current time.
*'''Problem 2:'''
:While fetching next_due_date from the DueDate model, it returns next due date from the assignments which have due dates greater than current time. If the due date is past, next due date is nil. The past due dates are not being retrieved in case if the next due date is nil.


=====Functionality=====
*'''Solution:'''If the next_due_date of an assignment is found to be nil while finding its current stage, its past_due_date is retrieved and name of the deadline type is looked up from the DeadlineTypes model with the deadline_type_id attribute of the past_due_date.  
* Any user irrespective of his/ her privileges can view all the versions.
::The versions which a particular user can view should be restricted based on the privileges of the user. For instance, only a user with Administrator privileges should be able to view all the versions in the system. However, that is not the case now. Every user can view all the versions irrespective of whether the user is a student or an administrator.
* Any user can delete any version
::The versions which a particular user can delete should be restricted based on the privileges of the user. For instance, a student should not be allowed to delete any version. According to the current implementation any user can delete any version in the system.
* Filtering of versions were restricted to the current user
::The filtering options on versions were restricted to the current user. Sometimes a user might want to view versions associated with other users. For instance, an instructor might want to view the list of versions created by a particular student. This is not possible with the current implementation.
 
=====Drawbacks and Solutions=====
* '''Problem 1''': The method paginate_list is doing more than one thing.
::The method paginate_list was building a complex search criteria based on the input params, getting the list of versions from the Database matching this search criteria and then calling the Page API. All these tasks in a single method made it difficult to understand.
<pre>
# For filtering the versions list with proper search and pagination.
  def paginate_list(id, user_id, item_type, event, datetime)
    # Set up the search criteria
    criteria = ''
    criteria = criteria + "id = #{id} AND " if id && id.to_i > 0
    if current_user_role? == 'Super-Administrator'
      criteria = criteria + "whodunnit = #{user_id} AND " if user_id && user_id.to_i > 0
    end
    criteria = criteria + "whodunnit = #{current_user.try(:id)} AND " if current_user.try(:id) && current_user.try(:id).to_i > 0
    criteria = criteria + "item_type = '#{item_type}' AND " if item_type && !(item_type.eql? 'Any')
    criteria = criteria + "event = '#{event}' AND " if event && !(event.eql? 'Any')
    criteria = criteria + "created_at >= '#{time_to_string(params[:start_time])}' AND "
    criteria = criteria + "created_at <= '#{time_to_string(params[:end_time])}' AND "
 
    if current_role == 'Instructor' || current_role == 'Administrator'
 
    end
 
    # Remove the last ' AND '
    criteria = criteria[0..-5]
 
    versions = Version.page(params[:page]).order('id').per_page(25).where(criteria)
    versions
  end
</pre>
* '''Solution''': The implementation has been changed in such a way that the versions which a user is allowed to see depends on the privileges of the user. The approach we have taken is as follows:
**An administrator can see all the versions
**An instructor can see all the versions created by him and other users who are in his course or are participants in the assignments he creates.
**A TA can see all the versions created by him and other users who are in the course for which he/ she assists.
**A Student can see all the versions created by him/ her.
* '''Problem 2''': The search criteria created in the method paginate_list was difficult to comprehend.
::The code which builds the search criteria in the method paginate_list uses many string literals and conditions and is hardly intuitive. The programmer will have to spend some time to understand what the code is really doing.
* '''Solution''': The implementation has been changed. A student is not allowed to delete any versions now. Other types of users, for instance administrators, instructors and TAs are allowed to delete only the versions they are authorized to view.
* '''Problem 3''': The paginate method can be moved to a helper class.
::VersionsController is not the only component which require to paginate items. There are other components too. For instance, the UsersController has to paginate the list of users. Hence the Paginate method can be moved to a helper class which can be accessed by other components as well.
* '''Solution''': The filtering options has also been enhanced. The current user can now choose as part of the version search filter any user from a list of users if the current user is authorized to see the versions created by that user.
 
===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===
* 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.
 
<pre>
def action_allowed?
    true
  end
</pre>
 
: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’.
 
<pre>
  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
</pre>
 
===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.
<pre>
user-expertiza $rspec spec
.
.
.
Finished in 5.39 seconds (files took 25.33 seconds to load)
66 examples, 0 failures
 
Randomized with seed 19254
.
.
</pre>
 
===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:
=== New Implementation ===
  http://152.46.16.81:3000/versions/search
*The method find_current_stage looks up for the past_due_date if the next_due_date is nil and returns it. Else it will return the found next_due_date.
<code>
  def find_current_stage(topic_id = nil)
    next_due_date = DueDate.get_next_due_date(self.id, topic_id)
    next_due_date.nil? ? DueDate.get_past_due_date(self.id, topic_id) : next_due_date
  end</code>
*The get_current_stage method returns the name of the deadline type from the DeadlineType controller with the deadline_type_id key. For this the deadline type
'Finished' is maintained in the deadline_types table.
<code>
  def get_current_stage(topic_id=nil)
    return 'Unknown' if topic_id.nil? and self.staggered_deadline?
    due_date = find_current_stage(topic_id)
    (due_date == nil || due_date == 'Finished') ? 'Finished' : DeadlineType.find(due_date.deadline_type_id).name
  end</code>
<code>
  class AddFinishedDeadlineType < ActiveRecord::Migration
    execute "INSERT INTO `deadline_types` VALUES (12,'Finished');"
  end</code>
*The late tasks are collected into latetasks by selecting the tasks which are in the 'Finished' stage but does not have any submitted content. These are shown in the student task view in the task list.
<code>
  def late_tasks?
    current_stage == 'Finished' && !started?
  end</code>
<code>
  <nowiki><strong>&nbsp;&nbsp;<span class="tasknum">&nbsp;<%= @latetasks.size.to_s %>&nbsp;</span> Late Tasks<br></strong><br>
    <% @latetasks.each do |student_task|
      participant = student_task.participant
      stage = student_task.current_stage
      controller = "submitted_content"
      action = "edit"
      puts "participant id is #{participant.parent_id}"
      id = participant.parent_id
      %>


4. Visit the same URL as step 3, you should see only the students under that instructor in the users dropdown.
    <span>&nbsp; &raquo;
      <%= link_to student_task.assignment.name + " " + student_task.current_stage, :controller => controller, :action => action, :id => id %>
      (<%= student_task.relative_deadline %> ago)
    </span><br/>
  <% end %></nowiki></code>
*The next immediate deadline from the current time is fetched from the student_task controller and compared with stage deadline of every assignment while iterating to find and highlight the assignemnt that has next immediate due.
<code>
  <nowiki>def next_dead_line
    ######## Next Immediate Due Date ###############
    next_deadline = nil
    @student_tasks = StudentTask.from_user current_user
    @student_tasks.reject! {|t| !t.assignment.availability_flag }
    @student_tasks.each do |student_task|
if(student_task.stage_deadline > Time.now)
  (next_deadline && next_deadline < student_task.stage_deadline) ? next_deadline : next_deadline = student_task.stage_deadline
end
    end
    next_deadline
  end</nowiki></code>
<code>
  <nowiki><% if @next_deadline == student_task.stage_deadline
          rowstyle = "font-weight:bold"
          else
          rowstyle = ""
        end %>
        <tr class="listingRow" style = <%= rowstyle%> >
        .. </tr></nowiki></code>


===References===
===References===


#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
#[https://github.com/SaiSameer/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
#[http://bit.ly/myexpertiza  Demo link]
#[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza project documentation wiki]
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

Latest revision as of 08:25, 29 October 2016

E1648. Adding past due assignments to the task list

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



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.

Problem Statement

The following tasks required completion in this project:

  • Adding past due assignments to the task list.
  • Highlighting the next immediate due date on the assignment list.
  • Correcting the stage deadline of finished assignments.

Current Implementation

Functionality
  • Assignments that are past due are not present in the task list
The assignments that the user is a participant of and are past the deadline but not yet completed are not part of the task list of the user. Currently only tasks that are not yet started by the user are being shown in the task list.
  • Next immediate due date
As per the current implementation, it is hard to pick the when and which assignment has the next immediate due.
  • Stage deadline of a finished assignment
According to the existing implementation, the stage deadline of any finished assignment is being shown as one year from the current time which is misleading from the actual final stage deadline of that assignment.
Problems and Solutions
  • Problem 1:next_due_date is being assigned string value instead of DueDate object
While finding the current stage of the assignment, the next due date returned from the DueDate model currently returns nil for the assignments which have their due dates past the current time. Then, if the next due date is found to be nil or 'Finished' it is being assigned the value 'Finished'. In the list.html.erb of the student_task view, the stage deadline of the student task for an assignment is fetched as the due_at attribute of the next due date object. As the finished assignments have a next due date as a string they are being caught as an exception and rescued by showing the stage deadline as an year from the current time.
  • Problem 2:
While fetching next_due_date from the DueDate model, it returns next due date from the assignments which have due dates greater than current time. If the due date is past, next due date is nil. The past due dates are not being retrieved in case if the next due date is nil.
  • Solution:If the next_due_date of an assignment is found to be nil while finding its current stage, its past_due_date is retrieved and name of the deadline type is looked up from the DeadlineTypes model with the deadline_type_id attribute of the past_due_date.

New Implementation

  • The method find_current_stage looks up for the past_due_date if the next_due_date is nil and returns it. Else it will return the found next_due_date.

 def find_current_stage(topic_id = nil)
   next_due_date = DueDate.get_next_due_date(self.id, topic_id)
   next_due_date.nil? ? DueDate.get_past_due_date(self.id, topic_id) : next_due_date
 end
  • The get_current_stage method returns the name of the deadline type from the DeadlineType controller with the deadline_type_id key. For this the deadline type

'Finished' is maintained in the deadline_types table. 

 def get_current_stage(topic_id=nil)
   return 'Unknown' if topic_id.nil? and self.staggered_deadline?
   due_date = find_current_stage(topic_id)
   (due_date == nil || due_date == 'Finished') ? 'Finished' : DeadlineType.find(due_date.deadline_type_id).name
 end


 class AddFinishedDeadlineType < ActiveRecord::Migration
   execute "INSERT INTO `deadline_types` VALUES (12,'Finished');"
 end
  • The late tasks are collected into latetasks by selecting the tasks which are in the 'Finished' stage but does not have any submitted content. These are shown in the student task view in the task list.

 def late_tasks?
   current_stage == 'Finished' && !started?
 end


 <strong>  <span class="tasknum"> <%= @latetasks.size.to_s %> </span> Late Tasks<br></strong><br>
     <% @latetasks.each do |student_task|
      participant = student_task.participant
      stage = student_task.current_stage
      controller = "submitted_content"
      action = "edit"
      puts "participant id is #{participant.parent_id}"
      id = participant.parent_id
      %>

    <span>  »
      <%= link_to student_task.assignment.name + " " + student_task.current_stage, :controller => controller, :action => action, :id => id %>
      (<%= student_task.relative_deadline %> ago)
    </span><br/>
  <% end %>
  • The next immediate deadline from the current time is fetched from the student_task controller and compared with stage deadline of every assignment while iterating to find and highlight the assignemnt that has next immediate due.

 def next_dead_line
    ######## Next Immediate Due Date ###############
    next_deadline = nil
    @student_tasks = StudentTask.from_user current_user
    @student_tasks.reject! {|t| !t.assignment.availability_flag }
    @student_tasks.each do |student_task|
	if(student_task.stage_deadline > Time.now)
	   (next_deadline && next_deadline < student_task.stage_deadline) ? next_deadline : next_deadline = student_task.stage_deadline
	end
     end
    next_deadline
  end


 <% if @next_deadline == student_task.stage_deadline
           rowstyle = "font-weight:bold"
          else
           rowstyle = ""
         end %>
        <tr class="listingRow" style = <%= rowstyle%> >
         .. </tr>

References

  1. Expertiza on GitHub
  2. GitHub Project Repository Fork
  3. The live Expertiza website
Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2016/E1648&oldid=103980"