Skip to content
Open
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
7e593fa
Added response type mapping hash, response.kind_name and unit tests
Oct 26, 2025
05ce778
Merge pull request #21 from bestinlalu/feature/response-type-mapping
bestinlalu Oct 26, 2025
21c2f4c
Updated score calculation logic to ensure accurate scoring based on u…
Oct 26, 2025
f903ff4
Merge branch 'main' of https://github.com/bestinlalu/reimplementation…
Oct 26, 2025
dfba845
[#4, #5, #6, #7, #8, #9, #20] Implement ResponseController class with…
bestinlalu Oct 27, 2025
a9732e4
Merge pull request #22 from bestinlalu/issue-4
YuWang1925wy Oct 27, 2025
1067bc9
Merge branch 'main' of https://github.com/bestinlalu/reimplementation…
Oct 27, 2025
a7b4311
Merge branch 'main' of https://github.com/bestinlalu/reimplementation…
Oct 28, 2025
ab3abbc
Updated test for deadline and update link logic
Oct 28, 2025
778de56
Removed the next action api
Oct 28, 2025
4c101db
Added the tests for the update logic
Oct 28, 2025
ea4eaae
removed next action
Oct 29, 2025
77138e3
Merge pull request #23 from bestinlalu/ScoreLogic
surabhi1914 Oct 29, 2025
b0aec87
Removed save_draft endpoint, Removed using Reviewer role, Updated rspec
bestinlalu Oct 29, 2025
a1c620d
Merge branch 'main' into issue-4
bestinlalu Oct 29, 2025
8bb157c
Merge pull request #24 from bestinlalu/issue-4
surabhi1914 Oct 29, 2025
9535b2f
Adding comment for test cases in kind name spec
bestinlalu Oct 29, 2025
ec5e0ab
edited case descriptions
Oct 29, 2025
e5bb8e4
Merge pull request #25 from bestinlalu/feature/response-type-mapping
YuWang1925wy Oct 29, 2025
d268e98
Removing mock data from rails_helper
bestinlalu Oct 29, 2025
1015d4c
Merge pull request #26 from bestinlalu/final-tweaks
bestinlalu Oct 29, 2025
9222eb7
Code clean up, removing output statements
bestinlalu Oct 29, 2025
fdc02a1
Merge pull request #27 from bestinlalu/final-tweaks
bestinlalu Oct 29, 2025
3178c85
Increase puma version to 6.4.2
bestinlalu Oct 29, 2025
d79fe2a
Merge pull request #28 from bestinlalu/final-tweaks
bestinlalu Oct 29, 2025
aff9052
Update the swagger with new response endpoints
bestinlalu Oct 29, 2025
11f4634
Merge pull request #29 from bestinlalu/final-tweaks
bestinlalu Oct 29, 2025
b1de8bd
Added small tweeks to comments for clarity and completeness.
Oct 31, 2025
281f33f
Merge branch 'main' of https://github.com/bestinlalu/reimplementation…
Oct 31, 2025
02d702d
Commented out the 80th line as requested
surabhi1914 Nov 3, 2025
f49b5d3
fix model tests error for PR #227
Nov 4, 2025
fb9b315
Score calculation test cases clean up
bestinlalu Nov 5, 2025
afa1c61
Merge pull request #30 from bestinlalu/final-tweaks
bestinlalu Nov 5, 2025
005c1ac
Changes as per few review comments in the response_controller
bestinlalu Nov 13, 2025
391d244
Merge pull request #31 from bestinlalu/final-tweaks
bestinlalu Nov 13, 2025
7377636
done requested changes related to response type mapping
Nov 14, 2025
6de4fe3
Merge pull request #32 from bestinlalu/pr-fixes
YuWang1925wy Nov 14, 2025
8fcb4ae
Changes as per few review comments in the response_controller
bestinlalu Nov 14, 2025
75bfed7
Merge pull request #33 from bestinlalu/final-tweaks
bestinlalu Nov 14, 2025
a166726
1. updated Response controller specs 2. renamed response map helpers …
Nov 14, 2025
921b75e
Merge pull request #34 from bestinlalu/pr-fixes
YuWang1925wy Nov 14, 2025
d0f2c54
Merge branch 'main' into main
ishani-rajput Nov 30, 2025
d913551
Refactor responses_controller: Add assignment ownership checks and im…
ishani-rajput Nov 30, 2025
8349a11
Auth: allow instructor, TA, and parent-admin; use questionnaire label…
ishani-rajput Dec 3, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ ruby '3.4.5'

gem 'mysql2', '~> 0.5.7'
gem 'sqlite3', '~> 1.4' # Alternative for development
gem 'puma', '~> 5.0'
gem 'puma', '~> 6.0'
gem 'rails', '~> 8.0', '>= 8.0.1'
gem 'mini_portile2', '~> 2.8' # Helps with native gem compilation
gem 'observer' # Required for Ruby 3.4.5 compatibility with Rails 8.0
Expand Down
38 changes: 34 additions & 4 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ GEM
term-ansicolor
thor
crass (1.0.6)
csv (3.3.5)
danger (9.5.3)
base64 (~> 0.2)
claide (~> 1.0)
Expand All @@ -128,6 +129,7 @@ GEM
debug (1.11.0)
irb (~> 1.10)
reline (>= 0.3.8)
delegate (0.4.0)
diff-lcs (1.6.2)
docile (1.4.1)
domain_name (0.6.20240107)
Expand All @@ -149,8 +151,11 @@ GEM
faraday (>= 0.8)
faraday-net_http (3.4.1)
net-http (>= 0.5.0)
faraday-retry (2.3.2)
faraday (~> 2.0)
find_with_order (1.3.1)
activerecord (>= 3)
forwardable (1.3.3)
git (2.3.3)
activesupport (>= 5.0)
addressable (~> 2.8)
Expand Down Expand Up @@ -197,10 +202,13 @@ GEM
mime-types-data (~> 3.2025, >= 3.2025.0507)
mime-types-data (3.2025.0924)
mini_mime (1.1.5)
mini_portile2 (2.8.9)
minitest (5.25.5)
mize (0.6.1)
monitor (0.2.0)
msgpack (1.8.0)
multi_json (1.17.0)
mutex_m (0.3.0)
mysql2 (0.5.7)
bigdecimal
nap (1.1.0)
Expand All @@ -217,7 +225,7 @@ GEM
net-protocol
netrc (0.11.0)
nio4r (2.5.9)
nokogiri (1.15.2-aarch64-linux)
nokogiri (1.18.10-aarch64-linux-gnu)
racc (~> 1.4)
nokogiri (1.18.10-arm64-darwin)
racc (~> 1.4)
Expand All @@ -230,6 +238,7 @@ GEM
faraday (>= 1, < 3)
sawyer (~> 0.9)
open4 (1.3.4)
ostruct (0.6.3)
parallel (1.27.0)
parser (3.3.9.0)
ast (~> 2.4.1)
Expand All @@ -244,7 +253,7 @@ GEM
date
stringio
public_suffix (6.0.2)
puma (6.6.1)
puma (6.4.2)
nio4r (~> 2.0)
racc (1.8.1)
rack (3.2.1)
Expand Down Expand Up @@ -350,6 +359,7 @@ GEM
addressable (>= 2.3.5)
faraday (>= 0.17.3, < 3)
securerandom (0.4.1)
set (1.1.2)
shoulda-matchers (6.5.0)
activesupport (>= 5.2.0)
simplecov (0.22.0)
Expand All @@ -358,7 +368,10 @@ GEM
simplecov_json_formatter (~> 0.1)
simplecov-html (0.13.2)
simplecov_json_formatter (0.1.4)
singleton (0.3.0)
spring (4.4.0)
sqlite3 (1.7.3)
mini_portile2 (~> 2.8.0)
stringio (3.1.7)
sync (0.5.0)
term-ansicolor (1.11.3)
Expand Down Expand Up @@ -398,18 +411,30 @@ PLATFORMS
DEPENDENCIES
active_model_serializers (~> 0.10.0)
bcrypt (~> 3.1.7)
bigdecimal
bootsnap (>= 1.18.4)
coveralls
csv
danger
database_cleaner-active_record
date
debug
delegate
factory_bot_rails
faker
faraday-retry
find_with_order
forwardable
jwt (~> 2.7, >= 2.7.1)
lingua
mysql2 (~> 0.5.5)
logger
mini_portile2 (~> 2.8)
monitor
mutex_m
mysql2 (~> 0.5.7)
observer
ostruct
psych (~> 5.2)
puma (~> 6.0)
rack-cors
rails (~> 8.0, >= 8.0.1)
Expand All @@ -418,14 +443,19 @@ DEPENDENCIES
rswag-specs
rswag-ui
rubocop
set
shoulda-matchers
simplecov
simplecov_json_formatter
singleton
spring
sqlite3 (~> 1.4)
timeout
tzinfo-data
uri

RUBY VERSION
ruby 3.2.7p253
ruby 3.4.5p51

BUNDLED WITH
2.4.14
162 changes: 162 additions & 0 deletions app/controllers/responses_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
# frozen_string_literal: true

class ResponsesController < ApplicationController
before_action :set_response, only: [:update, :submit, :unsubmit, :destroy]

# Authorization: determines if current user can perform the action
def action_allowed?
case action_name
when 'create'
true
when 'update', 'submit'
@response = Response.find(params[:id])
unless response_belongs_to? || has_privileges_of?('Instructor') || has_privileges_of?('Admin')
render json: { error: 'forbidden' }, status: :forbidden
end
when 'unsubmit', 'destroy'
unless has_privileges_of?('Instructor') || has_privileges_of?('Admin')
render json: { error: 'forbidden' }, status: :forbidden
end
else
render json: { error: 'forbidden' }, status: :forbidden
end
true
end

# POST /responses
def create
@response_map = ResponseMap.find_by(id: params[:response_map_id] || params[:map_id])
return render json: { error: 'ResponseMap not found' }, status: :not_found unless @response_map

@response = Response.new(
map_id: @response_map.id,
is_submitted: false,
created_at: Time.current
)

if @response.save
render json: { message: 'Response draft created successfully', response: @response }, status: :created
else
render json: { error: @response.errors.full_messages.to_sentence }, status: :unprocessable_entity
end
end

# PATCH /responses/:id
# Reviewer edits existing draft (still unsubmitted)
def update
puts "Updating Response ID: #{params[:id]}"
return render json: { error: 'forbidden' }, status: :forbidden if @response.is_submitted?

if @response.update(response_params)
render json: { message: 'Draft updated successfully', response: @response }, status: :ok
else
render json: { error: @response.errors.full_messages.to_sentence }, status: :unprocessable_entity
end
end

# PATCH /responses/:id/submit
# Lock the response and calculate final score
def submit
return render json: { error: 'User response not found' }, status: :not_found unless @response
if @response.is_submitted?
return render json: { error: 'User has already submitted the response' }, status: :unprocessable_entity
end
# Check deadline
unless submission_window_open?(@response)
return render json: { error: 'Deadline has passed' }, status: :forbidden
end

# Validate rubric completion
unanswered = @response.scores.select { |a| a.answer.nil? }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is just WRONG to build into the mechanism that ALL rubric items must have text (or numbers) entered for ALL rubrics of ANY KIND anywhere in the system. This should be dependent on policy. Remove this check.

unless unanswered.empty?
return render json: { error: 'All rubric items must be answered' }, status: :unprocessable_entity
end

# Lock response
@response.is_submitted = true

# Calculate score via ScorableHelper
total_score = @response.aggregate_questionnaire_score

if @response.save
render json: {
message: 'Response submitted successfully',
response: @response,
total_score: total_score
}, status: :ok
else
render json: { error: @response.errors.full_messages.to_sentence }, status: :unprocessable_entity
end
end

# PATCH /responses/:id/unsubmit
# Instructor/Admin reopens a submitted response
def unsubmit
return render json: { error: 'User response not found' }, status: :not_found unless @response

if @response.is_submitted?
@response.update(is_submitted: false)
render json: { message: 'User response has been unsubmitted', response: @response }, status: :ok
else
render json: { error: 'User response already unsubmitted' }, status: :unprocessable_entity
end
end

# DELETE /responses/:id
# Instructor/Admin deletes invalid/test response
def destroy
return render json: { error: 'User response not found' }, status: :not_found unless @response

@response.destroy
head :no_content
rescue StandardError => e
render json: { error: e.message }, status: :unprocessable_entity
end

private

def set_response
@response = Response.find_by(id: params[:id])
end

def response_params
params.require(:response).permit(
:map_id,
:is_submitted,
:submitted_at,
scores_attributes: [:id, :question_id, :answer, :comment]
)
end

def response_belongs_to?
# Member actions: we have @response from set_response
return @response.map&.reviewer&.id == current_user.id if @response&.map&.reviewer && current_user

# Collection actions (create, next_action): check map ownership
map_id = params[:response_map_id] || params[:map_id]
return false if map_id.blank?

map = ResponseMap.find_by(id: map_id)
return false unless map

map.reviewer == current_user
end

# Returns true if the assignment's due date is in the future or no due date is set
def submission_window_open?(response)
assignment = response.respond_to?(:response_map) ? response.response_map&.assignment : nil
return true if assignment.nil?
return true if assignment.respond_to?(:due_dates) && assignment.due_dates.nil?
# if due_date responds to future? use it, otherwise compare to now
if assignment.respond_to?(:due_dates) && assignment.due_dates.respond_to?(:future?)
return assignment.due_dates.first.future?
end

# fallback: compare
due = assignment.due_dates
return true if due.nil?

puts "Checking deadline: due_at=#{due.inspect}, current_time=#{Time.current}"
due.first.due_at > Time.current
end
end
3 changes: 2 additions & 1 deletion app/models/concerns/response_map_subclass_titles.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,6 @@ module ResponseMapSubclassTitles
METAREVIEW_RESPONSE_MAP_TITLE = 'Metareview'
QUIZ_RESPONSE_MAP_TITLE = 'Quiz'
REVIEW_RESPONSE_MAP_TITLE = 'Review'
SURVEY_RESPONSE_MAP_TITLE = 'Survey'
TEAMMATE_REVIEW_RESPONSE_MAP_TITLE = 'Teammate Review'
end
end
19 changes: 17 additions & 2 deletions app/models/response.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,24 @@ class Response < ApplicationRecord

belongs_to :response_map, class_name: 'ResponseMap', foreign_key: 'map_id', inverse_of: false
has_many :scores, class_name: 'Answer', foreign_key: 'response_id', dependent: :destroy, inverse_of: false
accepts_nested_attributes_for :scores

alias map response_map
delegate :questionnaire, :reviewee, :reviewer, to: :map

# returns a string of response name, needed so the front end can tell students which rubric they are filling out
def rubric_label
return 'Response' if map.nil?

if map.respond_to?(:response_map_label)
label = map.response_map_label
return label if label.present?
end

# response type doesn't exist
'Unknown Type'
end

def reportable_difference?
map_class = map.class
# gets all responses made by a reviewee
Expand All @@ -21,7 +35,7 @@ def reportable_difference?
existing_responses.each do |response|
unless id == response.id # the current_response is also in existing_responses array
count += 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be in the ReviewAggregator mixin?

total += response.aggregate_questionnaire_score.to_f / response.maximum_score
total += response.aggregate_questionnaire_score.to_f / response.maximum_score
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoo, this looks suspect! adding an aggregate score divided by the maximum? Shouldn't you wait till you get done and then divide by the maximum. Sounds like you might be trying to increase roundoff error :-)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each response only counts the scorable questions it actually answers, so different responses can have different denominators.

Also, the method itself came from the existing codebase - we might somehow deleted and added this line of code back, but we didn't change the logic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the code smell.

end
end

Expand All @@ -35,7 +49,8 @@ def reportable_difference?
score = aggregate_questionnaire_score.to_f / maximum_score
questionnaire = questionnaire_by_answer(scores.first)
assignment = map.assignment
assignment_questionnaire = AssignmentQuestionnaire.find_by(assignment_id: assignment.id, questionnaire_id: questionnaire.id)
assignment_questionnaire = AssignmentQuestionnaire.find_by(assignment_id: assignment.id,
questionnaire_id: questionnaire.id)

# notification_limit can be specified on 'Rubrics' tab on assignment edit page.
allowed_difference_percentage = assignment_questionnaire.notification_limit.to_f
Expand Down
Loading