Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Fall 2015/M1506-Refactor-GLES2-Implementation

2015-12-04T17:27:44Z

Asayal:

'''' Refactor GLES2 Student Project with SERVO & RUST ''''

Servo is a prototype web browser engine written in the [http://en.wikipedia.org/wiki/JavaScript RUST] language.Servo uses a variety of back-end implementations for drawing graphics depending on the operating system.One of such back-end is only compatible with Android right now, and we want to extend and refactor that back-end to enable on all Linux systems..

=='''Introduction'''==
===Servo===
[https://en.wikipedia.org/wiki/Servo_(layout_engine) Servo]Servo is an open source prototype web browser layout engine being developed by Mozilla, and it is written in [https://www.rust-lang.org/ Rust] language. The main idea is to create a highly parallel environment, in which different components can be handled by fine grained, isolated tasks. The different components can be rendering, HTML parsing, etc.<ref>https://en.wikipedia.org/wiki/Servo_(layout_engine)</ref>

===Rust===
[http://doc.rust-lang.org/book/README.html Rust] is an open source systems programming language developed by Mozilla. Servo is written in Rust. The main purpose behind it's design is to be thread safe and concurrent. The emphasis is also on speed, safety and control of memory layout.<ref>https://doc.rust-lang.org/stable/book/</ref><ref>https://en.wikipedia.org/wiki/Rust_(programming_language)</ref>

=='''Project Description'''==
===Setup of Development environment===

* Servo can be built and compiled in the following steps:
Servo is built with [https://mail.mozilla.org/pipermail/rust-dev/2014-March/009090.html Cargo], the Rust package manager. Mozilla's Mach tools are used to orchestrate the build and other tasks.

git clone https://github.com/servo/servo
cd servo
./mach build --dev
./mach run tests/html/about-mozilla.html

* Next, we need a cargo override for servo to use our local copy of Rust-layers. For this, we created a <code>.cargo</code> folder in our home directory(same place that servo and Rust-layers reside), and added a <code>config</code> file to that folder. The content of that config file is a path to our local Rust-layers.
paths = [path/to/rust_layers]

===Overview===
* After building servo and rust-layers successfully, we added a new command line argument in Servo, which would allow user to select the graphics backend (GL or ES2).

Example is as below
./mach run [url] -G GL or ./mach run [url] --graphics GL (for GL)
./mach run [url] -G ES2 or ./mach run [url] --graphics ES2 (for ES2)

* This option selected is passed onto the rust-layers, which is modified to support ES2 for both linux and android versions. Hence, based on the option selected by the user, rust-layers renders the corresponding graphic backend on linux and android targets.To achieve this, the main changes are done in the platform to remove the EGL specific implementation from the Android platform to a more generic one, so that it can be used by both android and linux systems.

=='''Requirement Analysis'''==

Earlier, only [https://en.wikipedia.org/wiki/OpenGL OpenGL] implementation was supported for Android targets and we would be extending the [https://en.wikipedia.org/wiki/OpenGL_ES#2.0 ES2] implementation to support it for Linux targets.

===Initial step===

Our [https://github.com/servo/servo/wiki/Refactor-GLES2-student-project initial step], implemented for the OSS project involved:
* compile Servo and ensure that it runs on tests/html/about-mozilla.html
* adding a new command-line option to allow selecting the graphics backend (GL or ES2) in <code>components/utils/opts.rs</code>
* clone rust-layers and build it independently of Servo
* adding a Cargo override for Servo to use the local copy of rust-layers

===Final Requirements===

* Enable selection of the render api working from the command line, so the user can pick whether to use GL or ES2
* Modify the necessary files so that the current display version of GL or ES2 are returned whenever either of them are selected
* Make the EGL libs available on Linux apart from android, for rust-layers to compile rust-egl on Linux build.
* Separate out the EGLImageNativeSurface implementation from Android and move it to EGL
* Enable use of EGL or GLX for Linux.
* In conclusion, implement a generic way of supporting EGL for both Linux and Android, such that ES2 is rendered as a display on linux targets as well.

=='''Implementation'''==

===Servo===

Servo/components/utils/opts.rs
added the new command line option to enable user to select the option of GL or ES2 as the rendering backend. The options of GL or ES2 are stored as an enum called RenderApi , such that running the Servo with option --render-api=chosen_option sets the corresponding Api.

Servo/ports/glutin/window.rs
<code>gl_version()</code> function is modified in a way that based on what is selected as the RenderApi option by the user, the corresponding version of the Api is set. To talk more in detail, if the render api is set to GL, it returns the current value (GL 2.1) & if set to ES2, it returns OpenGlEs 2.0.

<code>native_display()</code> for linux target is also modified to call the specific NativeDisplay constructor based on the RenderApi option chosen by the user. Hence for the GL case, the existing NativeDisplay::new constructor is called, but if ES2 is selected, NativeDisplay::from_es2() is called, which has been introduced by us in an attempt to support ES2 for the linux platform as well.

===Rust-Layers===

rust-layers/cargo.toml
In order to make the egl libraries available to the Linux too, the cargo.toml is edited which defines the different dependencies that various platforms has. Before, the dependency on rust-egl was added only for android targets but now these dependencies have been added for Linux targets as well.

rust-layers/src/platform/egl/surface.rs
In order to support EGL for both linux and android targets, we moved the implementation of <code>EGLImageNativeSurface</code> from the android specific <code>surface.rs</code> and introduced a new folder <code>egl</code> which contains its own <code>surface.rs</code> and implements a generic <code>EGLImageNativeSurface </code>

rust-layers/src/lib.rs
Since egl needs to be supported for linux too now, the extern crate egl call needs to be linked on Linux as well now. Hence in the cfg flag, the target_os="linux" is added in addition to android.

In the pub mod platform { }, introduced a new pub mod egl {} section to pull in the new <code>egl/surface.rs</code> implemented for both android and linux targets.

rust-layers/src/platform/linux/surface.rs
In order to ensure that ES2 is supported for linux targets too, we need to enable the use of EGL & GLX in the linux specific <code>surface.rs</code>
A new enum is created for NativeDisplay which has two different values of <code>EGLDisplayInfo</code> & <code>GlxDisplayInfo</code> and respectively, two constructors are created . The existing NativeDisplay::new constructor is modified to return <code>NativeDisplay::Glx(info)</code> for the GL version , and the NativeDisplay::from_es2() constructor introduced earlier can return <code>NativeDisplay::::Egl(egl_info)</code> for the ES2 version.

rust-layers/src/platform/surface.rs
Earlier, the android target was using its own <code>surface.rs</code> and the linux target was using its own <code>surface.rs</code>. But after the implementation of <code>egl/surface.rs</code>, changes were made to platform <code>surface.rs</code> which uses the egl surface.rs for both android and linux now.

=='''Design Principles'''==

We are adhering to the following design principles for our implementation:

'''Open-Closed Principle'''<ref>http://en.wikipedia.org/wiki/Open/closed_principle</ref>

Although our task is to modify the egl implementation to support it for linux targets too, we are not modifying the existing functionality of the android platform, instead we have extended the egl implementation to provide support for linux as well.

'''Interface Segregation Principle'''<ref>http://en.wikipedia.org/wiki/Interface_segregation_principle</ref>

The existing code currently has specific platform implementations for android and linux, and egl implementation is provided in android. We have extended the egl implementation to a more generic platform surface and divided the functionality for the native display into two small methods such that these methods can be individually called for whichever graphic backend option is chosen. This avoids the need to call the bigger functionality.

=='''Design Pattern'''==
The aim is to use ES2 implementation of rust layers for both android and Linux. So we need to use a design pattern that will allow us to extend the current implementation to support Linux targets too and give the user the flexibility to choose between the two implementations for rendering the backend.

We propose to use [https://en.wikipedia.org/wiki/Decorator_pattern Decorator] pattern. The decorator pattern gives the user the ability to choose between the implementations that are desired, and attach the corresponding implementation to the target dynamically at runtime.

==Flowchart describing Project Implementation==
<center>[[File:RefactorGLESMozilla1506OODD.png]]</center>

==Class Diagram==

<center>[[File:Class_diagram_Mozilla_1506.png]]</center>

== Proposed Test Cases ==

Some of the test cases which will be used to validate our changes are as follows:
* Printing the command line option selected by the user in the <code>opts.rs</code>.This ensures if the command line option selected is correctly passed onto the servo.
println!("selected {:?}", opts.render_api);
* Printing the command line option selected by the user in the <code>window.rs</code>.This ensures if the command line option selected is correctly propagated from the servo to the rust-layers.
* Test if the rust-egl compiles for the linux surface. A successful compilation ensures egl libraries are made available on Linux for rust-layers.
* Test if selecting an option ES2 though command line on a linux surface, renders a graphic display of ES2.

== Video ==
A walkthrough of our entire project is available on this link: [https://drive.google.com/file/d/0B9TVPg3YRoHqT2llQ0FfMWNvUEE/view Walkthrough Video]

=='''Useful libraries'''==

waffle-gl/waffle: A C library for selecting an OpenGL API and window system at runtime.

==Further Reading==
1. [https://github.com/servo/servo Servo GitHub] 
2. [https://github.com/servo/rust-layers Rust-layers GitHub] 
3. [http://doc.crates.io/guide.html#overriding-dependencies Overriding Dependencies] 
4. [http://rustbyexample.com/ Rust Language] 
5. [http://javarevisited.blogspot.com/2014/11/strategy-design-pattern-in-java-using-Enum-Example.html Strategy Design Pattern] 

==References==
<references/>

CSC/ECE 517 Fall 2015/oss E1553 AAJ

2015-10-31T16:12:47Z

Asayal: /* References */

==E1553. Refactoring the Versions Controller==

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

__TOC__

===Introduction to Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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 were accomplished in this project:

* Improved the clarity of code by improving the variable and parameter names.
* Long character strings were taken and given appropriate names.
* Handled pagination by a separate helper module, which can be used by multiple controllers.
* Implemented action_allowed for access_control to prevent unauthorized access of methods.
* Prevented displaying of all versions for all users and tables when a user views the index page.
* Added missing CRUD methods to Versions Controller
* Added RSPEC testcases for testing changes done in Versions Controller

===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===

=====Functionality=====
* 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===

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
#[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

CSC/ECE 517 Fall 2015/oss E1553 AAJ

2015-10-31T04:55:37Z

Asayal: /* Introduction to Expertiza */

CSC/ECE 517 Fall 2015/oss E1553 AAJ

2015-10-31T04:45:21Z

Asayal:

==E1553. Refactoring the Versions Controller==

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

__TOC__

===Introduction to Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is a large project developed as a combined effort of students and faculty using the [http://rubyonrails.org/ Ruby on Rails] framework. The main advantage of using Expertiza, in an educational environment, is for the instructor to introduce peer reviewing among the students. Expertiza allows the instructor to create and customize assignments, create a list of topics the students can sign up for, have students work on teams and then review each other's assignments at the end. Expertiza supports submission of almost any document type, including the URLs and wiki pages. 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.

===Problem Statement===
The following tasks were accomplished in this project:

* Improved the clarity of code by improving the variable and parameter names.
* Long character strings were taken and given appropriate names.
* Handled pagination by a separate helper module, which can be used by multiple controllers.
* Implemented action_allowed for access_control to prevent unauthorized access of methods.
* Prevented displaying of all versions for all users and tables when a user views the index page.
* Added missing CRUD methods to Versions Controller
* Added RSPEC testcases for testing changes done in Versions Controller

===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===

=====Functionality=====
* 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===

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
#[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]

CSC/ECE 517 Fall 2015/oss E1553 AAJ

2015-10-31T04:41:42Z

Asayal: /* Automated Testing using Rspec */

==E1553. Refactoring the Versions Controller==

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

__TOC__

===Introduction to Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is a large project developed as a combined effort of students and faculty using the [http://rubyonrails.org/ Ruby on Rails] framework. The main advantage of using Expertiza, in an educational environment, is for the instructor to introduce peer reviewing among the students. Expertiza allows the instructor to create and customize assignments, create a list of topics the students can sign up for, have students work on teams and then review each other's assignments at the end. Expertiza supports submission of almost any document type, including the URLs and wiki pages. 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.

===Problem Statement===
The following tasks were accomplished in this project:

* Improved the clarity of code by improving the variable and parameter names.
* Long character strings were taken and given appropriate names.
* Handled pagination by a separate helper module, which can be used by multiple controllers.
* Implemented action_allowed for access_control to prevent unauthorized access of methods.
* Prevented displaying of all versions for all users and tables when a user views the index page.
* Added missing CRUD methods to Versions Controller
* Added RSPEC testcases for testing changes done in Versions Controller

===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===

=====Functionality=====
* 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===

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
#[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]

CSC/ECE 517 Fall 2015/oss E1553 AAJ

2015-10-31T04:41:21Z

Asayal: /* Drawbacks and Solutions */

==E1553. Refactoring the Versions Controller==

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

__TOC__

===Introduction to Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is a large project developed as a combined effort of students and faculty using the [http://rubyonrails.org/ Ruby on Rails] framework. The main advantage of using Expertiza, in an educational environment, is for the instructor to introduce peer reviewing among the students. Expertiza allows the instructor to create and customize assignments, create a list of topics the students can sign up for, have students work on teams and then review each other's assignments at the end. Expertiza supports submission of almost any document type, including the URLs and wiki pages. 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.

===Problem Statement===
The following tasks were accomplished in this project:

* Improved the clarity of code by improving the variable and parameter names.
* Long character strings were taken and given appropriate names.
* Handled pagination by a separate helper module, which can be used by multiple controllers.
* Implemented action_allowed for access_control to prevent unauthorized access of methods.
* Prevented displaying of all versions for all users and tables when a user views the index page.
* Added missing CRUD methods to Versions Controller
* Added RSPEC testcases for testing changes done in Versions Controller

===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===

=====Functionality=====
* 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===

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
#[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]

CSC/ECE 517 Fall 2015/oss E1553 AAJ

2015-10-31T04:41:09Z

Asayal: /* Functionality */

==E1553. Refactoring the Versions Controller==

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

__TOC__

===Introduction to Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is a large project developed as a combined effort of students and faculty using the [http://rubyonrails.org/ Ruby on Rails] framework. The main advantage of using Expertiza, in an educational environment, is for the instructor to introduce peer reviewing among the students. Expertiza allows the instructor to create and customize assignments, create a list of topics the students can sign up for, have students work on teams and then review each other's assignments at the end. Expertiza supports submission of almost any document type, including the URLs and wiki pages. 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.

===Problem Statement===
The following tasks were accomplished in this project:

* Improved the clarity of code by improving the variable and parameter names.
* Long character strings were taken and given appropriate names.
* Handled pagination by a separate helper module, which can be used by multiple controllers.
* Implemented action_allowed for access_control to prevent unauthorized access of methods.
* Prevented displaying of all versions for all users and tables when a user views the index page.
* Added missing CRUD methods to Versions Controller
* Added RSPEC testcases for testing changes done in Versions Controller

===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===

=====Functionality=====
* 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===

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
#[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]

CSC/ECE 517 Fall 2015/oss E1553 AAJ

2015-10-31T04:40:39Z

Asayal: /* Problem Statement */

==E1553. Refactoring the Versions Controller==

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

__TOC__

===Introduction to Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is a large project developed as a combined effort of students and faculty using the [http://rubyonrails.org/ Ruby on Rails] framework. The main advantage of using Expertiza, in an educational environment, is for the instructor to introduce peer reviewing among the students. Expertiza allows the instructor to create and customize assignments, create a list of topics the students can sign up for, have students work on teams and then review each other's assignments at the end. Expertiza supports submission of almost any document type, including the URLs and wiki pages. 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.

===Problem Statement===
The following tasks were accomplished in this project:

* Improved the clarity of code by improving the variable and parameter names.
* Long character strings were taken and given appropriate names.
* Handled pagination by a separate helper module, which can be used by multiple controllers.
* Implemented action_allowed for access_control to prevent unauthorized access of methods.
* Prevented displaying of all versions for all users and tables when a user views the index page.
* Added missing CRUD methods to Versions Controller
* Added RSPEC testcases for testing changes done in Versions Controller

===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===

====Functionality====
* 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===

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
#[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]

CSC/ECE 517 Fall 2015/oss E1553 AAJ

2015-10-31T04:28:36Z

Asayal: /* About Versions Controller */

==E1553. Refactoring the Versions Controller==

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

__TOC__

===Introduction to Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is a large project developed as a combined effort of students and faculty using the [http://rubyonrails.org/ Ruby on Rails] framework. The main advantage of using Expertiza, in an educational environment, is for the instructor to introduce peer reviewing among the students. Expertiza allows the instructor to create and customize assignments, create a list of topics the students can sign up for, have students work on teams and then review each other's assignments at the end. Expertiza supports submission of almost any document type, including the URLs and wiki pages. 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.

===Problem Statement===

===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===

====Functionality====
* 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===

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
#[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]

CSC/ECE 517 Fall 2015/oss E1553 AAJ

2015-10-31T02:40:45Z

Asayal:

==E1553. Refactoring the Versions Controller==

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

__TOC__

===Introduction to Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is a large project developed as a combined effort of students and faculty using the [http://rubyonrails.org/ Ruby on Rails] framework. The main advantage of using Expertiza, in an educational environment, is for the instructor to introduce peer reviewing among the students. Expertiza allows the instructor to create and customize assignments, create a list of topics the students can sign up for, have students work on teams and then review each other's assignments at the end. Expertiza supports submission of almost any document type, including the URLs and wiki pages. 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.

===Problem Statement===

===About Versions Controller===
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===

====Functionality====
* 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===

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
#[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]

CSC/ECE 517 Fall 2015/oss E1553 AAJ

2015-10-31T02:39:35Z

Asayal:

==E1553. Refactoring the Versions Controller==

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

__TOC__

===Introduction to Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is a large project developed as a combined effort of students and faculty using the [http://rubyonrails.org/ Ruby on Rails] framework. The main advantage of using Expertiza, in an educational environment, is for the instructor to introduce peer reviewing among the students. Expertiza allows the instructor to create and customize assignments, create a list of topics the students can sign up for, have students work on teams and then review each other's assignments at the end. Expertiza supports submission of almost any document type, including the URLs and wiki pages. 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.

===Problem Statement===

===About Versions Controller===
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===

====Functionality====
* 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>

==References==

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
#[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]

CSC/ECE 517 Fall 2015/oss E1553 AAJ

2015-10-31T02:38:07Z

Asayal: /* Introduction to Expertiza */

==E1553. Refactoring the Versions Controller==

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

__TOC__

===Introduction to Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is a large project developed as a combined effort of students and faculty using the [http://rubyonrails.org/ Ruby on Rails] framework. The main advantage of using Expertiza, in an educational environment, is for the instructor to introduce peer reviewing among the students. Expertiza allows the instructor to create and customize assignments, create a list of topics the students can sign up for, have students work on teams and then review each other's assignments at the end. Expertiza supports submission of almost any document type, including the URLs and wiki pages. 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.

===Current Implementation===

====Functionality====
* 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>

==References==

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
#[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]

CSC/ECE 517 Fall 2015/oss E1553 AAJ

2015-10-31T02:17:47Z

Asayal: /* References */

==E1553. Refactoring the Versions Controller==

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

__TOC__

===Introduction to Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is a large project developed as a combined effort of students and faculty using the [http://rubyonrails.org/ Ruby on Rails] framework. The main advantage of using Expertiza, in an educational environment, is for the instructor to introduce peer reviewing among the students. Expertiza allows the instructor to create and customize assignments, create a list of topics the students can sign up for, have students work on teams and then review each other's assignments at the end. Expertiza supports submission of almost any document type, including the URLs and wiki pages.

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.
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===

====Functionality====
* 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>

==References==

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
#[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]

CSC/ECE 517 Fall 2015/oss E1553 AAJ

2015-10-31T01:18:50Z

Asayal: /* References */

==E1553. Refactoring the Versions Controller==

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

__TOC__

===Introduction to Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is a large project developed as a combined effort of students and faculty using the [http://rubyonrails.org/ Ruby on Rails] framework. The main advantage of using Expertiza, in an educational environment, is for the instructor to introduce peer reviewing among the students. Expertiza allows the instructor to create and customize assignments, create a list of topics the students can sign up for, have students work on teams and then review each other's assignments at the end. Expertiza supports submission of almost any document type, including the URLs and wiki pages.

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.
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===

====Functionality====
* 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>

==References==

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
#[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]

CSC/ECE 517 Fall 2015/oss E1553 AAJ

2015-10-31T01:16:10Z

Asayal:

==E1553. Refactoring the Versions Controller==

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

__TOC__

===Introduction to Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is a large project developed as a combined effort of students and faculty using the [http://rubyonrails.org/ Ruby on Rails] framework. The main advantage of using Expertiza, in an educational environment, is for the instructor to introduce peer reviewing among the students. Expertiza allows the instructor to create and customize assignments, create a list of topics the students can sign up for, have students work on teams and then review each other's assignments at the end. Expertiza supports submission of almost any document type, including the URLs and wiki pages.

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.
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===

====Functionality====
* 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>

==References==

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
#[http://bit.ly/myexpertiza Demo link] - This might not be available after May of 2015
#[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza project documentation wiki]

CSC/ECE 517 Fall 2015/oss E1553 AAJ

2015-10-31T01:11:48Z

Asayal:

==E1553. Refactoring the Versions Controller==

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

__TOC__

===Introduction to Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is a large project developed as a combined effort of students and faculty using the [http://rubyonrails.org/ Ruby on Rails] framework. The main advantage of using Expertiza, in an educational environment, is for the instructor to introduce peer reviewing among the students. Expertiza allows the instructor to create and customize assignments, create a list of topics the students can sign up for, have students work on teams and then review each other's assignments at the end. Expertiza supports submission of almost any document type, including the URLs and wiki pages.

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.
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===

====Functionality====
* 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>