Pipeline for report generation#340
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>
|
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:
WalkthroughThis pull request introduces a new streaming report pipeline infrastructure for the Expertiza backend. It adds data model scopes to support efficient querying, a base ChangesReport System Implementations
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 9 | ❌ 5❌ Failed checks (5 warnings)
✅ Passed checks (9 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. Your pull request is more than 500 LoC. This pull request contains 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: 4
🧹 Nitpick comments (1)
app/services/reports/feedback_report.rb (1)
66-83: Author hydration is still N+1.Line 70 does one
AssignmentParticipant.find_byper team member, and Lines 81-82 can then hitp.useragain per participant. On large assignments this turns finalize into a query-heavy path; load participants withincludes(:user)in one query keyed byuser_idinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/reports/feedback_report.rb` around lines 66 - 83, fetch_authors currently does an AssignmentParticipant.find_by per team user and format_participant then touches p.user causing N+1s; instead load all participants with their users in one query and map them by user_id: in fetch_authors collect team user ids from AssignmentTeam.includes(:users).where(parent_id: `@assignment.id`), then query AssignmentParticipant.includes(:user).where(parent_id: `@assignment.id`, user_id: user_ids) and return those participant records (or a lookup keyed by user_id) so format_participant can use the preloaded p.user without extra queries; update fetch_authors and any callers to work with the returned participants/list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/reports_controller.rb`:
- Around line 35-36: The rescue in ReportsController currently renders raw
e.message to clients; instead log the exception server-side (e.g., use
Rails.logger.error / logger.error with e.message and e.backtrace) and change the
response to render a generic JSON error like { error: "Internal server error" }
with status :internal_server_error; update the rescue StandardError => e block
to perform the logging first and then return the generic payload so internals
(SQL/class errors) are not exposed.
In `@app/models/feedback_response_map.rb`:
- Around line 27-71: The model method self.feedback_response_report duplicates
logic that belongs in Reports::FeedbackReport; remove the internal query/dedupe
logic in feedback_response_report and delegate to the canonical service
implementation in Reports::FeedbackReport, adapting any legacy return shape at
this boundary. Specifically, replace the body of
self.feedback_response_report(assignment_id) with a single call to the
Reports::FeedbackReport API (e.g., invoke the service class/method that computes
the authors and per-round or all_review_response_ids), and transform the service
result into the existing return tuple the caller expects (authors + round arrays
or authors + all_review_response_ids) so callers keep the same contract while
the service remains the single source of truth. Ensure you reference the same
semantics used by the service for varying_rubrics_by_round? and deduplication
(map_id vs map_id-round) when mapping the service output back to the model
method's return shape.
In `@app/services/reports/base_report.rb`:
- Around line 32-37: The use of ActiveRecord::Relation.find_each in
BaseReport#run ignores the custom ordering defined by FeedbackReport#source and
breaks the dedup logic in accumulate (which relies on "ordered newest-first");
update run to iterate while preserving order—either switch from find_each to the
Rails 8 cursor form (e.g., find_each(cursor: :created_at, order: :desc) or
equivalent) so the relation's order is respected, or replace the iteration by
selecting latest-per-(map, round) in SQL (push the "latest per group" logic into
the source query) and then iterate that ordered result; ensure the changes
reference the run method, source relation, and accumulate dedup behavior so
newest responses are seen first.
In `@app/services/reports/feedback_report.rb`:
- Around line 22-40: The deduplication key currently produced by the grouper
(->(r) { [r.map_id, r.round] }) causes multiple entries per map when
`@assignment.varying_rubrics_by_round`? is false; change the logic so the key used
for state[:seen] depends on the assignment: if varying_rubrics_by_round? then
keep [r.map_id, r.round], otherwise use r.map_id only. Update the grouper or
compute key inside accumulate (referencing grouper, accumulate,
varying_rubrics_by_round?, and initial_state) so that when rubrics do not vary
by round a map_id is deduped once and state[:all] only gets one response id per
map.
---
Nitpick comments:
In `@app/services/reports/feedback_report.rb`:
- Around line 66-83: fetch_authors currently does an
AssignmentParticipant.find_by per team user and format_participant then touches
p.user causing N+1s; instead load all participants with their users in one query
and map them by user_id: in fetch_authors collect team user ids from
AssignmentTeam.includes(:users).where(parent_id: `@assignment.id`), then query
AssignmentParticipant.includes(:user).where(parent_id: `@assignment.id`, user_id:
user_ids) and return those participant records (or a lookup keyed by user_id) so
format_participant can use the preloaded p.user without extra queries; update
fetch_authors and any callers to work with the returned participants/list.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 341c60ad-38b6-4404-9ea3-4c02238309e0
📒 Files selected for processing (5)
app/controllers/reports_controller.rbapp/helpers/report_formatter_helper.rbapp/models/feedback_response_map.rbapp/services/reports/base_report.rbapp/services/reports/feedback_report.rb
| rescue StandardError => e | ||
| render json: { error: e.message }, status: :internal_server_error |
There was a problem hiding this comment.
Do not return raw exception messages from the API.
Line 36 exposes e.message verbatim, which can leak SQL errors, class names, or other internals to callers. Log the exception server-side and return a generic 500 payload instead.
Proposed fix
rescue ActiveRecord::RecordNotFound
render json: { error: 'Assignment not found' }, status: :not_found
rescue StandardError => e
- render json: { error: e.message }, status: :internal_server_error
+ Rails.logger.error("[ReportsController#response_report] #{e.class}: #{e.message}")
+ render json: { error: 'Internal server error' }, status: :internal_server_error
end📝 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.
| rescue StandardError => e | |
| render json: { error: e.message }, status: :internal_server_error | |
| rescue StandardError => e | |
| Rails.logger.error("[ReportsController#response_report] #{e.class}: #{e.message}") | |
| render json: { error: 'Internal server error' }, status: :internal_server_error |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/reports_controller.rb` around lines 35 - 36, The rescue in
ReportsController currently renders raw e.message to clients; instead log the
exception server-side (e.g., use Rails.logger.error / logger.error with
e.message and e.backtrace) and change the response to render a generic JSON
error like { error: "Internal server error" } with status
:internal_server_error; update the rescue StandardError => e block to perform
the logging first and then return the generic payload so internals (SQL/class
errors) are not exposed.
| def self.feedback_response_report(assignment_id) | ||
| review_map_ids = ReviewResponseMap.where(reviewed_object_id: assignment_id).pluck(:id) | ||
|
|
||
| teams = AssignmentTeam.includes(:users).where(parent_id: assignment_id) | ||
| authors = teams.flat_map do |team| | ||
| team.users.filter_map do |user| | ||
| AssignmentParticipant.find_by(parent_id: assignment_id, user_id: user.id) | ||
| end | ||
| end | ||
|
|
||
| # Collect latest review responses per map (ordered newest-first) | ||
| temp_responses = Response.where(map_id: review_map_ids).order(created_at: :desc) | ||
|
|
||
| assignment = Assignment.find(assignment_id) | ||
| seen_map_round_keys = [] | ||
|
|
||
| if assignment.varying_rubrics_by_round? | ||
| round_one_ids = [] | ||
| round_two_ids = [] | ||
| round_three_ids = [] | ||
|
|
||
| temp_responses.each do |response| | ||
| key = "#{response.map_id}-#{response.round}" | ||
| next if seen_map_round_keys.include?(key) | ||
|
|
||
| seen_map_round_keys << key | ||
| round_one_ids << response.id if response.round == 1 | ||
| round_two_ids << response.id if response.round == 2 | ||
| round_three_ids << response.id if response.round == 3 | ||
| end | ||
|
|
||
| [authors, round_one_ids, round_two_ids, round_three_ids] | ||
| else | ||
| all_review_response_ids = [] | ||
|
|
||
| temp_responses.each do |response| | ||
| next if seen_map_round_keys.include?(response.map_id) | ||
|
|
||
| seen_map_round_keys << response.map_id | ||
| all_review_response_ids << response.id | ||
| end | ||
|
|
||
| [authors, all_review_response_ids] | ||
| end | ||
| end |
There was a problem hiding this comment.
This duplicates the new service and will drift.
Reports::FeedbackReport already owns this report, but this model method re-queries the same data, reintroduces the pre-service pluck + Ruby dedupe path, and already disagrees with the service on non-varying dedup semantics. That means callers can get different answers depending on which entry point they hit. Please keep one source of truth and adapt any legacy return shape at the boundary instead of maintaining both implementations.
🧰 Tools
🪛 RuboCop (1.86.1)
[convention] 27-71: Assignment Branch Condition size for feedback_response_report is too high. [<15, 34, 14> 39.71/20]
(Metrics/AbcSize)
[convention] 27-71: Cyclomatic complexity for feedback_response_report is too high. [11/7]
(Metrics/CyclomaticComplexity)
[convention] 27-71: Method has too many lines. [32/20]
(Metrics/MethodLength)
[convention] 27-71: Perceived complexity for feedback_response_report is too high. [12/8]
(Metrics/PerceivedComplexity)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/feedback_response_map.rb` around lines 27 - 71, The model method
self.feedback_response_report duplicates logic that belongs in
Reports::FeedbackReport; remove the internal query/dedupe logic in
feedback_response_report and delegate to the canonical service implementation in
Reports::FeedbackReport, adapting any legacy return shape at this boundary.
Specifically, replace the body of self.feedback_response_report(assignment_id)
with a single call to the Reports::FeedbackReport API (e.g., invoke the service
class/method that computes the authors and per-round or
all_review_response_ids), and transform the service result into the existing
return tuple the caller expects (authors + round arrays or authors +
all_review_response_ids) so callers keep the same contract while the service
remains the single source of truth. Ensure you reference the same semantics used
by the service for varying_rubrics_by_round? and deduplication (map_id vs
map_id-round) when mapping the service output back to the model method's return
shape.
| def run | ||
| state = initial_state | ||
| source.find_each(batch_size: 500) do |row| | ||
| accumulate(state, grouper.call(row), row) | ||
| end | ||
| finalize(state) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the base_report.rb file
find . -name "base_report.rb" -path "*/reports/*" -type fRepository: expertiza/reimplementation-back-end
Length of output: 117
🏁 Script executed:
# Locate feedback_report.rb
find . -name "feedback_report.rb" -path "*/reports/*" -type fRepository: expertiza/reimplementation-back-end
Length of output: 121
🏁 Script executed:
# Search for any reports directory
find . -type d -name "reports" 2>/dev/null | head -20Repository: expertiza/reimplementation-back-end
Length of output: 102
🏁 Script executed:
# Read base_report.rb to verify the code at lines 32-37
cat -n app/services/reports/base_report.rb | head -50Repository: expertiza/reimplementation-back-end
Length of output: 2293
🏁 Script executed:
# Read feedback_report.rb to check source definition and dedup logic
cat -n app/services/reports/feedback_report.rbRepository: expertiza/reimplementation-back-end
Length of output: 3123
🏁 Script executed:
# Check Rails version
cat Gemfile | grep -A 5 -B 5 "rails" | head -20Repository: expertiza/reimplementation-back-end
Length of output: 776
🌐 Web query:
Rails find_each order preservation behavior does it respect scoped order
💡 Result:
No, Rails find_each does not respect the order from a scoped relation. When called on a relation with an existing order (e.g., Model.order(:created_at).find_each), the scoped order is ignored to enforce batching order on the primary key (or specified cursor). A warning is issued by default ("Scoped order is ignored, use :cursor with :order to configure custom order."), or an ArgumentError if config.active_record.error_on_ignored_order = true. To use custom ordering, specify cursor: :column_name, order: :desc (or :asc) explicitly in find_each options, limited to orderable columns like primary keys.
Citations:
- 1: https://guides.rubyonrails.org/v8.0.0/active_record_querying.html
- 2: https://edgeguides.rubyonrails.org/active_record_querying.html
- 3: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/batches.rb
- 4: http://screenstaring.com/blog/active-record-s-find_each-find_in_batches-use-error_on_ignored_order/
- 5: https://github.com/rails/rails/blob/main/activerecord/lib/active_record/relation/batches.rb
- 6: https://api.rubyonrails.org/classes/ActiveRecord/Batches.html
- 7: https://api.rubyonrails.org/v8.1.0/classes/ActiveRecord/Batches.html
- 8: https://www.instisocialorderph.org
- 9: https://api.rubyonrails.org/v8.0.0.1/classes/ActiveRecord/Batches.html
- 10: https://github.com/rails/rails/blob/ee9b18fd/guides/source/active_record_querying.md
find_each silently ignores order, breaking dedup logic.
Line 34 uses find_each(batch_size: 500), which ignores the order(created_at: :desc) scoped on the relation in FeedbackReport#source. The deduplication logic in accumulate (line 29) depends on processing newest responses first—the comments explicitly state "ordered newest-first so the first occurrence per (map, round) is the latest revision." Instead, find_each batches by primary key, causing the dedup to silently keep older responses instead of the latest ones.
In Rails 8.0, use find_each(cursor: :created_at, order: :desc) to preserve custom ordering, or refactor to push "latest-per-group" selection into SQL (e.g., using a subquery).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/services/reports/base_report.rb` around lines 32 - 37, The use of
ActiveRecord::Relation.find_each in BaseReport#run ignores the custom ordering
defined by FeedbackReport#source and breaks the dedup logic in accumulate (which
relies on "ordered newest-first"); update run to iterate while preserving
order—either switch from find_each to the Rails 8 cursor form (e.g.,
find_each(cursor: :created_at, order: :desc) or equivalent) so the relation's
order is respected, or replace the iteration by selecting latest-per-(map,
round) in SQL (push the "latest per group" logic into the source query) and then
iterate that ordered result; ensure the changes reference the run method, source
relation, and accumulate dedup behavior so newest responses are seen first.
| def grouper = ->(r) { [r.map_id, r.round] } | ||
|
|
||
| def initial_state | ||
| { seen: Set.new, round_1: [], round_2: [], round_3: [], all: [] } | ||
| end | ||
|
|
||
| def accumulate(state, key, response) | ||
| return if state[:seen].include?(key) | ||
|
|
||
| state[:seen].add(key) | ||
|
|
||
| if @assignment.varying_rubrics_by_round? | ||
| case response.round | ||
| when 1 then state[:round_1] << response.id | ||
| when 2 then state[:round_2] << response.id | ||
| when 3 then state[:round_3] << response.id | ||
| end | ||
| else | ||
| state[:all] << response.id |
There was a problem hiding this comment.
Dedup is too granular when rubrics do not vary by round.
When varying_rubrics_by_round? is false, this still keys seen by [map_id, round], so one map can contribute multiple IDs to state[:all]. The legacy branch in app/models/feedback_response_map.rb dedupes by map_id only, so the new service changes the report output for multi-round assignments.
Proposed fix
- def grouper = ->(r) { [r.map_id, r.round] }
+ def grouper
+ if `@assignment.varying_rubrics_by_round`?
+ ->(r) { [r.map_id, r.round] }
+ else
+ ->(r) { r.map_id }
+ end
+ end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/services/reports/feedback_report.rb` around lines 22 - 40, The
deduplication key currently produced by the grouper (->(r) { [r.map_id, r.round]
}) causes multiple entries per map when `@assignment.varying_rubrics_by_round`? is
false; change the logic so the key used for state[:seen] depends on the
assignment: if varying_rubrics_by_round? then keep [r.map_id, r.round],
otherwise use r.map_id only. Update the grouper or compute key inside accumulate
(referencing grouper, accumulate, varying_rubrics_by_round?, and initial_state)
so that when rubrics do not vary by round a map_id is deduped once and
state[:all] only gets one response id per map.
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>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
app/models/reports/base_report.rb (1)
56-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
find_eachmay silently ignore scoped order in pipelines.This can break correctness for reports whose
sourcedepends on explicit ordering (e.g., first-seen dedup behavior). A low-friction safeguard is to fail fast when order would be ignored.Suggested safeguard
- source.find_each(batch_size: 500) do |row| + source.find_each(batch_size: 500, error_on_ignore: true) do |row| accumulate(state, grouper.call(row), row) endVerify whether any report
sourcemethods use explicit ordering:#!/bin/bash set -euo pipefail rg -n --type=ruby 'def source|\.order\(' app/models/reports -C2 rg -n --type=ruby 'find_each\(' app/models/reports/base_report.rb -C2Expected: if ordered sources exist, current
find_eachpath needs explicit handling to avoid silent order loss.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eafec4d5-6bba-4953-8e77-d50a996673ff
📒 Files selected for processing (11)
app/models/Item.rbapp/models/answer.rbapp/models/answer_tag.rbapp/models/assignment_questionnaire.rbapp/models/feedback_response_map.rbapp/models/reports/answer_tagging_report.rbapp/models/reports/base_report.rbapp/models/reports/review_report.rbapp/models/response.rbapp/models/review_response_map.rbapp/models/teams_user.rb
✅ Files skipped from review due to trivial changes (1)
- app/models/feedback_response_map.rb
📜 Review details
🧰 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/assignment_questionnaire.rbapp/models/answer.rbapp/models/teams_user.rbapp/models/Item.rbapp/models/answer_tag.rbapp/models/review_response_map.rbapp/models/response.rbapp/models/reports/base_report.rbapp/models/reports/review_report.rbapp/models/reports/answer_tagging_report.rb
🪛 ast-grep (0.42.3)
app/models/reports/answer_tagging_report.rb
[warning] 130-130: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: scope.where('LENGTH(comments) > ?', @deployment.answer_length_threshold)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🪛 RuboCop (1.86.2)
app/models/assignment_questionnaire.rb
[convention] 7-7: Use the lambda method for multiline lambdas.
(Style/Lambda)
[convention] 8-8: Line is too long. [121/120]
(Layout/LineLength)
app/models/answer.rb
[convention] 7-7: Inconsistent indentation detected.
(Layout/IndentationConsistency)
app/models/Item.rb
[convention] 9-9: Line is too long. [148/120]
(Layout/LineLength)
app/models/answer_tag.rb
[convention] 10-10: Unnecessary spacing detected.
(Layout/ExtraSpacing)
app/models/response.rb
[convention] 9-9: Use the lambda method for multiline lambdas.
(Style/Lambda)
[convention] 66-66: Missing space after #.
(Layout/LeadingCommentSpace)
app/models/reports/review_report.rb
[convention] 59-59: Pass &:reviewer_id as an argument to lambda instead of a block.
(Style/SymbolProc)
[convention] 109-121: Assignment Branch Condition size for accumulate is too high. [<10, 27, 8> 29.88/20]
(Metrics/AbcSize)
[convention] 109-121: Cyclomatic complexity for accumulate is too high. [9/7]
(Metrics/CyclomaticComplexity)
[convention] 109-121: Perceived complexity for accumulate is too high. [9/8]
(Metrics/PerceivedComplexity)
[convention] 148-159: Assignment Branch Condition size for accumulate is too high. [<8, 24, 8> 26.53/20]
(Metrics/AbcSize)
[convention] 148-159: Cyclomatic complexity for accumulate is too high. [9/7]
(Metrics/CyclomaticComplexity)
[convention] 148-159: Perceived complexity for accumulate is too high. [9/8]
(Metrics/PerceivedComplexity)
app/models/reports/answer_tagging_report.rb
[convention] 74-74: Pass &:user_id as an argument to lambda instead of a block.
(Style/SymbolProc)
[convention] 79-106: Assignment Branch Condition size for accumulate is too high. [<11, 24, 6> 27.07/20]
(Metrics/AbcSize)
[convention] 131-131: Modifier form of if makes the line too long.
(Style/IfUnlessModifier)
[convention] 131-131: Line is too long. [129/120]
(Layout/LineLength)
[convention] 145-145: Align .submitted_review_responses_for with Response on line 144.
(Layout/MultilineMethodCallIndentation)
[convention] 146-146: Align .pluck with Response on line 144.
(Layout/MultilineMethodCallIndentation)
[convention] 165-182: Assignment Branch Condition size for run is too high. [<13, 23, 5> 26.89/20]
(Metrics/AbcSize)
[convention] 175-175: Line is too long. [138/120]
(Layout/LineLength)
[convention] 177-177: Line is too long. [126/120]
(Layout/LineLength)
🔇 Additional comments (12)
app/models/answer_tag.rb (1)
3-14: LGTM!app/models/Item.rb (1)
9-9: LGTM!app/models/answer.rb (1)
7-7: LGTM!app/models/assignment_questionnaire.rb (1)
7-9: LGTM!app/models/response.rb (1)
9-14: LGTM!Also applies to: 36-36, 66-66
app/models/teams_user.rb (1)
7-7: LGTM!app/models/reports/review_report.rb (3)
13-28: LGTM!
30-47: LGTM!
88-122: LGTM!app/models/reports/answer_tagging_report.rb (3)
23-54: LGTM!
61-72: LGTM!
108-154: LGTM!
| reviewers = AssignmentParticipant.where(id: distinct_reviewer_ids, parent_id: assignment_id) | ||
| reviewers.sort_by { |r| r.user&.full_name.to_s.downcase } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type=ruby 'class AssignmentParticipant|belongs_to :user' app/models -C2
rg -n --type=ruby 'def self.review_response_report|AssignmentParticipant\.where|sort_by \{ \|r\| r\.user' app/models/review_response_map.rb -C2Repository: expertiza/reimplementation-back-end
Length of output: 3576
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact method and surrounding code
sed -n '1,120p' app/models/review_response_map.rb
# Confirm the association definition on AssignmentParticipant
sed -n '1,120p' app/models/assignment_participant.rbRepository: expertiza/reimplementation-back-end
Length of output: 2822
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' app/models/participant.rb
sed -n '1,200p' app/models/assignment_participant.rbRepository: expertiza/reimplementation-back-end
Length of output: 2712
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type=ruby 'review_response_report\(' app -S
rg -n --type=ruby 'includes\(:user\)|preload\(:user\)' app/models app/controllers app/services -SRepository: expertiza/reimplementation-back-end
Length of output: 370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Call sites for review_response_report
rg -n --type:ruby 'review_response_report\(' app -S
# 2) Inspect ReviewAggregator for eager loading/default_scope
rg -n --type=ruby 'module ReviewAggregator|default_scope|includes\(:user\)|preload\(:user\)' app/models -S
fd ReviewAggregator app/models -tf || trueRepository: expertiza/reimplementation-back-end
Length of output: 515
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Call sites
rg -n -S 'review_response_report\(' app
# Find ReviewAggregator definition and inspect it for eager-loading/default_scope
fd -t f 'review_aggregator*|ReviewAggregator*' app/models app -a || true
rg -n -S 'module\s+ReviewAggregator|class\s+ReviewAggregator' app/models app || true
rg -n -S 'default_scope|includes\(:user\)|preload\(:user\)' app/models/app -S || trueRepository: expertiza/reimplementation-back-end
Length of output: 494
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,200p' app/models/concerns/review_aggregator.rb
rg -n -S 'includes\(:user\)|preload\(:user\)|eager_load|joins\(:user\)' app/models/concerns/review_aggregator.rb app/models -SRepository: expertiza/reimplementation-back-end
Length of output: 732
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -S 'review_response_report\(' . || true
rg -n -S 'review_response_report' . || trueRepository: expertiza/reimplementation-back-end
Length of output: 255
Preload user before sorting to avoid N+1 queries.
File: app/models/review_response_map.rb (Lines: 30-31)
review_response_report loads AssignmentParticipant records without eager loading :user, then sorts via reviewers.sort_by { |r| r.user... }, which will fetch user per row (since AssignmentParticipant has belongs_to :user).
Suggested fix
- reviewers = AssignmentParticipant.where(id: distinct_reviewer_ids, parent_id: assignment_id)
+ reviewers = AssignmentParticipant.includes(:user).where(id: distinct_reviewer_ids, parent_id: assignment_id)
reviewers.sort_by { |r| r.user&.full_name.to_s.downcase }📝 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.
| reviewers = AssignmentParticipant.where(id: distinct_reviewer_ids, parent_id: assignment_id) | |
| reviewers.sort_by { |r| r.user&.full_name.to_s.downcase } | |
| reviewers = AssignmentParticipant.includes(:user).where(id: distinct_reviewer_ids, parent_id: assignment_id) | |
| reviewers.sort_by { |r| r.user&.full_name.to_s.downcase } |
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>
|
🚨 RSpec Tests Report |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
app/controllers/reports_controller.rb (1)
36-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop exposing internal exception messages in API responses.
Line 37 returns
e.messageverbatim, which can leak internals. Log server-side and return a generic 500 payload.Proposed fix
rescue ActiveRecord::RecordNotFound render json: { error: 'Assignment not found' }, status: :not_found rescue StandardError => e - render json: { error: e.message }, status: :internal_server_error + Rails.logger.error("[ReportsController#fetch_response_report] #{e.class}: #{e.message}") + render json: { error: 'Internal server error' }, status: :internal_server_error endAs per coding guidelines,
app/controllers/**/*.rb: "Review authorization, strong parameters, HTTP status codes, and response shape consistency."
🧹 Nitpick comments (1)
app/models/reports/answer_tagging_report.rb (1)
32-41: ⚡ Quick winAvoid reloading team memberships for every deployment.
TeamsUser.for_assignment(...).includes(:user).group_by(&:team_id)is recomputed in eachDeploymentReducerinstance even though assignment is constant across this run. Precompute once inAnswerTaggingReport#runand pass it down.Proposed refactor
def run per_deployment = {} + users_by_team = TeamsUser + .for_assignment(`@reportable.id`) + .includes(:user) + .group_by(&:team_id) # The coordinator runs one DeploymentReducer per TagPromptDeployment, # then passes the results to UserSummaryReducer for cross-deployment totals. TagPromptDeployment .where(assignment_id: `@reportable.id`) .includes(:tag_prompt, :questionnaire) .each do |deployment| - per_deployment[deployment.id] = DeploymentReducer.new(`@reportable`, deployment).run + per_deployment[deployment.id] = + DeploymentReducer.new(`@reportable`, deployment, users_by_team: users_by_team).run end- def initialize(reportable, deployment) + def initialize(reportable, deployment, users_by_team:) `@reportable` = reportable `@deployment` = deployment `@item_ids` = Item.for_questionnaire_and_type(deployment.questionnaire_id, deployment.question_type).pluck(:id) - `@users_by_team` = TeamsUser.for_assignment(deployment.assignment_id).includes(:user).group_by(&:team_id) + `@users_by_team` = users_by_team endAs per coding guidelines, "app/models/**/*.rb: Focus on ... data integrity, and query efficiency."
Also applies to: 69-74
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 14cd511c-1e7e-4427-9da1-5bcce3aeb3bd
📒 Files selected for processing (11)
app/controllers/reports_controller.rbapp/models/answer.rbapp/models/reports/answer_tagging_report.rbapp/models/reports/base_report.rbapp/models/reports/bookmark_rating_report.rbapp/models/reports/feedback_report.rbapp/models/reports/review_report.rbapp/models/reports/teammate_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/reports/feedback_report.rbapp/models/reports/base_report.rbapp/models/reports/teammate_review_report.rbapp/models/reports/bookmark_rating_report.rbapp/models/answer.rbapp/models/review_response_map.rbapp/models/reports/review_report.rbapp/models/reports/answer_tagging_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 (6)
app/models/response.rb (1)
app/models/review_response_map.rb (1)
reviewer_assignment(9-11)
app/models/reports/feedback_report.rb (1)
app/models/reports/review_report.rb (5)
source(51-55)state_key_for(68-68)initial_state(70-70)accumulate(72-83)accumulate(98-106)
app/models/reports/teammate_review_report.rb (1)
app/models/reports/review_report.rb (3)
run(26-45)ReviewReport(17-132)accumulate(72-83)
app/models/reports/bookmark_rating_report.rb (2)
app/models/reports/review_report.rb (4)
source(51-55)state_key_for(68-68)initial_state(70-70)accumulate(72-83)app/models/reports/feedback_report.rb (3)
initial_state(24-26)accumulate(28-42)finalize(44-62)
app/models/review_response_map.rb (1)
app/models/response.rb (2)
reviewer_assignment(30-30)questionnaire(29-31)
app/models/reports/review_report.rb (1)
app/models/response.rb (1)
maximum_score(112-120)
🪛 RuboCop (1.86.2)
app/models/response.rb
[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] 30-30: Redundant self detected.
(Style/RedundantSelf)
app/models/reports/feedback_report.rb
[convention] 79-79: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 80-80: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 81-81: 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)
app/controllers/reports_controller.rb
[convention] 9-9: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
app/models/reports/teammate_review_report.rb
[convention] 37-37: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 38-38: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 39-39: 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)
[convention] 43-43: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 66-86: Assignment Branch Condition size for accumulate is too high. [<7, 27, 7> 28.76/20]
(Metrics/AbcSize)
[convention] 66-86: Cyclomatic complexity for accumulate is too high. [8/7]
(Metrics/CyclomaticComplexity)
[convention] 82-82: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 83-83: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 99-99: Pass &:user_id as an argument to lambda instead of a block.
(Style/SymbolProc)
config/routes.rb
[convention] 3-223: Block has too many lines. [195/120]
(Metrics/BlockLength)
[convention] 4-5: Extra empty line detected at block body beginning.
(Layout/EmptyLinesAroundBlockBody)
[convention] 12-12: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 13-18: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 19-25: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 26-41: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 28-28: Space missing after comma.
(Layout/SpaceAfterComma)
[convention] 29-29: Space missing after comma.
(Layout/SpaceAfterComma)
[convention] 30-30: Space missing after comma.
(Layout/SpaceAfterComma)
[convention] 31-31: Space missing after comma.
(Layout/SpaceAfterComma)
[convention] 33-33: Space missing after comma.
(Layout/SpaceAfterComma)
[convention] 34-34: Space missing after comma.
(Layout/SpaceAfterComma)
[convention] 39-39: Space missing after comma.
(Layout/SpaceAfterComma)
[convention] 43-48: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 43-43: Use %i or %I for an array of symbols.
(Style/SymbolArray)
[convention] 49-54: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 56-63: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 65-70: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 72-78: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 75-75: Space missing after colon.
(Layout/SpaceAfterColon)
[convention] 76-76: Space missing after colon.
(Layout/SpaceAfterColon)
[convention] 79-79: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 80-80: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 81-98: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 82-82: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 98-98: end at 98, 6 is not aligned with resources :review_mappings, only: [] do at 81, 5.
(Layout/BlockAlignment)
[convention] 100-110: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 105-109: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 109-109: end at 109, 8 is not aligned with member do at 105, 9.
(Layout/BlockAlignment)
[convention] 111-111: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 112-121: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 123-133: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 135-140: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 142-148: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 150-155: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 157-166: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 168-175: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 170-170: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 173-173: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 177-187: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 188-197: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 197-197: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 198-202: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 204-214: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 205-205: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 215-219: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 220-222: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 221-221: Use %i or %I for an array of symbols.
(Style/SymbolArray)
[convention] 223-223: Final newline missing.
(Layout/TrailingEmptyLines)
app/models/reports/bookmark_rating_report.rb
[convention] 16-16: Unnecessary spacing detected.
(Layout/ExtraSpacing)
[convention] 16-16: Pass &:reviewee_id as an argument to lambda instead of a block.
(Style/SymbolProc)
app/models/answer.rb
[convention] 9-18: Inconsistent indentation detected.
(Layout/IndentationConsistency)
[convention] 9-9: Use the lambda method for multiline lambdas.
(Style/Lambda)
[convention] 11-11: Align .where with joins(response: :response_map) on line 10.
(Layout/MultilineMethodCallIndentation)
[convention] 12-12: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 13-13: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
app/models/review_response_map.rb
[convention] 10-10: Redundant return detected.
(Style/RedundantReturn)
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)
app/models/reports/answer_tagging_report.rb
[convention] 45-45: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 72-72: Line is too long. [122/120]
(Layout/LineLength)
[convention] 84-124: Assignment Branch Condition size for finalize is too high. [<18, 38, 6> 42.47/20]
(Metrics/AbcSize)
[convention] 84-124: Method has too many lines. [29/20]
(Metrics/MethodLength)
[convention] 98-98: Line is too long. [122/120]
(Layout/LineLength)
[convention] 106-106: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 107-107: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 108-108: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 109-109: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 110-110: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 111-111: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 112-112: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 118-118: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 119-119: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 120-120: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 122-122: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 150-150: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 156-156: Pass &:team_id as an argument to lambda instead of a block.
(Style/SymbolProc)
[convention] 189-189: Line is too long. [126/120]
(Layout/LineLength)
[convention] 192-192: Pass &:user_id as an argument to lambda instead of a block.
(Style/SymbolProc)
🔇 Additional comments (13)
app/models/answer.rb (1)
8-18: LGTM!app/models/review_response_map.rb (1)
3-3: LGTM!Also applies to: 8-16
app/models/response.rb (1)
9-21: LGTM!Also applies to: 23-27, 30-31, 33-43, 45-85, 88-89, 121-121
app/models/reports/base_report.rb (1)
4-5: LGTM!Also applies to: 15-16, 20-40, 58-59, 63-83, 88-88
app/models/reports/bookmark_rating_report.rb (1)
12-26: LGTM!app/models/reports/teammate_review_report.rb (1)
34-87: LGTM!app/models/reports/review_report.rb (4)
26-45: LGTM!
47-56: LGTM!
72-83: LGTM!
91-107: LGTM!app/models/reports/feedback_report.rb (3)
15-20: LGTM!
22-26: LGTM!
44-62: LGTM!
| assignment_id = params[:assignment_id] | ||
| type = params[:type] || 'basic' | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="app/controllers/reports_controller.rb"
echo "== File existence =="
ls -l "$FILE" || true
echo
echo "== Show lines 1-120 with numbers =="
nl -ba "$FILE" | sed -n '1,120p'
echo
echo "== Search for fetch_response_report and rescue blocks =="
rg -n "def fetch_response_report|rescue|status:|render json:" "$FILE" || true
echo
echo "== Show full method(s) that contain assignment_id/type =="
rg -n "assignment_id|params\[:assignment_id\]|params\[:type\]" "$FILE" || trueRepository: expertiza/reimplementation-back-end
Length of output: 286
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="app/controllers/reports_controller.rb"
echo "== Show lines 1-140 =="
cat -n "$FILE" | sed -n '1,140p'
echo
echo "== Search for fetch_response_report definition and params usage =="
rg -n "def fetch_response_report|assignment_id|params\[:assignment_id\]|params\[:type\]" "$FILE" || true
echo
echo "== Search for rescue blocks / status codes / render json =="
rg -n "rescue|status:|render json:" "$FILE" || true
echo
echo "== Print method containing assignment_id/type usage (best-effort) =="
# Grab from the first occurrence of assignment_id to the end of file (small file expected)
start_line=$(rg -n "assignment_id|params\[:assignment_id\]" "$FILE" | head -n1 | cut -d: -f1 || true)
if [ -n "${start_line:-}" ]; then
end_line=$((start_line+80))
cat -n "$FILE" | sed -n "${start_line},${end_line}p"
fiRepository: expertiza/reimplementation-back-end
Length of output: 3494
Enforce strong params for assignment_id/type and return 400 for missing required input
fetch_response_report reads params[:assignment_id] / params[:type] directly (lines 21-22) and then calls Assignment.find(assignment_id); missing/invalid assignment_id is handled by the ActiveRecord::RecordNotFound rescue (lines 34-35) and returns :not_found (404) instead of a client error (400). Add permit/require for assignment_id and rescue ActionController::ParameterMissing to return :bad_request.
Proposed fix
def fetch_response_report
- assignment_id = params[:assignment_id]
- type = params[:type] || 'basic'
+ request_params = params.permit(:assignment_id, :type)
+ assignment_id = request_params.require(:assignment_id)
+ type = request_params[:type].presence || 'basic'
@@
+ rescue ActionController::ParameterMissing => e
+ render json: { error: e.message }, status: :bad_request
rescue ActiveRecord::RecordNotFound
render json: { error: 'Assignment not found' }, status: :not_found| matching_tags = tagging_stats.fetch(user_id, []).select { |tag| taggable_answer_ids.include?(tag[:answer_id]) } | ||
| cnt_tagged = matching_tags.size | ||
| cnt_not_tagged = cnt_taggable - cnt_tagged | ||
|
|
||
| tag_times_in_order = matching_tags.map { |tag| tag[:updated_at] }.sort | ||
| intervals_between_tags = tag_times_in_order.each_cons(2).map { |earlier, later| later - earlier } | ||
|
|
||
| { | ||
| user_id: user_id, | ||
| name: info[:name], | ||
| full_name: info[:full_name], | ||
| percentage: cnt_taggable.zero? ? '0.0' : format('%.1f', cnt_tagged.to_f / cnt_taggable * 100), | ||
| cnt_tagged: cnt_tagged, | ||
| cnt_not_tagged: cnt_not_tagged, |
There was a problem hiding this comment.
Count unique tagged answers, not raw tag rows.
Line 99 currently counts tag records, so repeated tags on the same answer can inflate cnt_tagged and distort cnt_not_tagged/percentage. Count unique answer_ids for progress metrics.
Proposed fix
- matching_tags = tagging_stats.fetch(user_id, []).select { |tag| taggable_answer_ids.include?(tag[:answer_id]) }
- cnt_tagged = matching_tags.size
- cnt_not_tagged = cnt_taggable - cnt_tagged
+ matching_tags = tagging_stats.fetch(user_id, []).select do |tag|
+ taggable_answer_ids.include?(tag[:answer_id])
+ end
+ cnt_tagged_answers = matching_tags.map { |tag| tag[:answer_id] }.uniq
+ cnt_tagged = cnt_tagged_answers.size
+ cnt_not_tagged = cnt_taggable - cnt_taggedAs per coding guidelines, "app/models/**/*.rb: Focus on ... data integrity, and query efficiency."
📝 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.
| matching_tags = tagging_stats.fetch(user_id, []).select { |tag| taggable_answer_ids.include?(tag[:answer_id]) } | |
| cnt_tagged = matching_tags.size | |
| cnt_not_tagged = cnt_taggable - cnt_tagged | |
| tag_times_in_order = matching_tags.map { |tag| tag[:updated_at] }.sort | |
| intervals_between_tags = tag_times_in_order.each_cons(2).map { |earlier, later| later - earlier } | |
| { | |
| user_id: user_id, | |
| name: info[:name], | |
| full_name: info[:full_name], | |
| percentage: cnt_taggable.zero? ? '0.0' : format('%.1f', cnt_tagged.to_f / cnt_taggable * 100), | |
| cnt_tagged: cnt_tagged, | |
| cnt_not_tagged: cnt_not_tagged, | |
| matching_tags = tagging_stats.fetch(user_id, []).select do |tag| | |
| taggable_answer_ids.include?(tag[:answer_id]) | |
| end | |
| cnt_tagged_answers = matching_tags.map { |tag| tag[:answer_id] }.uniq | |
| cnt_tagged = cnt_tagged_answers.size | |
| cnt_not_tagged = cnt_taggable - cnt_tagged | |
| tag_times_in_order = matching_tags.map { |tag| tag[:updated_at] }.sort | |
| intervals_between_tags = tag_times_in_order.each_cons(2).map { |earlier, later| later - earlier } | |
| { | |
| user_id: user_id, | |
| name: info[:name], | |
| full_name: info[:full_name], | |
| percentage: cnt_taggable.zero? ? '0.0' : format('%.1f', cnt_tagged.to_f / cnt_taggable * 100), | |
| cnt_tagged: cnt_tagged, | |
| cnt_not_tagged: cnt_not_tagged, |
🧰 Tools
🪛 RuboCop (1.86.2)
[convention] 98-98: Line is too long. [122/120]
(Layout/LineLength)
[convention] 106-106: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 107-107: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 108-108: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 109-109: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 110-110: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 111-111: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
| if @reportable.varying_rubrics_by_round? | ||
| case response.round | ||
| when 1 then state[:round_1] << response.id | ||
| when 2 then state[:round_2] << response.id | ||
| when 3 then state[:round_3] << response.id | ||
| end | ||
| else | ||
| state[:all] << response.id | ||
| end |
There was a problem hiding this comment.
Responses with nil or unexpected round values are silently dropped.
When varying_rubrics_by_round? is true, responses with round values other than 1, 2, or 3 (including nil) are added to the seen set but never accumulated into any bucket. This could silently exclude valid data.
Consider adding a fallback or logging for unexpected values.
🛡️ Proposed fix with fallback bucket or default
if `@reportable.varying_rubrics_by_round`?
case response.round
when 1 then state[:round_1] << response.id
when 2 then state[:round_2] << response.id
when 3 then state[:round_3] << response.id
+ else
+ Rails.logger.warn("FeedbackReport: unexpected round=#{response.round.inspect} for response #{response.id}")
end
else
state[:all] << response.id
end| def fetch_authors | ||
| teams = AssignmentTeam.includes(:users).where(parent_id: @reportable.id) | ||
| teams.flat_map do |team| | ||
| team.users.filter_map do |user| | ||
| AssignmentParticipant.find_by(parent_id: @reportable.id, user_id: user.id) | ||
| end | ||
| end | ||
| end | ||
|
|
||
| def format_participant(p) | ||
| return {} unless p | ||
|
|
||
| { | ||
| id: p.id, | ||
| user_id: p.user_id, | ||
| name: p.user&.name, | ||
| full_name: p.user&.full_name | ||
| } | ||
| end |
There was a problem hiding this comment.
N+1 query pattern: individual find_by calls inside nested loop.
fetch_authors issues one AssignmentParticipant.find_by query per user per team. For an assignment with many teams/users, this creates O(n) queries. Additionally, the returned participants don't preload :user, causing another N+1 when format_participant accesses p.user&.name.
Batch-load participants with a single query and eager-load users.
🐛 Proposed fix
def fetch_authors
- teams = AssignmentTeam.includes(:users).where(parent_id: `@reportable.id`)
- teams.flat_map do |team|
- team.users.filter_map do |user|
- AssignmentParticipant.find_by(parent_id: `@reportable.id`, user_id: user.id)
- end
- end
+ user_ids = AssignmentTeam
+ .joins(:teams_users)
+ .where(parent_id: `@reportable.id`)
+ .pluck('teams_users.user_id')
+ .uniq
+
+ AssignmentParticipant
+ .includes(:user)
+ .where(parent_id: `@reportable.id`, user_id: user_ids)
end🧰 Tools
🪛 RuboCop (1.86.2)
[convention] 79-79: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 80-80: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 81-81: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
| class AvgRangesReducer < BaseReport | ||
| def source | ||
| AssignmentTeam | ||
| .where(parent_id: @reportable.id) | ||
| .includes(review_mappings: { responses: { scores: :item } }) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for aggregate_review_grade implementation and trace its calls
ast-grep --pattern 'def aggregate_review_grade
$$$
end'
# Also check for maximum_score calls in related code
rg -n 'maximum_score' --type ruby -C 3Repository: expertiza/reimplementation-back-end
Length of output: 10973
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show AvgRangesReducer and nearby reducers in review_report.rb
sed -n '80,170p' app/models/reports/review_report.rb | cat -n
# 2) Inspect AssignmentTeam#compute_average_review_score implementation
rg -n "def compute_average_review_score" app/models/assignment_team.rb
python3 - <<'PY'
import re, itertools, pathlib
p=pathlib.Path("app/models/assignment_team.rb")
text=p.read_text().splitlines()
for i,line in enumerate(text, start=1):
if "def compute_average_review_score" in line:
start=max(1,i-10); end=min(len(text), i+160)
for j in range(start, end+1):
print(f"{j:6d}:{text[j-1]}")
break
PY
# 3) Inspect Response#maximum_score body (to see how questionnaire is accessed)
python3 - <<'PY'
import pathlib
p=pathlib.Path("app/models/response.rb")
lines=p.read_text().splitlines()
for i,line in enumerate(lines, start=1):
if re.search(r"def maximum_score\b", line):
start=max(1,i-10); end=min(len(lines), i+120)
for j in range(start,end+1):
print(f"{j:6d}:{lines[j-1]}")
break
PY
# 4) Inspect ScorableHelper maximum_score implementation (if used by reducer/report)
sed -n '1,140p' app/helpers/scorable_helper.rb | cat -n
# 5) Find the code path that does "questionnaire.max_question_score" specifically
rg -n "max_question_score" app/models app/helpers | head -n 50Repository: expertiza/reimplementation-back-end
Length of output: 2530
🏁 Script executed:
#!/bin/bash
set +euo pipefail
# 1) Find compute_average_review_score and show around it
rg -n "def compute_average_review_score" app/models/assignment_team.rb
python3 - <<'PY'
import pathlib, re
p=pathlib.Path("app/models/assignment_team.rb")
lines=p.read_text().splitlines()
for i,l in enumerate(lines, start=1):
if "def compute_average_review_score" in l:
start=max(1,i-20); end=min(len(lines), i+220)
for j in range(start,end+1):
print(f"{j:6d}:{lines[j-1]}")
break
PY
# 2) Show aggregate_review_grade implementation around first occurrence
rg -n "def aggregate_review_grade" app/models/assignment_team.rb
python3 - <<'PY'
import pathlib
p=pathlib.Path("app/models/assignment_team.rb")
lines=p.read_text().splitlines()
idxs=[i for i,l in enumerate(lines, start=1) if "def aggregate_review_grade" in l]
for k,i in enumerate(idxs[:2], start=1):
print("\n--- aggregate_review_grade block",k,"at line",i,"---")
start=max(1,i-20); end=min(len(lines), i+220)
for j in range(start,end+1):
print(f"{j:6d}:{lines[j-1]}")
PY
# 3) Show Response#maximum_score implementation
python3 - <<'PY'
import pathlib, re
p=pathlib.Path("app/models/response.rb")
lines=p.read_text().splitlines()
for i,line in enumerate(lines, start=1):
if re.search(r"def maximum_score\b", line):
start=max(1,i-20); end=min(len(lines), i+260)
for j in range(start,end+1):
print(f"{j:6d}:{lines[j-1]}")
break
PY
# 4) Show ScorableHelper maximum_score implementation
python3 - <<'PY'
import pathlib, re
p=pathlib.Path("app/helpers/scorable_helper.rb")
lines=p.read_text().splitlines()
for i,line in enumerate(lines, start=1):
if re.search(r"def maximum_score\b", line):
start=max(1,i-20); end=min(len(lines), i+260)
for j in range(start,end+1):
print(f"{j:6d}:{lines[j-1]}")
# likely first occurrence; continue searching for next def maximum_score after that
# but keep minimal
break
PY
# 5) Find "questionnaire.max_question_score" usage
rg -n "questionnaire\.max_question_score" app/models app/helpersRepository: expertiza/reimplementation-back-end
Length of output: 12594
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# AssignmentTeam: locate compute_average_review_score and show its body
rg -n "def compute_average_review_score" app/models/assignment_team.rb
python3 - <<'PY'
import pathlib,re
p=pathlib.Path("app/models/assignment_team.rb")
lines=p.read_text().splitlines()
for i,l in enumerate(lines, start=1):
if "def compute_average_review_score" in l:
start=max(1,i-20); end=min(len(lines), i+260)
for j in range(start,end+1):
print(f"{j:6d}:{lines[j-1]}")
break
PY
# ScorableHelper: show maximum_score + questionnaire_by_answer
rg -n "def maximum_score|def questionnaire_by_answer" app/helpers/scorable_helper.rb
python3 - <<'PY'
import pathlib,re
p=pathlib.Path("app/helpers/scorable_helper.rb")
lines=p.read_text().splitlines()
# print around first def questionnaire_by_answer and surrounding maximum_score
for i,l in enumerate(lines, start=1):
if "def maximum_score" in l:
start=max(1,i-10); end=min(len(lines), i+110)
for j in range(start,end+1):
print(f"{j:6d}:{lines[j-1]}")
break
PY
# Review report: show header comment about eager-loading
sed -n '1,120p' app/models/reports/review_report.rb | cat -n
# Response model: show maximum_score method around its definition
rg -n "def maximum_score" app/models/response.rb
python3 - <<'PY'
import pathlib,re
p=pathlib.Path("app/models/response.rb")
lines=p.read_text().splitlines()
for i,l in enumerate(lines, start=1):
if re.search(r"def maximum_score\b", l):
start=max(1,i-25); end=min(len(lines), i+120)
for j in range(start,end+1):
print(f"{j:6d}:{lines[j-1]}")
break
PYRepository: expertiza/reimplementation-back-end
Length of output: 61
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find where compute_average_review_score is defined
rg -n "def compute_average_review_score" app/models
# 2) Also find usages that might show how it's called / what it returns
rg -n "compute_average_review_score" app/models
# 3) Locate any module/concern that is likely included by AssignmentTeam
rg -n "ReviewAggregator|include .*Aggregator|include .*Scorable|compute_average_review_score" app/models/assignment_team.rb
# 4) Inspect the first definition location(s) of compute_average_review_score
python3 - <<'PY'
import subprocess, re, os, textwrap, pathlib, json
import sys
res=subprocess.check_output(["bash","-lc","rg -n \"def compute_average_review_score\" app/models"]).decode()
print(res)
PYRepository: expertiza/reimplementation-back-end
Length of output: 856
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Review aggregator: how it computes average and calls response scoring
sed -n '1,220p' app/models/concerns/review_aggregator.rb | cat -n
# Show full Response#maximum_score method body (around it)
python3 - <<'PY'
import pathlib,re
p=pathlib.Path("app/models/response.rb")
lines=p.read_text().splitlines()
idx=None
for i,l in enumerate(lines, start=1):
if re.search(r"def maximum_score\b", l):
idx=i; break
if idx is None:
raise SystemExit("maximum_score not found")
start=max(1,idx-20); end=min(len(lines), idx+80)
for j in range(start,end+1):
print(f"{j:6d}:{lines[j-1]}")
PY
# Show ScorableHelper maximum_score + questionnaire_by_answer (the part that loads questionnaire)
sed -n '1,130p' app/helpers/scorable_helper.rb | cat -n
# Check whether any eager-load helper exists for item->questionnaire
rg -n "includes\\(.*questionnaire|questionnaire_by_answer\\(|Item\\.find\\(.*\\)\\.questionnaire" app/models app/helpersRepository: expertiza/reimplementation-back-end
Length of output: 5665
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find how review mappings aggregate reviewer scores
rg -n "aggregate_reviewers_score" app/models
# Inspect that method definition
python3 - <<'PY'
import pathlib, re, itertools
root=pathlib.Path("app/models")
# find file containing aggregate_reviewers_score
paths=[]
for p in root.rglob("*.rb"):
if "aggregate_reviewers_score" in p.read_text(errors="ignore"):
paths.append(p)
print("FILES:", paths)
PY
# Show definition(s) with context
rg -n "def aggregate_reviewers_score" app/models -S
# Inspect Response#maximum_score body around where questionnaire is set
python3 - <<'PY'
import pathlib, re
p=pathlib.Path("app/models/response.rb")
lines=p.read_text().splitlines()
for i,l in enumerate(lines, start=1):
if re.search(r"def maximum_score\b", l):
start=max(1,i-30); end=min(len(lines), i+160)
for j in range(start,end+1):
print(f"{j:6d}:{lines[j-1]}")
break
PY
# Inspect Response#questionnaire_by_answer call site around line ~70-110
sed -n '60,140p' app/models/response.rb | cat -n
# Inspect AssignmentTeam review_mappings association to ensure it maps to ReviewResponseMap
rg -n "has_many :review_mappings|review_mappings" app/models/assignment_team.rb | head -n 50Repository: expertiza/reimplementation-back-end
Length of output: 5596
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect ResponseMap file for includes and relevant methods (questionnaire_by_answer / maximum_score / aggregate_reviewers_score)
sed -n '1,220p' app/models/response_map.rb | cat -n
# Inspect Response model around questionnaire/questionnaire_by_answer usage
sed -n '1,200p' app/models/response.rb | cat -n
# Check whether ResponseMap includes ScorableHelper (or defines questionnaire_by_answer itself)
rg -n "include .*ScorableHelper|def questionnaire_by_answer|def questionnaire\b" app/models/response_map.rb app/models/response.rb app/helpers/scorable_helper.rbRepository: expertiza/reimplementation-back-end
Length of output: 13792
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Participant model for assignment association
rg -n "class Participant\b|belongs_to :assignment\b|has_many :assignment_questionnaires\b|assignment_questionnaires" app/models/participant*.rb app/models/*participant*.rb || true
# Locate where assignment_questionnaires association is defined (Assignment model)
rg -n "assignment_questionnaires" app/models | head -n 50
# Inspect AssignmentQuestionnaire model for belongs_to :questionnaire
rg -n "class AssignmentQuestionnaire\b" app/models | head -n 20
fd -e rb -p "assignment_questionnaire.rb" app/models 2>/dev/null || true
fd "assignment_questionnaire" app/models -t f | head -n 20
# Read likely model files with cat -n around association lines
for f in app/models/participant.rb app/models/participants.rb app/models/assignment.rb app/models/assignment_questionnaire.rb app/models/*assignment_questionnaire*.rb; do
if [ -f "$f" ]; then
echo "---- $f ----"
rg -n "belongs_to :assignment|assignment_questionnaires|belongs_to :questionnaire" "$f" || true
fi
doneRepository: expertiza/reimplementation-back-end
Length of output: 1621
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '35,90p' app/models/reports/review_report.rb | cat -nRepository: expertiza/reimplementation-back-end
Length of output: 2602
Fix AvgRangesReducer eager-loading to avoid N+1 in Response#maximum_score
AvgRangesReducer eager-loads scores: :item, but Response#maximum_score uses questionnaire.max_question_score where questionnaire comes from reviewer_assignment.assignment_questionnaires.find_by(used_in_round: round).questionnaire. reviewer.assignment.assignment_questionnaires/their questionnaire aren’t eager-loaded here, so scoring can trigger per-response queries (the item: :questionnaire path doesn’t match this call chain).
🐛 Proposed fix
def source
AssignmentTeam
.where(parent_id: `@reportable.id`)
- .includes(review_mappings: { responses: { scores: :item } })
+ .includes(review_mappings: {
+ reviewer: { assignment: { assignment_questionnaires: :questionnaire } },
+ responses: { scores: :item }
+ })
end📝 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.
| class AvgRangesReducer < BaseReport | |
| def source | |
| AssignmentTeam | |
| .where(parent_id: @reportable.id) | |
| .includes(review_mappings: { responses: { scores: :item } }) | |
| end | |
| class AvgRangesReducer < BaseReport | |
| def source | |
| AssignmentTeam | |
| .where(parent_id: `@reportable.id`) | |
| .includes(review_mappings: { | |
| reviewer: { assignment: { assignment_questionnaires: :questionnaire } }, | |
| responses: { scores: :item } | |
| }) | |
| end |
| def state_key_for = ->(teams_user) { teams_user.user_id } | ||
|
|
||
| def initial_state = {} | ||
|
|
||
| def accumulate(state, user_id, _teams_user) | ||
| # teammate_count is the number of TeamsUser rows for this user's team, | ||
| # which equals the number of teammates (including themselves). | ||
| state[user_id][:teammate_count] += 1 if state.key?(user_id) | ||
| end |
There was a problem hiding this comment.
Teammate count is computed per user-row, not per team size.
Line 99 keys by teams_user.user_id, and Line 106 increments once for that user’s membership row. That yields teammate_count ≈ 1 per reviewer (or duplicate membership count), not the number of teammates in the reviewer’s team.
💡 Suggested fix
class TeamSizeReducer < BaseReport
def source
- TeamsUser.for_assignment(`@reportable.id`).includes(:user)
+ TeamsUser.for_assignment(`@reportable.id`).select(:user_id, :team_id)
end
def state_key_for = ->(teams_user) { teams_user.user_id }
def initial_state = {}
- def accumulate(state, user_id, _teams_user)
- # teammate_count is the number of TeamsUser rows for this user's team,
- # which equals the number of teammates (including themselves).
- state[user_id][:teammate_count] += 1 if state.key?(user_id)
+ def accumulate(state, user_id, teams_user)
+ return unless state.key?(user_id)
+
+ `@team_sizes` ||= TeamsUser.for_assignment(`@reportable.id`).group(:team_id).count
+ state[user_id][:teammate_count] = `@team_sizes`[teams_user.team_id].to_i
end
endAs per coding guidelines, app/models/**/*.rb: “Focus on ... data integrity, and query efficiency.”
🧰 Tools
🪛 RuboCop (1.86.2)
[convention] 99-99: Pass &:user_id as an argument to lambda instead of a block.
(Style/SymbolProc)
| resources :reports, only: [] do | ||
| collection do | ||
| post :fetch_response_report | ||
| end | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Request specs mentioning reports endpoint =="
rg -nP -C2 --iglob '**/*_spec.rb' 'fetch_response_report|/reports/fetch_response_report|ReportsController' || true
echo
echo "== Swagger/OpenAPI docs mentioning reports endpoint =="
rg -nP -C2 \
--iglob '**/*swagger*' \
--iglob '**/*openapi*' \
--iglob '**/*.yml' \
--iglob '**/*.yaml' \
'fetch_response_report|/reports/fetch_response_report' || trueRepository: expertiza/reimplementation-back-end
Length of output: 183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Occurrences of fetch_response_report =="
rg -n --hidden --no-ignore --glob '!**/node_modules/**' 'fetch_response_report' . || true
echo
echo "== Occurrences of POST /reports (non-RSpec context) =="
rg -n --hidden --no-ignore --glob '!**/node_modules/**' 'POST\s*/reports' . || true
echo
echo "== Spec files referencing reports endpoint/action =="
rg -n -S --glob '**/*_spec.rb' 'reports.*fetch_response_report|fetch_response_report.*reports|/reports/fetch_response_report' . || true
echo
echo "== OpenAPI/Swagger artifacts (common file types) =="
fd -t f -e rb -e ts -e js -e json -e xml . 'openapi|swagger' 2>/dev/null || true
# List likely doc files and then grep for the route/action
echo
echo "== Swagger/OpenAPI files containing reports/fetch_response_report or fetch_response_report =="
# Use git ls-files if available for consistent file set
if git ls-files >/dev/null 2>&1; then
DOCS=$(git ls-files | rg -i 'swagger|openapi|api-doc|documentation' || true)
printf "%s\n" "$DOCS" | rg -iE '\.ya?ml|\.json' || true | head -n 200
echo
printf "%s\n" "$DOCS" | rg -iE '\.ya?ml|\.json' | xargs -r rg -n -i 'fetch_response_report|/reports/fetch_response_report' || true
fiRepository: expertiza/reimplementation-back-end
Length of output: 1491
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect ReportsController for inline API docs/annotations =="
if [ -f app/controllers/reports_controller.rb ]; then
sed -n '1,120p' app/controllers/reports_controller.rb
fi
echo
echo "== Search request specs for fetch_response_report action/routes =="
rg -n --glob '**/*_spec.rb' \
'fetch_response_report|/reports/fetch_response_report|ReportsController' . || true
echo
echo "== Search for controller/action specs (reports_controller.rb mention) =="
rg -n --glob '**/*_spec.rb' \
'ReportsController|reports_controller' . || true
echo
echo "== Search for OpenAPI/Swagger files (YAML/JSON) referencing the route/action =="
# Find likely spec/doc files first, then grep them
DOC_FILES=$(
(git ls-files 2>/dev/null || true) | \
rg -i 'openapi|swagger|api-doc|documentation' || true
)
if [ -z "${DOC_FILES}" ]; then
echo "(No obvious swagger/openapi doc files found by filename heuristics.)"
else
echo "$DOC_FILES" | rg -i '\.ya?ml|\.json' | head -n 200
echo
echo "Matches in those files:"
echo "$DOC_FILES" | rg -i '\.ya?ml|\.json' | xargs -r rg -n -i \
'fetch_response_report|/reports/fetch_response_report|reports.*fetch_response_report' || true
fi
echo
echo "== Search for OpenAPI/Swagger annotations in rb code =="
rg -n --glob '**/*.rb' \
'openapi|swagger|swagger_ui|rswag|api_doc|`@swagger`|OpenAPI|Swagger' \
. || trueRepository: expertiza/reimplementation-back-end
Length of output: 7167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search swagger/v1/swagger.yaml for fetch_response_report or /reports/fetch... =="
if [ -f swagger/v1/swagger.yaml ]; then
rg -n 'fetch_response_report|/reports/fetch_response_report|fetch response report|fetchResponse' swagger/v1/swagger.yaml || true
echo
echo "== Search swagger/v1/swagger.yaml for any /reports/ paths =="
rg -n '/reports/' swagger/v1/swagger.yaml || true
else
echo "swagger/v1/swagger.yaml not found"
fi
echo
echo "== Search request specs under spec/requests/api/v1 for reports or fetch_response =="
rg -n --glob '**/*_spec.rb' \
'fetch_response_report|fetch_response|/reports/fetch_response_report|ReportsController|reports/' \
spec/requests/api/v1 || true
echo
echo "== Search request specs anywhere for fetch_response_report (broader) =="
rg -n --glob '**/*_spec.rb' \
'fetch_response_report|/reports/fetch_response_report|ReportsController' . || trueRepository: expertiza/reimplementation-back-end
Length of output: 383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== swagger/v1/swagger.yaml size =="
if [ -f swagger/v1/swagger.yaml ]; then
wc -l swagger/v1/swagger.yaml
fi
echo
echo "== Search swagger/v1/swagger.yaml for any 'reports' path keys =="
if [ -f swagger/v1/swagger.yaml ]; then
rg -n '/reports|reports:' swagger/v1/swagger.yaml || true
fi
echo
echo "== Search swagger/v1/swagger.yaml for fetch.*report patterns =="
if [ -f swagger/v1/swagger.yaml ]; then
rg -n -i 'fetch[^\\n]{0,40}report|report[^\\n]{0,40}fetch|fetch_response|fetchresponse' swagger/v1/swagger.yaml || true
fi
echo
echo "== Search request specs for 'reports' route/prefixes =="
rg -n --glob '**/*_spec.rb' 'post\s*\(?[\'"]/reports|/reports' spec/requests || true
echo
echo "== Search request specs for 'reports_controller' usage =="
rg -n --glob '**/*_spec.rb' 'reports_controller|ReportsController' spec/requests || trueRepository: expertiza/reimplementation-back-end
Length of output: 419
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== routes.rb context around line 198-202 =="
nl -ba config/routes.rb | sed -n '160,240p'
echo
echo "== Search swagger/v1/swagger.yaml for fetch_response_report (case-insensitive) =="
rg -n -i 'fetch[_ -]*response[_ -]*report' swagger/v1/swagger.yaml || true
echo
echo "== Search swagger/v1/swagger.yaml for 'reports' path entries =="
rg -n '/reports(/|$)|reports:' swagger/v1/swagger.yaml || true
echo
echo "== Search request specs for fetch_response_report =="
rg -n --glob '**/*_spec.rb' 'fetch_response_report|fetch response report|/reports/fetch_response_report' spec || true
echo
echo "== Search request specs for any /reports usage =="
rg -n --glob '**/*_spec.rb' '/reports' spec/requests || true
echo
echo "== Search request specs for reports_controller / ReportsController usage =="
rg -n --glob '**/*_spec.rb' 'ReportsController|reports_controller' spec/requests || true
echo
echo "== Search request specs for 'reports' keyword (first 50 matches) =="
rg -n --glob '**/*_spec.rb' '\breports\b' spec/requests | head -n 50 || trueRepository: expertiza/reimplementation-back-end
Length of output: 184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== routes.rb context around line 198-202 (using cat -n) =="
cat -n config/routes.rb | sed -n '175,230p'
echo
echo "== Search swagger/v1/swagger.yaml for fetch_response_report =="
rg -n -i 'fetch[_ -]*response[_ -]*report' swagger/v1/swagger.yaml || true
echo
echo "== Search swagger/v1/swagger.yaml for reports paths =="
rg -n '/reports(/|reports:' swagger/v1/swagger.yaml || true
echo
echo "== Search request specs for fetch_response_report =="
rg -n --glob '**/*_spec.rb' 'fetch_response_report|/reports/fetch_response_report' spec || true
echo
echo "== Search request specs for ReportsController =="
rg -n --glob '**/*_spec.rb' 'ReportsController|reports_controller' spec/requests || true
echo
echo "== Search request specs for any reports prefix =="
rg -n --glob '**/*_spec.rb' '/reports|post\s*\(?[\'"]/reports' spec/requests || trueRepository: expertiza/reimplementation-back-end
Length of output: 2571
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== swagger/v1/swagger.yaml: fetch_response_report matches =="
rg -n -i 'fetch_response_report' swagger/v1/swagger.yaml || true
echo
echo "== swagger/v1/swagger.yaml: /reports matches (anywhere) =="
rg -n -i '/reports' swagger/v1/swagger.yaml || true
echo
echo "== swagger/v1/swagger.yaml: reports: matches (path keys) =="
rg -n -i 'reports:' swagger/v1/swagger.yaml || true
echo
echo "== swagger/v1/swagger.yaml: reports (word) matches (first 50) =="
rg -n -i '\breports\b' swagger/v1/swagger.yaml | head -n 50 || true
echo
echo "== request specs: fetch_response_report matches =="
rg -n --glob '**/*_spec.rb' -i 'fetch_response_report|/reports/fetch_response_report|fetch response report' spec || true
echo
echo "== request specs: /reports matches under spec/requests (first 50) =="
rg -n --glob '**/*_spec.rb' -i '/reports' spec/requests | head -n 50 || true
echo
echo "== request specs: reports_controller matches =="
rg -n --glob '**/*_spec.rb' -i 'ReportsController|reports_controller' spec/requests || trueRepository: expertiza/reimplementation-back-end
Length of output: 498
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== swagger/v1/swagger.yaml: head (top 80 lines) =="
head -n 80 swagger/v1/swagger.yaml
echo
echo "== swagger/v1/swagger.yaml: locate 'paths:' =="
rg -n '^\s*paths:\s*$' swagger/v1/swagger.yaml || true
echo
echo "== swagger/v1/swagger.yaml: show around first 'paths:' occurrence =="
LINE=$(rg -n '^\s*paths:\s*$' swagger/v1/swagger.yaml | head -n 1 | cut -d: -f1 || true)
if [ -n "${LINE}" ]; then
start=$((LINE-20))
end=$((LINE+120))
if [ "$start" -lt 1 ]; then start=1; fi
sed -n "${start},${end}p" swagger/v1/swagger.yaml
else
echo "No 'paths:' key found."
fi
echo
echo "== swagger/v1/swagger.yaml: broader substring searches =="
rg -n -i 'report' swagger/v1/swagger.yaml || true
rg -n -i 'fetch' swagger/v1/swagger.yaml || true
rg -n -i 'reports' swagger/v1/swagger.yaml || true
echo
echo "== request specs: list any files with 'report' in name =="
find spec -type f -name '*report*' | head -n 200 || true
echo
echo "== request specs: broader searches (limit output) =="
rg -n -i '\breport\b' spec/requests | head -n 50 || true
rg -n -i 'fetch' spec/requests | head -n 50 || true
rg -n -i 'reports' spec/requests | head -n 50 || trueRepository: expertiza/reimplementation-back-end
Length of output: 5830
Add/confirm request-spec + Swagger/OpenAPI coverage for POST /reports/fetch_response_report.
resources :reports, only: [] do
collection do
post :fetch_response_report
end
endconfig/routes.rb and app/controllers/reports_controller.rb define ReportsController#fetch_response_report, but there are no corresponding request specs or Swagger entries for this endpoint. Add an rswag request spec that documents and covers success (200) plus error paths: 422 (unknown type), 404 (assignment not found), and 500 (unexpected error).
🧰 Tools
🪛 RuboCop (1.86.2)
[convention] 198-202: Inconsistent indentation detected.
(Layout/IndentationConsistency)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🚨 RSpec Tests Report |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🚨 RSpec Tests Report |
|
🚨 RSpec Tests Report |
1 participant
Ports feedback_response_map report from Expertiza (repo X) to the JSON API architecture of repo Y using a streaming reduce pipeline.
accumulate -> finalize via find_each streaming)
Danger Policy Violations & Code Quality Issues
🔴 Critical Issues
Missing Database Migration for New Model
AnswerTagmodel (app/models/answer_tag.rb) with associations toanswer,tag_prompt_deployment, anduser, plus presence validations for all required fieldsanswer_tagstable — confirmed by absence fromdb/schema.rbtable listUnresolved TODO Marker in Production Code
app/models/reports/answer_tagging_report.rbcontains a TODO comment: "confirm with prof — if a reviewer submits multiple responses for the same round, only the latest submitted response should be counted as taggable"🟠 High Severity Issues
Zero Test Coverage for Major Architectural Refactoring
ReportsControllerwithfetch_response_reportaction andREPORT_CLASSESfactory mappingReports::BaseReportimplementing streaming reducer patternFeedbackReport,ReviewReport,TeammateReviewReport,BookmarkRatingReport,AnswerTaggingReportDuplicate File Implementations
feedback_report.rbexist with differing implementations:app/services/reports/feedback_report.rb(uses@assignment)app/models/reports/feedback_report.rb(uses@reportable)🟡 Medium Severity Issues
Large Refactoring Scope Without Isolated Testing
Weak Error Handling in Controller
StandardErrorand returns rawe.messageto client (potential information disclosure)Incomplete Model Relationships
AnswerTagmodel references associations totag_prompt_deploymentanduserℹ️ Additional Observations
Blocking Requirements Before Merge
Create database migration to create
answer_tagstable with:id,answer_id,tag_prompt_deployment_id,user_id,value, timestampsdb/schema.rbResolve TODO marker in
answer_tagging_report.rbwith explicit implementation decision (implement deduplication or document decision not to)Consolidate duplicate feedback_report.rb — verify which version is correct or document why both are needed
Recommended Test Coverage
ReportsController#fetch_response_reportcovering all report types, error cases, and permissionsReports::*class covering pipeline methods and finalizationAnswerTagassociations/validations