E2550 Response hierarchy and responses_controller back end#227
E2550 Response hierarchy and responses_controller back end#227bestinlalu wants to merge 44 commits intoexpertiza:mainfrom
Conversation
Response Type Mapping
…-back-end into ScoreLogic Adding controller updates to the ScoreLogic
Implemented name link from Edit to Update
Removed save_draft endpoint, Removed using Reviewer role, Updated rspec
edited case descriptions
Removing mock data from rails_helper
Code clean up, removing output statements
Increase puma version to 6.4.2
Generated by 🚫 Danger |
Update the swagger with new response endpoints
efg
left a comment
efg
left a comment
There was a problem hiding this comment.
Lots of low-level issues to handle and fix.
| true | ||
| when 'update', 'submit' | ||
| @response = Response.find(params[:id]) | ||
| unless owns_response_or_map? || has_role?('Instructor') || has_role?('Admin') |
There was a problem hiding this comment.
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') |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
deadline_open? is a bad name. What does that mean?
| return render json: { error: 'Deadline has passed' }, status: :forbidden unless deadline_open?(@response) | ||
|
|
||
| # Validate rubric completion | ||
| unanswered = @response.scores.select { |a| a.answer.nil? } |
There was a problem hiding this comment.
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.
|
|
||
| if @response.is_submitted? | ||
| @response.update(is_submitted: false) | ||
| render json: { message: 'Response reopened for revision', response: @response }, status: :ok |
There was a problem hiding this comment.
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!
| return nil unless reviewee | ||
|
|
||
| candidates = [] | ||
| candidates << reviewee.updated_at if reviewee.respond_to?(:updated_at) && reviewee.updated_at.present? |
There was a problem hiding this comment.
respond_to? is confusing; that method should be renamed to something that is more suggestive of what it does.
Changes as per few review comments in the response_controller
Pr fixes related to response type mapping
Changes as per few review comments in the response_controller
…and clarified comments
Updated response specs and clarified response_map helpers
…prove user messages
| true | ||
| when 'update', 'submit' | ||
| @response = Response.find(params[:id]) | ||
| unless response_belongs_to? || current_user_has_admin_privileges? || |
There was a problem hiding this comment.
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.
| # 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? |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
maybe current_user_on_staff_for_assignment?
| return true if assignment.nil? | ||
| return true if assignment.due_dates.nil? | ||
|
|
||
| # Check if due_date has a future? method, otherwise compare timestamps |
There was a problem hiding this comment.
I think there is an "upcoming" method that does what you think future? does.
There was a problem hiding this comment.
I think "upcoming" is a better name.
There was a problem hiding this comment.
It's E2566, finish DueDates, who is using the upcoming method.
| # (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 |
There was a problem hiding this comment.
Should this be in the ReviewAggregator mixin?
app/models/response.rb
Outdated
| unless id == response.id # the current_response is also in existing_responses array | ||
| count += 1 | ||
| total += response.aggregate_questionnaire_score.to_f / response.maximum_score | ||
| total += response.aggregate_questionnaire_score.to_f / response.maximum_score |
…s; prefer upcoming for deadlines; reduce rounding in report diffs
| ) | ||
|
|
||
| if @response.save | ||
| render json: { message: "#{response_map_label} submission started successfully", response: @response }, status: :created |
There was a problem hiding this comment.
This is ResponsesController. The user is submitting a Response, not performing a submission. So the message should say, e.g., "review started successfully"—but only if the ResponseMap is a ReviewResponseMap. For each type of ResponseMap, it should use the appropriate term, "review", "survey", or "quiz".
| 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 |
| # 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 |
There was a problem hiding this comment.
I still think "Submission" would be misleading. Choose a more appropriate term.
| return render json: { error: 'Submission has already been locked' }, status: :unprocessable_entity | ||
| end | ||
| # Check deadline | ||
| unless submission_window_open?(@response) |
There was a problem hiding this comment.
The very term "submission_window_open?" could cause confusion. Think about what a better name would be.
| 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 |
|
|
||
| # Returns true if current user is teaching staff for the assignment associated | ||
| # with the current response (instructor or TA mapped to the assignment's course) | ||
| def current_user_is_teaching_staff_for_response_assignment? |
There was a problem hiding this comment.
Should be current_user_on_teaching_staff_for_[response_]assignment?
| current_user_teaching_staff_of_assignment?(assignment.id) | ||
| end | ||
|
|
||
| # Returns true if the current user is the parent (creator) of the instructor |
There was a problem hiding this comment.
This is misleading. Suppose the (plain old) admin created the instructor, and the logged-in user is the super_admin. Then this should return true, but it will return false. The question should not be, Is the logged-in user a parent, but is the logged-in user an ancestor.
There was a problem hiding this comment.
But WHY IS THIS METHOD IN THIS CLASS? It should be in user.rb!
|
|
||
| # Returns true if the current user is the parent (creator) of the instructor | ||
| # for the assignment associated with the current response (params[:id]). | ||
| def current_user_is_parent_of_assignment_instructor_for_response? |
There was a problem hiding this comment.
NO, NO, NO! The "for_response" is going to be EXtREMELY confusing! You just need a method determining whether one use is an ancestor of another user.
| # 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 |
There was a problem hiding this comment.
Falling back to "response" would be better than falling back to "submission", wouldn't it? And take care that the capitalization is appropriate.
| # Check if due_date has a future? method, otherwise compare timestamps | ||
| due_dates = assignment.due_dates | ||
| # Prefer the `upcoming` API if available | ||
| if due_dates.respond_to?(:upcoming) |
There was a problem hiding this comment.
The name "respond_to?" is cryptic here. What does it mean? Can you rename the method to something more appropriate? Or at least add a comment explaining it?
| return true if next_due.nil? | ||
| return next_due.due_at > Time.current | ||
| end | ||
| # Fallback to legacy `future?` if present |
There was a problem hiding this comment.
"Fall back", not "Fallback"! "Fallback" is a noun.
| end | ||
|
|
||
| # returns a string of response name, needed so the front end can tell students which rubric they are filling out | ||
| def rubric_label |
There was a problem hiding this comment.
Why is there both a "rubric_label" and a "response_map_label"? Don't they do the same thing? Could one be constructed from the other?
| alias map_id id | ||
|
|
||
| # Shared helper for Response#rubric_label; looks up the declarative constant so each map advertises its UI label | ||
| def response_map_label |
There was a problem hiding this comment.
Wasn't this method defined in another class?
| end | ||
|
|
||
| # returns the assignment related to the response map | ||
| def response_assignment |
There was a problem hiding this comment.
Cryptic method name; choose something clearer.
| candidates.compact.max | ||
| end | ||
|
|
||
| # Infer the current review round from due dates when the assignment doesn’t provide it directly. |
There was a problem hiding this comment.
This looks too complicated. Why should there be multiple ways of deciding on a round? Suppose the various methods don't agree?
| total_score > 0 ? (response_score.to_f / total_score) : 0 | ||
| # Gather all due dates with round and due_at | ||
|
|
||
| due_dates = Array(assignment.due_dates).select do |d| |
There was a problem hiding this comment.
Why do we gather all the due dates instead of just iterating over them? Please decide whether this can be done more elegantly.
There was a problem hiding this comment.
Also, would this be more appropriate in the DueDates class?
6 participants
This PR focuses on improving the Response system within Expertiza, which handles how peer reviews, teammate evaluations, and other questionnaire-based responses are stored and managed. Previously, response handling was scattered across different map controllers (e.g., ReviewResponseMap, TeammateReviewResponseMap), so the goal of this project is to centralize and standardize response management through enhancements to the Response model and the creation of a new ResponsesController on the back end.
Wiki Link: https://github.com/bestinlalu/reimplementation-back-end/wiki/Background