Skip to content
Open
Show file tree
Hide file tree
Changes from 33 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
155 changes: 155 additions & 0 deletions app/controllers/responses_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
# 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 owns_response_or_map? || has_role?('Instructor') || has_role?('Admin')
Copy link
Member

Choose a reason for hiding this comment

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

The disjunction should be replaced by ~ has_privileges_of

render json: { error: 'forbidden' }, status: :forbidden
end
when 'unsubmit', 'destroy'
unless has_role?('Instructor') || has_role?('Admin')
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it also check to make sure this is the instructor's (or the admin's) assignment? Shouldn't be able to delete reviews for other people's assignments.

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: 'Response not found' }, status: :not_found unless @response
return render json: { error: 'Response already submitted' }, status: :unprocessable_entity if @response.is_submitted?

# Check deadline
return render json: { error: 'Deadline has passed' }, status: :forbidden unless deadline_open?(@response)
Copy link
Member

Choose a reason for hiding this comment

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

deadline_open? is a bad name. What does that mean?


# 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.

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

# 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: 'Response not found' }, status: :not_found unless @response

if @response.is_submitted?
@response.update(is_submitted: false)
render json: { message: 'Response reopened for revision', response: @response }, status: :ok
Copy link
Member

Choose a reason for hiding this comment

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

The word "Response" here could be confusing. That's what it is in the system, but the UI should use better names. Don't we have print names for each different kind of Questionnaire? Also, "reopened for revision" is much too cryptic. Say it in plain English!

else
render json: { error: 'Response already unsubmitted' }, status: :unprocessable_entity
end
end

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

Choose a reason for hiding this comment

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

"Response" is problematic, as above.


@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 owns_response_or_map?
Copy link
Member

Choose a reason for hiding this comment

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

What a confusing name! What should this be?
I assume it means "owns (response or map)".
Maybe something like "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 deadline_open?(response)
Copy link
Member

Choose a reason for hiding this comment

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

Really confusing! Why should you be able to do something if a due date is in the future or not set? And which due date are we talking about? Submission? Review? Why doesn't it say?!

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
38 changes: 36 additions & 2 deletions app/models/response.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,43 @@ 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

# response type to label mapping
KIND_LABELS = {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this defined by a table somewhere? Seems like this is a redundant definition.

Choose a reason for hiding this comment

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

It is defined in a module(app/models/concerns/response_map_subclass_titles.rb), and the ResponseMap subclasses wires its get_title to those constants. The hash was added as it was one of the project requirements.

The hash has been removed, ResponseMap#response_map_label is added to read the existing constants, and renamed Response#kind_name to rubric_label, which simply delegates to that helper.

'ReviewResponseMap' => 'Review',
'TeammateReviewResponseMap' => 'Teammate Review',
'BookmarkRatingResponseMap' => 'Bookmark Review',
'QuizResponseMap' => 'Quiz',
'SurveyResponseMap' => 'Survey',
'AssignmentSurveyResponseMap' => 'Assignment Survey',
'GlobalSurveyResponseMap' => 'Global Survey',
'CourseSurveyResponseMap' => 'Course Survey',
'FeedbackResponseMap' => 'Feedback'
}.freeze

def kind_name
Copy link
Member

Choose a reason for hiding this comment

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

At least it's not "nasty_name"!

return 'Response' if map.nil?

klass_name = map.class.name
# use hash for the mapping first
if (label = KIND_LABELS[klass_name]).present?
return label
end

# back up plan: use get_title
if map.respond_to?(:get_title)
title = map.get_title
return title if title.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 +54,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 +68,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