Clarify Disable Autolab Submissions Option #2224
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new JavaScript function, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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 (
|
jhs-panda
changed the title
![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
🧹 Outside diff range and nitpick comments (3)
app/views/assessments/_edit_handin.html.erb (3)
4-10: LGTM! Consider adding a null check for robustness.The
toggleHandinAutolabfunction effectively toggles the disabled state of the handin filename field and sets a default value when enabled. This improves the user experience.For added robustness, consider adding a null check before accessing the
handinField:function toggleHandinAutolab(disable_button) { var handinField = document.querySelector('.handin-filename-field'); + if (!handinField) return; handinField.disabled = disable_button.checked; if (!handinField.disabled) { handinField.value = 'handin.c'; } }
47-50: LGTM! Consider adding ARIA attributes for better accessibility.The changes to the disable_handins checkbox improve its visibility and functionality, aligning with the PR objectives. The updated display name, added class, and onchange event are all appropriate improvements.
To enhance accessibility, consider adding ARIA attributes:
<%= f.check_box :disable_handins, display_name: "Disable Autolab submissions", help_text: "Check this to disallow handins through Autolab. This option can be used to track scores for assignments that are not submitted through Autolab such as midterms and written assignments.", class: "disable-handins-toggle", - onchange: "toggleHandinAutolab(this)" %> + onchange: "toggleHandinAutolab(this)", + "aria-label": "Disable Autolab submissions", + "aria-describedby": "disable-handins-help" %> +<span id="disable-handins-help" class="sr-only">Check this to disallow handins through Autolab. This option can be used to track scores for assignments that are not submitted through Autolab such as midterms and written assignments.</span>
51-56: LGTM! Consider using a constant for the default filename.The changes to the handin_filename text field improve its usability and interaction with the JavaScript functionality. Always rendering the field and toggling its disabled state provides a smooth user experience.
For consistency and easier maintenance, consider using a constant for the default filename:
+<% DEFAULT_HANDIN_FILENAME = 'handin.c' %> <%= f.text_field :handin_filename, - value: "handin.c", + value: DEFAULT_HANDIN_FILENAME, disabled: true, help_text: "The suffix that is appended to student submission files. Autolab stores submission files in the handin directory as email/version_fname", placeholder: "E.g. mm.c", class: "handin-filename-field" %>Also, update the JavaScript function to use this constant:
function toggleHandinAutolab(disable_button) { var handinField = document.querySelector('.handin-filename-field'); if (!handinField) return; handinField.disabled = disable_button.checked; if (!handinField.disabled) { - handinField.value = 'handin.c'; + handinField.value = '<%= DEFAULT_HANDIN_FILENAME %>'; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- app/views/assessments/_edit_handin.html.erb (2 hunks)
- app/views/assessments/show.html.erb (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/views/assessments/show.html.erb
🔇 Additional comments (2)
app/views/assessments/_edit_handin.html.erb (2)
12-15: LGTM! Good use of DOMContentLoaded event.The addition of the DOMContentLoaded event listener ensures that the initial state of the handin filename field is set correctly when the page loads. This is a good practice for initializing UI states.
Line range hint
1-57: Overall, great job on improving the "Disable Autolab submissions" functionality!The changes effectively address the PR objectives by enhancing the visibility and functionality of the "Disable Autolab submissions" button. The JavaScript additions and UI updates work together to provide a smooth user experience. The minor suggestions provided in the review comments can further improve robustness, accessibility, and maintainability.
jhs-panda
requested review from
a team and
evanyeyeye
and removed request for
a team
There was a problem hiding this comment.
Mostly looks good!
However, this PR forces the "Handin filename" field to always be handin.c on the frontend view, even when it has been successfully modified.
Also, I noticed that in your "After" screenshot the "Autograder settings" button is gone in the left navigation column. Is that an effect of the PR?
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
app/views/assessments/_edit_handin.html.erb (1)
50-53: LGTM! Clear improvement to the UI messaging.The updated label and help text better communicate the purpose of this option, directly addressing the issue raised in #2216.
Consider enhancing the help text to explicitly mention terminal submissions:
- help_text: "Check this to disallow handins through Autolab. This option can be used to track scores for assignments that are not submitted through Autolab such as midterms and written assignments.", + help_text: "Check this to disallow handins through Autolab. Use this option for assignments that require submission through other means (e.g., terminal submissions for bomblab/attacklab) or for tracking scores of non-submitted work like midterms.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- app/views/assessments/_edit_handin.html.erb (2 hunks)
- lib/update_course.rb (1 hunks)
- test.txt (1 hunks)
- test2.txt (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- test.txt
- test2.txt
🔇 Additional comments (1)
app/views/assessments/_edit_handin.html.erb (1)
50-58: Verify impact on existing assessments with disabled submissions.The UI changes look good, but we should verify that existing assessments with disabled submissions aren't affected.
✅ Verification successful
Let me gather more information about the validation and UI interaction to ensure completeness.
Based on the gathered information, I can now provide the final response:
The changes to disable submissions feature are safe and well-implemented
The implementation is verified to be safe for existing assessments with disabled submissions because:
- The model has proper validations that skip handin directory checks when submissions are disabled
- The submissions controller correctly blocks submission attempts for disabled assessments
- The UI changes maintain consistency by disabling the handin filename field when submissions are disabled
- The JavaScript toggle functionality provides a smooth user experience while preserving the expected behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing assessments with disabled submissions # to ensure the new changes won't affect their configuration # Look for existing disabled submissions in ERB templates rg -t erb "disable_handins.*true" # Look for related controller/model logic rg "disable_handins" app/models/ rg "disable_handins" app/controllers/Length of output: 1053
Script:
#!/bin/bash # Check the validation logic and UI interaction # Look for JavaScript code handling the toggle rg "toggleHandinAutolab" -A 5 # Check form submission handling rg "disable_handins.*params" app/controllers/ # Look for any migrations related to disable_handins fd ".*migrate.*" -t f --exec rg "disable_handins" {}Length of output: 2059
I believe I have corrected this my recent commits.
Sorry, I chose different assessments pages to screenshot and one of them wasn't an autograded assignment. I've corrected the image in the "After" screenshot. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/views/assessments/_edit_handin.html.erb (1)
3-19: Add JSDoc documentation and modernize JavaScript code.The JavaScript implementation could benefit from better documentation and modern practices:
<script> + /** + * Toggles the handin filename field based on the disable button state + * @param {HTMLInputElement} disable_button - The checkbox that controls the handin state + */ function toggleHandinAutolab(disable_button) { - var handinField = document.querySelector('.handin-filename-field'); + const handinField = document.querySelector('.handin-filename-field'); handinField.disabled = disable_button.checked; if (!handinField.disabled && !handinField.value) { handinField.value = 'handin.c'; } } document.addEventListener('DOMContentLoaded', function() { - var disableHandinsCheckbox = document.querySelector('.disable-handins-toggle'); + const disableHandinsCheckbox = document.querySelector('.disable-handins-toggle'); toggleHandinAutolab(disableHandinsCheckbox); disableHandinsCheckbox.addEventListener('change', function() { toggleHandinAutolab(disableHandinsCheckbox); }); }); </script>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/views/assessments/_edit_handin.html.erb(2 hunks)
🧰 Additional context used
📓 Learnings (1)
app/views/assessments/_edit_handin.html.erb (1)
Learnt from: jhs-panda
PR: autolab/Autolab#2224
File: app/views/assessments/_edit_handin.html.erb:54-0
Timestamp: 2024-11-14T17:07:33.456Z
Learning: In `app/views/assessments/_edit_handin.html.erb`, adding `aria-label`, `pattern`, and `title` attributes to the `handin_filename` text field may cause functionality issues and should be avoided.
🔇 Additional comments (2)
app/views/assessments/_edit_handin.html.erb (2)
50-53: LGTM! Clear and well-documented UI element.
The checkbox implementation effectively addresses the PR objective by clearly communicating its purpose through the updated display name and help text.
54-60: Verify filename pattern compatibility with existing submissions.
While the pattern validation improves input safety, we should verify it doesn't break existing valid filenames.
Consider updating the help text to be more specific about filename requirements:
- help_text: "The suffix that is appended to student submission files. Autolab stores submission files in the handin directory as email/version_fname",
+ help_text: "The suffix that is appended to student submission files (e.g., 'handin.c'). Only alphanumeric characters, dots, and underscores are allowed. Autolab stores submission files in the handin directory as email/version_fname",✅ Verification successful
Pattern validation is safe to use with existing filenames
Based on the evidence from the codebase:
- Default handin filenames in examples and factories follow the pattern:
hello.c,hello,handin_file.c - The schema shows no existing constraints on the
handin_filenamefield - The Assessment model only validates presence when handins are enabled
- The pattern
^[a-zA-Z0-9._]+$matches all observed filenames in the codebase
The help text improvement suggestion is good but optional since the validation pattern and title already communicate the requirements clearly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if existing handin filenames match the new pattern
# Search for assessment configurations with handin filenames
rg -g '!*.{log,tmp}' -l 'handin_filename'
# Check if any existing handin filenames would be invalidated by the new pattern
find . -type f -name "*.rb" -o -name "*.yml" | xargs rg -e 'handin_filename.*=.*["'\'']([^"'\'']+)["'\'']' -o |
while read -r line; do
filename=$(echo "$line" | sed -E 's/.*["'\''']([^"'\'']+)["'\'']$/\1/')
if ! echo "$filename" | grep -qE '^[a-zA-Z0-9._]+$'; then
echo "Found potentially incompatible filename: $filename"
fi
done
Length of output: 804
Script:
#!/bin/bash
# Let's try a different approach to find handin filenames in the codebase
# First, check assessment factory for default values
echo "=== Checking factories ==="
rg "handin_filename" spec/factories/assessments.rb -C 2
echo -e "\n=== Checking example assessments ==="
# Check example assessment configs
cat examples/hello/hello.yml examples/hellocat/hellocat.yml 2>/dev/null | grep -A 1 -B 1 "handin_filename"
echo -e "\n=== Checking schema and migrations ==="
# Check schema and migrations for any constraints
rg "handin_filename" db/schema.rb db/migrate/* -C 2
echo -e "\n=== Checking model validations ==="
# Check if Assessment model has any filename validations
rg "handin_filename" app/models/assessment.rb -C 2
Length of output: 14054
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/views/assessments/_edit_handin.html.erb (1)
54-60: Consider enhancing the filename field's help text and validation.While the implementation is good, consider these improvements:
- The help text could be clearer about the version format, e.g., "...as email/N_fname where N is the submission version number".
- The pattern might be too restrictive for some file extensions. Consider allowing hyphens for common file extensions like
.jsxor.test.js.However, based on previous feedback, I noticed that adding certain attributes to this field may cause functionality issues. Therefore, please validate these suggestions thoroughly before implementing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/views/assessments/_edit_handin.html.erb(2 hunks)
🧰 Additional context used
📓 Learnings (1)
app/views/assessments/_edit_handin.html.erb (1)
Learnt from: jhs-panda
PR: autolab/Autolab#2224
File: app/views/assessments/_edit_handin.html.erb:54-0
Timestamp: 2024-11-14T17:07:33.456Z
Learning: In `app/views/assessments/_edit_handin.html.erb`, adding `aria-label`, `pattern`, and `title` attributes to the `handin_filename` text field may cause functionality issues and should be avoided.
🔇 Additional comments (1)
app/views/assessments/_edit_handin.html.erb (1)
50-53: LGTM! Clear and well-documented checkbox implementation.
The changes effectively improve the visibility and understanding of the disable handins feature, which directly addresses the PR objectives.
| <script> | ||
| function toggleHandinAutolab(disable_button) { | ||
| const handinField = document.querySelector('.handin-filename-field'); | ||
| handinField.disabled = disable_button.checked; | ||
| if (!handinField.disabled && !handinField.value) { | ||
| handinField.value = 'handin.c'; | ||
| } | ||
| } | ||
|
|
||
| document.addEventListener('DOMContentLoaded', function() { | ||
| const disableHandinsCheckbox = document.querySelector('.disable-handins-toggle'); | ||
| toggleHandinAutolab(disableHandinsCheckbox); | ||
| disableHandinsCheckbox.addEventListener('change', function() { | ||
| toggleHandinAutolab(disableHandinsCheckbox); | ||
| }); | ||
| }); | ||
| </script> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and defensive programming.
The JavaScript implementation needs more robust error handling to prevent potential runtime errors.
Apply these improvements:
<script>
+ const DEFAULT_FILENAME = 'handin.c';
+
function toggleHandinAutolab(disable_button) {
+ if (!disable_button) {
+ console.error('Invalid disable button parameter');
+ return;
+ }
const handinField = document.querySelector('.handin-filename-field');
+ if (!handinField) {
+ console.error('Handin filename field not found');
+ return;
+ }
handinField.disabled = disable_button.checked;
if (!handinField.disabled && !handinField.value) {
- handinField.value = 'handin.c';
+ handinField.value = DEFAULT_FILENAME;
}
}
document.addEventListener('DOMContentLoaded', function() {
const disableHandinsCheckbox = document.querySelector('.disable-handins-toggle');
+ if (!disableHandinsCheckbox) {
+ console.error('Disable handins checkbox not found');
+ return;
+ }
toggleHandinAutolab(disableHandinsCheckbox);
disableHandinsCheckbox.addEventListener('change', function() {
toggleHandinAutolab(disableHandinsCheckbox);
});
});
</script>📝 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.
| <script> | |
| function toggleHandinAutolab(disable_button) { | |
| const handinField = document.querySelector('.handin-filename-field'); | |
| handinField.disabled = disable_button.checked; | |
| if (!handinField.disabled && !handinField.value) { | |
| handinField.value = 'handin.c'; | |
| } | |
| } | |
| document.addEventListener('DOMContentLoaded', function() { | |
| const disableHandinsCheckbox = document.querySelector('.disable-handins-toggle'); | |
| toggleHandinAutolab(disableHandinsCheckbox); | |
| disableHandinsCheckbox.addEventListener('change', function() { | |
| toggleHandinAutolab(disableHandinsCheckbox); | |
| }); | |
| }); | |
| </script> | |
| <script> | |
| const DEFAULT_FILENAME = 'handin.c'; | |
| function toggleHandinAutolab(disable_button) { | |
| if (!disable_button) { | |
| console.error('Invalid disable button parameter'); | |
| return; | |
| } | |
| const handinField = document.querySelector('.handin-filename-field'); | |
| if (!handinField) { | |
| console.error('Handin filename field not found'); | |
| return; | |
| } | |
| handinField.disabled = disable_button.checked; | |
| if (!handinField.disabled && !handinField.value) { | |
| handinField.value = DEFAULT_FILENAME; | |
| } | |
| } | |
| document.addEventListener('DOMContentLoaded', function() { | |
| const disableHandinsCheckbox = document.querySelector('.disable-handins-toggle'); | |
| if (!disableHandinsCheckbox) { | |
| console.error('Disable handins checkbox not found'); | |
| return; | |
| } | |
| toggleHandinAutolab(disableHandinsCheckbox); | |
| disableHandinsCheckbox.addEventListener('change', function() { | |
| toggleHandinAutolab(disableHandinsCheckbox); | |
| }); | |
| }); | |
| </script> |
Description
Adjusted UI to make the "Disable Autolab submissions" button and functionality more noticeable. Also edited description on submissions page.
Before:
After:
Also added some error checking for the handin filename:
Motivation and Context
Some classes have assignments that shouldn't be submitted through Autolab. The functionality for disabling submissions through Autolab exists, but it is currently a bit unclear what the button does and many classes therefore don't use it. Fixes #2216.
How Has This Been Tested?
Tested locally (see screenshots) to make sure changes were saved and toggle between disable/enable handins and gray-out worked as expected.
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for lintingOther issues / help required