Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
6f32448
Add FeedbackReport pipeline and ReportsController dispatch
Apr 27, 2026
533cab0
Remove unused feedback_response_report model method
Apr 27, 2026
8fd2d66
Fix missing trailing newline in feedback_response_map.rb
Apr 27, 2026
b9332a6
Refactor AnswerTaggingReport to follow BaseReport pipeline pattern
May 24, 2026
3e2529e
Extract model scopes and fix indentation in answer tagging report
May 24, 2026
49871f8
Refactor ReviewReport pipelines for clarity and deduplication
May 24, 2026
b8f3ebc
Extract model scopes for ReviewReport pipeline queries
May 24, 2026
a118c02
Test commit to verify GitHub contribution tracking for aanand-1706
aanand-1706 May 24, 2026
06e3dc9
Refactor AnswerTaggingReport into two streaming pipelines
aanand-1706 May 24, 2026
36d78be
Simplify AnswerTaggingReport to match tags by answer_id instead of re…
aanand-1706 May 24, 2026
ea432c5
Fix ReviewReport bug and precompute max_q_scores once for both pipelines
aanand-1706 May 24, 2026
94cac89
Expand BaseReport comments to document grouper and accumulate roles
aanand-1706 May 24, 2026
1d96437
Rename grouper to state_key_for and deduplicate review responses by l…
aanand-1706 May 25, 2026
eeaa067
Update TeammateReviewReport to accumulate reviewees per reviewer
aanand-1706 May 25, 2026
10a6a4c
Document that BaseReport does not enforce state_key_for key usage in …
aanand-1706 May 25, 2026
52a6acb
Add shared state support to BaseReport and refactor TeammateReviewReport
aanand-1706 May 25, 2026
f270833
Refactor ReviewReport to use shared state and simplify score computation
aanand-1706 May 25, 2026
4871ec9
Fix stale comments in base_report and teammate_review_report
aanand-1706 May 25, 2026
ec2614a
Add answer_tagging report and use for_assignment factory in ReportsCo…
aanand-1706 May 25, 2026
0e5a061
Simplify ReportsController: POST-only, rename to fetch_response_report
aanand-1706 May 25, 2026
afb74c5
Merge branch 'main' into report-gen
aanand-1706 May 25, 2026
bbeb1f1
Add submit and unsubmit member routes for responses
aanand-1706 May 25, 2026
acdbeb3
Remove trailing whitespace in routes.rb
aanand-1706 May 25, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions app/controllers/reports_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# frozen_string_literal: true

class ReportsController < ApplicationController
REPORT_CLASSES = {
'review_response_map' => Reports::ReviewReport,
'feedback_response_map' => Reports::FeedbackReport,
'teammate_review_response_map' => Reports::TeammateReviewReport,
'bookmark_rating_response_map' => Reports::BookmarkRatingReport,
'answer_tagging' => Reports::AnswerTaggingReport,
'basic' => Reports::BasicReport
}.freeze

# Only TAs, instructors, and admins may view reports.
def action_allowed?
current_user_has_ta_privileges?
end

# POST /reports/fetch_response_report
# Returns the requested report as JSON.
def fetch_response_report
assignment_id = params[:assignment_id]
type = params[:type] || 'basic'

Comment on lines +21 to +23

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 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" || true

Repository: 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"
fi

Repository: 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

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
Comment on lines +36 to +37

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.

end
end
7 changes: 7 additions & 0 deletions app/helpers/report_formatter_helper.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# frozen_string_literal: true

# Report logic has been moved to app/services/reports/.
# This module is retained as a namespace placeholder for any future
# view-layer formatting helpers.
module ReportFormatterHelper
end
2 changes: 2 additions & 0 deletions app/models/Item.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ class Item < ApplicationRecord
belongs_to :questionnaire # each item belongs to a specific questionnaire
has_many :answers, dependent: :destroy, foreign_key: 'item_id'
attr_accessor :choice_strategy

scope :for_questionnaire_and_type, ->(questionnaire_id, question_type) { where(questionnaire_id: questionnaire_id, question_type: question_type) }

validates :seq, presence: true, numericality: true # sequence must be numeric
validates :txt, length: { minimum: 0, allow_nil: false, message: "can't be nil" } # text content must be provided
Expand Down
13 changes: 13 additions & 0 deletions app/models/answer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,17 @@
class Answer < ApplicationRecord
belongs_to :response
belongs_to :item

scope :for_items_and_responses, ->(item_ids, response_ids) { where(item_id: item_ids, response_id: response_ids) }

scope :taggable_for_assignment, ->(assignment_id, item_ids, type:, threshold: nil) {
scope = joins(response: :response_map)
.where(
item_id: item_ids,
responses: { is_submitted: true },
response_maps: { reviewed_object_id: assignment_id, type: type }
)
scope = scope.where('LENGTH(answers.comments) > ?', threshold) if threshold
scope
}
end
14 changes: 14 additions & 0 deletions app/models/answer_tag.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# frozen_string_literal: true

class AnswerTag < ApplicationRecord
belongs_to :answer
belongs_to :tag_prompt_deployment
belongs_to :user

scope :for_deployment, ->(deployment_id) { where(tag_prompt_deployment_id: deployment_id) }

validates :answer_id, presence: true
validates :tag_prompt_deployment_id, presence: true
validates :user_id, presence: true
validates :value, presence: true
end
4 changes: 4 additions & 0 deletions app/models/assignment_questionnaire.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,8 @@
class AssignmentQuestionnaire < ApplicationRecord
belongs_to :assignment
belongs_to :questionnaire

scope :for_assignment_and_type, ->(assignment_id, questionnaire_type) {
joins(:questionnaire).where(assignment_id: assignment_id, questionnaires: { questionnaire_type: questionnaire_type })
}
end
233 changes: 233 additions & 0 deletions app/models/reports/answer_tagging_report.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,233 @@
# frozen_string_literal: true

module Reports
# Answer-tagging report: shows per-user tagging progress for each
# TagPromptDeployment on the assignment, plus a cross-deployment summary.
#
# Output shape:
# {
# questionnaire_tagging_report: {
# <deployment_id> => {
# questionnaire_name:, prompt:, question_type:,
# answer_length_threshold:,
# user_stats: [{ user_id:, name:, full_name:, percentage:,
# cnt_tagged:, cnt_not_tagged:, cnt_taggable:,
# tag_update_intervals: }]
# }
# },
# user_tagging_report: {
# <user_id> => { user_id:, name:, full_name:, percentage:,
# cnt_tagged:, cnt_not_tagged:, cnt_taggable: }
# }
# }
class AnswerTaggingReport
def self.for_assignment(assignment)
new(assignment)
end

def initialize(assignment)
@reportable = assignment
end

def run
per_deployment = {}
# 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
end
user_summary = UserSummaryReducer.new(per_deployment).run
{
questionnaire_tagging_report: per_deployment,
user_tagging_report: user_summary
}
end

# -------------------------------------------------------------------------
# Coordinator — runs two streaming reducers for one TagPromptDeployment
# and merges their results:
#
# TaggableAnswersReducer — streams taggable Answer rows (joined with
# response_maps, filtered by item type and threshold). Returns per-user
# lists of taggable answer_ids.
# Output: { user_id => [answer_ids] }
#
# TaggingStatsReducer — streams all AnswerTag rows for this deployment
# (joined with answers to get answer_id). No item/threshold filtering
# in SQL — filtering happens in finalize by comparing answer_ids.
# Output: { user_id => [{ answer_id:, response_id:, updated_at: }] }
#
# Precomputed once, shared across both reducers:
# @item_ids — IDs of items in the deployment's questionnaire (tiny)
# @users_by_team — TeamsUser records grouped by team_id; also used in
# finalize to build per-user name info
# -------------------------------------------------------------------------
class DeploymentReducer
def initialize(reportable, deployment)
@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)
end

def run
taggable_data = TaggableAnswersReducer.new(@reportable, @deployment, @item_ids, @users_by_team).run
tagging_stats = TaggingStatsReducer.new(@reportable, @deployment).run
finalize(taggable_data, tagging_stats)
end

private

def finalize(taggable_data, tagging_stats)
user_info = @users_by_team.values.flatten.each_with_object({}) do |teams_user, info|
info[teams_user.user_id] = { name: teams_user.user.name, full_name: teams_user.user.full_name }
end

user_stats = user_info.map do |user_id, info|
taggable_answer_ids = taggable_data.fetch(user_id, []).to_set
cnt_taggable = taggable_answer_ids.size

# Filter tags to only those on answers that are taggable for this user.
# TODO: confirm with prof — if a reviewer submits multiple responses for the
# same round, only the latest submitted response should be counted as taggable.
# TaggableAnswersReducer may need to deduplicate by keeping only the most
# recently submitted response per (reviewer, round).
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,
Comment on lines +98 to +111

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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_tagged

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

Suggested change
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)

cnt_taggable: cnt_taggable,
tag_update_intervals: intervals_between_tags
}
end

{
questionnaire_name: @deployment.questionnaire.name,
prompt: @deployment.tag_prompt.prompt,
question_type: @deployment.question_type,
answer_length_threshold: @deployment.answer_length_threshold,
user_stats: user_stats
}
end

# -----------------------------------------------------------------------
# Reducer 1 — per-user taggable answer IDs.
#
# Streams taggable Answer rows (joined with responses and response_maps,
# filtered by item type and length threshold). Each row is one (answer, team)
# pair — answers.id is unique per row so find_each paginates correctly.
# For each row, adds the answer_id to all users of the team.
#
# Output: { user_id => [answer_ids] }
# -----------------------------------------------------------------------
class TaggableAnswersReducer < BaseReport
def initialize(reportable, deployment, item_ids, users_by_team)
super(reportable)
@deployment = deployment
@item_ids = item_ids
@users_by_team = users_by_team
end

def source
return Answer.none if @item_ids.empty?

Answer
.taggable_for_assignment(
@deployment.assignment_id, @item_ids,
type: 'ReviewResponseMap',
threshold: @deployment.answer_length_threshold
)
.select('answers.id, response_maps.reviewee_id as team_id')
end

def state_key_for = ->(answer) { answer.team_id }

def initial_state
Hash.new { |state, user_id| state[user_id] = [] }
end

def accumulate(state, team_id, answer)
(@users_by_team[team_id] || []).each do |teams_user|
state[teams_user.user_id] << answer.id
end
end
end

# -----------------------------------------------------------------------
# Reducer 2 — per-user answer tags with answer context.
#
# Streams all AnswerTag rows for this deployment (joined with answers).
# No item or threshold filtering in SQL — the finalize step filters tags
# by comparing their answer_id against the taggable answer_ids from
# Reducer 1.
#
# Output: { user_id => [{ answer_id:, response_id:, updated_at: }] }
# -----------------------------------------------------------------------
class TaggingStatsReducer < BaseReport
def initialize(reportable, deployment)
super(reportable)
@deployment = deployment
end

def source
AnswerTag
.for_deployment(@deployment.id)
.joins(:answer)
.select('answer_tags.id, answer_tags.user_id, answer_tags.answer_id, answer_tags.updated_at, answers.response_id')
end

def state_key_for = ->(tag) { tag.user_id }

def initial_state
Hash.new { |state, user_id| state[user_id] = [] }
end

def accumulate(state, user_id, tag)
state[user_id] << { answer_id: tag.answer_id, response_id: tag.response_id, updated_at: tag.updated_at }
end
end
end

# -------------------------------------------------------------------------
# Reducer 3 — cross-deployment per-user summary.
# Consumes DeploymentReducer output; no additional DB queries.
# -------------------------------------------------------------------------
class UserSummaryReducer
def initialize(per_deployment_result)
@per_deployment = per_deployment_result
end

def run
summary = {}
@per_deployment.each_value do |deployment_data|
deployment_data[:user_stats].each do |stat|
key = stat[:user_id]
if summary.key?(key)
entry = summary[key]
entry[:cnt_tagged] += stat[:cnt_tagged]
entry[:cnt_not_tagged] += stat[:cnt_not_tagged]
entry[:cnt_taggable] += stat[:cnt_taggable]
entry[:percentage] = entry[:cnt_taggable].zero? ? '-' : format('%.1f', entry[:cnt_tagged].to_f / entry[:cnt_taggable] * 100)
else
summary[key] = stat.slice(:user_id, :name, :full_name, :cnt_tagged, :cnt_not_tagged, :cnt_taggable, :percentage)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
end
end
end
summary
Comment thread
coderabbitai[bot] marked this conversation as resolved.
end
end
end
end
Loading
Loading