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. In the Ruby controllers, methods in both the autograde and submissions 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
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 RuboCop (1.69.1)app/controllers/assessment/autograde.rbError: No such file or directory: /app/controllers/assessment/autograde.rb app/controllers/submissions_controller.rbError: No such file or directory: /app/controllers/submissions_controller.rb ✨ Finishing Touches
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? 🪧 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.
| buttonIDs.forEach((id) => { | ||
| const button = $(id); | ||
| if (state) { | ||
| console.log(`Disabling: ${id}`, button); | ||
| if (id === "#download-selected") { | ||
| $(id).prop('href', baseURLs[id]); | ||
| } | ||
| button.addClass("disabled"); | ||
| button.off("click").prop("disabled", true); | ||
| } else { | ||
| button.removeClass("disabled").prop("disabled", false); | ||
| if (id == "#download-selected") { | ||
| var urlParam = $.param({'submission_ids': selectedSubmissions}); | ||
| buttonIDs.forEach(function(id) { | ||
| var newHref = baseURLs[id] + '?' + urlParam; | ||
| $(id).prop('href', newHref); | ||
| }); | ||
| return; | ||
| } | ||
| $(document).off("click", id).on("click", id, function (event) { | ||
| console.log(`${id} button clicked`); | ||
| event.preventDefault(); | ||
| if (selectedSubmissions.length === 0) { | ||
| alert("No submissions selected."); | ||
| return; | ||
| } | ||
| const endpoint = manage_submissions_endpoints[id.replace("#", "")]; | ||
| const requestData = { submission_ids: selectedSubmissions }; | ||
| if (id === "#delete-selected") { | ||
| if (!confirm("Deleting will delete all checked submissions and cannot be undone. Are you sure you want to delete these submissions?")) { | ||
| return; | ||
| } | ||
| } | ||
| let refreshInterval = setInterval(() => { | ||
| location.reload(); | ||
| }, 5000); | ||
| $.ajax({ | ||
| url: endpoint, | ||
| type: "POST", | ||
| contentType: "application/json", | ||
| data: JSON.stringify(requestData), | ||
| dataType: "json", | ||
| headers: { | ||
| "X-CSRF-Token": $('meta[name="csrf-token"]').attr("content"), | ||
| }, | ||
| success: function (response) { | ||
| clearInterval(refreshInterval); | ||
| if (response.redirect) { | ||
| window.location.href = response.redirect; | ||
| return; | ||
| } | ||
| if (response.error) { | ||
| alert(response.error); | ||
| } | ||
| if (response.success) { | ||
| alert(response.success); | ||
| } | ||
| selectedSubmissions = []; | ||
| changeButtonStates(true); | ||
| }, | ||
| error: function (error) { | ||
| alert("An error occurred while processing the request."); | ||
| }, | ||
| }); | ||
| }); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Prevent infinite page reload loop on AJAX errors.
Currently, setInterval is 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.");
},
...📝 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.
| buttonIDs.forEach((id) => { | |
| const button = $(id); | |
| if (state) { | |
| console.log(`Disabling: ${id}`, button); | |
| if (id === "#download-selected") { | |
| $(id).prop('href', baseURLs[id]); | |
| } | |
| button.addClass("disabled"); | |
| button.off("click").prop("disabled", true); | |
| } else { | |
| button.removeClass("disabled").prop("disabled", false); | |
| if (id == "#download-selected") { | |
| var urlParam = $.param({'submission_ids': selectedSubmissions}); | |
| buttonIDs.forEach(function(id) { | |
| var newHref = baseURLs[id] + '?' + urlParam; | |
| $(id).prop('href', newHref); | |
| }); | |
| return; | |
| } | |
| $(document).off("click", id).on("click", id, function (event) { | |
| console.log(`${id} button clicked`); | |
| event.preventDefault(); | |
| if (selectedSubmissions.length === 0) { | |
| alert("No submissions selected."); | |
| return; | |
| } | |
| const endpoint = manage_submissions_endpoints[id.replace("#", "")]; | |
| const requestData = { submission_ids: selectedSubmissions }; | |
| if (id === "#delete-selected") { | |
| if (!confirm("Deleting will delete all checked submissions and cannot be undone. Are you sure you want to delete these submissions?")) { | |
| return; | |
| } | |
| } | |
| let refreshInterval = setInterval(() => { | |
| location.reload(); | |
| }, 5000); | |
| $.ajax({ | |
| url: endpoint, | |
| type: "POST", | |
| contentType: "application/json", | |
| data: JSON.stringify(requestData), | |
| dataType: "json", | |
| headers: { | |
| "X-CSRF-Token": $('meta[name="csrf-token"]').attr("content"), | |
| }, | |
| success: function (response) { | |
| clearInterval(refreshInterval); | |
| if (response.redirect) { | |
| window.location.href = response.redirect; | |
| return; | |
| } | |
| if (response.error) { | |
| alert(response.error); | |
| } | |
| if (response.success) { | |
| alert(response.success); | |
| } | |
| selectedSubmissions = []; | |
| changeButtonStates(true); | |
| }, | |
| error: function (error) { | |
| alert("An error occurred while processing the request."); | |
| }, | |
| }); | |
| }); | |
| } | |
| }); | |
| } | |
| buttonIDs.forEach((id) => { | |
| const button = $(id); | |
| if (state) { | |
| console.log(`Disabling: ${id}`, button); | |
| if (id === "#download-selected") { | |
| $(id).prop('href', baseURLs[id]); | |
| } | |
| button.addClass("disabled"); | |
| button.off("click").prop("disabled", true); | |
| } else { | |
| button.removeClass("disabled").prop("disabled", false); | |
| if (id == "#download-selected") { | |
| var urlParam = $.param({'submission_ids': selectedSubmissions}); | |
| buttonIDs.forEach(function(id) { | |
| var newHref = baseURLs[id] + '?' + urlParam; | |
| $(id).prop('href', newHref); | |
| }); | |
| return; | |
| } | |
| $(document).off("click", id).on("click", id, function (event) { | |
| console.log(`${id} button clicked`); | |
| event.preventDefault(); | |
| if (selectedSubmissions.length === 0) { | |
| alert("No submissions selected."); | |
| return; | |
| } | |
| const endpoint = manage_submissions_endpoints[id.replace("#", "")]; | |
| const requestData = { submission_ids: selectedSubmissions }; | |
| if (id === "#delete-selected") { | |
| if (!confirm("Deleting will delete all checked submissions and cannot be undone. Are you sure you want to delete these submissions?")) { | |
| return; | |
| } | |
| } | |
| let refreshInterval = setInterval(() => { | |
| location.reload(); | |
| }, 5000); | |
| $.ajax({ | |
| url: endpoint, | |
| type: "POST", | |
| contentType: "application/json", | |
| data: JSON.stringify(requestData), | |
| dataType: "json", | |
| headers: { | |
| "X-CSRF-Token": $('meta[name="csrf-token"]').attr("content"), | |
| }, | |
| success: function (response) { | |
| clearInterval(refreshInterval); | |
| if (response.redirect) { | |
| window.location.href = response.redirect; | |
| return; | |
| } | |
| if (response.error) { | |
| alert(response.error); | |
| } | |
| if (response.success) { | |
| alert(response.success); | |
| } | |
| selectedSubmissions = []; | |
| changeButtonStates(true); | |
| }, | |
| error: function (error) { | |
| clearInterval(refreshInterval); | |
| alert("An error occurred while processing the request."); | |
| }, | |
| }); | |
| }); | |
| } | |
| }); | |
| } |
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.