Skip to content
Open
Show file tree
Hide file tree
Changes from 43 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.lock
Original file line number Diff line number Diff line change
Expand Up @@ -253,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
181 changes: 181 additions & 0 deletions app/controllers/responses_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,181 @@
# 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? || current_user_has_admin_privileges? ||
Copy link
Member

Choose a reason for hiding this comment

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

It's not whether current_user_has_admin_privileges?; it's whether this user created (is the parent of) the instructor whose course this is. One of the current projects is writing/has written code for this. Not sure which, but it should be fairly easy to guess.

(current_user_has_instructor_privileges? && current_user_instructs_response_assignment?)
render json: { error: 'forbidden' }, status: :forbidden
end
when 'unsubmit', 'destroy'
# Only allow if user is the instructor of the associated assignment or has admin privileges
unless current_user_has_admin_privileges? ||
(current_user_has_instructor_privileges? && current_user_instructs_response_assignment?)
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_map_label} submission started 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
return render json: { error: 'forbidden' }, status: :forbidden if @response.is_submitted?

if @response.update(response_params)
render json: { message: "#{response_map_label} submission saved 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: 'Submission not found' }, status: :not_found unless @response
if @response.is_submitted?
return render json: { error: 'Submission has already been locked' }, status: :unprocessable_entity
end
# Check deadline
unless submission_window_open?(@response)
return render json: { error: 'Submission deadline has passed' }, status: :forbidden
end

# Lock response
@response.is_submitted = true

# Calculate score via ScorableHelper
total_score = @response.aggregate_questionnaire_score

if @response.save
render json: {
message: "#{response_map_label} submission locked and scored 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 for further editing
def unsubmit
return render json: { error: "#{response_map_label} submission not found" }, status: :not_found unless @response

if @response.is_submitted?
@response.update(is_submitted: false)
render json: { message: "#{response_map_label} submission reopened for edits. The reviewer can now make changes.", response: @response }, status: :ok
else
render json: { error: "This #{response_map_label.downcase} submission is not locked, so it cannot be reopened" }, status: :unprocessable_entity
end
end

# DELETE /responses/:id
# Instructor/Admin deletes invalid/test response
def destroy
return render json: { error: 'Submission 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

# Checks whether the current_user is the instructor for the assignment
# associated with the response identified by params[:id].
# Uses the shared authorization method from Authorization concern.
def current_user_instructs_response_assignment?
Copy link
Member

Choose a reason for hiding this comment

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

Not only the instructor, but also TAs for the course and the admin who created the instructor, need to be able to perform this method.

resp = Response.find_by(id: params[:id])
return false unless resp&.response_map

assignment = resp.response_map&.assignment
return false unless assignment

# Delegate to the shared authorization helper
current_user_instructs_assignment?(assignment)
end

# Returns the friendly label for the response's map type (e.g., "Review", "Assignment Survey")
# Falls back to a generic "Submission" if the label cannot be determined.
def response_map_label
return 'Submission' unless @response&.response_map

map_label = @response.response_map&.response_map_label
map_label.presence || 'Submission'
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&.response_map&.assignment
return true if assignment.nil?
return true if assignment.due_dates.nil?

# Check if due_date has a future? method, otherwise compare timestamps
Copy link
Member

Choose a reason for hiding this comment

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

I think there is an "upcoming" method that does what you think future? does.

Copy link
Member

Choose a reason for hiding this comment

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

I think "upcoming" is a better name.

Copy link
Member

Choose a reason for hiding this comment

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

It's E2566, finish DueDates, who is using the upcoming method.

due_dates = assignment.due_dates
if due_dates.respond_to?(:future?)
return due_dates.first.future?
end

# Fallback: compare timestamps
return true if due_dates.first.nil?

due_dates.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
21 changes: 19 additions & 2 deletions app/models/response.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ 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 :response_assignment, :reviewee, :reviewer, to: :map
Expand All @@ -15,6 +16,20 @@ def questionnaire
response_assignment.assignment_questionnaires.find_by(used_in_round: self.round).questionnaire
end

# 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

# Returns true if this response's score differs from peers by more than the assignment notification limit
def reportable_difference?
map_class = map.class
# gets all responses made by a reviewee
Expand All @@ -23,10 +38,11 @@ def reportable_difference?
count = 0
total = 0
# gets the sum total percentage scores of all responses that are not this response
# (each response can omit questions, so maximum_score may differ and we normalize before averaging)
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 @@ -40,7 +56,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