Skip to content

Adding a rubric system to Autolab #2291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

ajain-1
Copy link

@ajain-1 ajain-1 commented May 5, 2025

Our team worked on a feature to add a rubric system to assessments in Autolab (#1716). Essentially this allows instructors and TAs to:

  • Create and manage an optional rubric per problem in an assessment to help standardize the manual grading of those problems. A rubric consists of rubric items, which are a description and point value.
  • Use a rubric to easily grade a problem in the submission viewer.
  • Optionally associate annotations with specific rubric items (similar to Gradescope) to make comments more granular.

Images

image image image

Description

This involved changes across the entire stack:

  • We updated the schema and created models/controllers to accommodate managing rubric items and assigning them on submissions.
  • We also made changes in the UI specifically in the problems view, annotation pane view, and annotation form view to be able to manage, assign, and annotate with rubric items respectively.
  • We made a common scoring module to calculate the total score accounting for annotations and any rubric items
  • We made changes to annotations.js to be able to listen for events in the UI across the annotation form and rubric item assignment area.

Motivation and Context

This was motivated by issue #1716 and aims to make it easier and more standardized to manually grade problems. We want to allow graders to use a rubric similar to Gradescope to be able to quickly change point totals.

An example could be an instructor making a rubric for style grading (done manually), with rubric items relating to code complexity, line length, etc. and then TAs following that rubric to quickly and more fairly grade the style grading problem.

How Has This Been Tested?

Currently this feature has been tested locally quite extensively by using the feature. We've tested for any possible interactions between the scoring of a rubric as well as annotation-based scoring - both can coexist. We will also look to add further tests.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Since it is optional to add rubrics and optional to associate annotations with rubric items, there are no breaking changes. The appropriate migrations have been made for the schema changes.

Checklist:

  • I have run rubocop and erblint for style check. If you haven't, run overcommit --install && overcommit --sign to use pre-commit hook for linting
  • My change requires a change to the documentation, which is located at Autolab Docs
  • I have updated the documentation accordingly, included in this PR

Other issues / help required

If unsure, feel free to submit first and we'll help you along.

@ajain-1 ajain-1 marked this pull request as ready for review May 5, 2025 22:40
Copy link
Contributor

coderabbitai bot commented May 5, 2025

📝 Walkthrough

Walkthrough

This change introduces a comprehensive rubric system for problem grading. It adds models, controllers, views, and database migrations to support rubric items, their assignment to submissions, and annotation association with rubric items. The submission and annotation UIs are updated to display and manage rubric items, and the score calculation logic is refactored to accommodate rubric-based grading.

Changes

File(s) Change Summary
app/assets/javascripts/annotations.js, app/assets/stylesheets/annotations.scss, app/views/submissions/_annotation_form.html.erb, app/views/submissions/_annotation_pane.html.erb, app/views/submissions/view.html.erb, app/views/submissions/_golden-layout.html.erb, app/views/annotations/_global_annotation.html.erb, app/views/annotations/_line_annotation.html.erb Added rubric item selection and display in annotation forms and panes; added styles and partials for rubric-related UI; updated pane title.
app/controllers/rubric_items_controller.rb, app/controllers/rubric_item_assignments_controller.rb Added new controllers for rubric item CRUD and assignment toggling.
app/controllers/annotations_controller.rb, app/controllers/submissions_controller.rb, app/helpers/annotations_helper.rb Updated controllers and helpers to support rubric item associations, structured annotation data, and new data flows for the UI.
app/models/rubric_item.rb, app/models/rubric_item_assignment.rb, app/models/concerns/score_calculation.rb Introduced new models and concern for rubric items, their assignments, and shared score calculation logic.
app/models/annotation.rb, app/models/problem.rb, app/models/submission.rb Added associations for rubric items and assignments; refactored annotation score update logic to use shared concern.
app/models/course_user_datum.rb Minor refactor in type checking logic.
app/views/problems/_fields.html.erb, app/views/rubric_items/new.html.erb, app/views/rubric_items/edit.html.erb Added rubric management UI to problem form and new/edit rubric item views.
config/routes.rb Added nested routes for rubric items under problems and submissions, with toggle actions.
db/migrate/20250426203028_create_rubric_items.rb, db/migrate/20240501000000_create_rubric_item_assignments.rb, db/migrate/20240501123456_add_rubric_item_id_to_annotations.rb, db/schema.rb Added migrations and schema changes for rubric items, assignments, and annotation associations.
Gemfile Restricted mysql2 gem to version 0.5.4.

Sequence Diagram(s)

sequenceDiagram
    participant Instructor/Assistant
    participant SubmissionView
    participant RubricItemsController
    participant RubricItemAssignmentsController
    participant AnnotationForm
    participant AnnotationsController
    participant Database

    Instructor/Assistant->>SubmissionView: Open submission
    SubmissionView->>Database: Fetch problems, rubric items, assignments, annotations
    SubmissionView->>Instructor/Assistant: Display problems, rubric items, and annotations

    Instructor/Assistant->>RubricItemAssignmentsController: Toggle rubric item assignment
    RubricItemAssignmentsController->>Database: Update assignment, recalculate score
    RubricItemAssignmentsController->>SubmissionView: Redirect/refresh with updated data

    Instructor/Assistant->>AnnotationForm: Add annotation (optionally select rubric item)
    AnnotationForm->>AnnotationsController: Submit annotation with rubric_item_id
    AnnotationsController->>Database: Create annotation, associate with rubric item
    AnnotationsController->>Database: Trigger score recalculation
    AnnotationsController->>SubmissionView: Display updated annotations and scores
Loading

Suggested reviewers

  • dwang3851
  • coder6583
  • jhs-panda
✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 16

🧹 Nitpick comments (20)
app/models/course_user_datum.rb (1)

52-52: Optional: simplify nil check and character removal
You can make the assignment more concise by leveraging safe navigation and String#delete:

-    self[column.name] = self[column.name].gsub(/</, "") unless self[column.name].nil?
+    self[column.name] = self[column.name]&.delete("<")

This avoids the explicit nil guard and clearly expresses the intent to remove all < characters.

app/models/rubric_item_assignment.rb (1)

7-7: Consider adding an index for improved query performance.

While the uniqueness validation is correct, you should ensure that a corresponding database index exists to enforce this constraint at the database level and improve query performance. Typically, you would add this in the migration that creates the table.

# In the migration file
add_index :rubric_item_assignments, [:rubric_item_id, :submission_id], unique: true
app/views/problems/_fields.html.erb (1)

41-49: Consider adding tooltips to the action icons

The icons for edit and delete are clear, but adding a tooltip attribute would improve accessibility and help users understand the actions.

-            <%= link_to "<i class=\"material-icons left\">mode_edit</i>".html_safe,
+            <%= link_to "<i class=\"material-icons left\" title=\"Edit rubric item\">mode_edit</i>".html_safe,
                         edit_course_assessment_problem_rubric_item_path(@course, @assessment, @problem, item),
                         { class: "small" } %>
-            <%= link_to "<i class=\"material-icons left\">delete</i>".html_safe,
+            <%= link_to "<i class=\"material-icons left\" title=\"Delete rubric item\">delete</i>".html_safe,
                         [@course, @assessment, @problem, item],
                         method: :delete,
                         class: "small",
                         data: { confirm: "Are you sure you want to delete this rubric item?" } %>
db/migrate/20240501000000_create_rubric_item_assignments.rb (1)

3-9: Consider adding an index on the submission_id column

Since you'll likely query rubric item assignments by submission, adding an index on submission_id could improve query performance.

 def change
   create_table :rubric_item_assignments do |t|
     t.references :rubric_item, null: false, foreign_key: true
     t.references :submission, null: false, foreign_key: true
     t.boolean :assigned, default: false

     t.timestamps
   end
+  
+  # Add index for querying by submission
+  add_index :rubric_item_assignments, :submission_id
   
   # Add index for faster lookups
   add_index :rubric_item_assignments, [:rubric_item_id, :submission_id], unique: true, name: 'index_ria_on_rubric_item_id_and_submission_id'
app/views/submissions/_annotation_form.html.erb (1)

49-54: Add disabled attribute to initial placeholder option

To prevent users from accidentally submitting the form with the placeholder option selected, add the disabled attribute.

         <select class="browser-default rubric-item-id" id="annotation-rubric-item-id" name="annotation[rubric_item_id]">
-            <option value="">-- Select a problem first --</option>
+            <option value="" disabled>-- Select a problem first --</option>
         </select>
app/views/rubric_items/new.html.erb (1)

1-17: LGTM! Form is well-structured and follows Rails conventions.

The form includes all necessary fields for creating a rubric item with appropriate validation hints and required fields. The form is properly nested within the course, assessment, and problem resources.

Consider adding a cancel button or link to allow users to navigate back without saving:

  <div class="action_buttons">
    <%= f.submit "Save Rubric Item", { class: "btn primary" } %>
+   <%= link_to "Cancel", course_assessment_problem_path(@course, @assessment, @problem), class: "btn" %>
  </div>
app/views/annotations/_global_annotation.html.erb (1)

14-31: Improve accessibility for annotation tools.

While the functionality is appropriate, the edit and delete buttons could be improved for accessibility.

Consider these enhancements:

  <div class="annotation-tools">
    <span
          data-annotationid="<%= annotation.id %>"
          data-problem="<%= problem_name %>"
          data-score="<%= annotation.value %>"
          data-comment="<%= annotation.comment %>"
          data-shared="<%= annotation.shared_comment %>">
-       <a class="global-annotation-edit-button" href="#"
-          title="Edit global annotation">
+       <button type="button" class="global-annotation-edit-button" 
+          aria-label="Edit global annotation">
          <i class="material-icons" aria-hidden="true">edit</i>
-       </a>
-       <a class="global-annotation-delete-button" href="#" title="Delete global annotation">
+       </button>
+       <button type="button" class="global-annotation-delete-button" 
+          aria-label="Delete global annotation">
          <i class="material-icons" aria-hidden="true">delete</i>
-       </a>
+       </button>
    </span>
  </div>
app/models/rubric_item.rb (2)

12-12: default_scope may introduce hidden query surprises

Using default_scope means every RubricItem query will be ordered, even when callers do not expect it, which can break association preloading and compound ordering. Prefer an explicit scope:

scope :ordered, -> { order(order: :asc) }

Then call problem.rubric_items.ordered in the few places that need ordering.


14-17: Extract a reusable “assigned” association

assigned_to? re-implements a filter each time. Consider:

has_many :assigned_item_assignments,
         -> { where(assigned: true) },
         class_name: 'RubricItemAssignment'

Then:

def assigned_to?(submission)
  assigned_item_assignments.exists?(submission:)
end

This keeps intent declarative and removes a query predicate from model logic.

app/controllers/rubric_item_assignments_controller.rb (1)

8-51: Method exceeds ABC complexity (RuboCop warning)

Extract small private helpers (e.g., find_or_toggle_assignment, update_score_for_problem) to improve readability and satisfy style checks.

🧰 Tools
🪛 RuboCop (1.73)

[convention] 8-51: Assignment Branch Condition size for toggle is too high. [<8, 31, 5> 32.4/23]

(Metrics/AbcSize)

config/routes.rb (1)

158-163: Do you really want full CRUD for rubric items inside a submission?

Granting resources :rubric_items inside submissions exposes routes like new, edit, destroy, which conceptually belong to instructors operating on problems, not graders operating on a single student’s submission. Consider scoping this nested resource to only: [:index] or even eliminating it if the toggle route suffices.

app/models/annotation.rb (1)

31-35: Remove unused local score variable

update_score recomputes the score internally, so the score = … lookup is now dead code:

-  # Get score for submission
-  score = Score.find_or_initialize_by(
-    submission_id: submission_id,
-    problem_id: problem_id
-  )

Eliminating it avoids an extra query on every annotation change.

app/models/concerns/score_calculation.rb (2)

34-34: Remove unused variable assignment.

The variable problem is assigned but never used in this method.

-    problem = Problem.find(problem_id)
🧰 Tools
🪛 RuboCop (1.73)

[warning] 34-34: Useless assignment to variable - problem. Did you mean problem_id?

(Lint/UselessAssignment)


31-59: Consider refactoring to reduce method complexity.

The calculate_total_score method is slightly above the ABC size threshold. Consider extracting the group submission annotation calculation into a separate private method to improve readability and reduce complexity.

 def calculate_total_score
   # Get problem ID depending on whether this is an annotation or rubric item assignment
   problem_id = problem_id_for_score
-  problem = Problem.find(problem_id)
   
   # Calculate rubric item points
   rubric_item_points = submission.rubric_item_assignments
                                 .joins(:rubric_item)
                                 .where(assigned: true, rubric_items: { problem_id: problem_id })
                                 .sum('rubric_items.points')
   
   # Calculate annotation points
-  annotation_points = 0
-  
-  if submission.group_key.empty?
-    annotation_points = Annotation.where(submission_id: submission_id, problem_id: problem_id)
-                                 .sum(:value)
-  else
-    # For group submissions, include annotations from all group submissions
-    submissions = Submission.where(group_key: submission.group_key)
-    submissions.each do |sub|
-      annotation_points += Annotation.where(submission_id: sub.id, problem_id: problem_id)
-                                    .sum(:value)
-    end
-  end
+  annotation_points = calculate_annotation_points(problem_id)
   
   # Include the problem's max_score in the final score calculation
   annotation_points + rubric_item_points
 end

+# Extract group annotation calculation to a separate method
+private
+
+def calculate_annotation_points(problem_id)
+  if submission.group_key.empty?
+    Annotation.where(submission_id: submission_id, problem_id: problem_id).sum(:value)
+  else
+    # For group submissions, include annotations from all group submissions
+    Submission.where(group_key: submission.group_key).sum do |sub|
+      Annotation.where(submission_id: sub.id, problem_id: problem_id).sum(:value)
+    end
+  end
+end
🧰 Tools
🪛 RuboCop (1.73)

[convention] 31-59: Assignment Branch Condition size for calculate_total_score is too high. [<9, 21, 3> 23.04/23]

(Metrics/AbcSize)


[warning] 34-34: Useless assignment to variable - problem. Did you mean problem_id?

(Lint/UselessAssignment)

app/assets/javascripts/annotations.js (2)

598-641: Memory-leak & UX glitch – repeatedly re-creating autocomplete widgets

Every change on .problem-id calls autocomplete() again, spawning a brand-new widget and leaving the old handlers in memory. Use the documented autocomplete("option", "source", newSource) path instead:

-    box.find('#comment-textarea').autocomplete({
-      source: getSharedCommentsForProblem(problem_id) || [],
-    });
+    box.find('#comment-textarea')
+        .autocomplete("option", "source",
+                      getSharedCommentsForProblem(problem_id) || []);

This reuses the existing instance and avoids duplicate event handlers & DOM clutter.


1380-1390: Redundant property + inconsistent payload

createAnnotation() already sets filename; adding it again in the object spread is harmless but unnecessary noise. More importantly, the new attribute rubric_item_id is always sent, even when it’s null, which Rails treats differently from “not present”.
Prefer omitting the key when no rubric is selected:

-    filename: fileNameStr,
+    filename: fileNameStr,
@@
-    rubric_item_id  // Add this parameter
+    ...(rubric_item_id ? { rubric_item_id } : {})
app/controllers/submissions_controller.rb (1)

630-637: Score-refresh logic executed twice

params[:refresh] branch appears twice. Consolidate to a single location to avoid confusion and inconsistent future edits.

Also applies to: 771-777

app/views/submissions/_annotation_pane.html.erb (1)

52-61: Consider remote: true to toggle rubric items without full page refresh

Each rubric checkbox submits a synchronous POST which reloads the entire submission view.
Adding remote: true to form_tag enables unobtrusive-JS (rails-ujs) for a snappier UX:

-<%= form_tag toggle_course_assessment_submission_rubric_item_path(...), method: :post, class: "rubric-toggle-form" do %>
+<%= form_tag toggle_course_assessment_submission_rubric_item_path(...),
+             method: :post, remote: true,
+             class: "rubric-toggle-form" do %>

You’ll need a small JS response to refresh the affected score and icon.

app/helpers/annotations_helper.rb (1)

45-66: Ensure RubricItemAssignment performance with bulk creation

The method looks good overall, but consider the performance implications if many rubric items and submissions need assignments.

You might want to consider using find_or_create_by with find_or_initialize_by inside a transaction if you need to ensure assignments exist in the database before returning the results. Otherwise, the current implementation is fine if assignments are saved elsewhere.

db/schema.rb (1)

13-14: Remove extra line after schema version definition

There's an extra empty line after the schema version definition that should be removed according to style conventions.

ActiveRecord::Schema.define(version: 2025_04_26_203028) do
-
  create_table "active_storage_attachments", force: :cascade do |t|
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9cc857f and 6d2ac2c.

⛔ Files ignored due to path filters (2)
  • Gemfile.lock is excluded by !**/*.lock, !**/*.lock
  • libsqlite3.dylib is excluded by !**/*.dylib, !**/*.dylib
📒 Files selected for processing (30)
  • Gemfile (1 hunks)
  • app/assets/javascripts/annotations.js (13 hunks)
  • app/assets/stylesheets/annotations.scss (3 hunks)
  • app/controllers/annotations_controller.rb (1 hunks)
  • app/controllers/application_controller.rb (1 hunks)
  • app/controllers/rubric_item_assignments_controller.rb (1 hunks)
  • app/controllers/rubric_items_controller.rb (1 hunks)
  • app/controllers/submissions_controller.rb (5 hunks)
  • app/helpers/annotations_helper.rb (1 hunks)
  • app/models/annotation.rb (3 hunks)
  • app/models/concerns/score_calculation.rb (1 hunks)
  • app/models/course_user_datum.rb (1 hunks)
  • app/models/problem.rb (1 hunks)
  • app/models/rubric_item.rb (1 hunks)
  • app/models/rubric_item_assignment.rb (1 hunks)
  • app/models/submission.rb (1 hunks)
  • app/views/annotations/_global_annotation.html.erb (1 hunks)
  • app/views/annotations/_line_annotation.html.erb (1 hunks)
  • app/views/problems/_fields.html.erb (1 hunks)
  • app/views/rubric_items/edit.html.erb (1 hunks)
  • app/views/rubric_items/new.html.erb (1 hunks)
  • app/views/submissions/_annotation_form.html.erb (1 hunks)
  • app/views/submissions/_annotation_pane.html.erb (2 hunks)
  • app/views/submissions/_golden-layout.html.erb (1 hunks)
  • app/views/submissions/view.html.erb (1 hunks)
  • config/routes.rb (2 hunks)
  • db/migrate/20240501000000_create_rubric_item_assignments.rb (1 hunks)
  • db/migrate/20240501123456_add_rubric_item_id_to_annotations.rb (1 hunks)
  • db/migrate/20250426203028_create_rubric_items.rb (1 hunks)
  • db/schema.rb (10 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
db/migrate/20240501123456_add_rubric_item_id_to_annotations.rb (1)
db/migrate/20240501000000_create_rubric_item_assignments.rb (2)
  • change (1-17)
  • change (2-16)
app/models/rubric_item_assignment.rb (2)
app/models/annotation.rb (1)
  • include (6-46)
app/models/rubric_item.rb (1)
  • belongs_to (1-18)
🪛 RuboCop (1.73)
app/controllers/rubric_item_assignments_controller.rb

[convention] 8-51: Assignment Branch Condition size for toggle is too high. [<8, 31, 5> 32.4/23]

(Metrics/AbcSize)

app/helpers/annotations_helper.rb

[convention] 20-42: Cyclomatic complexity for get_problem_annotations is too high. [9/7]

(Metrics/CyclomaticComplexity)


[convention] 20-42: Perceived complexity for get_problem_annotations is too high. [9/8]

(Metrics/PerceivedComplexity)


[convention] 69-93: Assignment Branch Condition size for get_non_rubric_annotations is too high. [<25, 15, 12> 31.53/23]

(Metrics/AbcSize)


[convention] 69-93: Cyclomatic complexity for get_non_rubric_annotations is too high. [11/7]

(Metrics/CyclomaticComplexity)


[convention] 69-93: Perceived complexity for get_non_rubric_annotations is too high. [11/8]

(Metrics/PerceivedComplexity)


[convention] 72-72: Avoid parameter lists longer than 5 parameters. [9/5]

(Metrics/ParameterLists)


[convention] 83-83: Avoid parameter lists longer than 5 parameters. [8/5]

(Metrics/ParameterLists)

app/controllers/rubric_items_controller.rb

[convention] 57-96: Assignment Branch Condition size for toggle is too high. [<6, 28, 10> 30.33/23]

(Metrics/AbcSize)


[convention] 57-96: Cyclomatic complexity for toggle is too high. [9/7]

(Metrics/CyclomaticComplexity)


[convention] 57-96: Perceived complexity for toggle is too high. [10/8]

(Metrics/PerceivedComplexity)

app/models/concerns/score_calculation.rb

[convention] 31-59: Assignment Branch Condition size for calculate_total_score is too high. [<9, 21, 3> 23.04/23]

(Metrics/AbcSize)


[warning] 34-34: Useless assignment to variable - problem. Did you mean problem_id?

(Lint/UselessAssignment)

db/schema.rb

[convention] 14-15: Extra empty line detected at block body beginning.

(Layout/EmptyLinesAroundBlockBody)

🔇 Additional comments (31)
app/models/course_user_datum.rb (1)

50-50: include? refactor is clear and correct
Switching from column.type == :string || column.type == :text to [:string, :text].include?(column.type) preserves the original behavior and improves readability.

app/views/submissions/_golden-layout.html.erb (1)

87-87: LGTM: Updated panel title reflects new functionality

The panel title change from "Annotations" to "Annotations and Rubrics" accurately reflects the expanded functionality that now includes both annotations and rubric items in this component.

app/models/problem.rb (1)

12-12: LGTM: Added appropriate association for rubric items

The new has_many :rubric_items, dependent: :destroy association correctly establishes the relationship between problems and rubric items, with appropriate cascading deletion when a problem is destroyed.

db/migrate/20240501123456_add_rubric_item_id_to_annotations.rb (2)

1-5: LGTM: Well-structured migration for associating annotations with rubric items

This migration correctly adds a nullable foreign key reference from annotations to rubric items, allowing annotations to be optionally associated with specific rubric items. This supports the core functionality of the new rubric system while maintaining compatibility with existing annotations.


3-3:

✅ Verification successful

Verify index creation for foreign key

While the migration adds the foreign key correctly, consider whether an index should be added for the rubric_item_id column, depending on your query patterns.

Note that add_reference creates an index by default in Rails 6.1, so this might not be necessary if you rely on that default behavior.


🏁 Script executed:

#!/bin/bash
# Check whether similar foreign keys have indexes in the schema
grep -A 2 "add_reference\|add_foreign_key" db/migrate/*.rb | grep -v "index: false" | wc -l

# Look at other annotation-related queries to see access patterns
rg -A 5 "annotation.*where|annotation.*find|annotation.*join" --type ruby app/

Length of output: 15269


No action needed: add_reference adds an index by default

Rails 6.1+’s add_reference :annotations, :rubric_item, foreign_key: true will automatically create an index on rubric_item_id (unless you explicitly pass index: false). No additional migration changes are required here.

app/controllers/application_controller.rb (1)

201-201: Improved code readability with Ruby idioms.

The change from (params[:action] == "edit" || params[:action] == "update" || params[:action] == "unsudo") to ["edit", "update", "unsudo"].include?(params[:action]) is a good refactoring that makes the code more readable and maintainable. It uses a more idiomatic Ruby approach for checking if an action is within a set of values.

app/controllers/annotations_controller.rb (1)

86-86: LGTM: Necessary parameter addition for rubric item association.

Adding the rubric_item_id to the permitted parameters is essential for allowing annotations to be associated with specific rubric items, which is a core part of the new rubric system. This change enables the controller to properly handle requests with rubric item associations.

app/views/submissions/view.html.erb (1)

110-113: LGTM: Necessary client-side data structure for rubric item rendering.

The addition of the rubricItemsByProblem variable provides the necessary data structure for the client-side code to display and interact with problem-specific rubric items. The implementation correctly maps problem IDs to their associated rubric items with relevant attributes (id, description, points).

app/models/rubric_item_assignment.rb (1)

1-10: Well-structured model with appropriate validations and callbacks.

This new model is well-designed with:

  1. Clear associations with both RubricItem and Submission
  2. A proper uniqueness validation to prevent duplicate assignments
  3. An after_save callback to automatically update scores
  4. Inclusion of the ScoreCalculation module for shared score calculation logic

The implementation follows Rails conventions and supports the core functionality of assigning rubric items to submissions for grading.

app/views/problems/_fields.html.erb (1)

24-57: Well-structured rubric items section with good UX!

The implementation effectively integrates a rubric management interface within the problem editing form. The table layout is clean and the action buttons are consistent with application UI patterns.

app/views/submissions/_annotation_form.html.erb (2)

41-42: Good addition of explicit id and name attributes

Adding explicit id and name attributes to the problem selection dropdown is a good practice for form handling and JavaScript interaction.


47-56:

✅ Verification successful

Well-structured rubric item selection UI

The implementation effectively integrates the rubric item selection into the annotation form with appropriate helper text explaining its optional nature.

Make sure the corresponding JavaScript to populate this dropdown dynamically based on the selected problem is properly implemented.


🏁 Script executed:

#!/bin/bash
# Check if there's JavaScript code that populates the rubric item dropdown
grep -r "annotation.*rubric-item-id" app/assets/javascripts/

Length of output: 62


🏁 Script executed:

#!/bin/bash
# Deep search for any JavaScript/TypeScript handling of the rubric-item dropdown
# in both the asset pipeline and webpacker (app/javascript) directories.
rg --color never -n 'rubric[-_]item[-_]id' app/assets/javascripts app/javascript
rg --color never -n 'rubric_item_id' app/assets/javascripts app/javascript

Length of output: 1609


Rubric item dropdown JS confirmed in annotations.js

The dynamic population and event handling for the select.rubric-item-id element are already implemented; no further action is needed.

Relevant locations:

  • app/assets/javascripts/annotations.js, lines 600–650: finding, showing/hiding, and binding change events on select.rubric-item-id
  • app/assets/javascripts/annotations.js, lines 663–684: reading rubric_item_id and passing it to submitNewAnnotation
  • app/assets/javascripts/annotations.js, lines 1379–1389: submitNewAnnotation signature updated to include rubric_item_id
app/models/submission.rb (1)

21-25: Associations correctly defined for the new rubric system.

The new associations are well-structured and follow Rails conventions:

  1. Direct association to rubric_item_assignments with dependent destruction
  2. Through association to retrieve only assigned rubric items with appropriate scoping

This implementation will ensure submissions properly access their associated rubric items and clean up related records when destroyed.

app/views/annotations/_global_annotation.html.erb (1)

1-13: Annotation display is well-structured with appropriate styling cues.

The markup provides clear visual distinction for different annotation values (positive, negative, neutral) and separates the badge from the description content.

app/views/rubric_items/edit.html.erb (1)

1-13: Form fields are well-designed with clear labels and help text.

The form provides appropriate field validation and maintains consistency with the new item form.

app/models/rubric_item.rb (1)

8-9: Consider constraining the points domain

validates :points, numericality: true accepts any numeric value, including NaN, Infinity, or very large/negative numbers.
If rubric points are meant to be finite and within a sensible range (e.g., -100‒100 or non-negative), add an explicit range check:

-validates :points, numericality: true
+validates :points,
+          numericality: { greater_than_or_equal_to: -100, less_than_or_equal_to: 100 }

Adjust the bounds to match policy.

app/controllers/rubric_item_assignments_controller.rb (1)

27-30: @cud can be nil for unauthenticated API calls

If score.grader_id must always be present, ensure @cud exists or fall back to current_user.

app/models/annotation.rb (1)

41-41: Skip autograded guard when score was not yet persisted

score.grader_id == 0 works only when a Score exists. If update_score creates a new score for an autograded problem, this guard never fires because score is nil at this point. Verify that update_score already handles autograded problems, or move the guard into the shared concern for consistency.

app/assets/stylesheets/annotations.scss (3)

431-450: Well-structured form styling for rubric item selection.

The annotation form styling for the rubric item dropdown is appropriately scoped and maintains consistent visual styling with the rest of the form elements.


711-714: Simple and effective problem item styling.

The .problem_item class provides clear visual separation between problem sections.


977-1067: Comprehensive and well-organized rubric UI styling.

The CSS for rubric-related elements is well-structured with clear organization for containers, toggles, descriptions, and nested annotations. The styling supports the interactive elements needed for rubric management while maintaining visual consistency with the existing UI.

app/views/annotations/_line_annotation.html.erb (1)

1-25: Clean and robust line annotation partial implementation.

This partial effectively encapsulates the presentation logic for line annotations with proper handling of edge cases (nil values) and consistent styling. The link generation with remote loading supports seamless navigation within the application.

app/controllers/rubric_items_controller.rb (1)

1-28: Well-structured controller with appropriate authorization.

The controller is well-organized with proper before_action filters and authentication levels for different actions. The create action follows Rails best practices for object creation and error handling.

app/helpers/annotations_helper.rb (4)

3-5: Looks good!

The method correctly groups annotations by their rubric_item_id using Rails' group_by method, which is efficient and clear.


8-12: Looks good!

This method cleanly separates global and file-specific annotations, returning them as a convenient array.


15-17: Looks good!

The method appropriately groups annotations by filename, handling nil values with an empty string fallback.


96-98: Looks good!

Simple and effective method to extract the basename from a filename, with proper handling of nil values.

db/schema.rb (4)

57-58: Looks good!

Adding the rubric_item_id column to the annotations table with an appropriate index establishes the relationship between annotations and rubric items.


324-333: Well structured RubricItemAssignments table

The table definition includes:

  • Required foreign keys to rubric_items and submissions
  • A boolean flag for assignment status
  • Proper indexes for efficient lookups
  • A composite unique index to prevent duplicate assignments

This structure will efficiently support the rubric system feature.


335-344: Well structured RubricItems table

The table definition includes:

  • Required association with a problem
  • Required description and points fields
  • An order field with a unique constraint per problem
  • Appropriate indexes for efficient lookups

This structure will properly organize rubric items within problems.


464-471: Good use of foreign key constraints

The foreign key constraints properly enforce referential integrity for the new rubric-related tables and their relationships, which will prevent orphaned records.

@@ -49,7 +49,7 @@ gem 'rake', '>=10.3.2'
gem 'populator', '>=1.0.0'

# To communicate with MySQL database
gem 'mysql2', '~>0.5'
gem 'mysql2', '=0.5.4'
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Verify rationale for pinning mysql2 to exact version 0.5.4

Changing from a pessimistic version constraint (~> 0.5) to an exact version constraint (=0.5.4) for the mysql2 gem prevents automatic updates to newer patch versions, which might include security fixes and bug fixes. While this ensures consistency across environments, it could introduce maintenance overhead.

Please also consider adding a comment in the Gemfile explaining why this exact version is required, as this can help future maintainers understand the rationale behind this constraint.


🏁 Script executed:

#!/bin/bash
# Check if there are security advisories for mysql2 gem versions above 0.5.4
curl -s "https://rubygems.org/api/v1/versions/mysql2.json" | jq -r '.[] | select(.number > "0.5.4" and .number < "0.6.0") | .number' | sort -V

# Check if this version is the latest in the 0.5.x series
curl -s "https://rubygems.org/api/v1/gems/mysql2.json" | jq -r '.version'

Length of output: 232


Re-evaluate mysql2 version constraint

The mysql2 gem has newer patch releases (0.5.5, 0.5.6) and the latest 0.5.x version is 0.5.6. Pinning to = 0.5.4 blocks these updates (which may include bug and security fixes). Consider one of the following:

  • Allow patch upgrades within 0.5.x by using a pessimistic constraint:
    gem 'mysql2', '~> 0.5.4'
  • If you must lock to an exact patch for reproducibility, bump to 0.5.6 and add an inline comment explaining the rationale:
    # Pin to 0.5.6 because of [REASON]
    gem 'mysql2', '= 0.5.6'

This ensures you get critical fixes while maintaining consistency.

🤖 Prompt for AI Agents (early access)
In the Gemfile at line 52, the mysql2 gem is pinned to an exact version 0.5.4, which blocks patch updates that may include important fixes. To fix this, either change the version constraint to a pessimistic one like '~> 0.5.4' to allow patch upgrades within the 0.5.x series, or if you need to lock to an exact version, update it to the latest patch 0.5.6 and add a comment explaining the reason for this pinning to help future maintainers.

Comment on lines +1 to +13
class CreateRubricItems < ActiveRecord::Migration[6.1]
def change
create_table :rubric_items do |t|
t.references :problem, null: false, foreign_key: true, type: :integer
t.string :description, null: false
t.float :points, null: false
t.integer :order, null: false
t.timestamps
end

add_index :rubric_items, [:problem_id, :order], unique: true
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Review the migration filename date

The migration has a future date (April 26, 2025) in the filename. Migration filenames should use the current timestamp when created.

Also, consider adding a comment to explain the purpose of the order field and how it's used.

 class CreateRubricItems < ActiveRecord::Migration[6.1]
   def change
     create_table :rubric_items do |t|
       t.references :problem, null: false, foreign_key: true, type: :integer
       t.string :description, null: false
       t.float :points, null: false
+      # Order field determines the display sequence of rubric items within a problem
       t.integer :order, null: false
       t.timestamps
     end

     add_index :rubric_items, [:problem_id, :order], unique: true
   end
-end 
+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
class CreateRubricItems < ActiveRecord::Migration[6.1]
def change
create_table :rubric_items do |t|
t.references :problem, null: false, foreign_key: true, type: :integer
t.string :description, null: false
t.float :points, null: false
t.integer :order, null: false
t.timestamps
end
add_index :rubric_items, [:problem_id, :order], unique: true
end
end
class CreateRubricItems < ActiveRecord::Migration[6.1]
def change
create_table :rubric_items do |t|
t.references :problem, null: false, foreign_key: true, type: :integer
t.string :description, null: false
t.float :points, null: false
# Order field determines the display sequence of rubric items within a problem
t.integer :order, null: false
t.timestamps
end
add_index :rubric_items, [:problem_id, :order], unique: true
end
end
🤖 Prompt for AI Agents (early access)
In db/migrate/20250426203028_create_rubric_items.rb lines 1 to 13, the migration filename uses a future date which should be replaced with the current timestamp to maintain proper migration order. Rename the file to start with the current date and time in the format YYYYMMDDHHMMSS. Additionally, add a comment above the `order` field definition explaining its purpose and usage to improve code clarity.

Comment on lines +11 to +15
# Add index for faster lookups
add_index :rubric_item_assignments, [:rubric_item_id, :submission_id], unique: true, name: 'index_ria_on_rubric_item_id_and_submission_id'

# Remove assigned column from rubric_items as it will now be tracked per submission
remove_column :rubric_items, :assigned, :boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Verify the existing assigned column in rubric_items

Line 15 removes an assigned column from the rubric_items table, but this column isn't created in the previous migration. Ensure there's another migration that adds this column, or this could fail during deployment.


🏁 Script executed:

#!/bin/bash
# Check if the assigned column exists in the schema
grep -A 20 "create_table \"rubric_items\"" db/schema.rb
# Look for any other migrations that might add the assigned column
find db/migrate -name "*.rb" -type f -exec grep -l "add_column.*rubric_items.*assigned" {} \;

Length of output: 917


🏁 Script executed:

#!/bin/bash
# Find the migration that creates the rubric_items table
grep -R "create_table.*rubric_items" -n db/migrate

# Display the contents of that migration to check if it ever defines an `assigned` column
grep -R "create_table.*rubric_items" -l db/migrate | xargs sed -n '1,200p'

Length of output: 614


Remove or guard removal of non-existent assigned column in rubric_items

The assigned column is not defined in any prior migration or in db/schema.rb, so calling

remove_column :rubric_items, :assigned, :boolean

will cause the migration to fail.

• File: db/migrate/20240501000000_create_rubric_item_assignments.rb
• Line: the remove_column :rubric_items, :assigned, :boolean call

Suggested fixes (pick one):

  1. Remove the line entirely if the assigned flag was never used.
  2. Wrap it in a guard to only drop the column if it exists:
    if column_exists?(:rubric_items, :assigned)
      remove_column :rubric_items, :assigned, :boolean
    end
🤖 Prompt for AI Agents (early access)
In db/migrate/20240501000000_create_rubric_item_assignments.rb around lines 11 to 15, the migration attempts to remove the assigned column from rubric_items, but this column does not exist in any prior migration or the schema, which will cause the migration to fail. To fix this, either remove the remove_column line entirely if the assigned column was never used, or wrap the remove_column call in a conditional guard that checks if the column exists using if column_exists?(:rubric_items, :assigned) before attempting to remove it.

Comment on lines +14 to +21
<div class="action_buttons">
<%= f.submit "Save Rubric Item", { class: "btn primary" } %>
<button class="delete_btn">
<%= link_to "Delete this Rubric Item", [@course, @assessment, @problem, @rubric_item],
method: :delete, class: 'btn btn-danger',
data: { confirm: "Are you sure you want to delete this rubric item?" } %>
</button>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix nested interactive elements in the delete button.

The current implementation nests a link inside a button element, which is not recommended for accessibility and can cause inconsistent behavior across browsers.

Modify the markup to avoid nesting interactive elements:

  <div class="action_buttons">
    <%= f.submit "Save Rubric Item", { class: "btn primary" } %>
-   <button class="delete_btn">
-     <%= link_to "Delete this Rubric Item", [@course, @assessment, @problem, @rubric_item],
-                 method: :delete, class: 'btn btn-danger',
-                 data: { confirm: "Are you sure you want to delete this rubric item?" } %>
-   </button>
+   <%= link_to "Delete this Rubric Item", [@course, @assessment, @problem, @rubric_item],
+               method: :delete, class: 'btn btn-danger delete_btn',
+               data: { confirm: "Are you sure you want to delete this rubric item?" } %>
+   <%= link_to "Cancel", course_assessment_problem_path(@course, @assessment, @problem), class: "btn" %>
  </div>

This change also adds a cancel button for better user experience.

🤖 Prompt for AI Agents (early access)
In app/views/rubric_items/edit.html.erb around lines 14 to 21, the delete link is nested inside a button element, which is invalid HTML and harms accessibility. Remove the outer button tag and apply the button classes directly to the link_to helper to make it a styled link button. Additionally, add a separate cancel button outside this block for improved user experience.

Comment on lines +31 to +38
# Calculate the sum of all assigned rubric items for this problem
assigned_points = @submission.rubric_item_assignments
.joins(:rubric_item)
.where(assigned: true, rubric_items: {
problem_id: @rubric_item.problem_id
})
.sum('rubric_items.points')

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Reuse the shared ScoreCalculation concern instead of manual SUM

You already centralised score logic in ScoreCalculation. Replicating the sum here risks divergence. Consider:

score.update_score!

(or move the computation into a service).

🤖 Prompt for AI Agents (early access)
In app/controllers/rubric_item_assignments_controller.rb around lines 31 to 38, the code manually calculates the sum of assigned rubric item points, duplicating logic already centralized in the ScoreCalculation concern. To fix this, replace the manual sum calculation with a call to the shared method, such as invoking score.update_score! to update the score using the existing centralized logic, ensuring consistency and avoiding code duplication.

Comment on lines +862 to +867
assignment = RubricItemAssignment.find_or_initialize_by(
rubric_item_id: item.id,
submission_id: @submission.id
)
item_data[:assigned] = assignment.assigned

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

N × M queries – find_or_initialize_by defeats preloading

@rubric_item_assignments is preloaded above, yet every loop iteration still issues RubricItemAssignment.find_or_initialize_by, re-hitting the DB. Replace with a hash lookup:

assignment = @rubric_item_assignments[item.id]
item_data[:assigned] = assignment&.assigned || false

This gives O(1) hash look-ups and avoids redundant queries.

🤖 Prompt for AI Agents (early access)
In app/controllers/submissions_controller.rb around lines 862 to 867, the code uses find_or_initialize_by inside a loop, causing repeated database queries despite preloading @rubric_item_assignments. Replace this with a hash lookup by accessing the assignment from @rubric_item_assignments using item.id as the key, then set item_data[:assigned] to assignment&.assigned or false if nil. This change avoids redundant queries by leveraging the preloaded data.

Comment on lines +584 to +600
# Make sure @problems is initialized early
@problems = @assessment.problems.ordered.to_a

# Initialize problem-related hashes
@problemAnnotations = {}
@problemMaxScores = {}
@problemScores = {}
@problemNameToId = {}

# Initialize all problems
@problems.each do |problem|
@problemAnnotations[problem.name] ||= []
@problemMaxScores[problem.name] ||= problem.max_score
@problemScores[problem.name] ||= 0
@problemNameToId[problem.name] ||= problem.id
end

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Duplicate initialisation of @problemAnnotations et al.

The same four hashes are initialised twice (lines 584-600 and again 808-812).
Apart from the wasted cycles, if later code mutates only one copy it can become a debugging nightmare. Initialise once and reuse.

-    # first copy (keep)
-
-    # … later …
-    @problemAnnotations = {}
-    @problemMaxScores = {}
-    @problemScores     = {}
-    @problemNameToId   = {}
+    # hashes are already initialised earlier – drop this block

Also applies to: 808-812

🤖 Prompt for AI Agents (early access)
In app/controllers/submissions_controller.rb around lines 584 to 600 and again at 808 to 812, the four hashes @problemAnnotations, @problemMaxScores, @problemScores, and @problemNameToId are being initialized twice. To fix this, remove the duplicate initialization at lines 808 to 812 and ensure the hashes are only initialized once early on, then reused throughout the code to avoid redundant processing and potential state inconsistencies.

Comment on lines +845 to +849
annotations_data = @annotations.select { |a| a.problem_id == problem.id }
annotations_data.select(&:global_comment)
file_annotations = annotations_data.reject(&:global_comment)
file_annotations.group_by { |a| a.filename || "" }

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Dead code – selections are thrown away

annotations_data.select(&:global_comment) and the following file_annotations.group_by … are executed but their results are never stored, so they do nothing except waste CPU.

-      annotations_data.select(&:global_comment)
-      file_annotations = annotations_data.reject(&:global_comment)
-      file_annotations.group_by { |a| a.filename || "" }
+      # compute once; value is reused later
+      global_annotations = annotations_data.select(&:global_comment)
+      file_annotations   = annotations_data.reject(&:global_comment)

If the values are not needed here, remove the lines entirely.

📝 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
annotations_data = @annotations.select { |a| a.problem_id == problem.id }
annotations_data.select(&:global_comment)
file_annotations = annotations_data.reject(&:global_comment)
file_annotations.group_by { |a| a.filename || "" }
annotations_data = @annotations.select { |a| a.problem_id == problem.id }
# compute once; value is reused later
global_annotations = annotations_data.select(&:global_comment)
file_annotations = annotations_data.reject(&:global_comment)
🤖 Prompt for AI Agents (early access)
In app/controllers/submissions_controller.rb around lines 845 to 849, the calls to annotations_data.select(&:global_comment) and file_annotations.group_by { |a| a.filename || "" } produce results that are not assigned or used, making them dead code. Remove these lines entirely if their results are not needed to avoid unnecessary computation.

Comment on lines +20 to +42
def get_problem_annotations(problem_id, global_annotations_data, annotations_by_file_data)
problem_annotations = []

# Add global annotations for this problem
if global_annotations_data.present?
global_annotations_data.each do |annotation_data|
id = annotation_data[4]
annotation = Annotation.find_by(id: id)
problem_annotations << annotation if annotation && annotation.problem_id == problem_id
end
end

# Add file-specific annotations for this problem
annotations_by_file_data.each do |_, file_annotations|
file_annotations.each do |annotation_data|
id = annotation_data[4]
annotation = Annotation.find_by(id: id)
problem_annotations << annotation if annotation && annotation.problem_id == problem_id
end
end

problem_annotations.compact
end
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize to prevent N+1 query performance issues

This method performs individual database queries within loops, which can lead to performance problems with large datasets.

Consider refactoring to fetch annotations in bulk:

def get_problem_annotations(problem_id, global_annotations_data, annotations_by_file_data)
-  problem_annotations = []
+  annotation_ids = []
  
  # Collect IDs from global annotations for this problem
  if global_annotations_data.present?
-    global_annotations_data.each do |annotation_data|
-      id = annotation_data[4]
-      annotation = Annotation.find_by(id: id)
-      problem_annotations << annotation if annotation && annotation.problem_id == problem_id
-    end
+    global_annotations_data.each do |annotation_data|
+      annotation_ids << annotation_data[4]
+    end
  end
  
  # Collect IDs from file-specific annotations for this problem
-  annotations_by_file_data.each do |_, file_annotations|
-    file_annotations.each do |annotation_data|
-      id = annotation_data[4]
-      annotation = Annotation.find_by(id: id)
-      problem_annotations << annotation if annotation && annotation.problem_id == problem_id
-    end
-  end
+  annotations_by_file_data.each do |_, file_annotations|
+    file_annotations.each do |annotation_data|
+      annotation_ids << annotation_data[4]
+    end
+  end
  
-  problem_annotations.compact
+  # Fetch all annotations in a single query and filter by problem_id
+  Annotation.where(id: annotation_ids, problem_id: problem_id)
end
🧰 Tools
🪛 RuboCop (1.73)

[convention] 20-42: Cyclomatic complexity for get_problem_annotations is too high. [9/7]

(Metrics/CyclomaticComplexity)


[convention] 20-42: Perceived complexity for get_problem_annotations is too high. [9/8]

(Metrics/PerceivedComplexity)

🤖 Prompt for AI Agents (early access)
In app/helpers/annotations_helper.rb around lines 20 to 42, the method performs individual Annotation.find_by calls inside loops, causing N+1 query issues. Refactor by first collecting all annotation IDs from global_annotations_data and annotations_by_file_data, then fetch all annotations in a single query using Annotation.where(id: ids). Finally, filter the fetched annotations by problem_id and return the result, eliminating the per-annotation database queries inside the loops.

Comment on lines +69 to +93
def get_non_rubric_annotations(problem_id, global_annotations_data, annotations_by_file_data)
# Process global annotations
filtered_global_annotations = []
global_annotations_data.each do |description, value, line, user, id, position, filename, shared, global|
annotation = Annotation.find_by(id: id)
if annotation && annotation.problem_id == problem_id && annotation.rubric_item_id.nil?
filtered_global_annotations << [description, value, line, user, id, position, filename, shared, global]
end
end

# Process file annotations
filtered_annotations_by_file = {}
annotations_by_file_data.each do |filename, annotations|
file_annotations = []
annotations.each do |description, value, line, user, id, position, filename, global|
annotation = Annotation.find_by(id: id)
if annotation && annotation.problem_id == problem_id && annotation.rubric_item_id.nil?
file_annotations << [description, value, line, user, id, position, filename, global]
end
end
filtered_annotations_by_file[filename] = file_annotations unless file_annotations.empty?
end

[filtered_global_annotations, filtered_annotations_by_file]
end
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize database queries to prevent N+1 issues

Similar to the get_problem_annotations method, this one also performs individual database queries within loops, which can be inefficient.

Consider refactoring to fetch annotations in bulk:

def get_non_rubric_annotations(problem_id, global_annotations_data, annotations_by_file_data)
+  # Collect all annotation IDs
+  annotation_ids = []
+  global_annotation_ids_map = {}
+  file_annotation_ids_map = {}
+  
+  global_annotations_data.each do |data|
+    id = data[4]
+    annotation_ids << id
+    global_annotation_ids_map[id] = data
+  end
+  
+  annotations_by_file_data.each do |filename, annotations|
+    file_annotation_ids_map[filename] ||= {}
+    annotations.each do |data|
+      id = data[4]
+      annotation_ids << id
+      file_annotation_ids_map[filename][id] = data
+    end
+  end
+  
+  # Fetch all relevant annotations in one query
+  relevant_annotations = Annotation.where(id: annotation_ids, problem_id: problem_id, rubric_item_id: nil)
+  relevant_ids = relevant_annotations.pluck(:id).to_set
+  
+  # Process global annotations
+  filtered_global_annotations = []
+  global_annotations_data.each do |data|
+    id = data[4]
+    filtered_global_annotations << data if relevant_ids.include?(id)
+  end
+  
+  # Process file annotations
+  filtered_annotations_by_file = {}
+  annotations_by_file_data.each do |filename, annotations|
+    file_annotations = []
+    annotations.each do |data|
+      id = data[4]
+      file_annotations << data if relevant_ids.include?(id)
+    end
+    filtered_annotations_by_file[filename] = file_annotations unless file_annotations.empty?
+  end
+  
+  [filtered_global_annotations, filtered_annotations_by_file]

- # Process global annotations
- filtered_global_annotations = []
- global_annotations_data.each do |description, value, line, user, id, position, filename, shared, global|
-   annotation = Annotation.find_by(id: id)
-   if annotation && annotation.problem_id == problem_id && annotation.rubric_item_id.nil?
-     filtered_global_annotations << [description, value, line, user, id, position, filename, shared, global]
-   end
- end
- 
- # Process file annotations
- filtered_annotations_by_file = {}
- annotations_by_file_data.each do |filename, annotations|
-   file_annotations = []
-   annotations.each do |description, value, line, user, id, position, filename, global|
-     annotation = Annotation.find_by(id: id)
-     if annotation && annotation.problem_id == problem_id && annotation.rubric_item_id.nil?
-       file_annotations << [description, value, line, user, id, position, filename, global]
-     end
-   end
-   filtered_annotations_by_file[filename] = file_annotations unless file_annotations.empty?
- end
- 
- [filtered_global_annotations, filtered_annotations_by_file]
end
🧰 Tools
🪛 RuboCop (1.73)

[convention] 69-93: Assignment Branch Condition size for get_non_rubric_annotations is too high. [<25, 15, 12> 31.53/23]

(Metrics/AbcSize)


[convention] 69-93: Cyclomatic complexity for get_non_rubric_annotations is too high. [11/7]

(Metrics/CyclomaticComplexity)


[convention] 69-93: Perceived complexity for get_non_rubric_annotations is too high. [11/8]

(Metrics/PerceivedComplexity)


[convention] 72-72: Avoid parameter lists longer than 5 parameters. [9/5]

(Metrics/ParameterLists)


[convention] 83-83: Avoid parameter lists longer than 5 parameters. [8/5]

(Metrics/ParameterLists)

🤖 Prompt for AI Agents (early access)
In app/helpers/annotations_helper.rb between lines 69 and 93, the method get_non_rubric_annotations performs individual Annotation.find_by calls inside loops, causing N+1 query issues. To fix this, refactor the method to first collect all annotation IDs from global_annotations_data and annotations_by_file_data, then fetch all corresponding Annotation records in a single query using Annotation.where(id: ids).index_by(&:id). Use this hash to look up annotations inside the loops instead of querying the database repeatedly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants