CSC/ECE 517 Fall 2024 - E2479. Reimplement teams users controller.rb

Expertiza

Expertiza is an open-source learning management system built with Ruby on Rails as its core. Its features include creating tests and assignments, managing assignment teams and courses, and having a solid framework to facilitate peer reviews and group comments. The main objective of this project is to reimplement teams_users_controller.rb. The goal is to reimplement teams_users_controller.rb from the Expertiza repository to the reimplementation-back-end repository to follow SOLID and DRY principles.

Problem Statement

The following were the problems with the previous implementation :

Lack of Modularity and SRP Violations: The create method is excessively long and handles multiple concerns such as finding users, checking assignments/courses, adding members, and handling various flash messages. This violates the Single Responsibility Principle (SRP) and makes the method difficult to maintain. Similar complexity is observed in other methods like auto_complete_for_user_name and delete.
Redundant and Repetitive Code: There is significant duplication in handling AssignmentTeam and CourseTeam logic in the create method. This could be abstracted into separate model methods or helper functions. The repeated checks for user membership and participant status introduce unnecessary redundancy.
Error Handling and Flash Message Management: Flash messages are scattered throughout the code, with some being overly complex and not user-friendly.
Inconsistent and Non-Descriptive Naming: Variable and method names, such as urlCreate, add_member_return, and urlCourseParticipantList, do not follow consistent naming conventions and are not very descriptive.
Tightly Coupled Logic: Business logic, such as validating membership and handling team participant status, resides within the controller instead of being abstracted into model methods. This makes the controller overly complex and difficult to test. Logic specific to AssignmentTeam and CourseTeam is handled in the controller, whereas it should ideally be part of respective models or services.
Poor Use of Access Control Mechanism: The action_allowed? method is hardcoded to check for privileges using strings and is difficult to extend.
Insufficient User Feedback and Validation: The implementation lacks comprehensive validation and feedback mechanisms, such as form-level client-side validation, detailed error messages, and modern UX elements like modal pop-ups or toast notifications.
Limited Error Handling in Batch Operations: The delete_selected method performs deletions in a loop without adequate error handling or feedback for each item.
Unclear Role Management: The update_duties method updates a duty using direct attribute assignment without validation or checks, potentially leading to data inconsistency.

Key Design Goals

Single Responsibility Principle (SRP): Each method will handle a single responsibility. Complex logic will be refactored into helper methods or moved to appropriate models.
Open/Closed Principle: Methods should be open for extension but closed for modification.
Dependency Inversion Principle: High-level modules should not depend on low-level modules; both should depend on abstractions where possible.
DRY (Don’t Repeat Yourself) Principle: Remove repetitive code through reusable helper methods. Ensure that shared logic is centralized.

Implementation Plan

Rename TeamsUsersController to TeamsParticipantsController:
- Update the controller name to reflect its true purpose: managing participants in teams.
- Change all references, associations, and routing paths across the codebase to ensure consistency.
- Modify the controller path under the `controllers` directory to `teams_participants_controller.rb`.
- Design principles that this will be satisfying:
  - Single Responsibility Principle (SRP): The controller focuses solely on participant-related operations.
  - Consistency: Proper naming reduces confusion and aligns with domain language.

Restructure the `create` Method:
- Extract validation logic into helper methods or models.
- Move the model-specific logic into the relevant assignment or course models.
- Ensure the user is not already part of another team unless they qualify as a mentor and use polymorphism to render it for better abstraction.
- Refactor repetitive code into reusable methods, such as adding a member to the team or generating error messages.
- Design principles that this will be satisfying:
  - DRY: Reduces duplication in validation and error handling.
  - Single Responsibility Principle (SRP): Delegate team and participant-specific logic to appropriate models or services.

Refactor Other Methods:
- Move methods not involving user interactions (e.g., adding participants to teams, checking constraints) to the corresponding model classes (`Team`, `AssignmentTeam`, or `CourseTeam`).
- Ensure helper methods are used for common logic like generating error messages or finding records.
- Design principles that this will be satisfying:
  - Single Responsibility Principle (SRP): The controller acts as a mediator between models and views.
  - Open/Closed Principle (OCP): The controller becomes extensible without modification by abstracting logic into models.

Centralize Error Handling:
- Use a shared error-handling module to manage common exceptions like `RecordNotFound` or validation errors.
- Move dynamic error messages into helper methods for consistent feedback.
- Design principles that this will be satisfying:
  - DRY: Avoid repeating error-handling logic across actions.

Use Strong Variable names
- Use an appropriate naming convention for the variables since they are inconsistent throughout the class.
- Design principles that this will be satisfying:
  - Single Responsibility Principle (SRP): Validating input is handled at the parameter level, keeping controller logic focused.

Write Tests for Each Method
- Test each method in the controller action thoroughly
- Design principles that this will be satisfying:
  - Robustness: Well-tested code ensures fewer bugs and reliable functionality.

Add Descriptive Comments
- Write clear comments for methods, helper functions, and validations.
- Document the purpose and flow of logic to make the code easier for future developers to understand and maintain.
- Design principles that this will be satisfying:
  - Maintainability: Descriptive comments enhance code readability.

Flow Chart for the teams_users_controller Page

UML Diagram

Implementation

Rename TeamsUsersController to TeamsParticipantsController:
Existing_implementation: The class was named TeamsUsersController, with inconsistent variable and method naming conventions.
Reimplementation: Renamed to TeamsParticipantsController to reflect the nature of the resource being managed (participants of teams). This aligns with domain-driven design and improves clarity.

Create Method Refactor:
Existing_implementation: The create method was lengthy, with deeply nested logic for adding participants to teams, error handling, and validation all in one place.

Existing create method in expertiza

def create
   user = User.find_by(name: params[:user][:name].strip)
   unless user
     urlCreate = url_for controller: 'users', action: 'new'
     flash[:error] = "\"#{params[:user][:name].strip}\" is not defined. Please <a href=\"#{urlCreate}\">create</a> this user before continuing."
   end
   team = Team.find(params[:id])
   unless user.nil?
     if team.is_a?(AssignmentTeam)
       assignment = Assignment.find(team.parent_id)
       if assignment.user_on_team?(user)
         flash[:error] = "This user is already assigned to a team for this assignment"
         redirect_back fallback_location: root_path
         return
       end
       if AssignmentParticipant.find_by(user_id: user.id, parent_id: assignment.id).nil?
         urlAssignmentParticipantList = url_for controller: 'participants', action: 'list', id: assignment.id, model: 'Assignment', authorization: 'participant'
         flash[:error] = "\"#{user.name}\" is not a participant of the current assignment. Please <a href=\"#{urlAssignmentParticipantList}\">add</a> this user before continuing."
       else
         begin
           add_member_return = team.add_member(user, team.parent_id)
         rescue
           flash[:error] = "The user #{user.name} is already a member of the team #{team.name}"
           redirect_back fallback_location: root_path
           return
         end
         flash[:error] = 'This team already has the maximum number of members.' if add_member_return == false
       end
     else # CourseTeam
       course = Course.find(team.parent_id)
       if course.user_on_team?(user)
         flash[:error] = "This user is already assigned to a team for this course"
         redirect_back fallback_location: root_path
         return
       end
       if CourseParticipant.find_by(user_id: user.id, parent_id: course.id).nil?
         urlCourseParticipantList = url_for controller: 'participants', action: 'list', id: course.id, model: 'Course', authorization: 'participant'
         flash[:error] = "\"#{user.name}\" is not a participant of the current course. Please <a href=\"#{urlCourseParticipantList}\">add</a> this user before continuing."
       else
         begin
           add_member_return = team.add_member(user, team.parent_id)
         rescue
           flash[:error] = "The user #{user.name} is already a member of the team #{team.name}"
           redirect_back fallback_location: root_path
           return
         end
         flash[:error] = 'This team already has the maximum number of members.' if add_member_return == false
         if add_member_return
           @teams_user = TeamsUser.last
           undo_link("The team user \"#{user.name}\" has been successfully added to \"#{team.name}\".")
         end
       end
     end
   end
   redirect_to controller: 'teams', action: 'list', id: team.parent_id
 end

Reimplementation:

We have split the create function that uses helper methods: find_participant_by_name: Finds a participant by name. find_team_by_id: Retrieves the team by ID. validate_participant_and_team: Ensures the participant is valid for the team. add_participant_to_team: Adds a participant to the team and handles constraints. handle_addition_result: Processes results of the addition attempt. Reduced nesting and reused shared logic through helper methods. Error messages were extracted to dedicated methods for reusability.

# Adds a new participant to a team after validation.
 def create_participant
   # Find the user by their name from the input.
   find_participant = find_participant_by_name
   # Fetch the team using the provided ID.
   current_team = find_team_by_id
   if validate_participant_and_team(participant, team)
     if team.add_participants_with_handling(participant, team.parent_id)
       undo_link("The participant \"#{participant.name}\" has been successfully added to \"#{team.name}\".")
     else
       flash[:error] = 'This team already has the maximum number of members.'
     end
   end
   # Redirect to the list of teams for the parent assignment or course.
   redirect_to controller: 'teams', action: 'list', id: current_team.parent_id
 end

 private

 # Helper method to find a user by their name.
 def find_participant_by_name
   # Locate the user by their name.
   find_participant = User.find_by(name: params[:user][:name].strip)

   # Display an error if the user is not found.
   unless find_participant
     flash[:error] = participant_not_found_error
     redirect_back fallback_location: root_path
   end
   participant
 end

 # Helper method to fetch a team by its ID.
 def find_team_by_id
   Team.find(params[:id])
 end

 # Validates whether a participant can be added to the given team.
 def validate_participant_and_team(participant, team)
   # Check if the participant is valid for the team type.
   validation_result = if team.is_a?(AssignmentTeam)
                         Assignment.find(team.parent_id).valid_team_participant?(participant)
                       else
                         Course.find(team.parent_id).valid_team_participant?(participant)
                       end
   # Handle validation errors if any.
   if validation_result[:success]
     true
   else
     flash[:error] = validation_result[:error]
     redirect_back fallback_location: root_path
     false
   end
 end

 # Adds the participant to the team while handling constraints.
 def add_participant_to_team(find_participant, team)
   # Add the participant to the team and handle the outcome.
   addition_result = find_team_by_id.add_participant(find_participant, team.parent_id)
   handle_addition_result(find_participant, team, addition_result)
 end

 # Handles the result of adding a participant to the team.
 def handle_addition_result(find_participant, team, addition_result)
   if addition_result == false
     flash[:error] = 'This team already has the maximum number of members.'
   else
     undo_link("The team user \"#{find_participant.name}\" has been successfully added to \"#{team.name}\".")
   end
 end

 # Generates an error message when a user is not found.
 def participant_not_found_error
   new_participnat_url = url_for controller: 'users', action: 'new'
   "\"#{params[:user][:name].strip}\" is not defined. Please <a href=\"#{new_participant_url}\">create</a> this user before continuing."
 end

 def non_participant_error(find_participant, parent_id, model)
   urlParticipantList = url_for controller: 'participants', action: 'list', id: parent_id, model: model, authorization: 'participant'
   "\"#{find_participant.name}\" is not a participant of the current course/assignment. Please <a href=\"#{urlParticipantList}\">add</a> this user before continuing."
 end

Model specific methods:
Existing_implementation: Logic to check if the user belongs to the assignment or course is present in the create method making it more complex.
Reimplementation: Moved these responsibilities to the respective models

#Course model
def valid_team_participant?(user)
  # Check if the user is already a member of another team for the same course.
  if user_on_team?(user)
    { success: false, error: "This user is already assigned to a team for this course" }
  # Check if the user is a participant in the course associated with this team.
  elsif CourseParticipant.find_by(user_id: user.id, parent_id: course_id).nil?
    { success: false, error: "#{user.name} is not a participant in this course" }
  # If both checks pass, the user is eligible to join the team.
  else
    { success: true }
  end
end

#Assignment Model
def valid_team_participant?(user)
  # Check if the user is already part of a team for this assignment.
  if user_on_team?(user)
    { success: false, error: "This user is already assigned to a team for this assignment" }
  # Check if the user is a registered participant in the assignment.
  elsif AssignmentParticipant.find_by(user_id: user.id, parent_id: assignment_id).nil?
    { success: false, error: "#{user.name} is not a participant in this assignment" }
  # If both checks pass, the user is eligible to join the team.
  else
    { success: true }
  end
 end

Bulk Operations:
bulk_delete_participants
Existing_implementation: Methods, like delete_selected, had repetitive logic for iterating and deleting participants.
Reimplementation: Encapsulated bulk operations within the teams_user model layer to keep the controller concise.

 #Deletes multiple team members in bulk.
 def self.bulk_delete_participants(team_user_ids)
   where(id: team_user_ids).destroy_all
 end

add_participants
Existing_implementation: The method to add a participant is present inside the create method affecting the Single Responsibility principle.
Reimplementation: Moved the logic to the teams model and used polymorphism to use it.

Check if a user is already a member of the team if not it adds the user

 #Teams model
 def add_participant(user, _assignment_id = nil)
   raise "The user #{user.name} is already a member of the team #{name}" if user?(user)
   can_add_member = false
   unless full?
     can_add_member = true
     t_user = TeamsUser.create(user_id: user.id, team_id: id)
     parent = TeamNode.find_by(node_object_id: id)
     TeamUserNode.create(parent_id: parent.id, node_object_id: t_user.id)
     add_participant(parent_id, user)
     ExpertizaLogger.info LoggerMessage.new('Model:Team', user.name, "Added member to the team #{id}")
   end
   can_add_member
 end

Error Handling and Flash Messages:
Existing_implementation: Error messages were scattered and inconsistent and all of them were in the create method making it more and more complex.
Reimplementation: Consolidated error messages into dedicated helper methods (participant_not_found_error, non_participant_error). Improving consistency and reusability of error reporting.

Code Comments and Variable names:
Existing_implementation: Sparse comments and inconsistent variable names, often not descriptive enough to explain the logic.
Reimplementation: Added descriptive comments for every method to explain the purpose, and key logic. And modified the variable names without affecting the implementation.

Testing

For Testing, we have used Rswag to create an integration spec to describe and test API using rails generate rspec:swagger Api::V1::RolesController

Testing action_allowed? method: This test suite validates whether the action_allowed? method in the Api::V1::TeamsParticipantsController correctly determines the permissions for users attempting to perform specific actions. The goal is to ensure that access is restricted or granted based on user roles and the requested action, adhering to the application's authorization policies.

RSpec.describe Api::V1::TeamsParticipantsController, type: :controller do

 let(:student_role) { create(:role, :student) }
 let(:instructor_role) { create(:role, :instructor) }
 let(:instructor) { create(:user, role: create(:role, :instructor)) }
 let(:course) { create(:course, instructor_id: instructor.id) }
 let(:user) { create(:user, role: student_role) }
 let(:ta) { create(:teaching_assistant) }
 let(:team) { create(:team, parent_id: create(:assignment).id) }
 let(:participant) { create(:user, name: 'Test Participant') }
 let(:team_participant) { create(:teams_user, user: participant, team: team) }

 describe '#action_allowed?' do
   context 'when action is update_duties' do
     it 'allows access for students' do
       allow(controller).to receive(:current_user_has_student_privileges?).and_return(true)
       allow(controller).to receive(:params).and_return({ action: 'update_duties' })
       expect(controller.action_allowed?).to be true
     end

     it 'denies access for non-students' do
       allow(controller).to receive(:current_user_has_student_privileges?).and_return(false)
       allow(controller).to receive(:params).and_return({ action: 'update_duties' })
       expect(controller.action_allowed?).to be false
     end
   end

   context 'when action is not update_duties' do
     it 'allows access for TAs' do
       allow(controller).to receive(:current_user_has_ta_privileges?).and_return(true)
       allow(controller).to receive(:params).and_return({ action: 'list_participants' })
       expect(controller.action_allowed?).to be true
     end
   end
 end

end

Expected Behavior:
- Students should be authorized to update duties.
- Non-students should not be authorized to update duties.
- Teaching assistants should be authorized for actions other than update_duties.

Output

Testing delete_participant using rswag:: The test validates the functionality of deleting a participant (TeamsUser) from a team. It ensures the deletion removes the participant's record and confirms successful removal.

require 'swagger_helper' RSpec.describe 'api/v1/teams_participants', type: :request do

 path '/api/v1/teams_participants/{id}/delete_participant' do
   parameter name: 'id', in: :path, type: :string, description: 'id'
   delete('delete_participant teams_participant') do
     response(200, 'successful') do
       let(:id) { '123' }
       after do |example|
         example.metadata[:response][:content] = {
           'application/json' => {
             example: JSON.parse(response.body, symbolize_names: true)
           }
         }
       end
       run_test!
     end
   end
 end

end

Expected Behaviour:
- The participant with <user_id> should be removed.
- A success message should be displayed to confirm the deletion.
- The system should maintain data consistency by ensuring no dangling references remain.

Output

Team

Manideepika Reddy Myaka (mmyaka@ncsu.edu)
Bhuvan Chandra Kurra (bkurra@ncsu.edu)

Mentor

Richard Li (rli14@ncsu.edu)

CSC/ECE 517 Fall 2024 - E2479. Reimplement teams users controller.rb

Contents

Expertiza

Problem Statement

Key Design Goals

Implementation Plan

Flow Chart for the teams_users_controller Page

UML Diagram

Implementation

Testing

Team

Mentor

Relavant Links

Navigation menu

CSC/ECE 517 Fall 2024 - E2479. Reimplement teams users controller.rb

Expertiza

Problem Statement

Key Design Goals

Implementation Plan

Flow Chart for the teams_users_controller Page

UML Diagram

Implementation

Testing

Team

Mentor

Relavant Links

Navigation menu

Search