Clarify Disable Autolab Submissions Option #2224
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new JavaScript function, Changes
Suggested reviewers
🪧 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.
There was a problem hiding this comment.
Aside from the two code comments, there is an edge case we should probably address.
If the handin name is modified but the disable box is checked, then upon a save the modified handin name is thrown away. Even though we are disabling it, if a user modifies it for some reason, the modification should probably still be kept.
I'm not sure about this, but I'd imagine that there is something in the saving code logic that does not update the handin name in the database if the disable box is checked (which should maybe be changed).
| document.addEventListener('DOMContentLoaded', function() { | ||
| const disableHandinsCheckbox = document.querySelector('.disable-handins-toggle'); | ||
| toggleHandinAutolab(disableHandinsCheckbox); | ||
| disableHandinsCheckbox.addEventListener('change', function() { |
There was a problem hiding this comment.
This inner event listener is likely redundant since we have onchange: "toggleHandinAutolab(this)" on the check box.
| function toggleHandinAutolab(disable_button) { | ||
| const handinField = document.querySelector('.handin-filename-field'); | ||
| handinField.disabled = disable_button.checked; | ||
| if (!handinField.disabled && !handinField.value) { |
There was a problem hiding this comment.
I think the overwrite with handin.c is probably undesirable because we have placeholder: "E.g. mm.c" on the check box. It kind of forces a somewhat arbitrary value into the database and also differs from older functionality (if it is blank leave it blank).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/views/assessments/_edit_handin.html.erb (1)
3-13: JavaScript implementation could benefit from error handlingYour JavaScript code correctly implements the toggling functionality but lacks error handling. While not critical in this controlled environment, adding defensive checks would make the code more robust.
Consider adding error handling to prevent potential DOM-related errors:
<script> function toggleHandinAutolab(disable_button) { const handinField = document.querySelector('.handin-filename-field'); + if (!handinField) { + console.error('Handin filename field not found'); + return; + } handinField.disabled = disable_button.checked; } document.addEventListener('DOMContentLoaded', function() { const disableHandinsCheckbox = document.querySelector('.disable-handins-toggle'); + if (!disableHandinsCheckbox) { + console.error('Disable handins checkbox not found'); + return; + } toggleHandinAutolab(disableHandinsCheckbox); }); </script>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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 (2)
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.
Learnt from: jhs-panda
PR: autolab/Autolab#2224
File: app/views/assessments/_edit_handin.html.erb:54-0
Timestamp: 2024-11-15T04:24:44.670Z
Learning: In `app/views/assessments/_edit_handin.html.erb`, when adding a pattern validation to the `handin_filename` input field, including hyphens in the pattern can allow other invalid characters to be accepted without flashing an error. Excluding hyphens from the pattern ensures proper input validation.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
app/views/assessments/_edit_handin.html.erb (3)
43-47: Good improvement to the checkbox label and help textThe renamed display name "Disable Autolab submissions" better clarifies the purpose of this feature, and the help text provides useful context for users. The addition of class and onchange attributes properly connects this element to the JavaScript functionality.
55-55: Clear help text for max_size fieldThe help text clearly explains the purpose of the field in megabytes.
48-54:❓ Verification inconclusive
Review potential issues with the handin_filename field
The handin_filename implementation may have some concerns:
- Based on past learnings, adding
patternandtitleattributes might cause functionality issues with this specific field- The default value "handin.c" is set, which may not align with previous functionality where blank values remained blank
Could you verify that:
- The pattern validation works as expected without causing any usability issues?
- The default "handin.c" value is the intended behavior rather than leaving blank values as blank?
🌐 Web query:
What are recommended best practices for form field validation in Ruby on Rails?💡 Result:
Form validation in Ruby on Rails requires a combination of server-side and client-side techniques to ensure data integrity while providing responsive user feedback. Here are the recommended best practices:
Server-Side Validation Fundamentals
Active Record Validations
Use Rails' built-in validation helpers for core checks:class Post < ApplicationRecord validates :title, presence: true validates :body, length: { minimum: 10 } validates :email, format: { with: URI::MailTo::EMAIL_REGEXP } endCommon helpers include
presence,uniqueness,length, andnumericality[2][4][6]Custom Validation Methods
Implement complex business rules through custom methods:validate :publish_date_cannot_be_in_past def publish_date_cannot_be_in_past errors.add(:publish_date, "can't be in past") if publish_date < Date.today end[2][6]
Contextual Validation
Use:onoption for create/update-specific rules:validates :password, confirmation: true, on: :create[4][6]
Client-Side Enhancements
HTML5 Form Validation
Leverage native browser validation for instant feedback:<%= form.email_field :email, required: true, pattern: "[^@]+@[^@]+\.[a-zA-Z]{2,6}" %>[7][9]
Real-Time Validation with Stimulus
Implement dynamic checks using AJAX:// app/javascript/controllers/form_validation_controller.js handleChange(event) { Rails.ajax({ url: this.urlValue, type: "POST", data: new FormData(this.formTarget), success: (data) => { this.outputTarget.innerHTML = data } }) }Add debouncing to optimize performance [1][3]
Error Presentation
Customize error display using Rails'field_error_proc:ActionView::Base.field_error_proc = Proc.new do |html_tag, instance| "<span class='field-with-errors'>#{html_tag}</span>".html_safe end[7][10]
Advanced Patterns
Technique Implementation Use Case Conditional Validation validates :phone, presence: true, if: :email_confirmed?Context-dependent rules Uniqueness Handling Database constraints + model validation + retry logic Race condition prevention Composite Keys validates_uniqueness_of :code, scope: [:department_id, :project_id]Complex uniqueness rules Custom Validation Classes validates_with EmailValidatorReusable validation logic Security Considerations
- Always pair client-side validation with server-side checks [2][9]
- Use CSRF protection with
protect_from_forgery[2]- Sanitize inputs with
sanitizehelper [7]For complex forms, consider:
# Hybrid validation approach class UserForm include ActiveModel::Model validates :terms, acceptance: true validate :complex_password_rules def save return false unless valid? # Persistence logic end end[2][12]
Performance Tips
- Use
valid?instead ofsavefor preliminary checks- Batch validations with
validate :methodchains- Cache expensive validation computations [6][12]
Always test validations through both unit tests and system tests to ensure they work as intended across full stack interactions[7][9].
Citations:
- 1: https://stevepolito.design/blog/rails-real-time-form-validation
- 2: https://clouddevs.com/ruby-on-rails/forms-and-input-validation/
- 3: https://dev.to/abeidahmed/form-validation-in-rails-56dc
- 4: https://edgeguides.rubyonrails.org/active_record_validations.html
- 5: https://guides.rubyonrails.org/active_record_validations.html
- 6: https://www.honeybadger.io/blog/active-record-validations/
- 7: https://dev.to/kanani_nirav/form-validations-in-ruby-on-rails-application-ojk
- 8: https://www.ruby-forum.com/t/how-can-i-check-form-input-before-submit/171356
- 9: https://www.jorgemanrubia.com/2019/02/16/form-validations-with-html5-and-modern-rails/
- 10: https://railsdesigner.com/inline-form-validations/
- 11: https://discuss.rubyonrails.org/t/how-would-you-handle-gracefully-validating-a-generated-unique-code-until-its-guaranteed-to-be-valid/84919
- 12: https://dev.to/kimrgrey/payload-and-parameters-validation-in-rails-4m87
Handin Filename Field: Verify Pattern Attribute & Default Value Behavior
Please verify that this field meets our intended use cases:
Pattern and Title Attributes:
Although HTML5 best practices support using apatternfor client-side validation, previous learnings in our project (see jhs-panda in Clarify Disable Autolab Submissions Option #2224) suggest that addingpatternandtitleto thehandin_filenamefield in this view may cause functionality issues. Please confirm that the regex pattern (^[a-zA-Z0-9._]+$) is working as expected without negatively impacting usability.Default Value Behavior:
The current default value is set to"handin.c". Historically, blank values were allowed. Confirm that this change is intentional and aligns with the expected behavior for cases where no handin filename is provided.
|
Also, the sanitization for handin filename is probably a little too restrictive. For example, hyphens are used pretty regularly for 15213 assessments. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/views/assessments/_edit_handin.html.erb (1)
48-54:⚠️ Potential issuePotential issue with pattern validation including hyphens.
According to previous feedback in this PR, including hyphens in the pattern validation for the filename field could allow other invalid characters to be accepted without proper error indication. Consider removing the hyphen from the pattern.
Based on your previous feedback in this PR, including hyphens in the pattern can lead to validation issues. Apply this fix:
- pattern: "^[a-zA-Z0-9._-]+$", - title: "Only alphanumeric characters, dots, hyphens, and underscores are allowed." + pattern: "^[a-zA-Z0-9._]+$", + title: "Only alphanumeric characters, dots, and underscores are allowed."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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 (2)
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.
Learnt from: jhs-panda
PR: autolab/Autolab#2224
File: app/views/assessments/_edit_handin.html.erb:54-0
Timestamp: 2024-11-15T04:24:44.670Z
Learning: In `app/views/assessments/_edit_handin.html.erb`, when adding a pattern validation to the `handin_filename` input field, including hyphens in the pattern can allow other invalid characters to be accepted without flashing an error. Excluding hyphens from the pattern ensures proper input validation.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (2)
app/views/assessments/_edit_handin.html.erb (2)
48-50: Default value handling for empty filename fields.The current implementation sets the value to null if the field is empty, which differs from the previous behavior. A reviewer noted that forcing a default value might be undesirable. Ensure this change is intentional.
Do you want to maintain this behavior where an empty field remains empty, or would you prefer to add a default value like 'handin.c' when the field is empty but not disabled?
43-47: Good implementation of the disable Autolab submissions option.The checkbox implementation clearly communicates its purpose with an appropriate display name and helpful explanation text. This directly addresses the PR objective of clarifying the "Disable Autolab submissions" option.
| <script> | ||
| function toggleHandinAutolab(disable_button) { | ||
| const handinField = document.querySelector('.handin-filename-field'); | ||
| handinField.disabled = disable_button.checked; | ||
| } | ||
|
|
||
| document.addEventListener('DOMContentLoaded', function() { | ||
| const disableHandinsCheckbox = document.querySelector('.disable-handins-toggle'); | ||
| toggleHandinAutolab(disableHandinsCheckbox); | ||
| }); | ||
| </script> |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
JavaScript implementation needs a change event listener.
The current implementation initializes the state correctly on page load, but it's missing an event listener on the checkbox to handle changes after initial load. While the onchange attribute on line 47 handles user interactions, the JavaScript would be more complete with a corresponding event listener as originally intended.
🏁 Script executed:
#!/bin/bash
# Check if the onchange attribute is working as expected by looking for other similar patterns in the codebase
# Looking for other instances where onchange HTML attribute is used with a JS event listener
rg -A 2 "onchange=\"[^\"]+\"" --type htmlLength of output: 40
Action Required: Add a JavaScript change event listener for the disable-handins toggle
The current code calls toggleHandinAutolab on page load but does not handle any state updates if the checkbox is toggled after the page has loaded. Although an inline onchange attribute on line 47 is meant to cover user interactions, it’s best to consolidate event handling within the JavaScript code for consistency and maintainability.
-
In app/views/assessments/_edit_handin.html.erb, update the script to attach a change event listener on the checkbox. For example:
document.addEventListener('DOMContentLoaded', function() { const disableHandinsCheckbox = document.querySelector('.disable-handins-toggle'); toggleHandinAutolab(disableHandinsCheckbox); // Attach a change event listener to update the state when the checkbox is toggled disableHandinsCheckbox.addEventListener('change', function() { toggleHandinAutolab(disableHandinsCheckbox); }); });
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