Review report#351
Conversation
Ports feedback_response_map report from Expertiza (repo X) to the JSON API architecture of repo Y using a streaming reduce pipeline. - FeedbackResponseMap: adds feedback_response_report class method (deduplicates responses per map/round, buckets by round for varying-rubric assignments) - BaseReport: new abstract pipeline base (source -> grouper -> accumulate -> finalize via find_each streaming) - FeedbackReport: concrete pipeline subclass; uses Set for O(1) deduplication; fetches authors once in finalize - ReportsController: replaces send(type) dispatch with REPORT_CLASSES hash; renders JSON from report.run - ReportFormatterHelper: emptied (logic moved to services) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
FeedbackReport pipeline handles all querying, deduplication, and round bucketing directly — the model class method is no longer called. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- DeploymentPipeline now inherits BaseReport, streaming TeamsUser grouped by user_id instead of nested team → user loops - Precompute taggable answer IDs and tags before the hot loop (zero DB queries in accumulate) - Remove varying_rubrics_by_round? branching — item_ids filter handles rubric scoping at the answer level - Extract model scopes: AnswerTag.for_deployment, TeamsUser.for_assignment, Answer.for_items_and_responses - Add pipeline benefit comment to BaseReport#run Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add Item.for_questionnaire_and_type scope - Add Response.submitted_review_responses_for scope; reuse in ReviewReport - Fix block indentation in AnswerTaggingReport#run - Fix Response chain indentation in fetch_responses_received_by_teams Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Extract ReviewResponsePipelineShared module with shared source and precompute_max_q_scores, included by ScoresPipeline and AvgRangesPipeline - Filter max question scores to ReviewQuestionnaire type only - Refactor AvgRangesPipeline to group by reviewee_id with round extracted in accumulate; simplify finalize with transform_values - Improve initial_state readability with named block params and comments - Remove unused for_course factory method - Rename terse lambda params (r → reviewer, response_map, response) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add ReviewResponseMap.for_assignment scope - Add AssignmentQuestionnaire.for_assignment_and_type scope (generic, reusable) - Use scopes in ReviewersPipeline source and precompute_max_q_scores Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace single DeploymentPipeline with TaggableAnswersPipeline and TaggingStatsPipeline. TaggableAnswersPipeline streams taggable Answer rows accumulating answer_ids and response_ids per user via a new Answer.taggable_for_assignment scope. TaggingStatsPipeline streams AnswerTag rows with response context; finalize filters by response_id membership instead of preloading all tags into memory. UserSummaryPipeline now keys by user_id to avoid collisions for users with identical names. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sponse_id
TaggableAnswersPipeline state simplified from {answer_ids, response_ids}
to a plain array of answer_ids. Finalize converts to a Set and filters
AnswerTags directly by answer_id membership, removing the response_id
indirection and the unused response_ids Set from the pipeline.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fix NameError bug (return unless r → return unless reviewer) in ReviewersPipeline. Remove stale grouper comment. Move max_q_scores precomputation from each pipeline's initialize into ReviewReport#run so it runs once and is passed to both ScoresPipeline and AvgRangesPipeline, eliminating the duplicate DB query. ReviewResponsePipelineShared now only holds the shared source definition. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add explanation of grouper's separation of concerns, how BaseReport#run wires grouper key into accumulate, and concrete examples from ScoresPipeline, AvgRangesPipeline, and TaggableAnswersPipeline. Also document that all domain math belongs in accumulate, not the base. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…atest submission Rename grouper → state_key_for across all report aggregators and BaseReport to better express its role: given a row, return the key used to look it up in the accumulated state. Update submitted_review_responses_for scope to return only the latest submitted response per (response_map, round) via a MAX(id) subquery, ensuring that only the most recent submission per reviewer-reviewee pair per round is counted in ScoresAggregator and AvgRangesAggregator. Rename Pipeline → Aggregator in all comments and class references across answer_tagging_report.rb, review_report.rb, and base_report.rb. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Store all reviewees for each reviewer instead of keeping only one entry per reviewer. Eagerly load reviewee: :user alongside reviewer: :user to avoid N+1 queries. Rename participant → reviewer for clarity. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…accumulate Add caveat to accumulate description noting that the key is passed but not enforced — subclasses are trusted to use it as the state bucket key. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
BaseReport#run now accepts an optional shared_state parameter so multiple aggregators can write into the same hash without a merge loop. When shared_state is provided, initial_state is ignored and finalize is skipped. TeammateReviewReport refactored into two aggregators sharing state keyed by user_id: TeammateReviewAggregator fills reviewer info, reviewees, and reviewed_count; TeamSizeAggregator fills teammate_count. Full state shape initialized upfront in Hash.new block for consistency. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Merge ReviewersReducer and ScoresReducer into a shared state keyed by reviewer_id so reviewer info and scores are co-located without a merge loop. ScoresReducer now delegates to calculate_total_score/maximum_score (ScorableHelper) with scores and questionnaires eagerly loaded to avoid N+1. AvgRangesReducer simplified to use aggregate_review_grade with full eager loading. Rename Aggregator → Reducer across all report files. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove redundant "reducer" from BaseReport class description - Fix AvgRangesReducer example: groups by team_id, not reviewee_id - Clarify finalize-always-called sentence in BaseReport#run - Fix teammate_review_report state key comments: reviewer_id → user_id - Rename leftover Aggregator → Reducer in teammate_review_report comments and class names Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ntroller - Register AnswerTaggingReport under the 'answer_tagging' type key - Switch from report_class.new(assignment) to report_class.for_assignment(assignment) to match the factory method convention used by all report classes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove GET route; keep only POST /reports/fetch_response_report
- Rename action from response_report to fetch_response_report
- Drop params[:id] fallback and params.dig(:report, :type) nesting —
JSON API clients send flat { assignment_id:, type: } body
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Reports API endpoint guarded by TA privileges, a streaming Reports::BaseReport scaffold, a concrete Reports::ReviewReport composed of three reducers (reviewers, scores, avg ranges), supporting ActiveRecord scopes, and registers POST /reports/fetch_report. ChangesReview Report Generation Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 12 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Thanks for the pull request, and welcome! The Expertiza team is excited to review your changes, and you should hear from us soon. Please make sure the PR passes all checks and you have run This repository is being automatically checked for code-quality issues using CodeRabbit, Danger, and CI workflows. Please address newly introduced issues before marking the PR ready for review. If you have any questions, please send email to expertiza-support@lists.ncsu.edu. There are code changes, but no corresponding tests. Generated by 🚫 Danger |
|
🚨 RSpec Tests Report |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9851c939-c032-4dc4-b6d4-f4a433e94533
📒 Files selected for processing (7)
app/controllers/reports_controller.rbapp/models/Item.rbapp/models/reports/base_report.rbapp/models/reports/review_report.rbapp/models/response.rbapp/models/review_response_map.rbconfig/routes.rb
📜 Review details
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: test
🧰 Additional context used
📓 Path-based instructions (3)
app/models/**/*.rb
⚙️ CodeRabbit configuration file
app/models/**/*.rb: Focus on associations, validations, callbacks, transactions, STI and polymorphic behavior,
foreign keys, data integrity, and query efficiency.
Files:
app/models/response.rbapp/models/review_response_map.rbapp/models/reports/base_report.rbapp/models/Item.rbapp/models/reports/review_report.rb
app/controllers/**/*.rb
⚙️ CodeRabbit configuration file
app/controllers/**/*.rb: Review authorization, strong parameters, HTTP status codes, and response shape consistency.
Flag N+1 queries, missing request-spec coverage, and behavior changes not reflected in Swagger docs.
Files:
app/controllers/reports_controller.rb
config/routes.rb
⚙️ CodeRabbit configuration file
config/routes.rb: Flag duplicate or shadowed routes, surprising non-RESTful patterns, and route changes without matching request specs.
Files:
config/routes.rb
🧬 Code graph analysis (3)
app/controllers/reports_controller.rb (1)
app/controllers/concerns/authorization.rb (1)
current_user_has_ta_privileges?(57-59)
app/models/reports/base_report.rb (1)
app/models/reports/review_report.rb (1)
for_assignment(17-132)
app/models/reports/review_report.rb (3)
app/models/reports/base_report.rb (3)
run(77-83)accumulate(91-93)initialize(59-61)app/models/response.rb (1)
include(3-121)app/models/review_response_map.rb (1)
include(2-30)
🪛 RuboCop (1.86.2)
app/models/response.rb
[convention] 9-9: Use the lambda method for multiline lambdas.
(Style/Lambda)
[convention] 13-13: Align .where with joins(:response_map) on line 12.
(Layout/MultilineMethodCallIndentation)
[convention] 17-17: Align .group with joins(:response_map) on line 12.
(Layout/MultilineMethodCallIndentation)
[convention] 18-18: Align .select with joins(:response_map) on line 12.
(Layout/MultilineMethodCallIndentation)
[convention] 105-105: Missing space after #.
(Layout/LeadingCommentSpace)
app/controllers/reports_controller.rb
[convention] 5-5: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 6-6: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 7-7: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 8-8: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 9-9: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 10-10: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
app/models/reports/base_report.rb
[convention] 87-87: Unnecessary spacing detected.
(Layout/ExtraSpacing)
[convention] 88-88: Unnecessary spacing detected.
(Layout/ExtraSpacing)
config/routes.rb
[convention] 80-97: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 81-81: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 97-97: end at 97, 6 is not aligned with resources :review_mappings, only: [] do at 80, 5.
(Layout/BlockAlignment)
[convention] 99-109: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 104-108: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 108-108: end at 108, 8 is not aligned with member do at 104, 9.
(Layout/BlockAlignment)
[convention] 110-110: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 169-169: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 172-172: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 196-196: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 197-201: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 204-204: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 3-228: Block has too many lines. [201/120]
(Metrics/BlockLength)
[convention] 228-228: Final newline missing.
(Layout/TrailingEmptyLines)
app/models/reports/review_report.rb
[convention] 29-29: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 30-30: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 31-31: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 33-33: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 34-34: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 42-42: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 68-68: Pass &:reviewer_id as an argument to lambda instead of a block.
(Style/SymbolProc)
[convention] 72-83: Assignment Branch Condition size for accumulate is too high. [<6, 19, 4> 20.32/20]
(Metrics/AbcSize)
[convention] 124-124: Pass &:id as an argument to lambda instead of a block.
(Style/SymbolProc)
🔇 Additional comments (1)
app/models/Item.rb (1)
8-8: LGTM!
| assignment_id = params[:assignment_id] | ||
| type = params[:type] || 'basic' | ||
|
|
||
| report_class = REPORT_CLASSES[type] | ||
| unless report_class | ||
| return render json: { | ||
| error: "Unknown report type: #{type}. Valid types: #{REPORT_CLASSES.keys.join(', ')}" | ||
| }, status: :unprocessable_entity | ||
| end | ||
|
|
||
| assignment = Assignment.find(assignment_id) | ||
| data = report_class.for_assignment(assignment).run | ||
| render json: { type: type, assignment_id: assignment_id.to_i }.merge(data) | ||
| rescue ActiveRecord::RecordNotFound | ||
| render json: { error: 'Assignment not found' }, status: :not_found |
There was a problem hiding this comment.
Return 400/422 for invalid assignment_id instead of 404.
If assignment_id is missing/blank, Line 31 raises RecordNotFound and returns Line 35’s 404, which misclassifies a malformed request as “not found”.
Proposed fix
def fetch_response_report
- assignment_id = params[:assignment_id]
+ assignment_id = params[:assignment_id]
+ if assignment_id.blank?
+ return render json: { error: 'assignment_id is required' }, status: :bad_request
+ end
+
type = params[:type] || 'basic'As per coding guidelines "app/controllers/**/*.rb: Review ... HTTP status codes."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assignment_id = params[:assignment_id] | |
| type = params[:type] || 'basic' | |
| report_class = REPORT_CLASSES[type] | |
| unless report_class | |
| return render json: { | |
| error: "Unknown report type: #{type}. Valid types: #{REPORT_CLASSES.keys.join(', ')}" | |
| }, status: :unprocessable_entity | |
| end | |
| assignment = Assignment.find(assignment_id) | |
| data = report_class.for_assignment(assignment).run | |
| render json: { type: type, assignment_id: assignment_id.to_i }.merge(data) | |
| rescue ActiveRecord::RecordNotFound | |
| render json: { error: 'Assignment not found' }, status: :not_found | |
| assignment_id = params[:assignment_id] | |
| if assignment_id.blank? | |
| return render json: { error: 'assignment_id is required' }, status: :bad_request | |
| end | |
| type = params[:type] || 'basic' | |
| report_class = REPORT_CLASSES[type] | |
| unless report_class | |
| return render json: { | |
| error: "Unknown report type: #{type}. Valid types: #{REPORT_CLASSES.keys.join(', ')}" | |
| }, status: :unprocessable_entity | |
| end | |
| assignment = Assignment.find(assignment_id) | |
| data = report_class.for_assignment(assignment).run | |
| render json: { type: type, assignment_id: assignment_id.to_i }.merge(data) | |
| rescue ActiveRecord::RecordNotFound | |
| render json: { error: 'Assignment not found' }, status: :not_found |
| rescue StandardError => e | ||
| render json: { error: e.message }, status: :internal_server_error |
There was a problem hiding this comment.
Do not expose raw exception messages in API responses.
Line 37 returns e.message directly, which can leak internal details (SQL/error internals) to clients. Return a generic error payload and log the exception server-side.
Proposed fix
rescue StandardError => e
- render json: { error: e.message }, status: :internal_server_error
+ Rails.logger.error("ReportsController#fetch_response_report failed: #{e.class} - #{e.message}")
+ render json: { error: 'Internal server error' }, status: :internal_server_error
endstate_key_for was not enforced — accumulate could use any key it wanted — making it documentation-only with misleading precision. Removed it entirely: - BaseReport#run now calls accumulate(state, row); key derivation lives in accumulate - ReviewersReducer, ScoresReducer, AvgRangesReducer each derive their own key locally Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… fix comments - Drop full_name from shared state and ReviewersReducer; only name is needed - Remove ReviewResponseShared module; inline source directly in ScoresReducer - Remove stale eager-loading note from class-level comment (now in ScoresReducer) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🚨 RSpec Tests Report |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/models/reports/review_report.rb (1)
87-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent orphan score buckets when reviewer metadata is missing.
On Lines 90-96,
ScoresReducercan autovivifystate[reviewer_id]even whenReviewersReducerskipped that reviewer (missing reviewer record). That creates report rows with nil reviewer identity fields but populated scores.Suggested fix
def accumulate(state, response) + response_map = response.response_map + return unless response_map + + reviewer_id = response_map.reviewer_id + return unless reviewer_id && state.key?(reviewer_id) + return if response.maximum_score.zero? - reviewer_id = response.response_map.reviewer_id - reviewee_id = response.response_map.reviewee_id + reviewee_id = response_map.reviewee_id round = response.round || 1 score_pct = (response.calculate_total_score.to_f / response.maximum_score * 100).round(2) state[reviewer_id][:scores][reviewee_id][round] = score_pct endAs per coding guidelines "app/models/**/*.rb: Focus on associations, validations, callbacks, transactions, STI and polymorphic behavior, foreign keys, data integrity, and query efficiency."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6794580d-87cc-4920-9e17-bbbeb24c8b13
📒 Files selected for processing (2)
app/models/reports/base_report.rbapp/models/reports/review_report.rb
📜 Review details
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: test
🧰 Additional context used
📓 Path-based instructions (1)
app/models/**/*.rb
⚙️ CodeRabbit configuration file
app/models/**/*.rb: Focus on associations, validations, callbacks, transactions, STI and polymorphic behavior,
foreign keys, data integrity, and query efficiency.
Files:
app/models/reports/base_report.rbapp/models/reports/review_report.rb
🪛 RuboCop (1.86.2)
app/models/reports/review_report.rb
[convention] 28-28: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 30-30: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 31-31: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 32-32: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 40-40: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
🔇 Additional comments (2)
app/models/reports/base_report.rb (1)
19-22: LGTM!Also applies to: 45-46, 55-56, 62-63, 72-74
app/models/reports/review_report.rb (1)
8-9: LGTM!Also applies to: 15-15, 28-33, 40-42, 57-68, 73-83, 116-118
efg
left a comment
efg
left a comment
There was a problem hiding this comment.
Good job! Please see if my suggestions help improve the code.
| 'teammate_review_response_map' => Reports::TeammateReviewReport, | ||
| 'bookmark_rating_response_map' => Reports::BookmarkRatingReport, | ||
| 'answer_tagging' => Reports::AnswerTaggingReport, | ||
| 'basic' => Reports::BasicReport |
There was a problem hiding this comment.
Why shouldn't 'basic' be first in the list?
There was a problem hiding this comment.
Moved to the top
|
|
||
| # Only TAs, instructors, and admins may view reports. | ||
| def action_allowed? | ||
| current_user_has_ta_privileges? |
There was a problem hiding this comment.
This seems to be wrong. It allows any instructor and any TA to see any report, if they know the URL.
There was a problem hiding this comment.
Updated to have current_user_teaching_staff_of_assignment?
|
|
||
| # POST /reports/fetch_response_report | ||
| # Returns the requested report as JSON. | ||
| def fetch_response_report |
There was a problem hiding this comment.
Why is the method name "fetch_response_report" when not all of the reports are called response reports?
There was a problem hiding this comment.
I was referring to all reports being related to responses submitted by a user. Changed to fetch_report
| type = params[:type] || 'basic' | ||
|
|
||
| report_class = REPORT_CLASSES[type] | ||
| unless report_class |
There was a problem hiding this comment.
I question whether this clause is useful. It seems to be testing whether the app is working correctly. That kind of check should be done by automated tests, not application code within the app.
There was a problem hiding this comment.
The check isn't testing internal app correctness - type comes from user-supplied POST params, which means an unknown value is a realistic scenario. Without the guard, passing an invalid type would hit nil.for_assignment and raise a NoMethodError.
| data = report_class.for_assignment(assignment).run | ||
| render json: { type: type, assignment_id: assignment_id.to_i }.merge(data) | ||
| rescue ActiveRecord::RecordNotFound | ||
| render json: { error: 'Assignment not found' }, status: :not_found |
There was a problem hiding this comment.
Similar question here. Is there any way someone could invoke an operation from the UI that would fail to find an assignment?
There was a problem hiding this comment.
In the ideal scenario, a person shouldn't be able to invoke an operation with an invalid assignment. But since the APIs could be called directly (i.e. curl, Postman, etc.), BE should validate independently.
| @reportable = reportable | ||
| end | ||
|
|
||
| def run |
| # Reducer 3 — per-team average review score. | ||
| # Streams AssignmentTeam rows with review_mappings, responses, and scores | ||
| # eagerly loaded to avoid N+1. Delegates score computation to | ||
| # aggregate_review_grade (via ReviewAggregator concern) which picks the |
There was a problem hiding this comment.
This is an already existing module being used in several other places as well. Should I update the module name and corresponding usages?
| belongs_to :response_map, class_name: 'ResponseMap', foreign_key: 'map_id', inverse_of: false | ||
|
|
||
| scope :submitted_review_responses_for, ->(assignment_id) { | ||
| # Subquery picks the latest submitted response per (response_map, round) |
There was a problem hiding this comment.
I think there should be a method (e.g., "latest_response") that returns the latest response for a round. That would make the code clearer.
There was a problem hiding this comment.
The submitted_review_responses_for scope already encapsulates this logic at the SQL level using a MAX(id) subquery grouped by (response_map_id, round), which fetches the latest submitted
response per round for all response maps in a single query. Extracting a separate latest_response method would either duplicate this logic or require calling it per response map in a loop,
reintroducing N+1 queries.
| patch :grade_review | ||
| delete :delete_mapping | ||
| end | ||
| resources :review_mappings, only: [] do |
There was a problem hiding this comment.
Most of the changes in this file are whitespace only. Removing those changes, or making them in a separate commit, would make the diff easier to follow.
| resources :duties, controller: 'assignments_duties', only: [:index, :create, :destroy] | ||
| end | ||
| end | ||
| end No newline at end of file |
… fetch_report - Add before_action :set_assignment to load @assignment from params[:assignment_id] - Replace current_user_has_ta_privileges? with current_user_teaching_staff_of_assignment? so only instructors/TAs mapped to that specific assignment can view its reports - Reuse @assignment in fetch_report instead of re-fetching - Rename action/route from fetch_response_report to fetch_report — not all report types are response reports (e.g. basic, answer_tagging, bookmark_rating) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add explicit reducer descriptions to ReviewReport class comment - Add run method comment with full output shape - Fix British spellings: materialised -> materialized, normalises -> normalizes - Fix wording: reduce-based -> reduction-based in BaseReport Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🚨 RSpec Tests Report |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/reports_controller.rb (1)
23-34:⚠️ Potential issue | 🟠 MajorAdd request-spec + Swagger coverage for
POST /reports/fetch_report
- Route exists:
POST /reports/fetch_reportinconfig/routes.rb(andReportsController#fetch_report).- No request specs cover
/reports/fetch_report.swagger/v1/swagger.yamlhas nofetch_reportor/reports/fetch_reportentry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d18d448-3e6a-4526-9aca-8510c1e7f8c3
📒 Files selected for processing (4)
app/controllers/reports_controller.rbapp/models/reports/base_report.rbapp/models/reports/review_report.rbconfig/routes.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- app/models/reports/base_report.rb
- app/models/reports/review_report.rb
📜 Review details
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: test
🧰 Additional context used
📓 Path-based instructions (2)
config/routes.rb
⚙️ CodeRabbit configuration file
config/routes.rb: Flag duplicate or shadowed routes, surprising non-RESTful patterns, and route changes without matching request specs.
Files:
config/routes.rb
app/controllers/**/*.rb
⚙️ CodeRabbit configuration file
app/controllers/**/*.rb: Review authorization, strong parameters, HTTP status codes, and response shape consistency.
Flag N+1 queries, missing request-spec coverage, and behavior changes not reflected in Swagger docs.
Files:
app/controllers/reports_controller.rb
🔇 Additional comments (3)
app/controllers/reports_controller.rb (2)
35-36: Do not expose raw exception messages in API responses.Line 36 still returns
e.messagedirectly, which can leak internal details (SQL/error internals) to clients. Log server-side and return a generic payload.Proposed fix
rescue StandardError => e - render json: { error: e.message }, status: :internal_server_error + Rails.logger.error("ReportsController#fetch_report failed: #{e.class} - #{e.message}") + render json: { error: 'Internal server error' }, status: :internal_server_error end
41-45: Blank/missingassignment_idreturns 404 instead of 400/422.When
params[:assignment_id]is blank,Assignment.find(nil)raisesRecordNotFoundand yields the Line 44 404, misclassifying a malformed request as "not found".Proposed fix
def set_assignment + if params[:assignment_id].blank? + return render json: { error: 'assignment_id is required' }, status: :bad_request + end + `@assignment` = Assignment.find(params[:assignment_id]) rescue ActiveRecord::RecordNotFound render json: { error: 'Assignment not found' }, status: :not_found endconfig/routes.rb (1)
197-201: LGTM!
- Fix indentation of review_mappings block - Fix extra space on signed_up_teams member block - Remove trailing whitespace on get :view and put '/leave' lines - Remove trailing whitespace on collection do and end lines - Add missing newline at end of file Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🚨 RSpec Tests Report |
A Reducer is-not-a Report — inner reducer classes inherit streaming scaffold behavior, not report behavior. Renaming makes the inheritance read correctly: ReviewersReducer < BaseReducer, etc. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🚨 RSpec Tests Report |
- Replace ReviewersReducer + shared state with a single AR query on
ReviewResponseMap serialized via as_json(MAP_JSON_OPTIONS)
- MAP_JSON_OPTIONS whitelist keeps output clean (no raw FKs, timestamps, STI type)
- ScoresReducer now runs standalone with its own nested Hash.new initial_state
- AvgRangesReducer unchanged
- Output shape: { reviews: [...], scores: {...}, avg_and_ranges: {...} }
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🚨 RSpec Tests Report |
- Fix ScoresReducer to use aggregate_questionnaire_score instead of calculate_total_score (which always returned 0 due to Item#scorable? returning false for non-STI base class instances) - Remove module Reports namespace from all report files and update controller REPORT_CLASSES references accordingly - Add seed data for review report: questionnaire, items, AssignmentQuestionnaire (rounds 1 & 2), ReviewResponseMaps, Responses, and Answers using find_or_create_by for idempotency - Remove unimplemented report types from REPORT_CLASSES to prevent NameError on controller load Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🚨 RSpec Tests Report |
Zeitwerk maps app/models/reports/*.rb to Reports::* constants, so the module wrapper is required. Reverts the namespace removal and restores Reports::BasicReport and Reports::ReviewReport references in the controller. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🚨 RSpec Tests Report |
- Add latest_submitted_response_by_round on ResponseMap: picks the most recent submitted response per round from already-loaded associations, avoiding N+1 queries - ScoresReducer now streams ReviewResponseMap rows instead of Response rows, using eager-loaded responses/scores/items for all computation - Remove hardcoded submitted_review_responses_for scope from Response - Fix base_reducer.rb indentation inside module Reports - Update comments to accurately reflect the new streaming approach Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Expand output schema comment with explicit types and annotations - Correct reviewee to be AssignmentTeam id (not a Participant with user) - Label score_pct and avg_score explicitly as percentages (0-100) - Annotate round as a 1-based integer Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Removes tag_prompts, tag_prompt_deployments, and answer_tags tables that were auto-generated from untracked migrations not belonging to this branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review report seed data is local-only and not needed in the branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🚨 RSpec Tests Report |
1 similar comment
|
🚨 RSpec Tests Report |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🚨 RSpec Tests Report |
`scores` was ambiguous (clashes with raw answer scores inside reviews[]). `avg_and_ranges` was a holdover from the original Expertiza that computed ranges — the current output is averages only, so team_averages is accurate. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🚨 RSpec Tests Report |
2 participants
Code Review Assessment Against Expertiza Danger Policy
🔴 Critical Violations
Missing Test Coverage (new production code, no tests)
Authorization & Error-Path Tests Missing
🟡 Medium Severity Issues
Overly Broad Error Handling
Unrelated/Whitespace Churn
Potential Performance Concerns
Danger/Debrief Signals Present
🟢 Low Severity / Checklist
Summary / Recommendation
Status: CANNOT MERGE
Required before merge:
The implementation and reducer design are plausible, but Expertiza Danger policy and repo standards require test coverage, tighter error handling, and avoidance of unrelated churn before approval.