Changing the manage submissions button to be ajax requests #2271
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several improvements for managing submissions. In the JavaScript file, new endpoints for deletion, downloading, and excusing submissions are added, and the button state logic is enhanced with detailed event handling and AJAX calls. The Ruby controllers are updated to parse submission IDs from the request body (ensuring array format) and to handle responses via Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant JS
participant Server
User->>Browser: Click on action button (delete, download, excuse)
Browser->>JS: Trigger changeButtonStates logic
JS->>Browser: Disable button and log action
alt Delete Action
JS->>User: Display confirmation prompt
end
JS->>Server: Send AJAX request to appropriate endpoint
Server-->>JS: Return response (JSON/redirect URL)
JS->>Browser: Update UI based on server response
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/controllers/assessment/autograde.rb (2)
89-93: Consider adding validation and error handling when parsing submission IDs.If
submission_idsare missing or invalid in the JSON payload, this code may throw an error or fail silently. It would be safer to check fornilor an unexpected type before proceeding. For instance, returning a user-friendly error if no valid submission IDs are found could improve reliability.
130-133: Provide richer JSON responses for asynchronous flows.The
respond_toblock works well for redirection, but consider returning additional information (e.g., success or error messages) in the JSON response to better support asynchronous UI actions and partial success scenarios.app/views/submissions/index.html.erb (1)
91-93: Ensure graceful fallback for regrade if JavaScript is disabled.Replacing
link_towith a plain<a>that relies on JavaScript can leave no fallback route if JS is turned off. If this is intentional, no changes are required, but a progressive enhancement approach may be beneficial, offering a non-JS alternative.app/controllers/submissions_controller.rb (1)
203-249: Consider adding request body validation.While the implementation works, it would be more robust to add validation for the request body format and handle potential JSON parsing errors.
Consider adding error handling:
def destroy_batch request_body = request.body.read + if request_body.blank? + respond_to do |format| + format.html do + flash[:error] = "No submission IDs provided" + redirect_to [@course, @assessment, :submissions] + end + format.json { render json: { error: "No submission IDs provided" }, status: :bad_request } + end + return + end + + begin submission_ids = JSON.parse(request_body)['submission_ids'] + rescue JSON::ParserError + respond_to do |format| + format.html do + flash[:error] = "Invalid JSON in request body" + redirect_to [@course, @assessment, :submissions] + end + format.json { render json: { error: "Invalid JSON in request body" }, status: :bad_request } + end + return + end + submission_ids = Array(submission_ids)Apply similar changes to the
excuse_batchmethod as well.Also applies to: 391-446
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/assets/javascripts/manage_submissions.js(2 hunks)app/controllers/assessment/autograde.rb(2 hunks)app/controllers/submissions_controller.rb(4 hunks)app/views/submissions/index.html.erb(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (7)
app/views/submissions/index.html.erb (2)
97-99: Looks good for async deletion handling.These lines align with the new AJAX-driven design. The plain anchor setup is consistent with the other buttons, and the JavaScript logic takes over seamlessly. No immediate issues here.
111-113: Consistent approach for excuse action.The excuse button follows the same pattern as regrade and delete. Just ensure that users understand there's no direct link if JavaScript is disabled, which may be acceptable for this management tool.
app/assets/javascripts/manage_submissions.js (1)
3-5: Validate endpoint consistency for download requests.While
'download-selected'is listed as'submissions/download_batch', be sure this endpoint is indeed intended for a GET request. The code appends query parameters for submission IDs, which should align with the server-side route’s expected method.app/controllers/submissions_controller.rb (4)
203-205: LGTM: Proper handling of request body data.The implementation correctly reads and parses the submission IDs from the request body, ensuring array format with
Array().
246-249: LGTM: Proper response format handling.The implementation correctly handles both HTML and JSON responses using Rails'
respond_toblock, providing appropriate redirects for each format.
391-393: LGTM: Consistent implementation with destroy_batch.The implementation mirrors the approach used in
destroy_batch, maintaining consistency in how request body data is handled.
443-446: LGTM: Consistent response format handling.The implementation mirrors the approach used in
destroy_batch, maintaining consistency in how responses are formatted.
|
Are the buttons visibility toggle broken? If no checkboxes are selected, buttons should be disabled?
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/assets/javascripts/manage_submissions.js (1)
313-379:⚠️ Potential issuePrevent infinite page reload loop on AJAX errors.
Currently,
setIntervalis cleared only on success, causing repeated page reloads every 5 seconds if an error occurs. To avoid trapping users in a reload loop, remember to clear the interval upon error:error: function (error) { + clearInterval(refreshInterval); alert("An error occurred while processing the request."); },
🧹 Nitpick comments (1)
app/assets/javascripts/manage_submissions.js (1)
357-375: Add visual feedback during AJAX processing.To improve user experience, add loading indicators and disable buttons during AJAX requests to prevent multiple submissions:
$(document).off("click", id).on("click", id, function (event) { console.log(`${id} button clicked`); event.preventDefault(); // Show loading state + const button = $(id); + button.addClass("processing"); + button.prop("disabled", true); // Ajax request... success: function (response) { clearInterval(refreshInterval); + button.removeClass("processing"); // rest of success handler }, error: function (error) { clearInterval(refreshInterval); + button.removeClass("processing"); + button.prop("disabled", false); alert("An error occurred while processing the request."); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/assets/javascripts/manage_submissions.js(2 hunks)app/views/submissions/index.html.erb(1 hunks)config/routes.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/submissions/index.html.erb
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: test
🔇 Additional comments (2)
config/routes.rb (1)
181-181: Route method change aligns with PR objectives.Changing
download_batchfrom GET to POST is appropriate when moving from URL parameters to request body for submission IDs. This supports the PR's goal of handling large requests more efficiently.app/assets/javascripts/manage_submissions.js (1)
3-5: LGTM: New endpoints support AJAX implementation.The new endpoint definitions for delete, download, and excuse operations align with the backend routes and enable the AJAX functionality described in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/assets/javascripts/manage_submissions.js (1)
323-330:⚠️ Potential issueFix early return that skips button event handlers.
The current implementation has a
returnstatement on line 329 that exits the function early after setting up the download URL, preventing other buttons from getting their event handlers. This logic needs to be restructured.if (id == "#download-selected") { var urlParam = $.param({'submission_ids': selectedSubmissions}); buttonIDs.forEach(function(id) { var newHref = baseURLs[id] + '?' + urlParam; $(id).prop('href', newHref); }); - return; }
🧹 Nitpick comments (1)
app/controllers/assessment/autograde.rb (1)
89-101: Implement robust request parsing for submission IDs.The code now properly extracts submission IDs from either the JSON request body or parameters, with good defensive programming to handle various edge cases.
There's an unused
evariable in the rescue block:- rescue JSON::ParserError => e + rescue JSON::ParserError params[:submission_ids] || []🧰 Tools
🪛 RuboCop (1.73)
[warning] 96-96: Useless assignment to variable -
e.(Lint/UselessAssignment)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/assets/javascripts/manage_submissions.js(2 hunks)app/controllers/assessment/autograde.rb(2 hunks)
🧰 Additional context used
🪛 RuboCop (1.73)
app/controllers/assessment/autograde.rb
[warning] 96-96: Useless assignment to variable - e.
(Lint/UselessAssignment)
🔇 Additional comments (4)
app/controllers/assessment/autograde.rb (1)
138-141: Good implementation of format-specific responses.The respond_to block enhances flexibility by supporting both HTML redirects for browser clients and JSON responses for AJAX requests.
app/assets/javascripts/manage_submissions.js (3)
3-5: Good addition of endpoints for AJAX operations.These endpoint definitions properly align with the controller actions that have been modified to accept POST requests.
331-377: Well-implemented AJAX submission handling.The event handlers for the buttons correctly implement AJAX POST requests with appropriate CSRF protection, error handling, and success responses. The code also properly clears the refresh interval in both success and error cases to prevent infinite reload loops.
313-380: Verify button state toggle behavior on selection changes.According to the PR comments, there's a concern about buttons not being properly disabled when no checkboxes are selected. This implementation should theoretically work, but consider testing it thoroughly on edge cases such as:
- Selecting and then deselecting all checkboxes
- Navigating between pages with selections
- Handling the "Select All" checkbox interaction
You can manually verify this by:
- Loading the manage submissions page
- Selecting a few checkboxes and confirming all buttons are enabled
- Deselecting all checkboxes and confirming all buttons are disabled
- Using "Select All" and confirming all buttons are enabled
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/assets/javascripts/manage_submissions.js (1)
343-350:⚠️ Potential issueFix early return that skips button event handlers.
The current implementation has a
returnstatement on line 350 that exits the function early after setting up the download URL, preventing other buttons from getting their event handlers.if (id == "#download-selected") { var urlParam = $.param({'submission_ids': selectedSubmissions}); buttonIDs.forEach(function(id) { var newHref = baseURLs[id] + '?' + urlParam; $(id).prop('href', newHref); }); - return; }
🧹 Nitpick comments (3)
app/controllers/assessment/autograde.rb (2)
90-99: Improve error handling for JSON parsing.The code successfully reads submission IDs from the request body, but there's a useless variable assignment to
ein the rescue block which is never used.- rescue JSON::ParserError => e + rescue JSON::ParserError params[:submission_ids] || []🧰 Tools
🪛 RuboCop (1.73)
[warning] 97-97: Useless assignment to variable -
e.(Lint/UselessAssignment)
90-102: Consider adding logging for JSON parse errors.When JSON parsing fails, the code silently falls back to params without logging the error, which could make debugging difficult in production.
rescue JSON::ParserError => e + Rails.logger.warn "JSON parse error in regradeBatch: #{e.message}" params[:submission_ids] || []🧰 Tools
🪛 RuboCop (1.73)
[warning] 97-97: Useless assignment to variable -
e.(Lint/UselessAssignment)
app/assets/javascripts/manage_submissions.js (1)
365-367: Consider using a more user-friendly approach than automatic page reload.The current implementation uses a setInterval to reload the page every 5 seconds while waiting for AJAX requests to complete. This might be disruptive to users, especially for long-running operations.
Consider replacing the automatic page reload with a loading indicator and only refreshing the page once the operation completes:
- let refreshInterval = setInterval(() => { - location.reload(); - }, 5000); + // Show loading state + button.addClass("loading").prop("disabled", true); + // Add a spinner or other loading indicator + const loadingIndicator = $("<span class='spinner'></span>"); + $(id).append(loadingIndicator);Then in the success/error handlers:
success: function (response) { - clearInterval(refreshInterval); + // Remove loading indicator + $(id).find('.spinner').remove(); + button.removeClass("loading");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/assets/javascripts/manage_submissions.js(2 hunks)app/controllers/assessment/autograde.rb(2 hunks)app/views/submissions/index.html.erb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/submissions/index.html.erb
🧰 Additional context used
🪛 RuboCop (1.73)
app/controllers/assessment/autograde.rb
[warning] 97-97: Useless assignment to variable - e.
(Lint/UselessAssignment)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (4)
app/controllers/assessment/autograde.rb (1)
144-147: Good use of respond_to block for AJAX support.The implementation correctly handles both HTML and JSON responses, which aligns with the PR objective of using AJAX requests.
app/assets/javascripts/manage_submissions.js (3)
1-7: Endpoints added appropriately for AJAX functionality.The new endpoints for delete, download, and excuse operations are consistent with the PR's objective to use AJAX requests instead of URL parameters.
351-397: Well-implemented AJAX request handling with proper error management.The code correctly sets up AJAX requests with appropriate headers, error handling, and clearing of refresh intervals in both success and error cases.
333-399:❓ Verification inconclusive
Implement consistent button state visibility across all buttons.
Based on the PR objectives, there's a concern about why only the download button is properly disabled. The current implementation may not be consistent in how it visually indicates disabled states across all buttons.
Could you verify that all buttons show a consistent disabled state by checking their CSS classes and appearance when no selections are made?
🏁 Script executed:
#!/bin/bash # Check for consistent button styling and disabled state indicators fd -e scss -e css | rg -A 2 "disabled|\.btn|\.button"Length of output: 55
Action Required: Manual Verification for Consistent Button Disabled Styling
The current implementation toggles the
.disabledclass across buttons, but automated searches for relevant CSS rules returned no evidence. Please double-check manually that all buttons—especially the download button versus the others—exhibit a consistent disabled appearance when no selections are made. Verify both the applied CSS rules (in your CSS/SCSS files) and the actual rendered state in the browser.
) * fixing request bodies * fixed disable buttons * Fixing bugs --------- Co-authored-by: Kester <kestertan040@gmail.com> (cherry picked from commit a8f07e8)
Description
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for lintingOther issues / help required
If unsure, feel free to submit first and we'll help you along.