CSC/ECE 517 Spring 2025 - E2524 Reimplement student review controller

Introduction

The goal of this project is to reimplement the `StudentReviewController` and its associated views. The reimplementation will introduce a new service layer, streamline logic, and remove obsolete code (e.g., references to metareviews). The new design prioritizes clarity, modularity, and maintainability, following Rails best practices and RESTful conventions.

Purpose and Scope

This reimplementation addresses longstanding design issues in the original controller, including overly complex methods, redundant logic, and tight coupling between controller and view layers.

Objectives

• Redesign the controller to delegate all business logic to a new service class
• Eliminate obsolete functionality (e.g., metareviews)
• Improve testability by enabling unit tests on the service layer
• Simplify the view templates by preparing data in the controller beforehand
• Adhere to Rails RESTful conventions and best practices.
• Improve maintainability, localization support, and scalability.

Requirements

Reimplement the `StudentReviewController` and ensure all associated components function correctly. The components involved include:

Models Used

• `AssignmentParticipant`
• `Assignment`
• `SignedUpTeam`
• `ReviewResponseMap`
• `SampleReview`
• `MetareviewResponseMap` (obsolete – all references to be removed)

Addressed Problems

• Controller Complexity:

The `list` method is too long and handles multiple responsibilities: data loading, logic, rendering setup, and redirects. Business logic is intertwined with HTTP concerns (e.g., redirection, instance variables).

• Redundant Logic:

The `action_allowed?` method checks for student privileges multiple times.

• Hard-Coded Values:

Literal strings like `"list"` and `"submitter"` are used instead of constants.

• Obsolete Code:

The `MetareviewResponseMap` model is no longer relevant but still referenced.

• Lack of Separation of Concerns:

Controller handles business logic that should reside in service/model layers.

Design Principles Analysis

Single Responsibility Principle (SRP)

• Focused Responsibilities:

StudentReviewService handles only review-related operations. ApplicationController manages cross-cutting concerns such as authorization and locale setting. Each class and method has one clear and dedicated purpose.

 def calculate_review_progress
    @num_reviews_total = @review_mappings.size
    @num_reviews_completed = @review_mappings.count do |map|
      !map.response.empty? && map.response.last.is_submitted
    end
    @num_reviews_in_progress = @num_reviews_total - @num_reviews_completed
  end

DRY (Don’t Repeat Yourself)

• Centralized Logic:

Shared methods like load_participant_and_assignment promote reuse. Authorization and locale-setting logic are abstracted into concerns.

 def load_participant_and_assignment(participant_id)
    @participant = AssignmentParticipant.find(participant_id)
    @assignment = @participant.assignment
    @topic_id = SignedUpTeam.topic_id(@assignment.id, @participant.user_id)
    @review_phase = @assignment.current_stage(@topic_id)
  end

Maintainability and Testability

• Modular Design:

Business logic is placed in services to simplify future updates. Private methods are used to improve unit test isolation. Clear method names improve code readability.

 def load_review_mappings
    reviewer = @participant.get_reviewer
    @review_mappings = reviewer ? ReviewResponseMap.where(...) : []
    @review_mappings = @review_mappings.sort_by { |m| m.id % 5 } if @assignment.is_calibrated
  end

RESTful Best Practices

• Rails-Conventional Controllers:

RESTful routes and actions are followed. AuthenticationController includes a clear and secure login endpoint.

 def login
    user = User.find_by(name: params[:user_name]) || User.find_by(email: params[:user_name])
    if user&.authenticate(params[:password])
      token = JsonWebToken.encode({ id: user.id, ... }, 24.hours.from_now)
      render json: { token: }, status: :ok
    else
      render json: { error: 'Invalid username / password' }, status: :unauthorized
    end
  end

Localization and Internationalization

• Locale Handling:

Locale is determined from parameters or browser headers. Rails i18n support is properly configured and applied.

def set_locale
    I18n.locale = extract_locale || I18n.default_locale
  end

Summary Table

Principle	Status	Notes
Single Responsibility	Implemented	Each class has one focused responsibility
DRY	Implemented	Shared logic centralized in service methods
Maintainability & Testability	Implemented	Modular code with helper methods and clear naming
RESTful Practices	Implemented	Follows Rails conventions
Localization/i18n	Implemented	Locale handled through params and headers

Additional Strengths

• Service Object Pattern:

Business logic is encapsulated in dedicated service classes, improving modularity and reuse.

• JWT Authentication:

Secure authentication is implemented using JSON Web Tokens with proper token generation and expiration handling.

• Readable Method Naming:

Method names clearly reflect their responsibilities, enhancing code readability and maintainability.

Controller Diagram

Design / Proposed Solution

Separation of Concerns

This reimplementation is based on a clear separation of responsibilities:

• The controller handles only routing, access validation, and view setup.
• The service object encapsulates all business logic and data handling.
• Obsolete functionality such as `MetareviewResponseMap` is eliminated to simplify the design.

Architectural Overview

The new design will be based on the following structure:

• A `StudentReviewController` that handles only HTTP responsibilities.
• A new `StudentReviewService` that encapsulates all business logic.
• Continued use of models like `AssignmentParticipant`, `Assignment`, and `ReviewResponseMap` (excluding `MetareviewResponseMap`).
• Views adjusted to work with service-driven data.

Breakdown of Controller Responsibilities

• Validate access using a simplified `action_allowed?` method
• Set locale using controller_locale
• Call `StudentReviewService.new(id, current_user).perform` to gather data
• Assign service output to instance variables for the view
• Redirect if bidding is enabled

Breakdown of Service Object Responsibilities

• Retrieve `AssignmentParticipant` and associated `Assignment`
• Calculate `topic_id` using `SignedUpTeam.topic_id`
• Determine review phase using `assignment.current_stage`
• Get reviewer via `participant.get_reviewer`
• Load and (if applicable) sort review mappings (`ReviewResponseMap`)
• Count completed and in-progress reviews
• Load sample reviews (`SampleReview`) and extract `response_ids`
• Check if bidding is enabled

Removal of Obsolete Code

• All logic involving `MetareviewResponseMap` was deleted
• Related counters for metareview progress was removed from both controller and views

action_allowed? Method

• Eliminate redundant privilege check: `|| current_user_has_student_privileges?`
• Use constants instead of literal strings (`LIST_ACTION = 'list'`)
• Restructure logic as:

  LIST_ACTION     = 'list'.freeze
  SUBMITTER_ROLE  = 'submitter'.freeze

  before_action :authorize_user, only: [LIST_ACTION.to_sym]
  before_action :load_service, only: [:list]

  private

  def authorize_user
    unless action_allowed?
      render json: { error: 'Unauthorized' }, status: :unauthorized
      return
    end
  end

  # Quick-exit unless they're a student.
  # Then, only allow the LIST_ACTION for users who also have the SUBMITTER_ROLE on this resource.
  def action_allowed?
    # guard clause: must be a student at all
    return false unless current_user_has_student_privileges?  # early return

    # only permit “list” for submitters
    action_name == LIST_ACTION &&
      are_needed_authorizations_present?(params[:id], SUBMITTER_ROLE)
  end

controller_locale Method

• Ensure the `controller_locale` method sets the locale appropriately for student users, supporting internationalization and accessibility. Adding the below code to ApplicationController Class.

  # 1) Set locale for every request
  before_action :set_locale
  # 2) Perform authorization
  before_action :authorize

  private

  # Sets I18n.locale based on params, Accept-Language header, or default
  def set_locale
    I18n.locale = extract_locale || I18n.default_locale
  end

  # Returns a valid locale symbol or nil
  def extract_locale
    # 1) Param override
    if params[:locale].present?
      sym = params[:locale].to_sym
      return sym if I18n.available_locales.include?(sym)
    end

    # 2) Accept-Language header fallback
    header = request.env['HTTP_ACCEPT_LANGUAGE'].to_s
    header
      .split(',')
      .map { |l| l[0..1].downcase.to_sym }
      .find { |loc| I18n.available_locales.include?(loc) }
  end
end

list Method

The `list` method will be removed and replaced with a simplified call to the service:

def list
  return unless valid_participant?
  @data = StudentReviewService.new(params[:id], current_user.id).perform
  redirect_to bidding_path if @data[:bidding_enabled]
end

View Refactoring Plan

• `list.html.erb`: Simplify to only display data passed in from `@data`
• `_responses.html.erb`: Remove inline logic (e.g., topic ID lookups) and rely on passed-in values
• `_set_dynamic_*`: May be removed if dynamic review logic is relocated or rewritten
• View logic for metareviews will be removed
• Ensure views follow Rails i18n localization conventions by replacing any hardcoded strings with translation keys where appropriate.

Test Coverage Summary

To ensure the integrity of the reimplementation, both the service objects and controller actions should have dedicated tests. It is recommended to generate test skeletons as soon as the methods are in place.

RSpec Tests Report

File Name	Test Coverage
app/controllers/api/v1/student_review_controller.rb	84.62%
app/controllers/application_controller.rb	100.00%
app/services/student_review_service.rb	84.48%

All checks have passed

Summary of Tests in StudentReviewController Spec

The test file provides comprehensive test coverage for the Api::V1::StudentReviewController. Here's a summary of what was tested:

Core Controller Functionality

• Controller Action Existence:

Verifies that the list action is defined.

• Response Structure:

Confirms the controller returns the expected JSON structure with all required fields.

• Authorization Logic:

Ensures that proper authorization checks are in place and function correctly.

Authorization Tests

• Proper Authorization:

Tests scenarios where a user is authorized to access resources.

• Unauthorized Access:

Ensures unauthorized users receive a 401 response.

• Student Privileges:

Tests the action_allowed? method with and without student privileges.

• Participant Authorization:

Verifies the authorized_participant? method behavior.

Bidding Functionality

• Bidding Enabled:

Verifies redirection occurs when bidding is enabled.

• Bidding Disabled:

Ensures no redirection occurs when bidding is disabled.

• check_bidding_redirect Method:

Tests the method independently to verify redirection logic.

Service Integration

• Service Loading:

Tests for proper initialization of the load_service method.

• Service Data Usage:

Confirms service output is correctly used in controller responses.

Reviewer-related Tests

• With Reviewer:

Tests the flow when a participant has an associated reviewer.

• Without Reviewer:

Tests behavior when no reviewer is present.

Special Assignment Types

• Calibrated Assignments:

Tests functionality related to calibration logic.

• Topic ID Handling:

Verifies accurate calculation of topic_id.

Internationalization

• Locale Setting:

Tests controller_locale method functionality.

• User Locale Preference:

Tests locale assignment when user has preferences.

• Default Locale Fallback:

Ensures fallback to default locale works properly.

Edge Cases

• Missing Reviewer:

Verifies behavior when the reviewer does not exist.

• Review Mappings Sorting:

Tests sorting logic for review mappings under various conditions.

This comprehensive test suite ensures that all aspects of the StudentReviewController are thoroughly tested—from basic functionality to edge cases and error handling—achieving a test coverage rate of 86.67%.

Summary of Tests in ApplicationController Spec

The application_controller_spec.rb file provides thorough test coverage for the internationalization (i18n) functionality in the ApplicationController. Here's a summary of what was tested:

Locale Setting Logic

• Set Locale Method:

Tests the set_locale method that configures the application's current locale.

• Invalid Locale Handling:

Verifies that invalid locales passed in parameters are properly ignored.

• Default Locale Fallback:

Ensures fallback to the default locale when no valid locale is found.

Locale Extraction

• Extract Locale Method:

Tests the extract_locale method for determining the locale to use.

• Parameter-based Extraction:

Verifies that the locale is correctly extracted from URL parameters.

• HTTP Header Extraction:

Tests extraction of locale from the Accept-Language HTTP header.

• Validation:

Confirms extracted locales are checked against I18n.available_locales.

HTTP Accept-Language Header Parsing

• Country-Specific Codes:

Tests proper handling of country-specific language codes such as en-US.

• Quality Factors:

Tests parsing of language quality preferences (e.g., fr;q=0.9).

• Multiple Languages:

Verifies behavior when the header includes multiple language options.

• Invalid Languages:

Ensures that unsupported languages are safely ignored.

• Empty/Nil Headers:

Tests how the system behaves when the language header is empty or missing.

Edge Cases

• Multi-part Headers:

Tests parsing of complex Accept-Language headers.

• Priority Handling:

Ensures that language preferences are honored based on quality factors.

• Fallback Behavior:

Verifies fallback logic when preferred languages are not available.

These tests use RSpec’s controller testing features with an anonymous controller inheriting from ApplicationController to isolate i18n behavior. Setup and teardown logic ensures that the original i18n configuration is restored after each test.

Overall, this test suite ensures robust support for multilingual scenarios, helping maintain internationalization integrity throughout the application.

Summary of Tests in StudentReviewService Spec

The student_review_service_spec.rb file provides comprehensive test coverage for the StudentReviewService class. Here's a breakdown of what was tested:

Initialization and Setup

• Constructor Testing:

Verifies proper initialization, including loading of the participant and assignment.

• Topic ID and Review Phase:

Ensures that topic_id and review_phase are set correctly.

• Error Handling:

Tests that appropriate exceptions are raised when the participant does not exist.

Core Functionality

• Bidding Enabled Check:

Tests the bidding_enabled? method under both enabled and disabled conditions.

• Review Mappings:

Verifies correct loading and sorting of review mappings.

• Response IDs:

Confirms that response IDs are correctly extracted from the database.

Special Assignment Cases

• Calibrated Assignments:

Tests sorting logic specific to calibrated assignments: Uses id % 5 criteria. Covers specific IDs (1, 2, 5, 6, 7) to ensure sorting correctness.

Edge Cases

• No Reviewer:

Ensures review_mappings is set to an empty array. Verifies that progress counters for reviews are zero.

Response Handling

• Response ID Loading:

Confirms response IDs are correctly retrieved from SampleReview.

• Empty Response Lists:

Tests proper handling when no responses are available.

Testing Approach

This test suite uses RSpec's mocking and stubbing features to isolate logic from external dependencies:

• Uses doubles for models like User, Assignment, and Participant.
• Stubs private methods when needed to focus tests on expected behavior.
• Mocks SampleReview for controlled data input.
• Uses RSpec's expectation syntax to assert correctness.

Overall, this test suite thoroughly validates the StudentReviewService class under both standard and edge-case scenarios, ensuring correct functionality for calibrated assignments, bidding, review progress, and reviewer absence.

Team

Mentor

• Janice Uwujaren

Students

• Kenil Patel (kpatel47)
• Smit Patel (spatel68)
• Katerina Vilkomir (evilkom)

Relevant Links

• Controller (Original Expertiza Repo):

 https://github.com/expertiza/expertiza/blob/main/app/controllers/student_review_controller.rb

• Views (one controller action view and three partials):

 https://github.com/expertiza/expertiza/tree/main/app/views/student_review

• StudentReviewService Implementation Repo (Kii4ka):

 https://github.com/Kii4ka/reimplementation-back-end

• Pull Request to Expertiza Reimplementation Repo (#197):

 https://github.com/expertiza/reimplementation-back-end/pull/197