CSC/ECE 517 Fall 2023 - E2359. Refactor user controller.rb, user.rb, and its child classes

Expertiza

Expertiza is a web application through which students can submit and peer-review learning objects (articles, code, web sites, etc). The National Science Foundation supports the Expertiza project.

It is used in select courses at NC State and by professors at several other colleges and universities.

Expertiza is a Ruby on Rails based open source project.

Problem Statement

Background: The User model is a critical component of the Expertiza application, handling user data and authentication/authorization features. It is linked to other models such as Participant, TeamsUser, and Invitation, allowing for associations and a personalized user experience.

The user model is large, containing 9 class methods and 23 instance methods and code for defining relationships with other models, validations, and named scopes.

While the user controller is smaller, it has 17 methods that could benefit from refactoring and improved comments. The code can be found here.

Refactor This model and a controller do not have conflated responsibilities but need a lot of refactoring to make them more readable and maintainable.

• User.rb
• SuperAdministrator.rb
• Instructor.rb
• Ta.rb
• UserController.rb

Files Modified

We have recently made some improvements to the codebase by refactoring several methods in the application. These changes were made to enhance code readability, reduce redundancy, and promote best coding practices. In particular, we have streamlined methods related to listing, retrieving, and filtering data, resulting in cleaner and more maintainable code. These modifications aim to make our codebase more efficient while maintaining the same functionality as before.

Changed files:

• app/controllers/impersonate_controller.rb
• app/controllers/users_controller.rb
• app/controllers/questionnaires_controller.rb
• app/models/instructor.rb
• app/models/ta.rb
• app/models/user.rb
• app/models/assignment_form.rb
• spec/models/ta_spec.rb
• spec/models/user_spec.rb

New Files:

• app/helpers/anonymized_helper.rb

Refactoring

The refactoring includes the following changes to improve code quality, readability, and maintainability while preserving the functionality of our software.

• Recognized the need for refactoring to improve code readability and maintainability.
• Introduced a new method, get_participants_from_instructed_entities, to handle the retrieval of participants from entities (either Course or Assignment) where the given user is an instructor.
• Replaced nested loops with more concise and efficient array operations using flat_map, reducing temporary arrays and enhancing code organization.
• Introduced a new method, filter_participants, to handle participant filtering based on user privileges. This reduced code duplication and improved readability.
• In the my_tas method, replaced the loop with a flat_map operation, extracting TA IDs directly from the TaMapping records. This simplified the code and improved efficiency.
• In the list_all, list_mine, and get methods, introduced a new method, filter_by_instructor, to handle filtering records based on the instructor ID and whether private records should be included. This reduced code duplication and improved readability.
• Throughout the refactoring process, maintained the same functionality while adding comments to explain the purpose of each method and operation.
• Used string interpolation instead of string concatenation for better readability.
• Removed redundant scopes in user.rb
• Refactored validations in user.rb
• Modified method names to reflect responsibility of the code
• Followed DRY principle and extracted repeated methods into helpers
• Added separate module for functionality pertaining to anonymized view.
• Made changes to refactor check_if_input_is_valid in impersonate_controller.rb using DRY principle to make code concise and readable.
• In user.rb renamed get_available_users method and it's function calls to get_visible_users_with_lesser_roles to make it more meaningful.
• Removed salt_first method in user_spec.rb as it does not contribute to application and is only used in test files.
• Moved functionality associated with anonymized view from user.rb to a new file app/helpers/anonymized_helper.rb.
• Replaced global variable usage of redis. Used singleton design pattern to enforce single instance of redis variable.
• Fixed lint errors

users_controller.rb

app/controllers/users_controller.rb

String Interpolation: The line flash[:note] = params[:user][:name] + ' does not exist.' has been changed to flash[:note] = "#{params[:user][:name]} does not exist.". This is a more idiomatic way to include variables in strings in Ruby.

Query Parameterization: The line @maps = ResponseMap.where('reviewee_id = ? or reviewer_id = ?', params[:id], params[:id]) has been changed to @maps = ResponseMap.where('reviewee_id = :id OR reviewer_id = :id', id: params[:id]). This is a more secure way to include parameters in SQL queries as it helps prevent SQL injection attacks.

Method Name Changes: The method name foreign has been changed to available_roles. This new name is more descriptive and makes the code easier to understand.

Removal of Redundant Code: The lines AssignmentParticipant.where(user_id: @user.id).each(&:delete) and TeamsUser.where(user_id: @user.id).each(&:delete) have been removed. This is because when a user is destroyed with @user.destroy, all associated records in AssignmentParticipant and TeamsUser are also automatically destroyed if the associations are set up with dependent: :destroy.

instructor.rb

app/models/instructor.rb

list_all Method: The list_all method retrieves instances of a given type where the user is either the instructor or the instance is not private. It uses an object_type.where query with an SQL condition for instructor and privacy.

list_mine Method: The list_mine method retrieves instances of a given type where the user is the instructor. It uses a similar object_type.where query with an SQL condition for instructor.

get Method: The get method retrieves a specific instance of a given type by its ID, considering the instructor or privacy conditions. It uses the object_type.find_by method with these conditions.

my_tas Method: The my_tas method retrieves a list of Teaching Assistant (TA) IDs associated with courses instructed by the given user. It has been refactored to use flat_map and a more efficient approach to collect TA IDs.

get_user_list Method: The get_user_list method retrieves a list of users who are participants in the courses and assignments where the given user is an instructor. The code has been refactored to provide a more efficient and organized approach.

get_participants_from_instructed_entities Method: This method is introduced to get participants from entities where the user is an instructor. It's used in get_user_list for both courses and assignments.

filter_participants Method: A new method to filter participants based on user privileges. It selects participants whose role's privileges are all included in the user's role's privileges.

ta.rb

app/models/ta.rb courses_assisted_with Method: The courses_assisted_with method has been updated for better readability. It now directly returns an array of courses associated with the TA, using a more concise TaMapping.where and Course.find combination.

instructor_or_co_ta? Method: The instructor_or_co_ta? method has been enhanced for readability. It checks if the TA is either the instructor or co-TA for a given questionnaire. The code has been refactored for better structure.

list_all and list_mine Methods: Both methods have been improved for better consistency and readability.

list_mine Method: The SQL queries have been updated for a more consistent and concise format.

get_my_instructors and get_mapped_instructor_ids Methods: These methods have been simplified to use more efficient mapping and querying, making the code cleaner and more readable.

instructor and instructor= Methods: These methods have been enhanced for readability. The instructor method is now a getter, and the instructor= method is a setter for better naming consistency.

get_user_list Method: The get_user_list method has been updated for improved efficiency and readability. It now utilizes helper methods for better organization and readability.

Helper Methods: Several private helper methods have been introduced to streamline the code and improve maintainability. These methods assist in retrieving participants, filtering participants based on roles, and other tasks.

These changes aim to enhance the code's readability, maintainability, and efficiency while maintaining the functionality of the Ta class. The use of helper methods and better-structured SQL queries contributes to a cleaner and more organized codebase.

assignment_form.rb

app/models/assignment_form.rb

Using update to modify multiple attributes of new_assign in a single database update operation. This is more efficient than multiple update_attribute calls. Removed the assignment of timestamps for created_at and updated_at fields as they are already being set during the update call.

These changes simplify the code, reduce unnecessary database queries, and make it more efficient.

ta_spec.rb

Test Case Changes: The test cases have been updated to reflect these method name changes. For example, the test case for get_instructor has been changed to test instructor instead.

Method Call Changes: The way the instructor= method is called has been changed. Instead of ta.set_instructor(assignment), it’s now ta.instructor = assignment.

Expectation Changes: The expectations in the test cases have been updated. For example, in the test case for instructor=, instead of expecting the method to return a value, it now checks that the instructor_id and course_id attributes of the assignment have been set correctly.

Query Changes: In the test case for ‘list_mine’, the SQL query string has been changed from using simple string concatenation to using a parameterized query. This can help prevent SQL injection attacks.

spec/models/ta_spec.rb

Method Name Changes: The method names get_instructor and set_instructor have been changed to instructor and instructor= respectively. This is more in line with Ruby’s convention for getter and setter methods.

questionnaires_controller.rb

app/controllers/questionnaires_controller.rb

Method Name Changes: The method is_instructor_or_co_ta? has been changed to instructor_or_co_ta?. (methods that return a boolean value).

impersonate_controller.rb

app/controllers/impersonate_controller.rb

Method Call Changes: The method get_available_users has been changed to get_visible_users_with_lesser_roles. This suggests a change in the logic for fetching users, now it seems to fetch users with roles lesser than the current user.

Code Simplification: The check_if_input_is_valid method has been simplified. Instead of checking separately for params[:user][:name] and params[:impersonate][:name], it now checks for a user_name that could come from either params[:user][:name] or params[:impersonate][:name]. This reduces code duplication.

Error Handling: The error handling in check_if_input_is_valid has been consolidated into a single line. If the user_name contains special characters, an error message is flashed and the user is redirected back.

user_spec.rb

spec/models/user_spec.rb

Method Name Changes: The method names get_instructor and get_available_users have been changed to instructor and get_visible_users_with_lesser_roles, respectively.

Test Case Changes: The test cases have been updated to reflect these method name changes. For example, the test case for get_instructor has been changed to test instructor instead, and the test case for get_available_users has been changed to test get_visible_users_with_lesser_roles.

Expectation Changes: The expectations in the test cases have been updated. For example, in the test case for instructor, it now checks that the instructor method returns the correct id.

anonymized_helper.rb

app/helpers/anonymized_helper.rb

anonymized_view?(ip_address = nil): This method checks if the view should be anonymized for a given IP address. It retrieves a string of starter IP addresses for anonymized views from Redis (an in-memory data structure store) and checks if the provided IP address is included in that string. If the IP address is included, it returns true, indicating that the view should be anonymized. If not, it returns false.

real_user_from_anonymized_name(anonymized_name): This method retrieves the original user from an anonymized name. It finds a user in the database with a name that matches the provided anonymized name and returns that user.

Next Steps

• We are going to continue with SuperAdministrator.rb refactoring.

Team

Mentor

• Devashish Vachhani

Members

• Doddaguni, Sachin R
• Mahesh, Amogh
• Villar, Sergio Vargas

References and Relevant Links

1. Expertiza
2. OSS Projects on Expertiza
3. Github
4. Pull Request
5. Expertiza VCL Server