CSC/ECE 517 Fall 2024 - E2477. Reimplement suggestion controller.rb (Design Document)
Jump to navigation
Jump to search
Problem Statement
The suggestion_controller.rb in Expertiza is responsible for managing all operations related to submitting and approving suggestions for new project topics proposed by students or teams. However, this controller currently violates the Single Responsibility Principle by directly interacting with members of other Model classes. To enhance maintainability and clarity, this functionality should be appropriately distributed among dedicated classes. Along with this, it needs to be updated to follow the new back end, which is being implemented using json and api paths.
Design Goal
- Enhance Documentation: Add comprehensive comments throughout the controller, clearly explaining the purpose and functionality of custom methods
- Reimplement Notification Method: Simplify the notification method by refining its control logic and reducing complexity for better readability and efficiency. Additionally, "notification" is a poor name; it should be renamed to a verb that clearly indicates the action being performed
- Clarify Method Names:
- Rename reject_suggestion to reject for clarity
- Rename update_suggestion to update to better reflect its functionality
- Rename approve_suggestion to approve for clarity
- Refactor Email Sending Logic: Move the send_email method to the Mailer class located in app/mailers/mailer.rb to separate concerns and streamline the code
- Address DRY Violations: In views/suggestion/show.html.erb and views/suggestion/student_view.html.erb, identify and resolve the DRY (Don't Repeat Yourself) violation by merging these two files into a single view to enhance maintainability
- Update Tests: Revise existing tests to ensure they pass after the changes. Additionally, improve test coverage for the controller by adding new tests to verify the updated functionality
- During the reimplementation, review the structure and functionality of existing suggestion-related classes for insights and best practices. Remember, this is a complete reimplementation, not a refactor
Since this is a reimplementation project, some functionalities may be altered. Ensure that all changes are properly tested and that existing tests are updated to reflect these modifications.
Solutions/Details of Changes Made
Enhance Documentation
The controller is now fully commented, and this wiki page goes more in-depth about the code, design, and testing.
Various other QoL changes are implemented, such as alphabetized hash parameters and collapsible source code views.
The RSpec file is still under construction.
Reimplement Notification Method
The original method is hard to follow due to its nested if-else blocks and performing of multiple tasks. Additionally, the related functionality is split among all methods that are part of the approval process, making the final outcome difficult to follow.
The entire approval process has been reimplemented to create logical segmentation and easy to follow and understand methods.
- 'approve_suggestion' is now 'approve', and performs the four high-level steps required: (skip steps 3 and 4 if automatic sign up is turned off)
- Mark the suggestion as approved
- Create a new topic from the suggestion
- Sign the suggester's team up (create new team if necessary)
- Send the topic approved message
- 'create_topic_from_suggestion!' is simply a wrapper method which creates a new record of the SignUpTopic model. It exists to keep the overarching 'approve' method short
- 'sign_team_up!' performs the necessary steps to signing the student and his team up for the newly created topic. It creates a new team if necessary and de-registers any waitlisted assignments that the team might have
- 'send_notice_of_approval!' sends out the notification email informing all team members that their topic has been approved
Clarify Method Names
Each method has been examined for relevance and conciseness. Most methods are renamed to standard rails controller methods, or now exhibit rails naming convention. These would be:
| Action | Method name | Comment | |
|---|---|---|---|
| no change | action_allowed? | Updated checks | |
| add_comment | compacted logic | ||
| approve | assimilated approve_suggestion | ||
| create | compacted logic | ||
| show | assimilated student_view | ||
| added | create_topic_from_suggestion! | chain of helper methods distilled into single call to create | |
| destroy | it should be possible to delete a suggestion (D in CRUD) | ||
| send_notice_of_rejection | complementary message to approval message | ||
| removed | approve_suggestion | merged into approve | |
| new | view methods don't apply to JSON APIs | ||
| student_edit | editing a suggestion is not allowed | ||
| student_view | merged into show | ||
| submit | call-this-or-that method | ||
| suggestion_params | inlined into before_action declaration | ||
| renamed | Old | New | |
| list | index | standard Rails controller method naming | |
| notification | sign_team_up! | method name and implementation didn't match | |
| reject_suggestion | reject | standard Rails controller method naming | |
| send_email | send_notice_of_approval | method names should be specific | |
| update_suggestion | update | standard Rails controller method naming | |
Refactor Email Sending Logic
The original implementation first constructs a list of recipients iteratively. This adds to the length of the method, but also causes and increased number of database queries, slowing the program down.
In the reimplemented version, the code that finds the recipients' emails has been compacted into a single query chain, and thus inlined into the call to the Mailer class. The act of sending the email is moved into the Mailer helper class to separate concerns and streamline the code. Additionally, the suggestions controller now notifies the suggester if his suggestion has been rejected instead of just silently updating the state.
Address DRY Violations
This task specifically targeted the view files. Since this project is to reimplement the controller as an API which works with JSON only, there are no view files and thus this objective does not require any action. It is perhaps a holdover from before the decision to split Expertiza into frontend and backend.
Even so, DRY was observed throughout the project by extracting common statements into helper functions, most notably the privilege check methods.
Update Tests
The testing procedure is explained below in the Test Plan section.
Class Diagram
Tables
The following tables are newly added to the database schema. The question mark at the end of foreign_key indicates that this field is nullable.
Model associations
While simple at first glance, the suggestions controller interacts with many models in the system.
The models in the first row of the diagram are the direct subject material of the controller as most API endpoints perform CRUD operations on only these two. The grayed out models in the second row are only ever read by the controller, they can be considered static throughout any call to the controller. The remaining four models in the last row and column of the diagram are handled by the suggestion approval process. The controller may perform any of the CRUD operations on these models.
The arrows in the diagram point in the direction of ownership, that is, an arrow points from models A to B if A belongs to B. The dashed arrow labeled 'optional' in the legend means that the foreign key is nullable.
Controller and collaborators
The following diagram shows the controller, its API endpoints, its collaborators, and the methods accessed from the controller. The AuthorizationHelper module is imported into the controller class.
JSON API
The backend reimplementation project aims to convert Expertiza into a split frontend-backend service. To this end, the backend will respond with JSON only.
Request Format
In addition to the authorization headers, a combination of seven fields may be required for the specific API endpoint. They are:
| Parameter | Included in API Endpoint | Type | Description | |||||||
|---|---|---|---|---|---|---|---|---|---|---|
| add_comment | approve | create | destroy | index | reject | show | update | |||
| id | ✓ | ✓ | ✕ | ✓ | ✓* | ✓ | ✓ | ✓ | integer | Suggestion ID |
| anonymous | ✕ | ✕ | ✓ | ✕ | ✕ | ✕ | ✕ | ✕ | boolean | Whether to nullify user ID of suggester |
| assignment_id | ✕ | ✕ | ✓ | ✕ | ✕ | ✕ | ✕ | ✕ | integer | Assignment ID |
| auto_signup | ✕ | ✕ | ✓ | ✕ | ✕ | ✕ | ✕ | ~ | boolean | Whether to sign up team when approved |
| comment | ✓ | ✕ | ✕ | ✕ | ✕ | ✕ | ✕ | ✕ | string | SuggestionComment content |
| description | ✕ | ✕ | ✓ | ✕ | ✕ | ✕ | ✕ | ~ | string | Suggestion description |
| title | ✕ | ✕ | ✓ | ✕ | ✕ | ✕ | ✕ | ~ | string | Suggestion title |
- *: The id parameter specifies the Assignment ID instead
- ✓: Field required
- ✕: Field ignored
- ~: Field optional
For example, the request data for the add_comment method would be { "id": 1, "comment": "This is a comment on one of the suggestions" }
Status Codes
Depending on the parameters and database records, different HTTP status codes may be returned. They are:
| Code | Mnemonic | Condition | API Endpoints | |
|---|---|---|---|---|
| 2xx | success | |||
| 200 | ok | Request successful | approve index reject show update (cRUd) | |
| 201 | created | Comment successfully created SuggestionComment successfully created |
add_comment create (Crud) | |
| 204 | no content | Suggestion successfully deleted | delete (cruD) | |
| 4xx | client error | |||
| 403 | forbidden | Privilege check fails | add_comment approve destroy index reject show update | |
| 404 | not found | ActiveRecord::RecordNotFound exception | add_comment approve destroy reject show update | |
| 422 | unprocessable entity | ActionController::ParameterMissing ActiveRecord::RecordInvalid exception ActiveRecord::RecordNotDestroyed exception Suggestion already approved when trying to reject |
add_comment approve create destroy index reject show update | |
Response Format
Depending on the API endpoint of the request, a different entity may be returned. Note that only a success (2xx) will return an entity, a client error (4xx) will return an error message instead. The returned entities are:
| API Endpoint | Rendered Model |
|---|---|
| add_comment | SuggestionComment |
| approve | Suggestion |
| create | Suggestion |
| destroy | { } |
| index | Array of Suggestions |
| reject | Suggestion |
| show | Suggestion |
| update | Suggestion |
Files Added/Modified
Each of the boxes with file names are expandable and contain the contents of the file. If applicable, a before-after view is provided.
Models:
suggestion.rb (Modified)
| Old | New |
|---|---|
class Suggestion < ApplicationRecord
validates :title, :description, presence: true
has_many :suggestion_comments
end
|
class Suggestion < ApplicationRecord
belongs_to :assignment
belongs_to :user
has_many :suggestion_comments, dependent: :delete_all
validates :title, uniqueness: { case_sensitive: false }
validates :description, presence: true
end
|
suggestion_comment.rb (Modified)
| Old | New |
|---|---|
class SuggestionComment < ApplicationRecord
validates :comments, presence: true
belongs_to :suggestion
end
|
class SuggestionComment < ApplicationRecord
belongs_to :suggestion
belongs_to :user
validates :comment, presence: true
end
|
20241029234902_create_suggestions.rb (Added)
class CreateSuggestions < ActiveRecord::Migration[7.0]
def change
create_table :suggestions do |t|
t.string :title
t.text :description
t.string :status
t.boolean :auto_signup
t.references :assignment, null: false, foreign_key: true
t.references :user, foreign_key: true
t.timestamps
end
end
end
20241029235713_create_suggestion_comments.rb (Added)
class CreateSuggestionComments < ActiveRecord::Migration[7.0]
def change
create_table :suggestion_comments do |t|
t.text :comment
t.references :suggestion, null: false, foreign_key: true
t.references :user, null: false, foreign_key: true
t.timestamps
end
end
end
The models and accompanying migrations form the basis of the reimplementation. The model files were updated to improve their attribute validation and fix some bugs with the model associations. The 'comments' field was removed from the 'Suggestion' model since the 'SuggestionComment' model exists. As a result of the migrations, 'schema.rb' is changed, but excluded from the above list since it is an auto-generated file.
Controller:
routes.rb (Appended)
resources :suggestions do
collection do
post :add_comment
post :approve
post :reject
end
end
suggestions_controller.rb (Reimplemented)
| Old | New |
|---|---|
class SuggestionController < ApplicationController
include AuthorizationHelper
def action_allowed?
case params[:action]
when 'create', 'new', 'student_view', 'student_edit', 'update_suggestion', 'submit'
current_user_has_student_privileges?
else
current_user_has_ta_privileges?
end
end
def add_comment
@suggestion_comment = SuggestionComment.new(vote: params[:suggestion_comment][:vote], comments: params[:suggestion_comment][:comments])
@suggestion_comment.suggestion_id = params[:id]
@suggestion_comment.commenter = session[:user].name
if @suggestion_comment.save
flash[:notice] = 'Your comment has been successfully added.'
else
flash[:error] = 'There was an error in adding your comment.'
end
if current_user_has_student_privileges?
redirect_to action: 'student_view', id: params[:id]
else
redirect_to action: 'show', id: params[:id]
end
end
# GETs should be safe (see http://www.w3.org/2001/tag/doc/whenToUseGet.html)
verify method: :post, only: %i[destroy create update],
redirect_to: { action: :list }
def list
@suggestions = Suggestion.where(assignment_id: params[:id])
@assignment = Assignment.find(params[:id])
end
def student_view
@suggestion = Suggestion.find(params[:id])
end
def student_edit
@suggestion = Suggestion.find(params[:id])
end
def show
@suggestion = Suggestion.find(params[:id])
end
def update_suggestion
Suggestion.find(params[:id]).update_attributes(title: params[:suggestion][:title],
description: params[:suggestion][:description],
signup_preference: params[:suggestion][:signup_preference])
redirect_to action: 'new', id: Suggestion.find(params[:id]).assignment_id
end
def new
@suggestion = Suggestion.new
session[:assignment_id] = params[:id]
@suggestions = Suggestion.where(unityID: session[:user].name, assignment_id: params[:id])
@assignment = Assignment.find(params[:id])
end
def create
@suggestion = Suggestion.new(suggestion_params)
@suggestion.assignment_id = session[:assignment_id]
@assignment = Assignment.find(session[:assignment_id])
@suggestion.status = 'Initiated'
@suggestion.unityID = if params[:suggestion_anonymous].nil?
session[:user].name
else
''
end
if @suggestion.save
flash[:success] = 'Thank you for your suggestion!' unless @suggestion.unityID.empty?
flash[:success] = 'You have submitted an anonymous suggestion. It will not show in the suggested topic table below.' if @suggestion.unityID.empty?
end
redirect_to action: 'new', id: @suggestion.assignment_id
end
def submit
if !params[:add_comment].nil?
add_comment
elsif !params[:approve_suggestion].nil?
approve_suggestion
elsif !params[:reject_suggestion].nil?
reject_suggestion
end
end
# If the user submits a suggestion and gets it approved -> Send email
# If user submits a suggestion anonymously and it gets approved -> DOES NOT get an email
def send_email
proposer = User.find_by(id: @user_id)
if proposer
teams_users = TeamsUser.where(team_id: @team_id)
cc_mail_list = []
teams_users.each do |teams_user|
cc_mail_list << User.find(teams_user.user_id).email if teams_user.user_id != proposer.id
end
Mailer.suggested_topic_approved_message(
to: proposer.email,
cc: cc_mail_list,
subject: "Suggested topic '#{@suggestion.title}' has been approved",
body: {
approved_topic_name: @suggestion.title,
proposer: proposer.name
}
).deliver_now!
end
end
def notification
if @suggestion.signup_preference == 'Y'
if @team_id.nil?
new_team = AssignmentTeam.create(name: 'Team_' + rand(10_000).to_s,
parent_id: @signuptopic.assignment_id, type: 'AssignmentTeam')
new_team.create_new_team(@user_id, @signuptopic)
else
if @topic_id.nil?
# clean waitlists
SignedUpTeam.where(team_id: @team_id, is_waitlisted: 1).destroy_all
SignedUpTeam.create(topic_id: @signuptopic.id, team_id: @team_id, is_waitlisted: 0)
else
@signuptopic.private_to = @user_id
@signuptopic.save
# if this team has topic, Expertiza will send an email (suggested_topic_approved_message) to this team
send_email
end
end
else
# if this team has topic, Expertiza will send an email (suggested_topic_approved_message) to this team
send_email
end
end
def approve_suggestion
approve
notification
redirect_to action: 'show', id: @suggestion
end
def reject_suggestion
@suggestion = Suggestion.find(params[:id])
if @suggestion.update_attribute('status', 'Rejected')
flash[:notice] = 'The suggestion has been successfully rejected.'
else
flash[:error] = 'An error occurred when rejecting the suggestion.'
end
redirect_to action: 'show', id: @suggestion
end
private
def suggestion_params
params.require(:suggestion).permit(:assignment_id, :title, :description,
:status, :unityID, :signup_preference)
end
def approve
@suggestion = Suggestion.find(params[:id])
@user_id = User.find_by(name: @suggestion.unityID).try(:id)
if @user_id
@team_id = TeamsUser.team_id(@suggestion.assignment_id, @user_id)
@topic_id = SignedUpTeam.topic_id(@suggestion.assignment_id, @user_id)
end
# After getting topic from user/team, get the suggestion
@signuptopic = SignUpTopic.new_topic_from_suggestion(@suggestion)
# Get success only if the signuptopic object was returned from its class
if @signuptopic != 'failed'
flash[:success] = 'The suggestion was successfully approved.'
else
flash[:error] = 'An error occurred when approving the suggestion.'
end
end
end
|
# https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2024_-_E2477._Reimplement_suggestion_controller.rb_(Design_Document)
class Api::V1::SuggestionsController < ApplicationController
include AuthorizationHelper
# Determine whether the current request is allowed
def action_allowed?
case params[:action]
when 'create'
# Create has no special permissions
return true
when 'show', 'update'
# When showing or updating a suggestion, a student that owns the suggestions passes
suggestion = Suggestion.find(params[:id])
return true if suggestion.user_id == @current_user.id
end
# A TA or above always passes
# TODO: Check if user is TA or above in the same course that the suggestion is
AuthorizationHelper.current_user_has_ta_privileges?
end
# Comment on a suggestion.
# A new SuggestionComment record is made.
def add_comment
params.require(%i[comment id])
Suggestion.find(params[:id])
render json: SuggestionComment.create!(
comment: params[:comment],
suggestion_id: params[:id],
user_id: @current_user.id
), status: :created # 201
rescue ActionController::ParameterMissing => e
render json: { error: "#{e.param} is missing" }, status: :unprocessable_entity # 422
rescue ActiveRecord::RecordNotFound => e
render json: e, status: :not_found # 404
rescue ActiveRecord::RecordInvalid => e
render json: e.record.errors, status: :unprocessable_entity # 422
end
# Approve a suggestion, even if it was previously rejected.
def approve
params.require(:id)
@suggestion = Suggestion.find(params[:id])
# Only go through the approval process if the suggestion isn't already approved.
unless @suggestion.status == 'Approved'
# Since this process updates multiple records at once, wrap them in
# a transaction so that either all of them succeed, or none of them.
Suggestion.transaction do
# The approval process is:
# 1. Mark the suggestion as approved
# 2. Create a new topic from the suggestion
# 3. Sign the suggester's team up (create new team if necessary)
# 4. Send the topic approved message
@suggestion.update_attribute('status', 'Approved')
create_topic_from_suggestion!
# If a suggestion is anonymous, the suggester is
# unknown and thus steps 3 and 4 can't be taken.
unless @suggestion.user_id.nil?
@suggester = User.find(@suggestion.user_id)
sign_team_up!
send_notice_of_approval
end
end
end
render json: @suggestion, status: :ok # 200
rescue ActionController::ParameterMissing => e
render json: { error: "#{e.param} is missing" }, status: :unprocessable_entity # 422
rescue ActiveRecord::RecordNotFound => e
render json: e, status: :not_found # 404
rescue ActiveRecord::RecordInvalid => e
if e.record
render json: e.record.errors, status: :unprocessable_entity # 422
else
render json: { error: 'Unable to approve suggestion due to an invalid record.' }, status: :unprocessable_entity # 422
end
end
# A new Suggestion record is made.
def create
params.require(%i[anonymous assignment_id auto_signup description title])
render json: Suggestion.create!(
assignment_id: params[:assignment_id],
auto_signup: params[:auto_signup],
description: params[:description],
status: 'Initialized',
title: params[:title],
# Anonymous suggestions are allowed by nulling user_id.
user_id: params[:anonymous] ? nil : @current_user.id
), status: :created # 201
rescue ActionController::ParameterMissing => e
render json: { error: "#{e.param} is missing" }, status: :unprocessable_entity # 422
rescue ActiveRecord::RecordInvalid => e
render json: e.record.errors, status: :unprocessable_entity # 422
end
# Delete a suggestion from the records.
def destroy
params.require(:id)
Suggestion.find(params[:id]).destroy!
render json: {}, status: :no_content # 204
rescue ActionController::ParameterMissing => e
render json: { error: "#{e.param} is missing" }, status: :unprocessable_entity # 422
rescue ActiveRecord::RecordNotFound => e
render json: e, status: :not_found # 404
rescue ActiveRecord::RecordNotDestroyed => e
render json: e, status: :unprocessable_entity # 422
end
# Get a list of all Suggestion records associated with a particular assignment.
def index
params.require(:id)
render json: Suggestion.where(assignment_id: params[:id]), status: :ok # 200
rescue ActionController::ParameterMissing => e
render json: { error: "#{e.param} is missing" }, status: :unprocessable_entity # 422
end
# Reject a suggestion unless it was already approved.
def reject
params.require(:id)
@suggestion = Suggestion.find(params[:id])
# Since the approval process makes changes to many records,
# rejecting a previously approved suggestion is not possible.
if @suggestion.status == 'Approved'
render json: { error: 'Suggestion has already been approved.' }, status: :unprocessable_entity # 422
return
elsif @suggestion.status == 'Initialized'
# The rejection process is:
# 1. Mark the suggestion as rejected
# 2. Send the topic rejected message
@suggestion.update_attribute('status', 'Rejected')
send_notice_of_rejection if @suggestion.user_id
end
render json: @suggestion, status: :ok # 200
rescue ActionController::ParameterMissing => e
render json: { error: "#{e.param} is missing" }, status: :unprocessable_entity # 422
rescue ActiveRecord::RecordNotFound => e
render json: e, status: :not_found # 404
end
# Get a single Suggestion record.
def show
params.require(:id)
@suggestion = Suggestion.find(params[:id])
render json: {
comments: SuggestionComment.where(suggestion_id: params[:id]),
suggestion: @suggestion
}, status: :ok # 200
rescue ActionController::ParameterMissing => e
render json: { error: "#{e.param} is missing" }, status: :unprocessable_entity # 422
rescue ActiveRecord::RecordNotFound => e
render json: e, status: :not_found # 404
end
# Change the details of a Suggestion.
def update
params.require(:id)
@suggestion = Suggestion.find(params[:id])
# Only title, description, and signup preference can be changed.
@suggestion.update!(params.permit(:title, :description, :auto_signup))
render json: @suggestion, status: :ok # 200
rescue ActionController::ParameterMissing => e
render json: { error: "#{e.param} is missing" }, status: :unprocessable_entity # 422
rescue ActiveRecord::RecordNotFound => e
render json: e, status: :not_found # 404
rescue ActiveRecord::RecordInvalid => e
render json: e.record.errors, status: :unprocessable_entity # 422
end
private
def create_topic_from_suggestion!
# Convert a suggestion into a fully fledged topic.
@signuptopic = SignUpTopic.create!(
assignment_id: @suggestion.assignment_id,
max_choosers: 1,
topic_identifier: "S#{Suggestion.where(assignment_id: @suggestion.assignment_id).count}",
topic_name: @suggestion.title
)
end
def send_notice_of_approval
# Email the suggester and CC the suggester's teammates that the suggestion was approved
Mailer.send_topic_approved_email(
cc: User.joins(:teams_users).where(teams_users: { team_id: @team.id }).where.not(id: @suggester.id).map(&:email),
subject: "Suggested topic '#{@suggestion.title}' has been approved",
suggester: @suggester,
topic_name: @suggestion.title
)
end
def send_notice_of_rejection
# Email the suggester that the suggestion was rejected
Mailer.send_topic_rejected_email(
subject: "Suggested topic '#{@suggestion.title}' has been rejected",
suggester: User.find(@suggestion.user_id),
topic_name: @suggestion.title
)
end
def sign_team_up!
return unless @suggestion.auto_signup == true
# Find the suggester's team which is signed up to the topic's assignment.
@team = Team.where(assignment_id: @signuptopic.assignment_id).joins(:teams_users)
.where(teams_users: { user_id: @suggester.id }).first
# Create the team if necessary.
if @team.nil?
@team = Team.create!(assignment_id: @signuptopic.assignment_id)
TeamsUser.create!(team_id: @team.id, user_id: @suggester.id)
end
# If the suggester's team has no assignment, then clear its
# waitlist and sign it up to the newly created topic.
unless SignedUpTeam.exists?(team_id: @team.id, is_waitlisted: false)
SignedUpTeam.where(team_id: @team.id, is_waitlisted: true).destroy_all
SignedUpTeam.create!(sign_up_topic_id: @signuptopic.id, team_id: @team.id, is_waitlisted: false)
end
# Since the suggester's team is signed up to the topic, make it private
# to the suggester so no other teams will attempt to sign up to it.
@signuptopic.update_attribute(:private_to, @suggester.id)
end
end
|
The controller defines the behavior around suggestions and suggestion comments. It has been completely reimplemented to follow API convention and streamline the various endpoints that the frontend will interact with. Notable changes are:
- Responses are in JSON instead of HTML. The JSON API is described in JSON API
- Switched from check-avoid to fail-recover coding style to minimize number of if statements and thus indentation.
Each method was given a 'rescue' clause for any error that might occur, and most ActiveRecords methods were switched from the ones that return a boolean to the ones that throw an exception.
As a result, the methods become easier to understand, as the intended behavior is programmed all in one block with each error handled all the way at the end of the method - Public method names have been changed to standard Rails controller methods or method naming conventions
- Private method names describe their public side effects and end in an exclamation point if the method modifies the database
- 'approve', 'notification', and 'send_email' are re-segmented into 'create_topic_from_suggestion!', 'send_notice_of_approval!', and 'sign_team_up!' methods to improve logical code segmentation
- 'submit' method removed entirely as it is a "do-this-or-that" method; instead, the sub-actions are called directly
- The method that sends the email is simplified to a single call to the Mailer class and now uses a Rails query chain to reduce the number of database queries.
Additionally, a complementary email method for rejections has been addded - Where required, API entry points call dedicated privilege check methods
Helpers:
mailer.rb (Added)
class Mailer < ActionMailer::Base
default from: 'expertiza.mailer@gmail.com'
def send_topic_approved_message(defn)
@suggester = defn[:suggester]
@topic_name = defn[:topic_name]
mail(to: @suggester.email, cc: defn[:cc], subject: defn[:subject]).deliver_now!
end
def send_topic_rejected_email(defn)
@suggester = defn[:suggester]
@topic_name = defn[:topic_name]
mail(to: @suggester.email, subject: defn[:subject]).deliver_now!
end
end
authorization_helper.rb (Trimmed)
| Old (authorization_helper.rb) | New (authorization_helper.rb) |
|---|---|
module AuthorizationHelper
# Notes:
# We use session directly instead of current_role_name and the like
# Because helpers do not seem to have access to the methods defined in app/controllers/application_controller.rb
# PUBLIC METHODS
# Determine if the currently logged-in user has the privileges of a Super-Admin
def current_user_has_super_admin_privileges?
current_user_has_privileges_of?('Super-Administrator')
end
# Determine if the currently logged-in user has the privileges of an Admin (or higher)
def current_user_has_admin_privileges?
current_user_has_privileges_of?('Administrator')
end
# Determine if the currently logged-in user has the privileges of an Instructor (or higher)
def current_user_has_instructor_privileges?
current_user_has_privileges_of?('Instructor')
end
# Determine if the currently logged-in user has the privileges of a TA (or higher)
def current_user_has_ta_privileges?
current_user_has_privileges_of?('Teaching Assistant')
end
# Determine if the currently logged-in user has the privileges of a Student (or higher)
def current_user_has_student_privileges?
current_user_has_privileges_of?('Student')
end
# ...
# Determine if the currently logged-in user has the privileges of the given role name (or higher privileges)
# Let the Role model define this logic for the sake of DRY
# If there is no currently logged-in user simply return false
def current_user_has_privileges_of?(role_name)
current_user_and_role_exist? && session[:user].role.has_all_privileges_of?(Role.find_by(name: role_name))
end
# ...
def current_user_and_role_exist?
user_logged_in? && !session[:user].role.nil?
end
end
|
module AuthorizationHelper
# Determine if the currently logged-in user has the privileges of a Super-Admin
def self.current_user_has_super_admin_privileges?
current_user_has_privileges_of?('Super-Administrator')
end
# Determine if the currently logged-in user has the privileges of an Admin (or higher)
def self.current_user_has_admin_privileges?
current_user_has_privileges_of?('Administrator')
end
# Determine if the currently logged-in user has the privileges of an Instructor (or higher)
def self.current_user_has_instructor_privileges?
current_user_has_privileges_of?('Instructor')
end
# Determine if the currently logged-in user has the privileges of a TA (or higher)
def self.current_user_has_ta_privileges?
current_user_has_privileges_of?('Teaching Assistant')
end
# Determine if the currently logged-in user has the privileges of a Student (or higher)
def self.current_user_has_student_privileges?
current_user_has_privileges_of?('Student')
end
# Determine if the currently logged-in user has the privileges of the given role name (or higher privileges)
# Let the Role model define this logic for the sake of DRY
# If there is no currently logged-in user simply return false
def self.current_user_has_privileges_of?(role_name)
current_user_and_role_exist? && @current_user.role.all_privileges_of?(Role.find_by(name: role_name))
end
def self.current_user_and_role_exist?
!@current_user.nil? && !@current_user.role.nil?
end
end
|
send_topic_approved_email.html.erb (Added)
<!DOCTYPE html>
<html>
<head>
<meta content='text/html; charset=UTF-8' http-equiv='Content-Type' />
</head>
<body>
Hi,<br><br>
The topic <b><%= @topic_name %></b> suggested by your teammate <b><%= @suggester %></b> has just been approved.<br>
If you want to work on this topic, you can log into Expertiza and switch topics.<br><br>
Thanks
<hr>
This message has been generated by <a href="http://expertiza.ncsu.edu">Expertiza</a><br>
</body>
</html>
send_topic_rejected_email.html.erb (Added)
<!DOCTYPE html>
<html>
<head>
<meta content='text/html; charset=UTF-8' http-equiv='Content-Type' />
</head>
<body>
Hi,<br><br>
The topic <b><%= @topic_name %></b> suggested by your teammate <b><%= @suggester %></b> has just been rejected.<br>
If you believe this to be an error, please contact your instructor or one of the TAs.<br><br>
Thanks
<hr>
This message has been generated by <a href="http://expertiza.ncsu.edu">Expertiza</a><br>
</body>
</html>
The helpers exist for the single responsibility principle, and to allow common features to be defined in one place and usable everywhere. The two helpers that are implemented are:
- The mailer helper, which is responsible for sending emails
- The authorization helper, which helps determine the permissions level of the current user
The mailer helper is accompanied by its template files for each type of message sent, in this case the topic approved and topic rejected messages. They also exist as plain text template files, since the original repo had both html and plaintext versions of the email templates. The plaintext versions of the template files are not shown as their content is the same, just stripped of the HTML tags.
Controller spec:
The controller spec file is part of the test plan and explained in the following section:
Test Plan
Update suggestions_controller test file, to test the changed file, follow the change to api format, and use RSpec testing.
Detailed Test Plan
1. Method: show
* Only shows when user is authorized to view the specified suggestion * Does not show when user is unauthorized
suggestions_controller_spec.rb (show test)
path '/api/v1/suggestions/{id}' do
get 'show suggestion' do
tags 'Suggestions'
consumes 'application/json'
parameter name: 'Authorization', in: :header, type: :string, required: true
parameter name: :id, in: :path, type: :integer, required: true, description: 'ID of the suggestion'
# tests for when user is a instructor/has ta privileges
context '| when user is instructor | ' do
# set user to have ta privileges
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(true)
end
response '200', 'suggestion details and comments returned' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
data = JSON.parse(response.body)
expect(data['suggestion']['id']).to eq(suggestion.id)
expect(data['comments']).to be_an(Array)
expect(response.status).to eq(200)
end
end
end
# test cases for when user is a student/doesn't have ta privilges
context ' | when user is student | ' do
# set user to not have ta privileges and make sure current user is set properly
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(false)
allow(controller).to receive(:current_user).and_return(@current_user)
end
# test cases for when user is owner of suggestion
context ' | when user is student owner to suggestion | ' do
# make sure user is set as owner of suggestion
before(:each) do
# Simulate the student owning the suggestion
suggestion.update!(user_id: @user.id)
end
# test for student being able to view their own suggestion
response '200', 'student can view their own suggestion' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
data = JSON.parse(response.body)
expect(data['suggestion']['id']).to eq(suggestion.id)
expect(data['comments']).to be_an(Array)
expect(response.status).to eq(200)
end
end
end
# test case for when user is not owner of suggestion
context ' | when user is not student owner to suggestion | ' do
before(:each) do
# Simulate the student not owning the suggestion
suggestion_double = double('Suggestion', id: suggestion.id, user_id: 9999)
allow(Suggestion).to receive(:find).with(suggestion.id.to_s).and_return(suggestion_double)
end
# make sure student can't view suggestions they don't own/are a part of
response '403', 'student can\'t view other suggestions' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
expect(response.status).to eq(403)
end
end
end
end
# test for when suggestion doesn't exist
response '404', 'suggestion not found' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { -1 }
run_test! do |response|
expect(response.status).to eq(404)
end
end
end
end
2. Method: add_comment
* adds a comment and then returns showing said comment * returns an error when comment creation fails
suggestions_controller_spec.rb (add comment test)
path '/api/v1/suggestions/{id}/add_comment' do
post 'Add a comment to a suggestion' do
tags 'Suggestions'
consumes 'application/json'
parameter name: 'Authorization', in: :header, type: :string, required: true
parameter name: :id, in: :path, type: :integer, required: true, description: 'ID of the suggestion'
parameter name: :comment, in: :body, schema: {
type: :object,
properties: {
comment: { type: :string }
},
required: ['comment']
}
# successful comment addition test
response '201', 'comment_added' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
let(:comment) { { comment: 'This is a test comment' } }
run_test! do |response|
data = JSON.parse(response.body)
expect(data['comment']).to eq('This is a test comment')
expect(response.status).to eq(201)
end
end
# test for missing or empty comment
response '422', 'unprocessable entity for missing or empty comment' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
let(:comment) { '' }
before do
# Mock the params to simulate the request
allow_any_instance_of(ActionController::Parameters).to receive(:require).with(:id).and_return(id)
allow_any_instance_of(ActionController::Parameters).to receive(:require).with(:comment).and_return(comment)
# Mock params[:id] and params[:comment] in the controller context
allow_any_instance_of(ActionController::Parameters).to receive(:[]).with(:id).and_return(id)
allow_any_instance_of(ActionController::Parameters).to receive(:[]).with(:comment).and_return(comment)
end
run_test! do |response|
expect(response.status).to eq(422) # Expect 400 if the comment is missing or empty
end
end
# test for suggestion not found
response '404', 'suggestion not found' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { -1 }
let(:comment) { { comment: 'Invalid ID' } }
run_test! do |response|
expect(response.status).to eq(404)
end
end
end
end
3. Method: approve
* approves the suggestion and shows updated suggestion if user is authorized to do so * returns a forbidden error when user is unauthorized to approve suggestions
suggestions_controller_spec.rb (approve test)
path '/api/v1/suggestions/{id}/approve' do
post 'Approve suggestion' do
tags 'Suggestions'
consumes 'application/json'
parameter name: 'Authorization', in: :header, type: :string, required: true
parameter name: :id, in: :path, type: :integer, required: true, description: 'ID of the suggestion'
# tests for when the user is an instructor/has ta privileges
context '| when user is instructor | ' do
# set user to have ta privileges
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(true)
end
# successful suggestion approval
response '200', 'suggestion approved' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
data = JSON.parse(response.body)
expect(data['status']).to eq('Approved')
end
end
# test for unprocessable with a record
response '422', 'unprocessable entity' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
before do
# Simulating an error in the approval process (e.g., ActiveRecord::RecordInvalid)
allow_any_instance_of(Suggestion).to receive(:update_attribute).and_raise(ActiveRecord::RecordInvalid.new(suggestion))
end
run_test! do |response|
expect(response.status).to eq(422)
end
end
# test for unprocessable without a record
response '422', 'unprocessable entity' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
before do
# Simulating an error in the approval process (e.g., ActiveRecord::RecordInvalid)
allow_any_instance_of(Suggestion).to receive(:update_attribute).and_raise(ActiveRecord::RecordInvalid)
end
run_test! do |response|
expect(response.status).to eq(422)
end
end
# test for when a suggestion is not found
response '404', 'suggestion not found' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { -1 }
run_test! do |response|
expect(response.status).to eq(404)
end
end
end
# test cases for when user is a student/doesn't have ta_privileges
context ' | when user is student | ' do
# set user as not having ta_privileges
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(false)
end
# test for students not being able to approve suggestions
response '403', 'students cannot approve suggestions' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
expect(response.status).to eq(403)
expect(JSON.parse(response.body)['error']).to eq('Students cannot approve a suggestion.')
end
end
end
end
end
4. Method: reject
* rejects the suggestion and returns to suggestion when user is authorized * returns a forbidden error when user is unauthorized to reject suggestions
suggestions_controller_spec.rb (reject test)
path '/api/v1/suggestions/{id}/reject' do
post 'Reject suggestion' do
tags 'Suggestions'
consumes 'application/json'
parameter name: 'Authorization', in: :header, type: :string, required: true
parameter name: :id, in: :path, type: :integer, required: true, description: 'ID of the suggestion'
# test cases for when user is instructor/ta privileges
context '| when user is instructor | ' do
# set user to have ta privileges
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(true)
end
# test for successful suggestion rejection
response '200', 'suggestion rejected' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
data = JSON.parse(response.body)
expect(data['status']).to eq('Rejected')
end
end
# test for suggestion already being approved
response '422', 'suggestion already approved' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
before do
# Simulate suggestion status as 'Approved'
suggestion.update!(status: 'Approved')
end
run_test! do |response|
expect(response.status).to eq(422)
parsed_response = JSON.parse(response.body)
expect(parsed_response['error']).to eq('Suggestion has already been approved.')
end
end
# test for when the suggestion not being found
response '404', 'suggestion not found' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { -1 }
run_test! do |response|
expect(response.status).to eq(404)
end
end
end
# test cases for when user is a student/doesn't have ta privileges
context ' | when user is student | ' do
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(false)
end
response '403', 'students cannot reject suggestions' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
expect(response.status).to eq(403)
expect(JSON.parse(response.body)['error']).to eq('Students cannot reject a suggestion.')
end
end
end
end
end
5. Method: edit
* edits suggestion and shows updated suggestion when authorized to edit the suggestion * returns forbidden error when user is authorized
suggestions_controller_spec.rb (edit/update test)
path '/api/v1/suggestions/{id}' do
patch 'update suggestion' do
tags 'Suggestions'
consumes 'application/json'
parameter name: 'Authorization', in: :header, type: :string, required: true
parameter name: :id, in: :path, type: :integer, required: true, description: 'ID of the suggestion'
# test cases for when user is an instructor/has ta privileges
context '| when user is instructor | ' do
# set user to have ta privileges
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(true)
end
# test for successful suggestion update
response '200', 'suggestion updated' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
expect(response.status).to eq(200)
end
end
end
# test cases for when user is a student
context ' | when user is student | ' do
# set user to be a student and setup current user
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(false)
allow(controller).to receive(:current_user).and_return(@current_user)
end
# test cases for when student is owner/a part of suggestion
context ' | when user is student owner to suggestion | ' do
before(:each) do
# Simulate the student owning the suggestion
suggestion.update!(user_id: @user.id)
end
# test to make sure students can update their own suggestion(s)
response '200', 'student can update their own suggestion' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
expect(response.status).to eq(200)
end
end
end
# test cases for when student is not owner to suggestion
context ' | when user is not student owner to suggestion | ' do
before(:each) do
# Simulate the student not owning the suggestion
suggestion_double = double('Suggestion', id: suggestion.id, user_id: 9999)
allow(Suggestion).to receive(:find).with(suggestion.id.to_s).and_return(suggestion_double)
end
# test for forbidding student(s) from updating suggestions other then their own
response '403', 'student can\'t updates other suggestions' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
expect(response.status).to eq(403)
end
end
end
end
# test for suggestion not being found
response '404', 'suggestion not found' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { -1 }
run_test! do |response|
expect(response.status).to eq(404)
end
end
# test for suggestion being invalid in some form/missing
response '422', 'unprocessable entity' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
before do
allow(Suggestion).to receive(:find).and_return(suggestion) # Mock finding the suggestion
allow(suggestion).to receive(:update!).and_raise(ActiveRecord::RecordInvalid.new(suggestion))
end
run_test! do |response|
expect(response.status).to eq(422)
end
end
end
end
6. Method: create
* creates suggestion if given suggestion data is valid, otherwise return an error
suggestions_controller_spec.rb (create test)
path '/api/v1/suggestions' do
post 'Create suggestion' do
tags 'Suggestions'
consumes 'application/json'
parameter name: 'Authorization', in: :header, type: :string, required: true
parameter name: :suggestion, in: :body, type: :object, required: true,
description: 'Suggestion object with attributes'
# test for successful suggestion creation
response '201', 'suggestion created successfully' do
let(:Authorization) { "Bearer #{@token}" }
let(:suggestion) do
{
assignment_id: assignment.id,
auto_signup: true,
comment: 'This is a great suggestion!',
description: 'Detailed suggestion description',
title: 'Suggestion title',
anonymous: false
}
end
# set up current user to return properly for test
before do
allow(controller).to receive(:current_user).and_return(@current_user)
end
run_test! do |response|
expect(response.status).to eq(201)
data = JSON.parse(response.body)
expect(data['title']).to eq('Suggestion title')
expect(data['description']).to eq('Detailed suggestion description')
expect(data['auto_signup']).to eq(true)
expect(data['status']).to eq('Initialized')
end
end
# test for a missing parameter
response '422', 'missing title' do
let(:Authorization) { "Bearer #{@token}" }
let(:suggestion) do
{
assignment_id: assignment.id,
auto_signup: true,
comment: 'This is a great suggestion!',
description: 'Detailed suggestion description',
title: '',
anonymous: false
}
end
# set up current user to return current user properly
before do
allow(controller).to receive(:current_user).and_return(@current_user)
end
run_test! do |response|
expect(response.status).to eq(422)
data = JSON.parse(response.body)
expect(data['error']).to eq('title is missing')
end
end
# test for invalid suggestion given
response '422', 'unprocessable entity' do
let(:Authorization) { "Bearer #{@token}" }
let(:suggestion) do
{
assignment_id: assignment.id,
auto_signup: true,
comment: 'This is a great suggestion!',
description: 'Detailed suggestion description',
title: 'Sample Suggestion',
anonymous: false
}
end
before do
allow(controller).to receive(:current_user).and_return(@current_user)
suggestion = build(:suggestion, assignment_id: assignment.id, user_id: @user.id)
allow(Suggestion).to receive(:create!).and_raise(ActiveRecord::RecordInvalid.new(suggestion))
end
run_test! do |response|
expect(response.status).to eq(422)
end
end
end
end
7. Method: destroy
* deletes the suggestion if user has the permissions to delete suggestions
suggestions_controller_spec.rb (destroy test)
path '/api/v1/suggestions/{id}' do
delete 'Delete suggestion' do
tags 'Suggestions'
consumes 'application/json'
parameter name: 'Authorization', in: :header, type: :string, required: true
parameter name: :id, in: :path, type: :integer, required: true, description: 'ID of the suggestion'
# test cases for when the user is an instructor
context '| when user is instructor | ' do
# set user to have ta privileges
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(true)
end
# test for successful suggestion deletion
response '204', 'suggestion deleted' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
expect(response.status).to eq(204)
expect(response.body).to be_empty
end
end
# test for record not being destroyed
response '422', 'unprocessable entity' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
before do
# Simulating an error in the deletion process
allow_any_instance_of(Suggestion).to receive(:destroy!).and_raise(ActiveRecord::RecordNotDestroyed)
end
run_test! do |response|
expect(response.status).to eq(422)
end
end
# test for when suggestion is not found/doesn't exist
response '404', 'suggestion not found' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { -1 }
run_test! do |response|
expect(response.status).to eq(404)
end
end
end
# test cases for when the user is a student
context ' | when user is student | ' do
# set user to not have ta privileges
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(false)
end
# test to make sure students cannot delete suggestions
response '403', 'students cannot delete suggestions' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
expect(response.status).to eq(403)
expect(JSON.parse(response.body)['error']).to eq('Students cannot delete suggestions.')
end
end
end
end
end
8. Method: index
* show all suggestions if user has the privileges otherwise returns forbidden error
suggestions_controller_spec.rb (index test)
path '/api/v1/suggestions' do
get 'list all suggestions' do
tags 'Suggestions'
consumes 'application/json'
parameter name: 'Authorization', in: :header, type: :string, required: true
parameter name: :id, in: :query, type: :integer, required: true, description: 'ID of the assignment'
# test cases for when the user is an instructor/has ta privileges
context '| when user is instructor | ' do
# set user to have ta privileges
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(true)
suggestion.update(assignment_id: assignment.id)
end
# test for successful indexing
response '200', 'suggestions listed' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { assignment.id }
run_test! do |response|
expect(response.status).to eq(200)
expect(JSON.parse(response.body).size).to eq(1)
end
end
end
# test case for when user is a student
context ' | when user is student | ' do
# set user to not have ta privileges
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(false)
end
# test for student to be forbidden from indexing suggestions
response '403', 'students cannot index suggestions' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { assignment.id }
run_test! do |response|
expect(response.status).to eq(403)
expect(JSON.parse(response.body)['error']).to eq('Students cannot view all suggestions of an assignment.')
end
end
end
end
end
API Documentation and Testing with Swagger
Allows for quick and easy understanding and testing of API through interactive documentation.
Setup: Integrate Swagger with Rails application using gems like rswag.
Documentation: Annotate the testing code so that Swagger can automatically generate documentation. Make sure to describe each parameter and requirement, give details on how responses are structured and ensure that the endpoints are described nicely. The Swagger UI also allows for interactive testing, allowing for easy testing of the API even without a good understanding of the technical know-how on how to do so.
Updated Spec file
suggestions_controller_spec.rb (Modified)
require 'swagger_helper'
require 'rails_helper'
def login_user
# Create a user using the factory
user = create(:user)
# Make a POST request to login
post '/login', params: { user_name: user.name, password: 'password' }
# Parse the JSON response and extract the token
json_response = JSON.parse(response.body)
# Return the token from the response
{ token: json_response['token'], user: }
end
RSpec.describe 'Suggestions API', type: :request do
# Login user, grab token, set user and current user
before(:each) do
auth_data = login_user
@token = auth_data[:token]
@user = auth_data[:user]
@current_user = @user
end
# create default assignment and suggestion
let(:assignment) { create(:assignment) }
let(:suggestion) { create(:suggestion, assignment_id: assignment.id, user_id: @user.id) }
# Testing for add_comment method
path '/api/v1/suggestions/{id}/add_comment' do
post 'Add a comment to a suggestion' do
tags 'Suggestions'
consumes 'application/json'
parameter name: 'Authorization', in: :header, type: :string, required: true
parameter name: :id, in: :path, type: :integer, required: true, description: 'ID of the suggestion'
parameter name: :comment, in: :body, schema: {
type: :object,
properties: {
comment: { type: :string }
},
required: ['comment']
}
# successful comment addition test
response '201', 'comment_added' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
let(:comment) { { comment: 'This is a test comment' } }
run_test! do |response|
data = JSON.parse(response.body)
expect(data['comment']).to eq('This is a test comment')
expect(response.status).to eq(201)
end
end
# test for missing or empty comment
response '422', 'unprocessable entity for missing or empty comment' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
let(:comment) { '' }
before do
# Mock the params to simulate the request
allow_any_instance_of(ActionController::Parameters).to receive(:require).with(:id).and_return(id)
allow_any_instance_of(ActionController::Parameters).to receive(:require).with(:comment).and_return(comment)
# Mock params[:id] and params[:comment] in the controller context
allow_any_instance_of(ActionController::Parameters).to receive(:[]).with(:id).and_return(id)
allow_any_instance_of(ActionController::Parameters).to receive(:[]).with(:comment).and_return(comment)
end
run_test! do |response|
expect(response.status).to eq(422) # Expect 400 if the comment is missing or empty
end
end
# test for suggestion not found
response '404', 'suggestion not found' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { -1 }
let(:comment) { { comment: 'Invalid ID' } }
run_test! do |response|
expect(response.status).to eq(404)
end
end
end
end
# Tests for approving suggestions
path '/api/v1/suggestions/{id}/approve' do
post 'Approve suggestion' do
tags 'Suggestions'
consumes 'application/json'
parameter name: 'Authorization', in: :header, type: :string, required: true
parameter name: :id, in: :path, type: :integer, required: true, description: 'ID of the suggestion'
# tests for when the user is an instructor/has ta privileges
context '| when user is instructor | ' do
# set user to have ta privileges
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(true)
end
# successful suggestion approval
response '200', 'suggestion approved' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
data = JSON.parse(response.body)
expect(data['status']).to eq('Approved')
end
end
# test for unprocessable with a record
response '422', 'unprocessable entity' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
before do
# Simulating an error in the approval process (e.g., ActiveRecord::RecordInvalid)
allow_any_instance_of(Suggestion).to receive(:update_attribute).and_raise(ActiveRecord::RecordInvalid.new(suggestion))
end
run_test! do |response|
expect(response.status).to eq(422)
end
end
# test for unprocessable without a record
response '422', 'unprocessable entity' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
before do
# Simulating an error in the approval process (e.g., ActiveRecord::RecordInvalid)
allow_any_instance_of(Suggestion).to receive(:update_attribute).and_raise(ActiveRecord::RecordInvalid)
end
run_test! do |response|
expect(response.status).to eq(422)
end
end
# test for when a suggestion is not found
response '404', 'suggestion not found' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { -1 }
run_test! do |response|
expect(response.status).to eq(404)
end
end
end
# test cases for when user is a student/doesn't have ta_privileges
context ' | when user is student | ' do
# set user as not having ta_privileges
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(false)
end
# test for students not being able to approve suggestions
response '403', 'students cannot approve suggestions' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
expect(response.status).to eq(403)
expect(JSON.parse(response.body)['error']).to eq('Students cannot approve a suggestion.')
end
end
end
end
end
# test cases for creating a suggestion
path '/api/v1/suggestions' do
post 'Create suggestion' do
tags 'Suggestions'
consumes 'application/json'
parameter name: 'Authorization', in: :header, type: :string, required: true
parameter name: :suggestion, in: :body, type: :object, required: true,
description: 'Suggestion object with attributes'
# test for successful suggestion creation
response '201', 'suggestion created successfully' do
let(:Authorization) { "Bearer #{@token}" }
let(:suggestion) do
{
assignment_id: assignment.id,
auto_signup: true,
comment: 'This is a great suggestion!',
description: 'Detailed suggestion description',
title: 'Suggestion title',
anonymous: false
}
end
# set up current user to return properly for test
before do
allow(controller).to receive(:current_user).and_return(@current_user)
end
run_test! do |response|
expect(response.status).to eq(201)
data = JSON.parse(response.body)
expect(data['title']).to eq('Suggestion title')
expect(data['description']).to eq('Detailed suggestion description')
expect(data['auto_signup']).to eq(true)
expect(data['status']).to eq('Initialized')
end
end
# test for a missing parameter
response '422', 'missing title' do
let(:Authorization) { "Bearer #{@token}" }
let(:suggestion) do
{
assignment_id: assignment.id,
auto_signup: true,
comment: 'This is a great suggestion!',
description: 'Detailed suggestion description',
title: '',
anonymous: false
}
end
# set up current user to return current user properly
before do
allow(controller).to receive(:current_user).and_return(@current_user)
end
run_test! do |response|
expect(response.status).to eq(422)
data = JSON.parse(response.body)
expect(data['error']).to eq('title is missing')
end
end
# test for invalid suggestion given
response '422', 'unprocessable entity' do
let(:Authorization) { "Bearer #{@token}" }
let(:suggestion) do
{
assignment_id: assignment.id,
auto_signup: true,
comment: 'This is a great suggestion!',
description: 'Detailed suggestion description',
title: 'Sample Suggestion',
anonymous: false
}
end
before do
allow(controller).to receive(:current_user).and_return(@current_user)
suggestion = build(:suggestion, assignment_id: assignment.id, user_id: @user.id)
allow(Suggestion).to receive(:create!).and_raise(ActiveRecord::RecordInvalid.new(suggestion))
end
run_test! do |response|
expect(response.status).to eq(422)
end
end
end
end
# test cases for deleting suggestions
path '/api/v1/suggestions/{id}' do
delete 'Delete suggestion' do
tags 'Suggestions'
consumes 'application/json'
parameter name: 'Authorization', in: :header, type: :string, required: true
parameter name: :id, in: :path, type: :integer, required: true, description: 'ID of the suggestion'
# test cases for when the user is an instructor
context '| when user is instructor | ' do
# set user to have ta privileges
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(true)
end
# test for successful suggestion deletion
response '204', 'suggestion deleted' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
expect(response.status).to eq(204)
expect(response.body).to be_empty
end
end
# test for record not being destroyed
response '422', 'unprocessable entity' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
before do
# Simulating an error in the deletion process
allow_any_instance_of(Suggestion).to receive(:destroy!).and_raise(ActiveRecord::RecordNotDestroyed)
end
run_test! do |response|
expect(response.status).to eq(422)
end
end
# test for when suggestion is not found/doesn't exist
response '404', 'suggestion not found' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { -1 }
run_test! do |response|
expect(response.status).to eq(404)
end
end
end
# test cases for when the user is a student
context ' | when user is student | ' do
# set user to not have ta privileges
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(false)
end
# test to make sure students cannot delete suggestions
response '403', 'students cannot delete suggestions' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
expect(response.status).to eq(403)
expect(JSON.parse(response.body)['error']).to eq('Students cannot delete suggestions.')
end
end
end
end
end
# test cases for indexing/listing all suggestions
path '/api/v1/suggestions' do
get 'list all suggestions' do
tags 'Suggestions'
consumes 'application/json'
parameter name: 'Authorization', in: :header, type: :string, required: true
parameter name: :id, in: :query, type: :integer, required: true, description: 'ID of the assignment'
# test cases for when the user is an instructor/has ta privileges
context '| when user is instructor | ' do
# set user to have ta privileges
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(true)
suggestion.update(assignment_id: assignment.id)
end
# test for successful indexing
response '200', 'suggestions listed' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { assignment.id }
run_test! do |response|
expect(response.status).to eq(200)
expect(JSON.parse(response.body).size).to eq(1)
end
end
end
# test case for when user is a student
context ' | when user is student | ' do
# set user to not have ta privileges
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(false)
end
# test for student to be forbidden from indexing suggestions
response '403', 'students cannot index suggestions' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { assignment.id }
run_test! do |response|
expect(response.status).to eq(403)
expect(JSON.parse(response.body)['error']).to eq('Students cannot view all suggestions of an assignment.')
end
end
end
end
end
# test cases for rejecting suggestions
path '/api/v1/suggestions/{id}/reject' do
post 'Reject suggestion' do
tags 'Suggestions'
consumes 'application/json'
parameter name: 'Authorization', in: :header, type: :string, required: true
parameter name: :id, in: :path, type: :integer, required: true, description: 'ID of the suggestion'
# test cases for when user is instructor/ta privileges
context '| when user is instructor | ' do
# set user to have ta privileges
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(true)
end
# test for successful suggestion rejection
response '200', 'suggestion rejected' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
data = JSON.parse(response.body)
expect(data['status']).to eq('Rejected')
end
end
# test for suggestion already being approved
response '422', 'suggestion already approved' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
before do
# Simulate suggestion status as 'Approved'
suggestion.update!(status: 'Approved')
end
run_test! do |response|
expect(response.status).to eq(422)
parsed_response = JSON.parse(response.body)
expect(parsed_response['error']).to eq('Suggestion has already been approved.')
end
end
# test for when the suggestion not being found
response '404', 'suggestion not found' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { -1 }
run_test! do |response|
expect(response.status).to eq(404)
end
end
end
# test cases for when user is a student/doesn't have ta privileges
context ' | when user is student | ' do
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(false)
end
response '403', 'students cannot reject suggestions' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
expect(response.status).to eq(403)
expect(JSON.parse(response.body)['error']).to eq('Students cannot reject a suggestion.')
end
end
end
end
end
# test cases for showing suggestions
path '/api/v1/suggestions/{id}' do
get 'show suggestion' do
tags 'Suggestions'
consumes 'application/json'
parameter name: 'Authorization', in: :header, type: :string, required: true
parameter name: :id, in: :path, type: :integer, required: true, description: 'ID of the suggestion'
# tests for when user is a instructor/has ta privileges
context '| when user is instructor | ' do
# set user to have ta privileges
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(true)
end
response '200', 'suggestion details and comments returned' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
data = JSON.parse(response.body)
expect(data['suggestion']['id']).to eq(suggestion.id)
expect(data['comments']).to be_an(Array)
expect(response.status).to eq(200)
end
end
end
# test cases for when user is a student/doesn't have ta privilges
context ' | when user is student | ' do
# set user to not have ta privileges and make sure current user is set properly
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(false)
allow(controller).to receive(:current_user).and_return(@current_user)
end
# test cases for when user is owner of suggestion
context ' | when user is student owner to suggestion | ' do
# make sure user is set as owner of suggestion
before(:each) do
# Simulate the student owning the suggestion
suggestion.update!(user_id: @user.id)
end
# test for student being able to view their own suggestion
response '200', 'student can view their own suggestion' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
data = JSON.parse(response.body)
expect(data['suggestion']['id']).to eq(suggestion.id)
expect(data['comments']).to be_an(Array)
expect(response.status).to eq(200)
end
end
end
# test case for when user is not owner of suggestion
context ' | when user is not student owner to suggestion | ' do
before(:each) do
# Simulate the student not owning the suggestion
suggestion_double = double('Suggestion', id: suggestion.id, user_id: 9999)
allow(Suggestion).to receive(:find).with(suggestion.id.to_s).and_return(suggestion_double)
end
# make sure student can't view suggestions they don't own/are a part of
response '403', 'student can\'t view other suggestions' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
expect(response.status).to eq(403)
end
end
end
end
# test for when suggestion doesn't exist
response '404', 'suggestion not found' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { -1 }
run_test! do |response|
expect(response.status).to eq(404)
end
end
end
end
# test cases for updating a suggestion
path '/api/v1/suggestions/{id}' do
patch 'update suggestion' do
tags 'Suggestions'
consumes 'application/json'
parameter name: 'Authorization', in: :header, type: :string, required: true
parameter name: :id, in: :path, type: :integer, required: true, description: 'ID of the suggestion'
# test cases for when user is an instructor/has ta privileges
context '| when user is instructor | ' do
# set user to have ta privileges
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(true)
end
# test for successful suggestion update
response '200', 'suggestion updated' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
expect(response.status).to eq(200)
end
end
end
# test cases for when user is a student
context ' | when user is student | ' do
# set user to be a student and setup current user
before(:each) do
allow(AuthorizationHelper).to receive(:current_user_has_ta_privileges?).and_return(false)
allow(controller).to receive(:current_user).and_return(@current_user)
end
# test cases for when student is owner/a part of suggestion
context ' | when user is student owner to suggestion | ' do
before(:each) do
# Simulate the student owning the suggestion
suggestion.update!(user_id: @user.id)
end
# test to make sure students can update their own suggestion(s)
response '200', 'student can update their own suggestion' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
expect(response.status).to eq(200)
end
end
end
# test cases for when student is not owner to suggestion
context ' | when user is not student owner to suggestion | ' do
before(:each) do
# Simulate the student not owning the suggestion
suggestion_double = double('Suggestion', id: suggestion.id, user_id: 9999)
allow(Suggestion).to receive(:find).with(suggestion.id.to_s).and_return(suggestion_double)
end
# test for forbidding student(s) from updating suggestions other then their own
response '403', 'student can\'t updates other suggestions' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
run_test! do |response|
expect(response.status).to eq(403)
end
end
end
end
# test for suggestion not being found
response '404', 'suggestion not found' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { -1 }
run_test! do |response|
expect(response.status).to eq(404)
end
end
# test for suggestion being invalid in some form/missing
response '422', 'unprocessable entity' do
let(:Authorization) { "Bearer #{@token}" }
let(:id) { suggestion.id }
before do
allow(Suggestion).to receive(:find).and_return(suggestion) # Mock finding the suggestion
allow(suggestion).to receive(:update!).and_raise(ActiveRecord::RecordInvalid.new(suggestion))
end
run_test! do |response|
expect(response.status).to eq(422)
end
end
end
end
end
Test Coverage
Next Steps
- 'action_allowed?' filters requests based on whether the user is a TA or above. It currently does not check whether the user has these privileges in the same course that the Suggestion was made in; this check needs to be added.
- The 'sign_team_up!' method performs actions related to other models, namely on the Team, TeamUser, and SignedUpTeam models. It might be better to move these statements into the Team and SignedUpTeam models respectively to observe the single responsibility principle, or move the entire method as a whole into Team.
Team
Mentor
- Pranavi Sanganabhatla (psangan@ncsu.edu)
Members
- Anthony Spendlove (aspendl@ncsu.edu)
- Sean McLellan (spmclell@ncsu.edu)
- Dhananjay Raghu (draghu@ncsu.edu)