Manage submissions page shows progress for regrading selected submissions #2278
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a new yellow color variable ( Changes
Suggested reviewers
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 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: 0
🧹 Nitpick comments (4)
app/views/submissions/index.html.erb (1)
206-221: Nice enhancement of the score display logic.The modified score display logic appropriately handles the case where a submission is being regraded by showing a clock icon instead of the score. This visual feedback helps users understand which submissions are currently being processed.
Consider adding a tooltip to the clock icon to explicitly state that the submission is being regraded, which would further improve user understanding.
- <%= link_to "<i class='tiny material-icons'>access_time</i>".html_safe, - { controller: "jobs", action: "getjob", id: submission.jobid } %> + <%= link_to "<i class='tiny material-icons'>access_time</i>".html_safe, + { controller: "jobs", action: "getjob", id: submission.jobid }, + { title: "This submission is being regraded" } %>app/controllers/submissions_controller.rb (3)
24-25: Improve readability with a ternary operatorThe current if-else structure could be more concise. Simplify this using a ternary operator for better readability.
- @regrading = if params[:regrading].nil? - false else params[:regrading] end + @regrading = params[:regrading].nil? ? false : params[:regrading]🧰 Tools
🪛 RuboCop (1.73)
[warning] 25-25: Odd
elselayout detected. Did you mean to useelsif?(Lint/ElseLayout)
24-25: Consider type conversion for the regrading parameterThe code doesn't convert
params[:regrading]to a boolean, which might lead to unexpected behavior since params are strings. A string like "false" would be evaluated as truthy.- @regrading = if params[:regrading].nil? - false else params[:regrading] end + @regrading = !params[:regrading].nil? && params[:regrading].to_s.downcase == 'true'🧰 Tools
🪛 RuboCop (1.73)
[warning] 25-25: Odd
elselayout detected. Did you mean to useelsif?(Lint/ElseLayout)
29-40: Consider pagination or filtering in the TangoClient API callFetching all jobs from TangoClient might become inefficient if there are many jobs. Consider working with the TangoClient team to implement filtering or pagination if this becomes a performance issue.
If TangoClient supports filtering by assessment ID or course ID, you could reduce the number of jobs fetched and improve performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/assets/stylesheets/_variables.scss(1 hunks)app/assets/stylesheets/style.css.scss(1 hunks)app/controllers/assessment/autograde.rb(1 hunks)app/controllers/submissions_controller.rb(1 hunks)app/views/submissions/index.html.erb(2 hunks)
🧰 Additional context used
🪛 RuboCop (1.73)
app/controllers/submissions_controller.rb
[warning] 25-25: Odd else layout detected. Did you mean to use elsif?
(Lint/ElseLayout)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (8)
app/assets/stylesheets/_variables.scss (1)
28-28: Good addition of a named color variable.Adding the
$autolab-yellowvariable follows the project's established pattern of defining semantic color variables, which promotes consistency and makes future style changes easier.app/assets/stylesheets/style.css.scss (1)
1722-1730: Well-structured styling for the new regrade notification.The
.regrade-refreshclass is properly implemented with appropriate styling that matches the UI design patterns used elsewhere in the application. Good use of the newly defined$autolab-yellowvariable for color consistency.app/views/submissions/index.html.erb (1)
53-60: Good implementation of regrading status message.The conditional block displays a helpful message with the count of submissions being regraded and provides clear navigation options for users to check progress or refresh the page. This directly addresses the issue mentioned in the PR description where TAs were confused about regrade status.
app/controllers/assessment/autograde.rb (2)
120-123: Suppress success notifications in favor of status display.Commenting out the success notification is acceptable since you're replacing it with a more persistent and informative status display on the submissions page. This change helps improve the user experience by providing continuous feedback about regrading status.
128-128: Good implementation of redirect with regrading parameter.The addition of the
regrading: 'true'parameter to the redirect allows the view to conditionally display the regrading status message, creating a cohesive user experience throughout the regrading process.app/controllers/submissions_controller.rb (3)
29-40: Approve the TangoClient job fetching implementationGood implementation of fetching live jobs with proper error handling for TangoClient exceptions.
47-49: Approve the submission tracking logicThe code correctly identifies submissions that are currently being regraded by checking if their job ID is in the list of active jobs.
52-54: Good fallback to handle empty regrading submissionsThis is a good safety check that ensures the regrading status is reset if no submissions are actually being regraded, avoiding confusion in the UI.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/submissions_controller.rb(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (3)
app/controllers/submissions_controller.rb (3)
29-43: LGTM - Error handling for TangoClient jobsThe exception handling for TangoClient.jobs is appropriate. The code properly catches exceptions and displays an error message to the user.
50-52: LGTM - Identifying regrading submissionsThis code correctly identifies which submissions are currently being regraded by checking if their job IDs exist in the active jobs from TangoClient.
55-57: LGTM - Fallback for empty regrading submissionsThis is a good failsafe to avoid displaying the regrading message when no submissions are actually being regraded, providing a better user experience.
KesterTan
requested review from
a team and
jhs-panda
and removed request for
a team
There was a problem hiding this comment.
I think the functionality is a bit broken? I am submitting with Proxylab (takes a long time to run so can test the regrade update properly).
After regrading some submissions and letting them finish running (no jobs left), I regraded one more submission that was not previously regraded. At this point, the Jobs list is empty and none of the submissions have the timer.
The "current regrading" count includes some submissions that were already regraded, and these submissions had the timer marking even though the jobs had already been run and did not show up on the Jobs list "waiting" or "running" sections. Eventually, after some refreshing, the timer symbol and regrading message disappear.
It also seems like each time this error occurs, the same submissions are considered "regrading again" even though they weren't.
Let me know if you can't replicate it and I'll try with another assessment/set of submissions. Looking at your code it doesn't seem obvious why this happening and tbh it isn't that big of a deal, but it could be confusing for instructors.
Description
When a submission is being graded, a clock icon replaces the score. There is a message on top of the page that tells the user to either check the jobs page or refresh the page so that they update the score. The message only goes away when there is no more submissions that are being graded.
Motivation and Context
Currently, when the user pressed the regrade selected button on Manage Submissions, it is unclear when the regrade all ended. The only way a user knows if the regrade has finished is if they go to the jobs page and see that all the jobs have finished running. This was a problem raised in issue #2223, by 213 and 122 TAs.
How Has This Been Tested?
I followed these steps:
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for lintingOther issues / help required
One issue with my implementation is that all submissions that are currently graded will have the status of being regraded. Therefore, if I regrade all submissions, then another user decides to submit a new file while the regrade is running, the message will only go away when that new submission also finishes grading. One way I thought of to address this issue is just making the clock icons and the yellow message a default feature regardless of if a regrade is going on or not. Therefore, if an instructor goes to Manage Submissions while a student creates a new submission, the instructor will see on the page that one grading process is going on.