|
|
| Line 12: |
Line 12: |
| =====Solutions===== | | =====Solutions===== |
|
| |
|
| Once TA clicks on assignment > view scores, they will no longer be able to see the form to add/edit the grade and comment for the course in which they are participating as a student. | | Once TA clicks on Assignment > view scores, they will no longer be able to see the form to add/edit the grade and comment for the course in which they are participating as a student. |
|
| |
|
| Files modifies | | Files modified: view_team.html |
|
| |
|
| | We are rendering the TA view(to grade and comment) only if the TA ID has an entry in the ta_mapping table. This ensures that the TA will be able modify the grades for courses for which they are assigned as TA. |
|
| |
|
| | * Get the course ID for the course which the student is currently viewing. |
| | * Get the user ID, which will be teacher ID as well |
| | * Using these two fields we are restricting the access for the student to modify the grades. |
|
| |
|
| | Only for the courses for which a user is a TA, he will be able to see 'TA Grade-Comment:' section under Assignment > view scores |
|
| |
|
| | [[File:E2057_view_team.png]] |
|
| |
|
| * '''Problem 1''': The method paginate_list is doing more than one thing.
| | The green-highlighted lines indicate the changes. |
| ::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'
| | =====Student/TA view===== |
|
| |
|
| end
| | The below screenshot shows the TA view for the course he is the TA: |
| | |
| # Remove the last ' AND '
| |
| criteria = criteria[0..-5]
| |
| | |
| versions = Version.page(params[:page]).order('id').per_page(25).where(criteria)
| |
| versions
| |
| end
| |
| </pre>
| |
| * '''Solution''': The implementation has been changed in such a way that the versions which a user is allowed to see depends on the privileges of the user. The approach we have taken is as follows:
| |
| **An administrator can see all the versions
| |
| **An instructor can see all the versions created by him and other users who are in his course or are participants in the assignments he creates.
| |
| **A TA can see all the versions created by him and other users who are in the course for which he/ she assists.
| |
| **A Student can see all the versions created by him/ her.
| |
| * '''Problem 2''': The search criteria created in the method paginate_list was difficult to comprehend.
| |
| ::The code which builds the search criteria in the method paginate_list uses many string literals and conditions and is hardly intuitive. The programmer will have to spend some time to understand what the code is really doing.
| |
| * '''Solution''': The implementation has been changed. A student is not allowed to delete any versions now. Other types of users, for instance administrators, instructors and TAs are allowed to delete only the versions they are authorized to view.
| |
| * '''Problem 3''': The paginate method can be moved to a helper class.
| |
| ::VersionsController is not the only component which require to paginate items. There are other components too. For instance, the UsersController has to paginate the list of users. Hence the Paginate method can be moved to a helper class which can be accessed by other components as well.
| |
| * '''Solution''': The filtering options has also been enhanced. The current user can now choose as part of the version search filter any user from a list of users if the current user is authorized to see the versions created by that user.
| |
| | |
| ===New Implementation===
| |
| *The method paginate_list has been split into 2 methods now.
| |
| ** BuildSearchCriteria – as the name suggests the sole purpose of this method is to build a search criteria based on the input search filters when the current user initiates a search in versions.
| |
| ** paginate_list – this method will call the paginate API.
| |
| :First the search criteria is built, then the criteria is applied to versions in the database to get all versions which matches the criteria and then the retrieved versions are paginated.
| |
| <pre>
| |
| # pagination.
| |
| def paginate_list(versions)
| |
| paginate(versions, VERSIONS_PER_PAGE);
| |
| end
| |
| | |
| def BuildSearchCriteria(id, user_id, item_type, event)
| |
| # Set up the search criteria
| |
| search_criteria = ''
| |
| search_criteria = search_criteria + add_id_filter_if_valid(id).to_s
| |
| if current_user_role? == 'Super-Administrator'
| |
| search_criteria = search_criteria + add_user_filter_for_super_admin(user_id).to_s
| |
| end
| |
| search_criteria = search_criteria + add_user_filter
| |
| search_criteria = search_criteria + add_version_type_filter(item_type).to_s
| |
| search_criteria = search_criteria + add_event_filter(event).to_s
| |
| search_criteria = search_criteria + add_date_time_filter
| |
| search_criteria
| |
| end
| |
| </pre>
| |
| * The string literals and conditions in the method paginate_list were replaced with methods with intuitive names so that the programmer can understand the code more easily. We also removed an empty if clause and a redundant statement.
| |
| <pre>
| |
| def add_id_filter_if_valid (id)
| |
| "id = #{id} AND " if id && id.to_i > 0
| |
| end
| |
| | |
| def add_user_filter_for_super_admin (user_id)
| |
| "whodunnit = #{user_id} AND " if user_id && user_id.to_i > 0
| |
| end
| |
| | |
| def add_user_filter
| |
| "whodunnit = #{current_user.try(:id)} AND " if current_user.try(:id) && current_user.try(:id).to_i > 0
| |
| end
| |
| | |
| def add_event_filter (event)
| |
| "event = '#{event}' AND " if event && !(event.eql? 'Any')
| |
| end
| |
| | |
| def add_date_time_filter
| |
| "created_at >= '#{time_to_string(params[:start_time])}' AND " +
| |
| "created_at <= '#{time_to_string(params[:end_time])}'"
| |
| end
| |
| | |
| def add_version_type_filter (version_type)
| |
| "item_type = '#{version_type}' AND " if version_type && !(version_type.eql? 'Any')
| |
| end
| |
| </pre>
| |
| * The paginate method has been moved to the helper class Pagination_Helper. This new method can be now reused by the different components like UsersController etc. The method receives two parameters, first the list to paginate and second the number of items to be displayed in a page.
| |
| | |
| <pre>
| |
| module PaginationHelper
| |
| | |
| def paginate (items, number_of_items_per_page)
| |
| items.page(params[:page]).per_page(number_of_items_per_page)
| |
| end
| |
| | |
| end
| |
| </pre>
| |
| | |
| ===Code improvements===
| |
| * Introduced a constant VERSIONS_PER_PAGE and assigned the value 25 to it. The pagination algorithm for VersionsController displays at most 25 versions in a page. The existing implementation uses the value 25 straight in the code and there are few problems associated with such an approach.
| |
| ** It is not easy to understand what 25 is unless the programmer takes a close look at the code.
| |
| ** In case if the value 25 is used at more than one places and in future a new requirement comes to show at most 30 versions in a page, all the values will have to be modified. It is not very DRY.
| |
| * The VersionsController was overriding AccessHelper - action_allowed? method to return true in all the cases. This was violating the whole purpose of the method action_allowed?. The purpose of this method is to determine whether the user who is triggering a CRUD operation is allowed to do so. So when the current user invokes a CRUD operation, the action_allowed? method is invoked first and if the method returns true the CRUD operation is triggered or else the user is intimated with a message and gracefully exited. Hence, when the action_allowed? method is overridden to return true always, it results in providing unauthorized access to certain users.
| |
| | |
| <pre>
| |
| def action_allowed?
| |
| true
| |
| end
| |
| </pre>
| |
| | |
| :With the new implementation the AccessHelper - action_allowed? method has been modified in such a way that unauthorized access is prevented. As per the new algorithm, 'new', 'create', 'edit', 'update' cannot be invoked by any user. These operations can be accessed only by ‘papertrail’ gem. Only an ‘Administrator’ or ‘Super-Administrator’ can call 'destroy_all' method. All the other methods are accessible to ‘Administrator’, ‘Super-Administrator’, ‘Instructor’, ‘Teaching Assistant’ and ‘Student’.
| |
| | |
| <pre>
| |
| def action_allowed?
| |
| case params[:action]
| |
| when 'new', 'create', 'edit', 'update'
| |
| #Modifications can only be done by papertrail
| |
| return false
| |
| when 'destroy_all'
| |
| ['Super-Administrator',
| |
| 'Administrator'].include? current_role_name
| |
| else
| |
| #Allow all others
| |
| ['Super-Administrator',
| |
| 'Administrator',
| |
| 'Instructor',
| |
| 'Teaching Assistant',
| |
| 'Student'].include? current_role_name
| |
| end
| |
| end
| |
| </pre>
| |
| | |
| ===Automated Testing using RSPEC===
| |
| The current version of expertiza did not have any test for VersionsController. Using the test driven development(TDD) approach, we have added an exhaustive set of RSPEC tests for VersionsController, to test all the modifications we have done to the code of the controller class. The tests use double and stub features of rspec-rails gem, to fake the log in by different users - Administrator, Instructor, Student etc. The tests can be executed "rpec spec" command as shown below.
| |
| <pre>
| |
| user-expertiza $rspec spec
| |
| .
| |
| .
| |
| .
| |
| Finished in 5.39 seconds (files took 25.33 seconds to load)
| |
| 66 examples, 0 failures
| |
| | |
| Randomized with seed 19254
| |
| .
| |
| .
| |
| </pre>
| |
| | |
| ===Testing from UI===
| |
| Following are a few testcases with respectto our code changes that can be tried from UI:
| |
| 1. To go to versions index page, type in the following url after logging in:
| |
| http://152.46.16.81:3000/versions
| |
| | |
| 2. After logging in as student/instructor or admin : Try accessing the new, create, edit, update actions. These actions are not allowed to any of the users.
| |
| http://152.46.16.81:3000/versions/new
| |
| This calls the new action. In the current production version of expertiza, it is unhandled and application gives a default 404 page.
| |
| | |
| 3. Another feature that can be tested from UI is Pagination. Try searching for a user's versions and see if the results are paginated or not. Search here:
| |
| http://152.46.16.81:3000/versions/search
| |
| | |
| 4. Visit the same URL as step 3, you should see only the students under that instructor in the users dropdown.
| |
| | |
| ===References===
| |
| | |
| #[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
| |
