Manage Submissions

[KesterTan]

Description

This is an initial PR for the revamp of Manage Submissions.

How Has This Been Tested?

Navigate to Manage Submissions. Test that all buttons work as expected.

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)

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

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request introduces comprehensive enhancements to the Autolab platform's submission and annotation management system. The changes span multiple components, including JavaScript modules, controllers, views, and stylesheets. Key improvements focus on annotation handling, submission management, and user interface refinements. The modifications introduce new methods for batch submission operations, enhanced annotation functionality, and improved styling across various components of the application.

Changes

File	Change Summary
app/assets/javascripts/*	Added multiple JavaScript modules for annotations, submissions, and component management with new functions for handling annotations, shared comments, and UI interactions
app/assets/stylesheets/*	Introduced new CSS variables, updated styling for submissions, annotations, and added new classes for improved UI components
app/controllers/*	Enhanced controllers for annotations, assessments, and submissions with new methods for batch operations, excuse management, and improved data handling
app/views/*	Updated view templates to support new annotation and submission management features, including new popover and form modifications
config/routes.rb	Added new routes for submission-related batch operations and management functionalities
db/schema.rb	Added has_svn boolean column to assessments table

Sequence Diagram

sequenceDiagram
    participant User
    participant SubmissionsController
    participant AnnotationsController
    participant Submission
    participant Annotation

    User->>SubmissionsController: Request batch operations
    SubmissionsController->>Submission: Process batch actions
    Submission-->>SubmissionsController: Return operation results
    SubmissionsController->>User: Provide feedback

    User->>AnnotationsController: Create/Update Annotation
    AnnotationsController->>Annotation: Save annotation
    Annotation-->>AnnotationsController: Confirm save
    AnnotationsController->>User: Return annotation details

Loading

Possibly related PRs

• Second Batch of Submissions Tests #2132: Enhances testing framework for SubmissionsController, which complements the submission management improvements in this PR.

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

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. (Beta)
• @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.

[coderabbitai]

Actionable comments posted: 6

🧹 Nitpick comments (49)

app/assets/javascripts/autolab_component.js (4)

1-24: Great usage documentation.

Including an inline usage guide within the code is very helpful for future maintainers and consumers. Consider adopting JSDoc or a similar standard for even more structured, discoverable documentation.

---

27-31: Consider validating element existence.

It can be beneficial to validate whether the DOM element with elementId exists before proceeding. This ensures your component will not silently fail if a non-existent element ID is specified. For instance:

 function AutolabComponent(elementId, initialState = {}, template = () => {}) {
-  this.elementId = elementId;
+  if (!document.getElementById(elementId)) {
+    console.warn(`Element with ID "${elementId}" not found.`);
+  }

---

32-35: Consider using a deep merge if needed.

$.extend defaults to a shallow merge. If your state structure is nested, or you anticipate needing a deeper merge, you might want to use { deep: true } or consider using another library (e.g., Lodash merge) to prevent unintended references.

---

37-40: Provide a more descriptive default template.

The default template returns an empty <div>, which can be confusing if someone expects rendering. It might be more user-friendly to display a short placeholder message or instructions for overriding.

app/views/submissions/index.html.erb (6)

5-8: Encapsulate or version-check external libraries.
These stylesheet links reference external dependencies (jquery-ui). Consider pinning specific versions or integrating via a package manager (e.g., Yarn, npm) for better maintainability and reproducibility.

---

10-39: Avoid polluting the global JS namespace.
This inline script sets several global variables (is_autograded, excused_cids, etc.). Consider wrapping them in a namespace or an IIFE (Immediately Invoked Function Expression) to reduce potential naming collisions.

---

73-79: Clarify the TODO comment on filtering missing submissions.
The code references a future “missing submissions” filter feature. If it’s not part of this PR’s scope, consider opening an issue to track its progress.

---

88-123: Improve discoverability of batch actions.
Hiding the batch action buttons in placeholder divs is a good approach to keep the DOM minimal. Just ensure they remain keyboard-accessible and that toggling their visibility doesn’t conflict with screen readers.

---

257-264: Modal structure and naming are consistent.
The modal for score details is straightforward. Consider lazy-loading content to reduce the page’s initial payload, if performance is a concern for large classes.

---

267-272: Annotation modal usage aligns with the new annotation system.
Rendering _annotation_popup inside of a modal helps keep the code modular. Make sure accessibility guidelines (focus management, ARIA roles) are followed for a seamless user experience.

app/assets/javascripts/manage_submissions.js (6)

10-21: Minor opportunity to reduce inline HTML in JS strings.
EditTweakButton returns multiline HTML. Consider breaking out into a small templating function or partial for maintainability and clarity.

---

23-37: Use short-circuit for error handling in get_score_details.
Your promise logic is correct, but consider adding a .fail or .catch block in addition to the existing error callback for thorough coverage.

---

66-68: Initializing modals immediately.
$('.modal').modal(); on document ready is standard. Confirm that no subsequent dynamic content injection is interfering with the materialize modal instance.

---

115-148: Inlined logic for download vs. view button generation.
While functional, it might be more readable to refactor or unify the button creation logic in a small helper function to avoid repetition.

---

177-220: DataTables usage with custom ordering and hidden search features.
The approach is fine. If performance becomes an issue with large data sets or many DOM manipulations, consider server-side processing or specialized DataTables modes.

---

370-381: “only-latest” filter.
The DataTables custom filter is straightforward. If you expand filtering options (like date range or specific user), consider a uniform approach with a single function.

app/controllers/submissions_controller.rb (6)

39-72: Graceful error handling in score_details.
The JSON structure is well-formed, but inline rescue returning nil after rendering might hide logic flow issues. Logging or re-raising the error after responding could aid debugging.

---

195-240: Batch destroy logic and permission checks.
Your loop checks if the user “has permission” each time. This is correct but might be more concise with a single scoping query that filters out submissions the current user cannot delete.

---

Line range hint 270-290: Check for @assessment validity.
Returning early if no submissions exist helps avoid generating an empty zip. Keep in mind that showing an explicit message might be better than an abrupt return for the user’s clarity.

---

299-301: No explicit feedback on empty selections.
If submissions.empty? is true, we simply return. For user-friendliness, consider a flash or redirect to inform them that no valid submissions were found.

---

327-370: Download batch logic.
This mostly mirrors download_all. Consider deduplicating shared logic (e.g., creation of filedata, permission checks) into a helper to reduce future maintenance effort.

---

434-460: Unexcuse flow.
Looks consistent with excuse flow. If submission is nil or aud is nil, consider quickly failing with a user-facing message rather than quietly proceeding.

app/views/assessments/_excuse_popover.html.erb (1)

9-9: Consider adding an explicit href or event handler for the "CANCEL" button.
Currently, the "CANCEL" button has no href or JavaScript event handler. If it is intended to close the popover or navigate back, consider adding the appropriate href or click handler for clarity and user guidance.

spec/features/manage_submissions_spec.rb (1)

31-31: Expand or remove the TODO-style comment to ensure clarity of test coverage.
A single-line comment stating “Check user flow for adding tweaks is okay” provides limited context. Consider elaborating on the expected behavior or removing the comment if it’s already captured by actual test code.

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

26-31: Enrich the conditional select element with labeling and default states.
When popup is true, the problem ID select field appears. Consider adding a label for accessibility (e.g., <label for="problem-id">) and a default “Select a problem” placeholder option to guide users, especially if multiple problems share similar names.

app/assets/javascripts/annotations_helpers.js (3)

63-63: Use Number.isNaN instead of global isNaN

isNaN can produce unexpected results due to type coercion. Prefer Number.isNaN for safer numeric checks.

-  if (isNaN(n)) n = 0;
+  if (Number.isNaN(n)) n = 0;

🧰 Tools

🪛 Biome (1.9.4)

[error] 63-63: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

---

73-73: Avoid redeclaring function parameter

You are shadowing the problem_id function parameter by redeclaring it using var. Rename the local variable to avoid potential confusion.

function getProblemNameWithId(problem_id) {
-  var problem_id = parseInt(problem_id, 10);
+  const parsedProblemId = parseInt(problem_id, 10);
   var problem = _.findWhere(problems, { id: parsedProblemId });
   if (problem == undefined) return "Deleted Problem(s)";
   return problem.name;
}

🧰 Tools

🪛 Biome (1.9.4)

[error] 73-73: Shouldn't redeclare 'problem_id'. Consider to delete it or rename it.

'problem_id' is defined here:

(lint/suspicious/noRedeclare)

---

[error] 73-73: This variable is used before its declaration.

The variable is declared here:

(lint/correctness/noInvalidUseBeforeDeclaration)

---

85-85: Replace hasOwnProperty with Object.hasOwn()

Silence the linter warning and align with recommended safer practices by calling Object.hasOwn().

-      if (a.hasOwnProperty(attr))
+      if (Object.hasOwn(a, attr))

🧰 Tools

🪛 Biome (1.9.4)

[error] 85-85: Do not access Object.prototype method 'hasOwnProperty' from target object.

It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.

(lint/suspicious/noPrototypeBuiltins)

spec/features/assessment_spec.rb (1)

103-104: XPath usage clarity

Using ./parent::td is a workable approach. However, consider a more direct selector for improved clarity (e.g., selecting the <tr> directly). This is optional, as the current test still appears maintainable.

app/assets/javascripts/annotations_popup.js (4)

1-7: Consider adding error-handling or retry logic.

updateEditTweakButtons() calls get_tweak_total without a .catch() block. If any request fails, there is no fallback path or messaging for the user. You might also want to display partial results while skipping the failures to improve resiliency.

---

8-23: Use jQuery deferred or fetch-based promises directly.

get_tweak_total() wraps $.ajax() in a manually created Promise. Instead, consider using jQuery’s built-in promise methods (.done(), .fail()) or fetch() + async/await to streamline code and reduce potential promise mishandling.

---

24-103: Consider breaking down the function for maintainability.

newAnnotationFormCode() handles clone creation, UI event wiring, problem list generation, and form validation in a single block. Extract smaller helper functions (e.g., populateProblemSelect(), bindAnnotationEvents()) for improved readability and testability.

---

138-157: Add more granular success feedback to users.

updateAnnotation() updates an annotation via AJAX but removes the UI only on success. Consider indicating a loading state before success and a confirmation message afterward, to assure users that their update was applied.

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

1-1: Enhance clarity with a short comment.

You’re conditionally setting the page title to "Annotate Submission" only if the user is an instructor or course assistant. A brief comment explaining this logic can help future maintainers quickly identify why the title changes based on roles.

config/routes.rb (1)

223-224: Prevent confusion of route naming.

excuse_popover is introduced here, while excuse_batch and unexcuse appear at the collection level. Ensure naming is consistent and that it accurately reflects each action’s scope and behavior (singular vs. batch).

spec/controllers/submissions_controller_spec.rb (3)

11-15: Avoid unused lookups or store them in variables for clarity.
Currently, Course.find(cid).name and Assessment.find(assessment_id).name are being called, but their return values are not used or stored. If the intent is simply to verify presence, consider adding assertions or storing the names in a variable for clarity.

---

25-30: Reduce duplication of retrieval logic.
This block replicates the logic from lines 11–15 above to fetch cud, cid, assessment_id, and names. Consider extracting these lookups into a helper method or a before block to reduce duplication.

---

163-163: Prevent potential duplication in “destroy-confirm-check”.
This line mirrors the logic in line 150. If you anticipate different behavior in different contexts, clarify or factor it out to a shared line. Otherwise, ensure the parameter presence is strictly enforced in the controller.

app/models/submission.rb (1)

399-402: Guard output in as_json more thoroughly.
You’re already conditionally exposing scores and penalties only if seen_by is present. Consider logging or handling the scenario where seen_by isn’t provided or is unexpected.

app/assets/stylesheets/_variables.scss (1)

14-23: Maintain a consistent naming convention for newly added color variables.

The new variables greatly enhance maintainability and consistency. However, notice that some variables use “gray” while others use “grey.” Standardizing on one naming approach (e.g., “grey”) may improve consistency across the codebase.

app/assets/stylesheets/datatable.adapter.css (6)

5-7: Enhance layout consistency around .dataTables_filter.

The added padding and margin help spacing, but verify that the left float in .dataTables_filter still meets user interface expectations across different viewport sizes and themes.

---

10-11: Ensure consistent font usage for .dataTables_filter input.

Specifying "Source Sans Pro", sans-serif directly here can cause inconsistency if other components rely on a shared font variable. Consider centralizing font declarations in a shared stylesheet or _variables.scss for maintainability.

---

41-42: Duplicate cursor property.

.paginate_button, .paginate_active sets cursor twice: pointer and hand. The hand is non-standard in modern CSS. Remove or unify the cursor property to avoid confusion.

 a.paginate_button,
 a.paginate_active {
   display: inline-block;
   padding: 2px 4px;
   margin-left: 2px;
-  cursor: pointer;
-  cursor: hand;
+  cursor: pointer;
 }

---

49-53: Improve button container alignment.

The .dt-buttons container uses flex-wrap: wrap with a fixed height of 75px. Depending on content, buttons may overflow or appear truncated. Consider removing the fixed height if you want truly responsive wrap behavior.

---

56-58: Spacing between buttons.

.dt-button { margin-right: 10px !important; } helps keep buttons tidy, but watch out for unintended spacing if the design eventually changes or if other utilities set global margins. Consolidate margin in one place to maintain layout consistency.

---

64-65: Review margin around icons.

.dt-button i { margin: 0 6px 0 4px; } ensures separation from surrounding text or elements, but if you add more icons, an inconsistent spacing approach could complicate design. Consider a reusable or variables-based approach for icon spacing too.

app/assets/stylesheets/style.css.scss (3)

825-839: Consider using CSS custom properties for magic numbers.

The styles are well-structured, but consider extracting magic numbers into CSS custom properties for better maintainability.

+:root {
+  --submissions-td-padding: 5px;
+  --checkbox-left-spacing: 6px;
+  --checkbox-checked-left-spacing: 3px;
+}
+
 .submissions-td {
   border-style: none;
-  padding: 5px 0 5px 0;
+  padding: var(--submissions-td-padding) 0;
 }

 .submissions-cbox-label {
   display: flex;
   justify-content: center;
   span::before {
-    left: 6px;
+    left: var(--checkbox-left-spacing);
   };
   [type="checkbox"]:checked + span:not(.lever):before {
-    left: 3px;
+    left: var(--checkbox-checked-left-spacing);
   };
 }

---

1129-1136: Use design system variables for colors.

Replace hardcoded color values with design system variables for consistency.

 .switch > label > b {
   font-size: 1.1rem;
-  color: black
+  color: $autolab-black;
 }

 label[for="switch"] {
   font-size: 1rem;
-  color: darkslategrey;
+  color: $autolab-dark-grey;
 }

---

1560-1724: Well-structured styles with room for improvement.

The manage submissions styles are well-organized and follow BEM naming convention. Consider these improvements:

1. Simplify long selectors
2. Extract magic numbers to CSS custom properties
3. Consolidate button styles

+:root {
+  --submissions-button-padding: 5px;
+  --submissions-icon-spacing: 6px;
+  --submissions-border-radius: 11px;
+  --submissions-popover-padding: 12px;
+}
+
+/* Base button styles */
+.btn--submissions {
+  margin: 0;
+  padding: var(--submissions-button-padding);
+  display: flex;
+  align-items: center;
+}
+
 .btn.submissions-main {
-  margin: 0;
+  @extend .btn--submissions;
   padding: 5px 10px 5px 5px;
   min-height: 36px;
   height: auto;
   line-height: 1.3;
-  display: flex;
-  align-items: center;
 }

 .btn.submissions-selected {
-  margin: 0;
+  @extend .btn--submissions;
   margin-right: 10px;
   padding: 0px 10px 0px 5px;
-  display: flex;
 }

The styles are functional and maintainable, but these improvements would make them even more robust.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 8c81601 and 4854c5c.

📒 Files selected for processing (34)

• app/assets/javascripts/annotations_helpers.js (1 hunks)
• app/assets/javascripts/annotations_popup.js (1 hunks)
• app/assets/javascripts/autolab_component.js (1 hunks)
• app/assets/javascripts/manage_submissions.js (1 hunks)
• app/assets/stylesheets/_variables.scss (1 hunks)
• app/assets/stylesheets/annotations.scss (0 hunks)
• app/assets/stylesheets/assessments.scss (1 hunks)
• app/assets/stylesheets/assessments/quiz.css.scss (1 hunks)
• app/assets/stylesheets/datatable.adapter.css (2 hunks)
• app/assets/stylesheets/gradesheet.css.scss (2 hunks)
• app/assets/stylesheets/instructor_gradebook.scss (1 hunks)
• app/assets/stylesheets/manage_submissions.css (1 hunks)
• app/assets/stylesheets/style.css.scss (10 hunks)
• app/controllers/annotations_controller.rb (1 hunks)
• app/controllers/assessments_controller.rb (2 hunks)
• app/controllers/submissions_controller.rb (10 hunks)
• app/form_builders/form_builder_with_date_time_input.rb (1 hunks)
• app/helpers/application_helper.rb (2 hunks)
• app/models/assessment.rb (1 hunks)
• app/models/submission.rb (2 hunks)
• app/views/assessments/_excuse_popover.html.erb (1 hunks)
• app/views/assessments/show.html.erb (1 hunks)
• app/views/submissions/_annotation.html.erb (1 hunks)
• app/views/submissions/_annotation_form.html.erb (1 hunks)
• app/views/submissions/_annotation_global.html.erb (1 hunks)
• app/views/submissions/_annotation_popup.html.erb (1 hunks)
• app/views/submissions/index.html.erb (1 hunks)
• app/views/submissions/view.html.erb (1 hunks)
• config/initializers/assets.rb (1 hunks)
• config/routes.rb (2 hunks)
• db/schema.rb (1 hunks)
• spec/controllers/submissions_controller_spec.rb (5 hunks)
• spec/features/assessment_spec.rb (1 hunks)
• spec/features/manage_submissions_spec.rb (1 hunks)

💤 Files with no reviewable changes (1)

• app/assets/stylesheets/annotations.scss

✅ Files skipped from review due to trivial changes (5)

• app/models/assessment.rb
• app/assets/stylesheets/assessments.scss
• app/assets/stylesheets/gradesheet.css.scss
• app/assets/stylesheets/manage_submissions.css
• app/form_builders/form_builder_with_date_time_input.rb

🧰 Additional context used

🪛 Biome (1.9.4)

app/assets/javascripts/annotations_helpers.js

[error] 85-85: Do not access Object.prototype method 'hasOwnProperty' from target object.

It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.

(lint/suspicious/noPrototypeBuiltins)

---

[error] 73-73: Shouldn't redeclare 'problem_id'. Consider to delete it or rename it.

'problem_id' is defined here:

(lint/suspicious/noRedeclare)

---

[error] 73-73: This variable is used before its declaration.

The variable is declared here:

(lint/correctness/noInvalidUseBeforeDeclaration)

---

[error] 63-63: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

🔇 Additional comments (42)

app/views/submissions/index.html.erb (6)

56-72: Enhanced button UI for submission management.
These changes introduce new submission management buttons. Be sure to confirm that each button’s route or path is only displayed to authorized users, especially for operations like “Download Final Submissions.”

---

80-86: Good addition for managing extensions.
The “Manage Extensions” link is straightforward and consistent. Double-check user role checks (instructor vs. assistant) to ensure only authorized roles can manage extensions.

---

126-155: Sortable columns logic looks consistent.
The dynamic insertion of icons and listening for sort actions is workable. Watch out for potential indexing issues if a column’s label changes, since you rely on exact string matches.

---

230-250: Confirm autograded logic.
This block checks @autograded to conditionally show a “Regrade” icon. Ensure that regrading is locked behind instructor or CA roles to avoid revealing it to unauthorized users.

---

159-186: Ensure correct reference to ignored_submission_style helper.
The inline style calls ignored_submission_style submission. Verify that the helper is well-defined and handles edge cases (e.g., submission is nil).

---

44-49: Validate external script dependencies.
Check that each included JavaScript plugin (e.g., datatables-buttons) is up-to-date and compatible with the existing code. Outdated plugins can introduce performance or security concerns.

✅ Verification successful

Let me gather more information about the JavaScript dependencies.

---

Let me check the content of these JavaScript files to understand the dependency usage better.

---

Let me check how these external scripts are being loaded by looking at the helper implementation.

---

External script dependencies are properly managed through CDN with specific versions

Based on the verification, the external JavaScript dependencies are well-managed through Cloudflare's CDN with explicit version pinning in the external_javascript_include_tag helper:

• jquery-ui: v1.13.2 (latest stable version)
• jquery.dataTables: v1.13.4 (recent stable version)
• datatables-buttons: loaded from DataTables CDN (follows the DataTables version)

The implementation shows careful version management and use of official CDN sources, which helps with both security and performance. The dependencies are up-to-date and compatible with each other.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Searching for plugin versions mentioned in the codebase
rg "datatables-buttons" -A 2

Length of output: 307

---

Script:

#!/bin/bash
# Check for version information in package.json or similar files
fd -e json | xargs rg -l "datatables|jquery-ui"

# Look for any configuration files related to these dependencies
fd "config|webpack" | xargs rg -l "datatables|jquery-ui"

# Check for any asset manifest files
fd "manifest|assets" | xargs rg -l "datatables|jquery-ui"

Length of output: 297

---

Script:

#!/bin/bash
# Check gradesheet.js.erb for DataTables/jQuery UI usage
rg -A 3 "datatables|jquery-ui|DataTable" app/assets/javascripts/gradesheet.js.erb

# Check gradebook.js for DataTables/jQuery UI usage
rg -A 3 "datatables|jquery-ui|DataTable" app/assets/javascripts/gradebook.js

# Look for external script loading patterns
rg "external_javascript_include_tag" -A 1

Length of output: 3650

---

Script:

#!/bin/bash
# Check the implementation of external_javascript_include_tag helper
cat app/helpers/application_helper.rb

Length of output: 6943

app/assets/javascripts/manage_submissions.js (9)

1-4: Centralize endpoint definitions.
Storing endpoints in a dedicated manage_submissions_endpoints object is excellent for maintainability. Consider adding error handling or fallback logic if an endpoint is missing.

---

39-49: Good pattern for dynamic path construction.
selectSubmission updates path variables for annotations. Ensure that you sanitize or validate them if user-provided data is involved.

---

51-64: Popup invocation flow.
selectTweak triggers a modal after retrieving data. Double-check that all event handlers (.modal('open'), etc.) are properly unbound on closure to avoid memory leaks if triggered repeatedly.

---

149-176: Conditional display of submission details.
This block effectively renders each submission in a table row. Double-check that empty or invalid data for submission.filename or submission.problems is handled gracefully.

---

222-245: selectedStudentCids vs. selectedSubmissions.
Tracking separate arrays is useful but be sure to keep them in sync. If an unexpected code path modifies one without handling the other, there could be inconsistent states.

---

246-308: Batch button state toggles are well-organized.
changeButtonStates reassigns the href with query params for each button. This is a clever approach—just confirm that large sets of selected submissions don’t exceed query string limits.

---

310-362: Efficient row toggling.
The toggleRow logic is well-structured. Confirm that removing a submission’s selection also removes its course_user_datum’s ID from selectedStudentCids if no other submission from that CUD is selected.

---

363-369: Checkbox click differentiation.
Skipping event propagation for <input> elements prevents accidental row toggling, which is user-friendly. No concerns here.

---

384-459: Popover logic for excuses.
The popover flow is intuitive but be aware of potential concurrency if multiple popovers open quickly. For best UX, ensure only one popover is open at once—a scenario you’re already handling with current_popover.

app/controllers/submissions_controller.rb (5)

13-14: Checklist for newly added actions.
Including tweak_total in the before_action list is consistent, but ensure it cannot be accessed by non-instructors if that would cause unintended data disclosure.

---

22-37: Efficient data structure for @submissions_to_cud.
Building a hash of submission IDs to CUD IDs is straightforward. Performance is fine for moderate submission counts. For extremely large classes, consider building a more direct DB query approach.

---

180-187: Destructive action uses confirm-check.
This pattern is good, ensuring an intentional user action. However, if integrators bypass the UI, we rely on params["destroy-confirm-check"] still. Consider server-side role checks.

---

379-432: Excusing multiple submissions.
Implementation is clear. One small potential improvement is to handle partial successes (some but not all submissions excused) with more granular feedback.

---

867-873: Role-based redirect.
appropriate_redirect_path cleanly routes TAs vs. students. Watch for new roles in the future that might not fit this logic.

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

1-6: Popup structure is well-defined.
Using a base hidden annotation line (.base-annotation-line) is a good approach for dynamic form clones. Verify that the partial _annotation_form has everything required for the popup context, e.g., ID uniqueness.

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

3-3: Explicit popup: false parameter.
The additional argument clarifies usage in a non-popup context. Good consistency with _annotation_popup.html.erb, preventing collisions or unexpected behavior.

app/views/assessments/_excuse_popover.html.erb (1)

10-14: Verify the unexcuse path and ensure users understand the action.
Linking directly to unexcuse_course_assessment_submissions_path with method: :post indicates a destructive action, so confirm that the user is aware it’s irreversible or has the ability to undo. Consider wrapping this link in a form with explicit labeling or adding an additional confirmation prompt for enhanced clarity.

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

7-7: Confirm that passing popup: false aligns with the intended UI flow.
Adding the popup: false prop helps differentiate between in-page and popup annotations. Ensure that this updated behavior doesn’t conflict with other parts of the UI that may rely on popup-based conditions.

app/controllers/annotations_controller.rb (1)

16-20: Ensure filename fallback logic meets future file-handling needs

The conditional logic properly uses @submission.handin_file_path when filename is blank. However, verify that other relevant file paths (e.g., multiple handin files) won’t be overlooked in the future.

config/initializers/assets.rb (1)

41-42: No issues with asset precompilation additions

These additions ensure the new annotation JS files are compiled and served in production.

app/assets/javascripts/annotations_popup.js (1)

105-136: Ensure concurrency safety when multiple annotations are submitted.

submitNewAnnotation() sets global variables like fileNameStr and does an AJAX POST without checking concurrency. If multiple submissions are triggered rapidly, the global state might be overwritten unexpectedly. Consider storing relevant data in local variables or scoping it to the form element.

app/helpers/application_helper.rb (2)

8-10: Validate that @Assessment is present before linking.

current_assessment_link references @assessment.display_name. In contexts without @assessment, this might cause errors. Consider conditionally rendering the link only if @assessment is defined.

---

189-190: Maintain consistent DataTables version usage across the codebase.

jquery.dataTables is upgraded to 1.13.4. Ensure other scripts or CDN references also match to avoid version conflicts or compatibility issues.

config/routes.rb (1)

175-175: Ensure authorized access.

The get "tweak_total" route should be constrained to authorized users only, especially if it displays or modifies sensitive grading data. Validate that proper authorization checks are in place in the corresponding controller action.

spec/controllers/submissions_controller_spec.rb (2)

150-150: Confirm the destroy confirmation check.
Adding "destroy-confirm-check": "filled-in" is a good way to ensure the user has confirmed their destructive action. Make sure the controller properly validates this parameter to avoid accidental deletions.

---

276-276: Method rename consistency.
Renaming from downloadAll to download_all aligns the test suite with Rails conventions. Confirm there are no broken references elsewhere in the test code or in the codebase.

app/views/assessments/show.html.erb (1)

87-87: Confirm that all references to downloadAll are updated to download_all.
This updated link path properly reflects the renamed route and method. Double-check any partials or other views that might still reference the old path.

db/schema.rb (1)

111-111: Validate usage of the new has_svn column.
Ensure this new boolean attribute is backed by appropriate validations or conditionals if it drives new functionality. Without a default value specified, ensure this field is properly set or handled in migrations and forms.

app/models/submission.rb (1)

258-260: Expose or document usage of global_annotations.
This new method returns only global comments. Make sure whatever calls this method handles shared vs. global logic consistently and that the naming is clear to both maintainers and consumers.

app/controllers/assessments_controller.rb (1)

48-50: Clarify authorization for the new manage submissions action.

By introducing action_auth_level :excuse_popover, :course_assistant, you've allowed TAs (course assistants) to access the excuse_popover action. Ensure this aligns with the intended scope of who can manage submission excuses (e.g., TAs vs. instructors). If the design calls for more specific role-based restrictions, consider adjusting the authorization.

app/assets/stylesheets/datatable.adapter.css (2)

43-46: Sticky header improvement.

Setting position: sticky on table headers is helpful for large tables. Ensure you also handle potential z-index stacking contexts (e.g., nested containers) so the sticky header doesn’t appear behind other elements.

---

60-61: Use a standard layout approach for .dt-button span.

.dt-button span { display: flex; } is quite flexible but might conflict with certain inline elements or icons. Confirm that the child elements rendering inside the <span> remain visually aligned as intended.

app/assets/stylesheets/assessments/quiz.css.scss (1)

18-18: Great adoption of the new variable $autolab-light-grey.

Switching from a hardcoded color to a variable boosts consistency across the application. Ensure this variable is also applied where similar shades of grey were used, to maximize consistency.

app/assets/stylesheets/instructor_gradebook.scss (1)

136-136: LGTM! Good use of design system variable.

Using $autolab-light-grey instead of a hardcoded color value improves maintainability and ensures color consistency across the application.

app/assets/stylesheets/style.css.scss (2)

754-754: LGTM! Good use of design system variables.

Using $autolab-sky-blue, $autolab-light-grey, and $autolab-black instead of hardcoded color values improves maintainability and ensures color consistency.

Also applies to: 774-775

---

785-819: LGTM! Well-structured sorting styles.

The sorting styles are well-organized and follow BEM naming convention. The icon visibility toggling is handled cleanly.

[coderabbitai]

💡 Codebase verification

Based on the analysis of the code and search results, I can now provide the final response:

The template rendering is vulnerable to XSS attacks - HTML escaping is required

The AutolabComponent class uses jQuery's html() method to directly inject template output into the DOM without any sanitization. While there is an escapeHtml utility function available in watchlist.js, it's not being used in the component's rendering logic. This creates a potential XSS vulnerability when user-provided data from this.state is interpolated into templates.

• Add HTML escaping in the template function before interpolating state values:

this.template = function() {
  return `<div>
    <p>Name: ${escapeHtml(this.state.name)}</p>
    <p>Age: ${escapeHtml(this.state.age)}</p>
  </div>`;
};

• Import and use the existing escapeHtml utility function from watchlist.js or implement HTML escaping directly in the component.

🔗 Analysis chain

Verify potential XSS vectors when rendering the template.

Because the entire string is injected via jQuery html(), ensure that any user-provided data passed into this.state is sanitized to prevent cross-site scripting attacks.

Generate the following script to confirm that user-provided data is sanitized before being assigned to this.state:

---

Let me gather more information about the template rendering and potential XSS vectors.

---

Let me check one more thing to see how the state data is actually used in templates.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Searches for calls to setState that might be constructed from user input.
rg -A 5 "setState\s*\(\s*\{.*" 

Length of output: 1263

---

Script:

#!/bin/bash
# Check how the template is used and if there's any sanitization
rg -A 5 "template\s*=.*" 

# Check for any sanitization functions or libraries
rg -l "(sanitize|escape|xss)"

# Look for the actual template implementations
ast-grep --pattern 'template = function() {
  $$$
}'

Length of output: 9040

---

Script:

#!/bin/bash
# Look for template implementations that use state data
rg -A 10 "this\.state\." app/assets/javascripts/

# Check for any HTML escaping utilities
rg -A 5 "escape" app/assets/javascripts/

Length of output: 45506

[coderabbitai]

⚠️ Potential issue

Score details modal logic.
Populating HTML via innerHTML is flexible but raises a potential for XSS if data or email is not sanitized server-side. Ensure content from the server is safe or properly escaped.

[coderabbitai]

💡 Codebase verification

Missing DOM elements for some button IDs

The code references four button IDs but only two are found in the templates:

• #download-selected and #delete-selected exist in app/views/file_manager/index.html.erb
• #regrade-selected and #excuse-selected are missing from the templates

This mismatch could lead to JavaScript errors since the code tries to interact with elements that don't exist in the DOM. The buttons array should be updated to only include IDs that correspond to actual elements.

🔗 Analysis chain

Consistent naming of button ID selectors.
buttonIDs is a good approach for referencing specific actions. Check that all IDs match what's actually in the DOM to avoid silent failures.

---

Let me search for the actual button elements in the HTML/ERB templates to verify these IDs match.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
rg '#regrade-selected'
rg '#delete-selected'
rg '#download-selected'
rg '#excuse-selected'

Length of output: 772

---

Script:

#!/bin/bash
# Search for button elements with these IDs in templates
rg -t html -t erb 'id=["'\''](?:regrade|delete|download|excuse)-selected["'\'']'

Length of output: 359

[coderabbitai]

⚠️ Potential issue

Review batch operations for potential side effects.

These newly added batch routes (destroy_batch, download_batch, excuse_batch, etc.) can significantly alter multiple records at once. Ensure that robust validations and transaction handling occur server-side to prevent partial updates or data inconsistencies if an error arises.

[coderabbitai]

⚠️ Potential issue

Validate the submission's course ownership.

Although the code rescues missing submissions with a 404, it doesn't check whether the found submission actually belongs to the current course or whether the user has permission to view it. This may leak data if a submission from a different course is accessed by manipulating the submission_id. Consider adding a course ownership check, for example:

 def excuse_popover
   submission_id = params[:submission_id]
   @submission = Submission.find(submission_id)
+  if @submission.course != @course
+    render plain: "Unauthorized", status: :forbidden
+    return
+  end
   @assessment = @submission.assessment
   ...
 end

Committable suggestion skipped: line range outside the PR's diff.

[coderabbitai]

⚠️ Potential issue

Remove duplicate CSS declaration.

The .field_with_errors class is declared twice with the same properties. Remove one of the declarations to avoid confusion and maintain clean code.

 div.field_with_errors {
   display: inline;
 }
-
-div.field_with_errors {
-  display: inline;
-}

📝 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

	div.field_with_errors {
	display: inline;
	}
	div.field_with_errors {
	display: inline;
	}
	div.field_with_errors {
	display: inline;
	}