Tweaked Confirmation Prompt and Added Test #2253
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the hand-in form for assessments, focusing on the clarity and phrasing of messages related to late submissions and grace days. Changes include updates to the Changes
Assessment against linked issues
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: 2
🧹 Outside diff range and nitpick comments (3)
spec/models/handin_form_spec.rb (1)
6-6: Fix style issues for better code consistency.Minor style improvements needed:
- Remove trailing whitespace on line 6
- Split long lines (11, 16) to stay within 100-character limit
- let(:grace_late_info) { "using 1 late day" } + let(:grace_late_info) { "using 1 late day" } - render partial: "assessments/handin_form", locals: { f: ActionView::Helpers::FormBuilder.new(nil, nil, self, {}) } + render partial: "assessments/handin_form", + locals: { f: ActionView::Helpers::FormBuilder.new(nil, nil, self, {}) } - expect(rendered).to have_selector("input[data-confirm='You are #{grace_late_info}. Are you sure you want to continue?']") + expect(rendered).to have_selector( + "input[data-confirm='You are #{grace_late_info}. Are you sure you want to continue?']" + )Also applies to: 11-11, 16-16
🧰 Tools
🪛 rubocop (1.68.0)
[convention] 6-6: Trailing whitespace detected.
(Layout/TrailingWhitespace)
app/views/assessments/_handin_form.html.erb (2)
18-19: Consider providing more specific warning information.While the warning message is clear, it could be more helpful by:
- Specifying the exact penalty rate
- Clarifying if grace days are automatically applied
- <p><b>Warning:</b> Submitting late will result in grace day usage or penalties as per the syllabus!</p> + <p><b>Warning:</b> Submitting late will automatically use available grace days or apply a <%= @assessment.late_penalty %>% penalty per day!</p>
47-48: Inconsistent confirmation messaging between UI elements.The confirmation messages differ between the checkbox and submit button:
- Checkbox: "and are #{grace_late_info}"
- Submit button: "You are #{grace_late_info}. Click to confirm!"
Consider unifying the message format for better user experience.
- <%= content_tag("span", "I affirm that I comply with this course's academic - integrity policy#{late_confirm}.") %> + <%= content_tag("span", "I affirm that I comply with this course's academic integrity policy and understand that #{grace_late_info}.") %> - <%= f.submit("Submit Late", id: "fake-submit", class: "btn primary handin-btn disabled", data: { confirm: "Autolab Notification: You are #{grace_late_info}. Click to confirm!" }) %> + <%= f.submit("Submit Late", id: "fake-submit", class: "btn primary handin-btn disabled", data: { confirm: "Please confirm: This submission will use #{grace_late_info}. Do you want to proceed?" }) %> - <%= f.submit("Submit", id: "fake-submit", class: "btn primary handin-btn disabled", data: { confirm: "Autolab Notification: Confirm submission?" }) %> + <%= f.submit("Submit", id: "fake-submit", class: "btn primary handin-btn disabled", data: { confirm: "Please confirm: Do you want to submit this assignment?" }) %>Also applies to: 56-56, 60-60
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/views/assessments/_handin_form.html.erb(2 hunks)spec/models/handin_form_spec.rb(1 hunks)
🧰 Additional context used
🪛 rubocop (1.68.0)
spec/models/handin_form_spec.rb
[convention] 6-6: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 11-11: Line is too long. [118/100]
(Layout/LineLength)
[convention] 16-16: Line is too long. [125/100]
(Layout/LineLength)
🔇 Additional comments (1)
app/views/assessments/_handin_form.html.erb (1)
12-16: LGTM! Clear and consistent late day messaging.
The grace day information messages are well-structured and provide clear information about the late submission status.
spec/models/handin_form_spec.rb
Outdated
| it "renders the Submit Late button with a confirmation prompt" do | ||
| expect(rendered).to have_selector("input[type='submit'][id='fake-submit']") | ||
| expect(rendered).to have_selector("input[data-confirm='You are #{grace_late_info}. Are you sure you want to continue?']") | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for late submission scenarios.
The current test only verifies the basic presence of the confirmation prompt. Consider adding test cases for:
- Different grace day scenarios (no grace days left, partial grace days available)
- Non-late submission case (should not show confirmation)
- Various late day calculations
Would you like me to help generate additional test cases?
🧰 Tools
🪛 rubocop (1.68.0)
[convention] 16-16: Line is too long. [125/100]
(Layout/LineLength)
| <% end %> | ||
|
|
||
| <%= f.file_field :file %> | ||
| <div class="row" style="padding-top: 10px"> | ||
| <div id="submission_error" style="color:red" class="col s6 m8 center-align"></div> | ||
| <% if @aud.past_due_at? then %> | ||
| <div class="col s6 m4 center-align"> | ||
| <%= f.submit("Submit Late", id: "fake-submit", class: "btn primary handin-btn disabled") %> | ||
| <%= f.submit("Submit Late", id: "fake-submit", class: "btn primary handin-btn disabled", data: { confirm: "Autolab Notification: You are #{grace_late_info}. Click to confirm!" }) %> |
There was a problem hiding this comment.
💡 Codebase verification
Based on the findings, I can now provide the final response:
Add ARIA attributes to confirmation dialogs for better accessibility
The codebase uses Rails' default data-confirm attribute for confirmation dialogs, which relies on jquery_ujs (found in application.js). These default dialogs lack proper accessibility attributes. To improve accessibility:
- Add
role="dialog"andaria-modal="true"to the confirmation dialog - Include an
aria-labelledbyattribute referencing a dialog title - Ensure keyboard focus is properly managed
- Consider replacing the default Rails confirmation with a custom accessible dialog
🔗 Analysis chain
Verify accessibility of confirmation dialogs.
Ensure that the confirmation dialogs are accessible to screen readers and keyboard navigation.
Also applies to: 60-60
Let me check the confirmation dialog implementation further to provide a comprehensive verification.
Let me try one more search to check for any JavaScript confirmation handling in the application.
Let me check the handin.js and application.js files since they might contain confirmation dialog implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for ARIA attributes or role="alert" in confirmation dialogs
rg -l 'role="(alert|dialog)"' app/views/assessments/
rg -l 'aria-' app/views/assessments/
Length of output: 137
Script:
#!/bin/bash
# Check for data-confirm usage and related JavaScript handlers
rg -l 'data-confirm' app/views/assessments/
rg -l 'data-confirm' app/assets/javascripts/
rg -l 'confirm:' app/assets/javascripts/
# Check for any Rails UJS or custom confirmation dialog implementations
rg -l 'rails-ujs|rails/ujs' app/assets/
rg -l 'confirmDialog|confirm_dialog' app/assets/
Length of output: 222
Script:
#!/bin/bash
# Search for confirmation-related JavaScript files and implementations
fd -e js . app/javascript app/assets/javascripts
# Check for any Rails UJS or Turbolinks configuration files
fd -e js . app/javascript -x grep -l "confirm"
fd -e coffee . app/assets -x grep -l "confirm"
# Look for application.js or similar main JS files
fd "application.js" app/
fd "application.coffee" app/
Length of output: 2611
Script:
#!/bin/bash
# Check handin.js and application.js for confirmation dialog implementations
cat app/assets/javascripts/handin.js
cat app/assets/javascripts/application.js
# Also check for any Rails UJS related configurations
rg "data-confirm" -A 5 -B 5 app/assets/javascripts/
Length of output: 6316
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
spec/models/handin_form_spec.rb (2)
11-11: Improve FormBuilder setup for more realistic testing.The current FormBuilder initialization with nil parameters might not accurately represent real-world usage. Consider using a more realistic setup.
- render partial: "assessments/handin_form", locals: { f: ActionView::Helpers::FormBuilder.new(nil, nil, self, {}) } + form_object = assessment # or whatever object the real form uses + render partial: "assessments/handin_form", locals: { + f: ActionView::Helpers::FormBuilder.new('assessment', form_object, self, {}) + }🧰 Tools
🪛 rubocop (1.68.0)
[convention] 11-11: Line is too long. [118/100]
(Layout/LineLength)
14-17: Strengthen test assertions for the confirmation prompt.The current test only verifies basic presence. Consider adding assertions for:
- Button state (enabled/disabled based on conditions)
- Exact text structure and formatting
- Accessibility attributes
it "renders the Submit Late button with a confirmation prompt" do expect(rendered).to have_selector("input[type='submit'][id='fake-submit']") - expect(rendered).to have_selector("input[data-confirm='Autolab Notification: You are #{grace_late_info}. Click to confirm!']") + submit_button = rendered.find("input[type='submit'][id='fake-submit']") + + # Verify button properties + expect(submit_button['value']).to eq('Submit Late') + expect(submit_button['class']).to include('btn-warning') # assuming we want warning styling + + # Verify confirmation message structure + expected_message = "Autolab Notification: You are #{grace_late_info}. Click to confirm!" + expect(submit_button['data-confirm']).to eq(expected_message) + + # Verify accessibility + expect(submit_button['aria-label']).to be_present end🧰 Tools
🪛 rubocop (1.68.0)
[convention] 16-16: Line is too long. [130/100]
(Layout/LineLength)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
spec/models/handin_form_spec.rb(1 hunks)
🧰 Additional context used
🪛 rubocop (1.68.0)
spec/models/handin_form_spec.rb
[convention] 6-6: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 11-11: Line is too long. [118/100]
(Layout/LineLength)
[convention] 16-16: Line is too long. [130/100]
(Layout/LineLength)
🔇 Additional comments (1)
spec/models/handin_form_spec.rb (1)
4-6: 🛠️ Refactor suggestion
Enhance test coverage with different late submission scenarios.
The current setup only tests a single scenario with "1 late day". Consider using shared contexts or multiple test cases to cover:
- Different grace day counts (0, partial, maximum allowed)
- Penalty late days vs grace days
- Non-late submission scenarios
Here's a suggested enhancement:
- let(:grace_late_info) { "using 1 late day" }
+ shared_context "late submission scenarios" do
+ let(:assessment) { create(:assessment) }
+
+ context "with grace days" do
+ let(:grace_late_info) { "using 2 grace days" }
+ let(:aud) { create(:assessment_user_datum, assessment: assessment, past_due_at: true) }
+ it_behaves_like "late submission confirmation"
+ end
+
+ context "with penalty days" do
+ let(:grace_late_info) { "using 1 penalty late day" }
+ let(:aud) { create(:assessment_user_datum, assessment: assessment, past_due_at: true) }
+ it_behaves_like "late submission confirmation"
+ end
+
+ context "not late" do
+ let(:aud) { create(:assessment_user_datum, assessment: assessment, past_due_at: false) }
+ it "does not show confirmation prompt" do
+ expect(rendered).not_to have_selector("[data-confirm]")
+ end
+ end
+ endLikely invalid or redundant comment.
🧰 Tools
🪛 rubocop (1.68.0)
[convention] 6-6: Trailing whitespace detected.
(Layout/TrailingWhitespace)
|
Could add more test cases to ensure better coverage. |
Tested Confirmation Prompt
Description
This update modifies the data-confirm attribute for the "Submit Late" button in the handin_form. When students attempt to submit an assignment late, they are now prompted with a custom confirmation message that includes dynamic details about their late submission, such as the number of grace days or penalty late days being used. This change enhances the user experience by providing clearer, more informative feedback relevant to Autolab's policies. The test is used to ensure that the updated confirmation message appears correctly in the rendered handin_form view. It checks that the "Submit Late" button renders with the correct data-confirm attribute, ensuring the confirmation prompt dynamically includes the grace_late_info.
Motivation and Context
Students mistakenly submit to the wrong assignments, either wasting late days unnecessarily or submitting to assignments where late days are not allowed. As a solution, we can add a confirmation prompt when students are about to use late days. This should be tested to verify that it shows up. Fixes #2211
How Has This Been Tested?
Tested locally to make sure that the confirmation prompt appears when submitting (see screenshots) and ran
rspec spec/models/handin_form_spec.rb
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Checklist:
[ x ] I have run rubocop and erblint for style check. If you haven't, run overcommit --install && overcommit --sign to use pre-commit hook for linting
My change requires a change to the documentation, which is located at Autolab Docs
I have updated the documentation accordingly, included in this PR