CSC/ECE 517 Spring 2016/Refactor user course participant and assignment participant model
E1603. Refactor user, course_participant and assignment_participant model
This page provides details about the changes made to Expertiza under project E1603. Refactor user, course_participant and assignment_participant model.
Background
Expertiza is an educational web application built on top of Ruby on Rails. The project is open source with code available on Github and maintained by both the students and faculty of the NC State University. The application allows instructors to manage courses and assignments. The students can choose a topic from the list for an assignment, form a team, submit their work and do multiple reviews on other's work. The application helps instructors and teaching assistants in scoring and maintaining the grades for the assignment.
Description
The users of the expertiza are modeled using the user model. A participant signs up for courses and assignments. A user can have multiple participants and all participants belong to a user. The participant model is inherited by the course and assignment participant model. Our work involves refactoring functions defined in these models. Some methods were moved to places with better fit and some methods were improvised with better implementations. The further sections in the document will explain the changes complement and how it will impact the overall usage of expertiza.
Tasks accomplished in this project
- Moved the export_fields method to the User model from CourseParticipant model to remove redudancy.
- Added attr_accessible modifier to CourseParticipant to improve security.
- Refactored the get_user_list method into smaller methods, one method for each if condition in User model.
- Moved the ‘files’ method from AssignmentParticipant model which was duplicate and used in assignment_team.rb as well to a common place (FileHelper module).
- Refactored the average_question_score method by using sql summation to find values for sum_of_scores and number_of_scores variables in AssignmentParticipant model.
Models
User
The user model stores details of the user like name,email,role,timezone and his preferences on how expertiza should behave to him. A user can have many roles like student, instructor or administrator. The user model is used for logging in, participating in courses, assignments and creating teams. The user model maintains parent child relationship, a user will have one parent and can have many children.
Assignment Participant
Expertiza uses a participant abstraction for modeling the participance of a user in any of the activities like courses and assignments. A participant belongs to a user and it represents a connection between a user and an activity. Assignment_participant, which is a child class of participant models the association between users and assignments. This association also needs to be mapped to other activities performed by a user related to the assignment. These activities include reviews, quiz, response to reviews, teammate reviews etc.. The assignment_participant model is related to models which is responsible for all the mentioned activities.
Course Participant
The course_participant is a child class of the participant abstraction and it represents the association between a user and a course. The model also stores fields required for the association and supports bulk importing and exporting.
Changes
Refactoring: Duplicated export_fields method
This method exports the headers of the csv file to be downloaded. The method export_fields occurs in various models with different implementations. However, the implementation for this method had been duplicated in course_participant and user models. This led to the disadvantage of having redundant code in the system and an inefficient coding practice. User model represents the base class and other models inherit from this class. Hence, deleting the method from User model made little sense. The refactoring invoked the User models export_fields method from within the export_fields of course_participant. Since the export_file controller calls this method on the course_participant object, there wasn't a choice to delete the method completely. This refactoring has the advantage that code is reused and the proper object is tied to the header fields instead of just the generic User object.
Refactoring: Breaking get_user_list method
This method contained a lot of if-else statements that executed different codes depending on the type of user. The refactoring broke the large method into smaller methods, one for each user type. This improved the modularity of the code. It made the code more manageable and understandable from maintenance point of view (separation of responsibilities).
Refactoring: Use SQL summation to calculate average_question_score
The average of the score for a question in the review given by all other students who reviewed an assignment needs to be calculated in the average_question_score method in assignment participant model. This was originally implemented by using two for loops and fetching all the answers given by all the reviewers and then filtering using an if statement. This method consumes more CPU cycles and impacts the performance of the application server. It also consumes more bandwidth since the application fetches all the data from the database but returns only one value to the user. This was reimplemented to use the SQL summation inside the database and returns the only desired value to the application. This method also takes advantage of the database indexing and fetches only values required instead of filtering in the application.
Refactoring: Add attr_accessible to class to improve security
The course_participant model has a number of instance variables like can_submit, can_review, user_id, parent_id, submitted_at, permission_granted, penalty_accumulated, grade, type, handle, time_stamp, digital_signature, duty, can_take_quiz. Earlier, these variables were less secure and hence susceptible to tampering with URLs or forms from malicious users. In general, elements are filled based on the data sent as a hash from the view. This happens as mass assignment where any matching key in the hash will be set. To protect this mass assignment from malicious attacks, a good design is to add attr_accessible macro. Adding this keyword to instance variables ensures that only the specified attributes in the list of attr_accessible are allowed for mass assignment. This ensures that developers can carefully consider which attributes should be allowed to do mass assignment. This way, the refactoring is done.
attr_accessible :can_submit, :can_review, :user_id, :parent_id, :submitted_at, :permission_granted, :penalty_accumulated, :grade, :type, :handle, :time_stamp, :digital_signature, :duty, :can_take_quiz
Refactoring: files method is duplicate and used in assignment_team.rb as well. Move it to either a common place or keep it at the most appropriate place
This refactoring belongs to AssignmentParticipant model. This model is a special class of Participant and it denotes the participants who have collaborated for a particular Assignment. This method takes a parameter which is a directory and it returns all the files present in that directory. The directory argument is basically a string containing the file path for reviewer uploaded files during peer review. This argument comes from AssignmentParticipant model but the files method as a whole is independent of the model as its function is to return files present in a directory. It does not need to belong to any model. Hence the ‘files’ method must be placed in file_helper.rb. Thus the ‘files’ method is moved from the AssignmentParticipant model to a helper module like file_helper.rb. The design principle that is reinforced here is that a class must have responsibilities that belong to itself and not other “helper” responsibilities. This is an implementation of Separation Of Concerns design principle.
def files(directory)
files_list = Dir[directory + "/*"]
files = Array.new
files_list.each do |file|
if File.directory?(file)
dir_files = files(file)
dir_files.each{|f| files << f}
end
files << file
end
files
end
Refactoring: topic_name in Participant.rb
In participant.rb model, there is a method topic_name. However, this method doesn't seem to be required here. A course participant cannot have a topic. If it is an assignment participant object, we can find which team that participant is in first, then find which topic is this team holding. In fact, there is a topic_name field in SignedUpTeam class. On thorough investigation, we found that the method has no caller, which is the right implementation in any case. Hence, the refactoring involved deleting this method altogether. The refactoring was verified by the fact that none of the existing test cases failed.
Testing
Expertiza mainly uses RSpec for testing and has a comprehensive set of test cases overall. However, there are some models that do not have the required rspecs. We have included test cases for all the refactoring that we have done. We also ensured that the existing test cases do not fail.
Test adding attr_accessible to class to improve security
The earlier version of Expertiza did not have an rspec for the CourseParticipant model. We added an rspec named course_participant_spec.rb as the rspec for this model covering tests for the refactorings done in this project. The attr_accessible improves security by allowing only those fields that are in the list of attr_accessible macro for mass assignment. We test this by using “should allow_mass_assignment_of” and “should_not allow_mass_assignment_of”. Those attributes that are not in the list of attr_accessible will pass the test only when tested as “should_not allow_mass_assignment_of” and the attributes that are in the list of attr_accessible will pass the test only when tested as “should allow_mass_assignment_of”. The following is the code snippet of the test case.
describe "#accessible attributes" do
it { should allow_mass_assignment_of(:can_submit) }
it { should allow_mass_assignment_of(:can_review) }
it { should allow_mass_assignment_of(:user_id) }
it { should allow_mass_assignment_of(:parent_id) }
it { should allow_mass_assignment_of(:submitted_at) }
it { should allow_mass_assignment_of(:permission_granted) }
it { should allow_mass_assignment_of(:penalty_accumulated) }
it { should allow_mass_assignment_of(:grade) }
it { should allow_mass_assignment_of(:type) }
it { should allow_mass_assignment_of(:handle) }
it { should allow_mass_assignment_of(:time_stamp) }
it { should allow_mass_assignment_of(:digital_signature) }
it { should allow_mass_assignment_of(:duty) }
it { should allow_mass_assignment_of(:can_take_quiz) }
end
Test the export_fields method
This refactoring was done in the CourseParticipant model. The duplication is removed and the export_fields method is kept only in the User model and a call to the User model is made from CourseParticipant. To test this method, we test the functionality of this method as there is no test case for this method in the previous version. The export_fields method takes an argument of options which is a list of all the fields that the end user wants to export as course participant data and it returns the corresponding fields by making a call to User model. The ideal test case will test only the export_fields in the course_participant model and not the one in user model. This is in line with the strategy of testing incoming query messages by making assertions about what they send back. Therefore, for a given set of options, only the required right set of fields are to be returned. We expect that the returned list of fields will match the ones the method asked using the options field. Below is a sample test snippet.
describe "#export_fields" do
it "returns export fields for a csv based on radio buttons checked" do
options = {"personal_details"=>"true", "role"=>"true", "parent"=>"true", "email_options"=>"true", "handle"=>"true"}
fields = ["name", "full name", "email", "role", "parent", "email on submission", "email on review", "email on metareview", "handle"]
expect(CourseParticipant.export_fields(options)).to match_array(fields)
end
Test refactoring of get_user_list method into smaller methods
In order to test get_user_list, we create a dummy object of User and set its role as instructor and fetch the user list of that instructor which returns the list of participants of that instructor’s course. The list could be empty or non empty depending on the user object.
describe "#get_user_list" do
it 'user list' do
@role = Role.new name: "Instructor"
user1 = User.new name: "instructor4", role: @role
expect(user1.get_user_list).to be_empty
expect(user1.get_users_instructor)
end
end
Test files method in the FileHelper module
The files method takes an argument of a directory path and returns all the files present in that directory. In the original repository, there was no test case for this method. We have added a test for this method in the file_helper_spec.rb file. The test case takes an example directory like /tmp/expertiza1603/ and returns the files present in that directory. Since the test cases can be run in any environment, the result of the test case is dependent on the presence of such a directory structure. We have created a sample directory with two files on the VCL environment(152.7.99.160). If such a directory is not present, it is important to create a directory by that name.
describe FileHelper do
class DummyClass
end
before(:each) do
@dummy_class = DummyClass.new
@dummy_class.extend(FileHelper)
end
describe "#files" do
it 'return files present in a dir' do
expected_array = ["/tmp/expertiza1603/1.txt", "/tmp/expertiza1603/2.txt"]
expect(@dummy_class.files("/tmp/expertiza1603")).to match_array(expected_array)
end
end
end
Testing through GUI
Test export fields method
- Login as super_administrator2 or administrator5 with password as "password"
- Click on "Manage..."" and click on "Courses"
- On the "Actions" tab, click on "Add Participants"
- On the page that opens up, go to the bottom and select "Export course participants"
- Click on "Export" button and a csv file should download with the selected radio button fields.
Test get_user_list method
- Login as super_administrator2 or administrator5 with password as "password"
- Click on "Manage..."" and click on "Users"
- You should see all the users on the page.