-
Notifications
You must be signed in to change notification settings - Fork 229
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
base: master
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThis 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
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
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this 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: simplifynil
check and character removal
You can make the assignment more concise by leveraging safe navigation andString#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: trueapp/views/problems/_fields.html.erb (1)
41-49
: Consider adding tooltips to the action iconsThe 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 columnSince 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 optionTo 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 surprisesUsing
default_scope
means everyRubricItem
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:) endThis 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
insidesubmissions
exposes routes likenew
,edit
,destroy
, which conceptually belong to instructors operating on problems, not graders operating on a single student’s submission. Consider scoping this nested resource toonly: [:index]
or even eliminating it if the toggle route suffices.app/models/annotation.rb (1)
31-35
: Remove unused localscore
variable
update_score
recomputes the score internally, so thescore = …
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 meanproblem_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 meanproblem_id
?(Lint/UselessAssignment)
app/assets/javascripts/annotations.js (2)
598-641
: Memory-leak & UX glitch – repeatedly re-creating autocomplete widgetsEvery
change
on.problem-id
callsautocomplete()
again, spawning a brand-new widget and leaving the old handlers in memory. Use the documentedautocomplete("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 setsfilename
; adding it again in the object spread is harmless but unnecessary noise. More importantly, the new attributerubric_item_id
is always sent, even when it’snull
, 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
: Considerremote: true
to toggle rubric items without full page refreshEach rubric checkbox submits a synchronous POST which reloads the entire submission view.
Addingremote: true
toform_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 creationThe 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
withfind_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 definitionThere'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
⛔ 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 fromcolumn.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 functionalityThe 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 itemsThe 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 itemsThis 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 defaultRails 6.1+’s
add_reference :annotations, :rubric_item, foreign_key: true
will automatically create an index onrubric_item_id
(unless you explicitly passindex: 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:
- Clear associations with both
RubricItem
andSubmission
- A proper uniqueness validation to prevent duplicate assignments
- An
after_save
callback to automatically update scores- Inclusion of the
ScoreCalculation
module for shared score calculation logicThe 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 attributesAdding explicit
id
andname
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/javascriptLength 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 tosubmitNewAnnotation
- app/assets/javascripts/annotations.js, lines 1379–1389:
submitNewAnnotation
signature updated to includerubric_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:
- Direct association to rubric_item_assignments with dependent destruction
- 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 thepoints
domain
validates :points, numericality: true
accepts any numeric value, includingNaN
,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 callsIf
score.grader_id
must always be present, ensure@cud
exists or fall back tocurrent_user
.app/models/annotation.rb (1)
41-41
: Skip autograded guard whenscore
was not yet persisted
score.grader_id == 0
works only when aScore
exists. Ifupdate_score
creates a new score for an autograded problem, this guard never fires becausescore
is nil at this point. Verify thatupdate_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 tableThe 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 tableThe 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 constraintsThe 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' |
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
# 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 |
There was a problem hiding this comment.
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):
- Remove the line entirely if the
assigned
flag was never used. - 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.
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
# 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') | ||
|
There was a problem hiding this comment.
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.
assignment = RubricItemAssignment.find_or_initialize_by( | ||
rubric_item_id: item.id, | ||
submission_id: @submission.id | ||
) | ||
item_data[:assigned] = assignment.assigned | ||
|
There was a problem hiding this comment.
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.
# 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 | ||
|
There was a problem hiding this comment.
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.
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 || "" } | ||
|
There was a problem hiding this comment.
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.
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.
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 |
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
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.
Our team worked on a feature to add a rubric system to assessments in Autolab (#1716). Essentially this allows instructors and TAs to:
Images
Description
This involved changes across the entire stack:
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
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:
overcommit --install && overcommit --sign
to use pre-commit hook for lintingOther issues / help required
If unsure, feel free to submit first and we'll help you along.